NewAttributes: various improvements to make

[jrfnl]

The PHPCompatibility.Attributes.NewAttributes sniff is a new sniff in PHPCompatibility 10.0.0.

While it is working correctly, I believe there are some tweaks which we should make to prevent the sniff from becoming too annoying.

This has been on my local to-do list for too long already, so I'm opening this issue to make sure it's addressed before the 10.0.0 release, especially as it came up again in a feedback thread on Twitter today.

1. Attributes which cause code loss/parse errors versus attributes which are ignored in PHP < 8.

When attributes are on a line by themselves or "trailing", they will, of course, still have no impact on PHP < 8 code, but they also won't necessarily cause any errors as they will be considered as comments in PHP < 8.

I believe it may be prudent to change the message from error to warning for those attributes and adjust the message text.
Alternatively (or in conjunction), I also think it may be good to have a separate error code for these so that these can be more easily ignored from a PHPCS ruleset.

#[ShouldBeWarning]
function foo(
    $param1, #[ShouldBeWarning]
    $param2,
    #[ShouldBeWarning]
    $param3,
    #[ShouldBeError] $param4 // This should still be an error as `$param4` will be considered commented out in PHP < 8.
) {}

2. PHP native attributes for cross-version compatibility

PHP itself has introduced a number of attributes specifically to allow for PHP cross-version compatibility by silencing deprecation notices and such. Examples: #[ReturnTypeWillChange], #[AllowDynamicProperties].

In those cases - as long as the attribute is on a line by itself or trailing -, these attributes will not cause any loss of functionality in PHP < 8.

I believe we should adjust the sniff in one or more of the following ways (to be decided, options in the comments welcome):

• Change from error to warning.
• Lower the severity to something below 5, which means the message won't show unless the user specifically passes the --severity=.. flag and sets it to the lower number.
• Have a separate error code for these attributes.
• Ignore these completely.

Mind: an attribute can contain references to multiple classes - #[AllowDynamicProperties, SomeOtherAttribute] -, so this does mean the contents of the attribute will need to be analyzed and if the attribute is not stand-alone the "default" behaviour for the sniff should prevail.

There are also a number of typical PHPStorm attributes which we may also want to treat in this same way (with their own error code). Also see: https://www.exakat.io/en/adoption-of-php-8-attributes-in-2022/

We may also want to have a look at the PHPUnit 10.0 attributes and treat them in a similar way. Ref: https://phpunit.de/announcements/phpunit-10.html

[Luc45]

Just noting that I stumbled upon this issue looking for this:

as long as the attribute is on a line by itself or trailing -, these attributes will not cause any loss of functionality in PHP < 8.

If we could differentiate between Attributes that cause syntax errors in PHP 7 and those that don't, that would be a great improvement.

I vote for this: Have a separate error code for these attributes. - which should be a Warning by default. This way, the developer has the option to ignore it if they want.

[Luc45]

Something like BackwardsCompatibleAttribute found.

[Luc45]

I'm GPT'ing my way through this to try to come up with a PR.

[Luc45]

I won't be able to come up with the PR, but maybe something like this?

protected function isBackwardsCompatibleAttribute($phpcsFile, $stackPtr, $tokens)
    {
        $currentLine = $tokens[$stackPtr]['line'];

        // Check if the attribute starts the line (ignoring whitespace)
        $startOfLineToken = $phpcsFile->findPrevious(T_WHITESPACE, ($stackPtr - 1), null, true);
        if ($tokens[$startOfLineToken]['line'] < $currentLine) {
            // The attribute starts a new line
            $attributeCloser = $tokens[$stackPtr]['attribute_closer'];

            if ($tokens[$attributeCloser]['line'] == $currentLine) {
                // The attribute closes on the same line, now check for PHP closing tags.
                for ($i = $stackPtr; $i <= $attributeCloser; $i++) {
                    if ($tokens[$i]['code'] === T_CLOSE_TAG) {
                        // Found a closing PHP tag within the attribute, so it's not backwards compatible.
                        return false;
                    }
                }

                // No closing PHP tag found in the attribute, so it's likely backwards compatible.
                return true;
            }
        }

        // If the attribute does not start its own line, is multi-line, or contains a closing PHP tag, it's not backwards compatible.
        return false;
    }

PS: Edited to account for "?>" in attribute contents.

[jrfnl]

@Luc45 I appreciate your willingness to try, but know this:

I'm GPT'ing my way through this to try to come up with a PR.

GPT-ed PRs are not welcome as they will be incorrect 100% of the time and cost reviewers too much time because of it as the person pulling the PR doesn't understand the code (as otherwise they wouldn't need GPT), so it takes five times as much time and text to explain what's wrong and then you still have to hope the OP will understand the explanation....

[MarkMaldaba]

I'm hitting this at the moment, with the #[\ReturnTypeWillChange] being reported as a problem, despite the fact it is only present in order to fix cross-compatibility issues.

I don't particularly want to ignore PHPCompatibility.Attributes.NewAttributes.Found as it might hide genuine attribute errors that could crop-up in the future, with attributes that affect functionality in some important way.

My initial thought was that this should be reported as PHPCompatibility.Attributes.NewAttributes.Found.ReturnTypeWillChange which makes it trivial to globally ignore specific attributes, or to target specific attributes in a given file, without masking other things that you may need to be made aware of. In terms of your question about multiple attributes in the same block, I would expect each to be raised separately (so #[AllowDynamicProperties, SomeOtherAttribute] would result in two notifications: PHPCompatibility.Attributes.NewAttributes.Found.AllowDynamicProperties and PHPCompatibility.Attributes.NewAttributes.Found.SomeOtherAttribute).

I agree with your first point in that attributes which cause backwards-compatibility breakages (i.e. where live code would end up being treated as part of a comment) should trigger an error, but I would not want those to be suppressed if I was ignoring that specific attribute. Maybe a separate error should be raised, such as PHPCompatibility.Attributes.AttributesPrecedingCode.Found independently of the notification about the fact that attributes are being used at all.

Having looked at your alternatives, I think this is my preferred implementation as it makes it possible to ignore specific attributes without ignoring errors introduced by new syntax.

As a side note, I am longing for the ability to wild-card rules in PHPCS, which would really help in this kind of situation, e.g. by excluding PHPCompatibility.Attributes.NewAttributes.Found.*. I can't remember whether that's something that's on the road map or something that's been dismissed as not planned, but I for one would find it incredibly useful in a number of situations, including this.