Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InvalidAttribute for SensitiveParameter when running on php 8.2 and targeting 8.1 or lower #9728

Closed
Xerkus opened this issue May 1, 2023 · 17 comments

Comments

@Xerkus
Copy link

Xerkus commented May 1, 2023

SensitiveParameter attribute results in InvalidAttribute error when psalm runs on PHP 8.2 but targets 8.1 or below.

$ php --version
PHP 8.2.5 (cli)

$ vendor/bin/psalm
Target PHP version: 8.0 (inferred from composer.json).

ERROR: InvalidAttribute - src/Uri.php:246:11 - The class SensitiveParameter doesn't have the Attribute attribute (see https://psalm.dev/242)
        #[SensitiveParameter]

When Psalm runs on PHP 8.1 or lower the error is then UndefinedAttributeClass as expected:

Error: src/Uri.php:246:11: UndefinedAttributeClass: Attribute class SensitiveParameter does not exist (see https://psalm.dev/241)

The problem is inconsistency of result depending on runtime.

Expected result:
Psalm does not produce InvalidAttribute error when running on PHP 8.2 but targeting 8.1 or below.

@psalm-github-bot
Copy link

Hey @Xerkus, can you reproduce the issue on https://psalm.dev ?

@Xerkus
Copy link
Author

Xerkus commented May 1, 2023

I can not reproduce it on psalm.dev. The issue is encountered in https://github.com/laminas/laminas-diactoros/tree/8862d24e24c0aa2efc6f704cc6a80c827e0ec154

@orklah
Copy link
Collaborator

orklah commented May 1, 2023

SensitiveParameter is only available in PHP 8.2 but your analysing your project on 8.0

@Xerkus
Copy link
Author

Xerkus commented May 1, 2023

CI failure with UndefinedAttributeClass for the commit running on PHP 8.0 is expected outcome and it is not an issue. However, when running it locally on php 8.2 the error InvalidAttribute is not expected.

Should I change composer.json to require ^8.2 as minimum so that psalm no longer targets current minimum PHP version the InvalidAttribute error disappears.

@orklah
Copy link
Collaborator

orklah commented May 1, 2023

You can leave 8.0 in composer but actually run Psalm with a specified version instead of letting it fallback to composer

But you can possibly expect the other issue around (some errors will appear because something don't make sense in 8.2 but they're necessary to support 8.0)

For this kind of issue, I'd just suppress the error now that you get what Psalm is trying to raise here

@Xerkus
Copy link
Author

Xerkus commented May 1, 2023

I think what happens here is SensitiveParameter declared as stub by psalm which is loaded when psalm targets 8.2. When it targets 8.0 as in our case it can detect SensitiveParameter class present in php but without stub psalm does not understand it as valid attribute class.

@Xerkus
Copy link
Author

Xerkus commented May 1, 2023

Relevant PR for similar issue #6873
@orklah I see it was done by you. Did approach to such attributes change?

@orklah
Copy link
Collaborator

orklah commented May 1, 2023

Well, ReturnTypeWillChange was pretty special for two reasons:

  • It was specifically created to create a migration path for codebases that dealt with older PHP version
  • It was created on the very beggining of attributes when attributes were processed on older PHP version

SensitiveParameter is different because trying to use it on oldest PHP version where attributes are processed will make PHP crash because the class does not exists

If Psalm was perfect, it would raise an UndefinedClass for SensitiveParameter but given you run on PHP 8.2, Psalm fallbacks on Reflection and finds the class there

@Xerkus
Copy link
Author

Xerkus commented May 1, 2023

SensitiveParameter is different because trying to use it on oldest PHP version where attributes are processed will make PHP crash because the class does not exists

SensitiveParameter attribute does not crash php. It is simply ignored on older versions.

https://3v4l.org/auFYg

It does not affect behavior of the code either so it is safe to use on any php version but it will affect backtrace only in 8.2+

@Xerkus
Copy link
Author

Xerkus commented May 1, 2023

They were originally available for all version but were moved to 8.2 only in #9445

@Xerkus
Copy link
Author

Xerkus commented May 1, 2023

They were originally available for all version but were moved to 8.2 only in #9445

AllowDynamicProperties reversed in that PR is backward compatibility attribute similar to the ReturnTypeWillChange
https://3v4l.org/RAsMo

williamdes referenced this issue in phpmyadmin/sql-parser Jun 5, 2023
Pull-request: #453

Signed-off-by: William Desportes <williamdes@wdes.fr>
@TimWolla
Copy link
Contributor

TimWolla commented Sep 5, 2023

It does not affect behavior of the code either so it is safe to use on any php version but it will affect backtrace only in 8.2+

As the author of the RFC that introduced the SensitiveParameter attribute, this is correct and emitting an error when encountering the attribute on PHP versions before PHP 8.2 is not unlikely to hurt adoption, because folks can no longer simply add it and benefit from increased safety in PHP 8.2 without any drawbacks in older PHP versions.

SensitiveParameter is different because trying to use it on oldest PHP version where attributes are processed will make PHP crash because the class does not exists

Attributes generally do not need to be backed by a class, they can be arbitrary strings. The class only needs to exist if you call ReflectionAttribute::newInstance().

@weirdan
Copy link
Collaborator

weirdan commented Sep 5, 2023

As far as I can see no error is reported when Psalm runs on 8.2 and targets 8.0-8.2:

@Xerkus can you confirm you still experience the issue with vimeo/psalm@dev-master?

The error was reported on 7.4:

https://psalm.dev/r/819e67d494?php=7.4

@psalm-github-bot
Copy link

psalm-github-bot bot commented Sep 5, 2023

I found these snippets:

https://psalm.dev/r/819e67d494
<?php

function f(
   #[SensitiveParameter]
   string $_password
): void {}
Psalm output (using commit 37cc4fd):

No issues!
https://psalm.dev/r/819e67d494
<?php

function f(
   #[SensitiveParameter]
   string $_password
): void {}
Psalm output (using commit 37cc4fd):

No issues!
https://psalm.dev/r/819e67d494
<?php

function f(
   #[SensitiveParameter]
   string $_password
): void {}
Psalm output (using commit 37cc4fd):

No issues!
https://psalm.dev/r/819e67d494
<?php

function f(
   #[SensitiveParameter]
   string $_password
): void {}
Psalm output (using commit 37cc4fd):

ERROR: InvalidAttribute - 4:6 - The class SensitiveParameter doesn't have the Attribute attribute

@TimWolla
Copy link
Contributor

TimWolla commented Sep 5, 2023

@weirdan I'm seeing the issue with https://github.com/paragonie/constant_time_encoding and Psalm 5 (I wanted to contribute a PR for the Psalm update there):

root@badba6398f4f:/pwd# php -v
PHP 8.2.10 (cli) (built: Sep  2 2023 06:38:01) (NTS)
Copyright (c) The PHP Group
Zend Engine v4.2.10, Copyright (c) Zend Technologies
    with Zend OPcache v8.2.10, Copyright (c), by Zend Technologies
    with Xdebug v3.2.2, Copyright (c) 2002-2023, by Derick Rethans
root@badba6398f4f:/pwd# vendor/bin/psalm -v
Psalm 5.15.0@5c774aca4746caf3d239d9c8cadb9f882ca29352
root@badba6398f4f:/pwd# vendor/bin/psalm 
Warning: "findUnusedBaselineEntry" will default to "true" in Psalm 6. You should explicitly enable or disable this setting.
Warning: "findUnusedCode" will default to "true" in Psalm 6. You should explicitly enable or disable this setting.
Warning: "findUnusedBaselineEntry" will default to "true" in Psalm 6. You should explicitly enable or disable this setting.
Warning: "findUnusedCode" will default to "true" in Psalm 6. You should explicitly enable or disable this setting.
Target PHP version: 7.0 (inferred from composer.json).
Scanning files...
Analyzing files...



ERROR: InvalidAttribute - src/Base32.php:48:11 - The class SensitiveParameter doesn't have the Attribute attribute (see https://psalm.dev/242)
        #[\SensitiveParameter]

@weirdan
Copy link
Collaborator

weirdan commented Sep 5, 2023

So for target version below 8.0:

Target PHP version: 7.0 (inferred from composer.json).

@Xerkus
Copy link
Author

Xerkus commented Sep 5, 2023

I can see attributes were moved to a separate stub file in #9920 and included on php 8.0 and up. Available since psalm 5.13.0

They are not included for target versions below 8.0 which is fine by me but might be an issue for others i suppose.

Btw, there is another similar attribute coming with php 8.3 - Override. See https://wiki.php.net/rfc/marking_overriden_methods

@Xerkus Xerkus closed this as completed Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants