Skip to content

Conversation

@jrfnl
Copy link
Member

@jrfnl jrfnl commented Jul 28, 2024

Attributes have been implemented in a manner which makes them largely cross-version compatible (no parse error) with older PHP versions, however, there are a few exceptions.

Most notably, the following usages of attributes are not cross-version compatible:

  • Multi-line attributes.
  • Inline attributes, i.e. an attribute followed by other code on the same line.
  • Attributes containing (something which looks like) a PHP close tag in a string argument passed to the attribute on the same line as the attribute opener.

Other than that, attributes are cross-version compatible, though (obviously), the functionality attached to the attribute will not work when the code is run on PHP < 8.0.

Until now, the NewAttributes sniff did not distinguish between these four situations, which made the sniff annoyingly noisy.

This PR goes a long way to fixing this.

Follow up on #1279.
Fixes #1480

Commits

Attributes/NewAttributes: split off flagging of multiline attributes

Until now, the NewAttributes sniff did not distinguish between these four situations, which made the sniff annoyingly noisy.

This commit is the first step towards changing this and introduces a separate error (and error code) for multi-line attributes, which are not cross-version compatible.

The change is covered by the pre-existing test cases, some of which will now throw the new error as well.

Attributes/NewAttributes: split off flagging of inline attributes

This commit is the second step towards changing this and introduces a separate error (and error code) for inline attributes, which are not cross-version compatible.

Notes:

  • This new error will not flag multiple attributes on one line. While yes, an attribute after an attribute closer is "code", it is code only on PHP 8.0, where this wouldn't be a problem.
  • If an attribute is both multi-line as well as inline (code after), both errors should be thrown, as hiding one error behind another is bad practice for sniffs.
  • In contrast to the other errors, this error will not be thrown on the attribute opener, but on the attribute closer, to make it more straight forward to see the issue in the code view (as the code will be directly after the attribute closer.

The change is covered by the pre-existing test cases, some of which will now throw the new error as well.
Additionally, a new test case is introduced to verify that when an attribute is both inline as well as multi-line, both errors will be thrown.

Attributes/NewAttributes: add specific error for close tag in text string param

This commit is the third step towards changing this and introduces a separate error (and error code) for attributes containing a text string which contains text which looks like a PHP close tag, when the text string is on the same line as the attribute opener.

Example of the problematic behaviour: https://3v4l.org/oarGV

Notes:

  • This third error is will not cause parse errors and may easily go unnoticed, but can cause a security issue (code exposure) and broken functionality.
  • Also note that text string parameters which is not on the same line as the attribute opener are not flagged as those have the parse error problem.
    The code exposure problem only comes into play once the parse error has been fixed as until then, the text string containing the ?> is not seen as part of the comment in PHP < 8.0, while the incompatibility problem lies in PHP respecting close tags within comment and being able to jump out of PHP when those are encountered.

The change is covered by pre-existing test cases, one of which will now (also) throw the new error.
A small tweak has been made to the test case to allow for safeguarding properly that the sniff will throw both the FoundMultiLine, FoundInline, as well as the FoundCloseTag error and doesn't hide one behind the other.

Attributes/NewAttributes: move "close tag in text string" tests to separate file

These tests have to be the last tests in a file, so having them in the main file, makes updating the tests fiddly as you need to keep updating the line numbers for the tests related to the "close tag in text string" errors.

This commit splits the test case file into two files to make adding new tests to the main file more straight forward.

Attributes/NewAttributes: downgrade the "generic" attributes error to a warning

Now each of the situations which cause real PHP compatibility issues are flagged as errors with their own error code, the generic "attribute found" error can be downgraded to a warning as it will only flag attributes which are cross-version compatible, though there may still be loss of functionality.

Includes a change to the message text to make it more informative.

Attributes/NewAttributes: ignore PHP native attributes for cross-version compatibility

PHP itself has introduced a couple of attribute classes since PHP 8.0 to help userland code get round problematic PHP deprecations:

  • ReturnTypeWillChange
  • AllowDynamicProperties

As long as those attributes are not using an incompatible (multi-line, inline) syntax, using these attributes is absolutely fine and should not be flagged.

To allow for excluding warnings about these type of attributes, we need to examine which attribute classes are used within an attribute, as, if there are multiple attributes classes referenced and not all of these are the attributes listed above, the attribute should still be flagged with a warning.

This commit updates the sniff to handle this in the following manner:

  • The names of all attribute classes encountered are collected first.
    Note: issue Feature suggestion: utilities to examine PHP attributes PHPCSStandards/PHPCSUtils#616 has been opened to potentially move that logic to PHPCSUtils.
  • Instead of throwing one warning per attribute, the sniff will now throw a warning for each attribute class within the attribute.
  • If the attribute class in question is one of the PHP native attributes for cross-version compatibility, the sniff will stay silent.
  • As the warning is now thrown for each attribute class name, the warning will now also be thrown on the attribute class name.
    This means some of the warnings are now thrown on different lines.
    It also means that the warning will be shown with higher precision for the code highlighting in the PHPCS code report.

Notes:

  • While the property holding the attribute names includes information about the PHP version which introduced the PHP native attribute, this information is not used as it is irrelevant, what with missing attribute classes not causing a fatal "can't find class" error unless the attribute is being accessed.
  • No namespace and import use statement resolution is executed at this time.
    This means that if an application would introduce its own Vendor\Package\ReturnTypeWillChange attribute class and reference it in the attribute as #[ReturnTypeWillChange], there would be a false negative.
    This will be fixed in the future once the namespace and import use statement resolution feature planned for PHPCSUtils become available.

Includes tests.

Attributes/NewAttributes: more modular error codes for the warning

The Found warning is still very generic and would be thrown for every cross-version compatible attribute, which can make it very noisy and make the temptation to turn it off large, while at times, the warning can prevent bugs from slipping in due to it flagging functionality which will not be executed on PHP < 8.0.

So this commit attempts to make the error code a little more modular to allow for more selectively excluding the error code.

Considerations which were taken into account:

  • If the error code was made fully modular ([AttributeName]Found), it would mean that for (valid uses of) each attribute a separate ruleset exclusion would need to be added, which would make the sniff very fiddly.
  • At the same time, just having the Found error and excluding that, would, as pointed out above, quickly ignore too much.

Ideally, we would be able to group all attributes by "source application", but at this time, that would cause a huge maintenance overhead.
Once namespace and import use statement resolution is added to the sniff (once available in PHPCSUtils), this will become easier though as groups (and error codes!) could be defined based on the namespace "prefix".

With this last bit in mind, I've currently implemented the modularization only for the most commonly adopted attribute classes.

I have looked at which applications offer attributes and whether these are (semi-)widely adopted or are expected to be widely adopted soonish, basing this in part on the Exakat articles and in part on publicly available information about dependencies from GitHub and Packagist.

For those attribute "groups", separate error codes have now been introduced which allow for selectively excluding the warning for a whole group of attributes.

Once namespace and import use statement resolution is added to the sniff, this code can be simplified a lot (no more need for the attribute class name lists, just base the error code on the namespace prefix in the FQN).
Additionally, once namespace and import use statement resolution is added to the sniff, further modularization based on namespace prefix can be added to the sniff.

This is also the reason not make the warnings remaining in the Found code more modular at this time as the above mentioned future change would then invalidate all previously made excludes/ignore annotations and be a breaking change in itself, while the partially modularization done now allows more easily for expanding this in the future without necessarily breaking existing excludes.

Notes:

  • Even though the Exakat articles mention ShopWare, Tuleap and Contao as other common sources of attribute, based on the same articles and a dependency check, no lists/condition for these have been added as they are mostly, if not exclusively, used within their own application and not adopted by the wider field of PHP applications.

Includes tests for the additional error codes, though this needs to be verified manually until the test framework would be adjusted to test error codes.

Refs:

Attributes/NewAttributes: improve sniff documentation

Attributes/NewAttributes: add XML user documentation

To view:

phpcs --generator=Text --standard=PHPCompatibility --sniffs=PHPCompatibility.Attributes.NewAttributes

jrfnl added 9 commits July 28, 2024 21:53
Attributes have been implemented in a manner which makes them largely cross-version compatible (no parse error) with older PHP versions, however, there are a few exceptions.

Most notably, the following usages of attributes are not cross-version compatible:
- Multi-line attributes.
- Inline attributes, i.e. an attribute followed by other code on the same line.
- Attributes containing (something which looks like) a PHP close tag in a string argument passed to the attribute on the same line as the attribute opener.

Other than that, attributes are cross-version compatible, though (obviously), the functionality attached to the attribute will not work when the code is run on PHP < 8.0.

Until now, the `NewAttributes` sniff did not distinguish between these four situations, which made the sniff annoyingly noisy.

This commit is the first step towards changing this and introduces a separate error (and error code) for multi-line attributes, which are not cross-version compatible.

The change is covered by the pre-existing test cases, some of which will now throw the new error as well.

Related to 1480
Attributes have been implemented in a manner which makes them largely cross-version compatible (no parse error) with older PHP versions, however, there are a few exceptions.

Most notably, the following usages of attributes are not cross-version compatible:
- Multi-line attributes.
- Inline attributes, i.e. an attribute followed by other code on the same line.
- Attributes containing (something which looks like) a PHP close tag in a string argument passed to the attribute on the same line as the attribute opener.

Other than that, attributes are cross-version compatible, though (obviously), the functionality attached to the attribute will not work when the code is run on PHP < 8.0.

Until now, the `NewAttributes` sniff did not distinguish between these four situations, which made the sniff annoyingly noisy.

This commit is the second step towards changing this and introduces a separate error (and error code) for inline attributes, which are not cross-version compatible.

Notes:
* This new error will not flag multiple attributes on one line. While yes, an attribute after an attribute closer is "code", it is code only on PHP 8.0, where this wouldn't be a problem.
* If an attribute is both multi-line as well as inline (code after), both errors should be thrown, as hiding one error behind another is bad practice for sniffs.
* In contrast to the other errors, this error will not be thrown on the attribute _opener_, but on the attribute _closer_, to make it more straight forward to see the issue in the code view (as the code will be directly after the attribute closer.

The change is covered by the pre-existing test cases, some of which will now throw the new error as well.
Additionally, a new test case is introduced to verify that when an attribute is both inline as well as multi-line, both errors will be thrown.

Related to 1480
…ring param

Attributes have been implemented in a manner which makes them largely cross-version compatible (no parse error) with older PHP versions, however, there are a few exceptions.

Most notably, the following usages of attributes are not cross-version compatible:
- Multi-line attributes.
- Inline attributes, i.e. an attribute followed by other code on the same line.
- Attributes containing (something which looks like) a PHP close tag in a string argument passed to the attribute on the same line as the attribute opener.

Other than that, attributes are cross-version compatible, though (obviously), the functionality attached to the attribute will not work when the code is run on PHP < 8.0 (where the attribute will just be ignored and seen as a comment).

Until now, the `NewAttributes` sniff did not distinguish between these four situations, which made the sniff annoyingly noisy.

This commit is the third step towards changing this and introduces a separate error (and error code) for attributes containing a text string which contains text which looks like a PHP close tag, when the text string is on the same line as the attribute opener.

Example of the problematic behaviour: https://3v4l.org/oarGV

Notes:
* This third error is will not cause parse errors and may easily go unnoticed, but can cause a security issue (code exposure) and broken functionality.
* Also note that text string parameters which is not on the same line as the attribute opener are not flagged as those have the parse error problem.
    The code exposure problem only comes into play once the parse error has been fixed as until then, the text string containing the `?>` is not seen as part of the comment in PHP < 8.0, while the incompatibility problem lies in PHP respecting close tags within comment and being able to jump out of PHP when those are encountered.

The change is covered by pre-existing test cases, one of which will now (also) throw the new error.
A small tweak has been made to the test case to allow for safeguarding properly that the sniff will throw both the `FoundMultiLine`, `FoundInline`, as well as the `FoundCloseTag` error and doesn't hide one behind the other.

Related to 1480
…parate file

These tests have to be the last tests in a file, so having them in the main file, makes updating the tests fiddly as you need to keep updating the line numbers for the tests related to the "close tag in text string" errors.

This commit splits the test case file into two files to make adding new tests to the main file more straight forward.
… a warning

Now each of the situations which cause real PHP compatibility issues are flagged as errors with their own error code, the generic "attribute found" error can be downgraded to a warning as it will only flag attributes which are cross-version compatible, though there may still be loss of functionality.

Includes a change to the message text to make it more informative.

Related to 1480
…ion compatibility

PHP itself has introduced a couple of attribute classes since PHP 8.0 to help userland code get round problematic PHP deprecations:
- `ReturnTypeWillChange`
- `AllowDynamicProperties`

As long as those attributes are not using an incompatible (multi-line, inline) syntax, using these attributes is absolutely fine and should not be flagged.

To allow for excluding warnings about these type of attributes, we need to examine which attribute classes are used within an attribute, as, if there are multiple attributes classes referenced and not all of these are the attributes listed above, the attribute should still be flagged with a warning.

This commit updates the sniff to handle this in the following manner:
* The names of all attribute classes encountered are collected first.
    Note: issue PHPCSStandards/PHPCSUtils 616 has been opened to potentially move that logic to PHPCSUtils.
* Instead of throwing one warning per attribute, the sniff will now throw a warning for each attribute class within the attribute.
* If the attribute class in question is one of the PHP native attributes for cross-version compatibility, the sniff will stay silent.
* As the warning is now thrown for each attribute class name, the warning will now also be thrown on the attribute class name.
    This means some of the warnings are now thrown on different lines.
    It also means that the warning will be shown with higher precision for the code highlighting in the PHPCS `code` report.

Notes:
* While the property holding the attribute names includes information about the PHP version which introduced the PHP native attribute, this information is not used as it is irrelevant, what with missing attribute classes not causing a fatal "can't find class" error _unless_ the attribute is being accessed.
* No namespace and import use statement resolution is executed at this time.
    This means that if an application would introduce its own `Vendor\Package\ReturnTypeWillChange` attribute class and reference it in the attribute as `#[ReturnTypeWillChange]`, there would be a false negative.
    This will be fixed in the future once the namespace and import use statement resolution feature planned for PHPCSUtils become available.

Includes tests.

Related to 1480
The `Found` warning is still very generic and would be thrown for every cross-version compatible attribute, which can make it very noisy and make the temptation to turn it off large, while at times, the warning can prevent bugs from slipping in due to it flagging functionality which will not be executed on PHP < 8.0.

So this commit attempts to make the error code a little more modular to allow for more selectively excluding the error code.

Considerations which were taken into account:
* If the error code was made _fully_ modular (`[AttributeName]Found`), it would mean that for (valid uses of) each attribute a separate ruleset exclusion would need to be added, which would make the sniff very fiddly.
* At the same time, just having the `Found` error and excluding that, would, as pointed out above, quickly ignore too much.

Ideally, we would be able to group all attributes by "source application", but at this time, that would cause a huge maintenance overhead.
Once namespace and import use statement resolution is added to the sniff (once available in PHPCSUtils), this will become easier though as groups (and error codes!) could be defined based on the namespace "prefix".

With this last bit in mind, I've currently implemented the modularization only for the most commonly adopted attribute classes.

I have looked at which applications offer attributes and whether these are (semi-)widely adopted or are expected to be widely adopted soonish, basing this in part on the Exakat articles and in part on publicly available information about dependencies from GitHub and Packagist.

For those attribute "groups", separate error codes have now been introduced which allow for selectively excluding the warning for a whole group of attributes.

Once namespace and import use statement resolution is added to the sniff, this code can be simplified a lot (no more need for the attribute class name lists, just base the error code on the namespace prefix in the FQN).
Additionally, once namespace and import use statement resolution is added to the sniff, further modularization based on namespace prefix can be added to the sniff.

This is also the reason not make the warnings remaining in the `Found` code more modular at this time as the above mentioned future change would then invalidate all previously made excludes/ignore annotations and be a breaking change in itself, while the partially modularization done now allows more easily for expanding this in the future without necessarily breaking existing excludes.

Notes:
* Even though the Exakat articles mention ShopWare, Tuleap and Contao as other common sources of attribute, based on the same articles and a dependency check, no lists/condition for these have been added as they are mostly, if not exclusively, used within their own application and not adopted by the wider field of PHP applications.

Includes tests for the additional error codes, though this needs to be verified manually until the test framework would be adjusted to test error codes.

Related to 1480

Refs:
* https://www.php.net/manual/en/reserved.attributes.php
* https://docs.phpunit.de/en/main/attributes.html
* https://github.com/JetBrains/phpstorm-attributes
* https://www.doctrine-project.org/projects/doctrine-orm/en/3.2/reference/attributes-reference.html
* https://www.exakat.io/en/adoption-of-php-8-attributes-in-2022/
* https://www.exakat.io/en/php-8-attribute-usage-in-2023/
To view:
```bash
phpcs --generator=Text --standard=PHPCompatibility --sniffs=PHPCompatibility.Attributes.NewAttributes
```
@wimg wimg merged commit 51c4d17 into develop Aug 31, 2024
@wimg wimg deleted the feature/1480-newattributes-improvements branch August 31, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NewAttributes: various improvements to make

3 participants