Squiz.NamingConventions.ValidVariableName.NotCamelCaps + property promotion = false positive

[sampsasaarela]

Describe the bug

This bug was originally reported here: squizlabs/PHP_CodeSniffer#3564

Forgive me if there is already a ticket for this or if it should not reported again here. But I couldn't find anything related to this repo.

Property promotions handling like variables, but they are class property

Code sample

<?php declare(strict_types = 1);

namespace Tests;

class Test {
    public string $normal_property = 'abc';

    public function __construct(
        public string $promoted_property = 'abc',
    ) {
        // empty
    }
}

Custom ruleset

<?xml version="1.0"?>
<ruleset name="My Custom Standard">
  <rule ref="Squiz.NamingConventions.ValidVariableName.NotCamelCaps"/>
</ruleset>

To reproduce Steps to reproduce the behavior:

1. Create a file called test.php with the code sample above...
2. Run phpcs test.php ...
3. See error message displayed

----------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------
 9 | ERROR | Variable "promoted_property" is not in valid camel caps format
----------------------------------------------------------------------------

Expected behavior

No error.

Versions (please complete the following information):

• OS: Ubuntu and OSx
• PHP: 8.1.28
• PHPCS: 3.10.1

[jrfnl]

@sampsasaarela Thank you for reporting this. Porting tickets which are still valid (and viable) over is fine.

I don't think this is a false positive though, as promoted properties are technically both a parameter as well as a property: https://3v4l.org/N9Bvg

With this in mind, they should, strictly speaking, comply with the naming conventions for both parameters as well as properties, which depending on your coding standard, can lead to conflicts (like I assume it does in yours).

The primary solution for this, in my opinion, would be to have a consistent coding standard with naming convention rules for parameters and properties which are compatible with each other.

On a technical (sniff) level, the only thing I can think off, would be to have a separate error code for parameters which are also properties, but the sniff doesn't even differentiate between "ordinary" variables and function parameters, so introducing a separate error code for promoted properties, while not differentiating between parameters and other variables, feels off to me.

[jrfnl]

P.S.: preferably only port your own tickets over. I just noticed the original ticket was not created by you.

[sampsasaarela]

I don't think this is a false positive though, as promoted properties are technically both a parameter as well as a property: https://3v4l.org/N9Bvg

It would be a valid error in your example, but in this example, it is not used as a variable, only as a property, so I would consider it a false positive.

In this case, the solution is to ignore the error or set variables in an old-fashioned way without property promotion.

On a technical (sniff) level, the only thing I can think off, would be to have a separate error code for parameters which are also properties, but the sniff doesn't even differentiate between "ordinary" variables and function parameters, so introducing a separate error code for promoted properties, while not differentiating between parameters and other variables, feels off to me.

I feel it should do exactly that. But of course, it was not my decision, and I am not sure how hard it would be to implement. I just wanted to point this out after I found the original ticket and hit the same issue and it feels wrong. Now I just manually set properties from function args without promotion to make sniff happy.

The primary solution for this, in my opinion, would be to have a consistent coding standard with naming convention rules for parameters and properties which are compatible with each other.

I agree, but unfortunately, it is not possible in our case.

P.S.: preferably only port your own tickets over. I just noticed the original ticket was not created by you.

Sure, I just decided to copy the original to make sure it is referenced if someone else finds it from Google (as I did).

[jrfnl]

On a technical (sniff) level, the only thing I can think off, would be to have a separate error code for parameters which are also properties, but the sniff doesn't even differentiate between "ordinary" variables and function parameters, so introducing a separate error code for promoted properties, while not differentiating between parameters and other variables, feels off to me.

I feel it should do exactly that. But of course, it was not my decision, and I am not sure how hard it would be to implement.

It's not about how hard it is to implement. The problem with making any change like this, is that a change in error code is a breaking change for any user which references the error code in their ruleset.

Now, for promoted properties, that could be justified up to a point, what with it being a (relatively) new PHP feature, but for parameters, that's a whole different matter.

[sampsasaarela]

Is it breaking change? For me, it looks like expected behaviour;

public function __construct(
        public string $promoted_property = 'abc',
) {
    // No error expected; it is property-only
}

public function __construct(
        public string $promoted_property = 'abc',
) {
    // No error expected; it is property-only
   echo 'dump';
}

public function __construct(
        public string $promoted_property = 'abc',
) {
    // No error expected; it is property-only
   echo $this->promoted_property;
}

public function __construct(
        public string $promoted_property = 'abc',
) {
  echo $promoted_property; // error expected, it is used as local variable
}

public function __construct(
        string $promoted_property = 'abc',
) {
  // error expected, it is local variable only
}

Technically it is the same as doing, it just shortcut.

public string $promoted_property;

public function __construct(
        string $promotedProperty = 'abc',
) {
    $this->promoted_property = $promotedProperty;
}