phpstan 1.12.0 introduces final classes, breaking ReadWriteConfigPropertiesRule

[JamesDPC]

Hi

phpstan 1.12 made the class PHPStan\Rules\DeadCode\UnusedPrivatePropertyRule final.

Here's the change which mentions v2: phpstan/phpstan-src@4010b73

This affects ReadWriteConfigPropertiesRule: Class Syntro\SilverstripePHPStan\Rule\ReadWriteConfigPropertiesRule cannot extend final class PHPStan\Rules\DeadCode\UnusedPrivatePropertyRule

I don't know if there is an easy fix for this? workaround is to use 1.11 for now

Thanks
James

[Cambis]

Perhaps the solution here is to remove the ReadWriteConfigPropertiesRule? The @config annotation is no longer recommended by Silverstripe https://github.com/silverstripe/developer-docs/pull/268/files#diff-4279843c08150f597889cfdc1a46f91bcfbae036ffa0a9367e97ca2559471379

Configuration properties are just any private static property that don't have the @internal annotation

[wernerkrauss]

That broken ReadWriteConfigPropertiesRule adds a hint for devs, that @config is missing.
PHPstan still expects @config for ignoring Silverstripe's config properties. So this hint is still worth being printed out. (I wrote a configurable rector rule to add this type hints in many cases, see https://github.com/wernerkrauss/silverstripe-rector )

What could we do? At a first look I didn't see any method to add notices to error messages.
We could also write our own extended copy of UnusedPrivatePropertyRule, which has the disadvantage, that it won't update if the original rule recieves bugfixes.

[JamesDPC]

Is it possible for the ReadWriteConfigPropertiesRule rule to not extend \PHPStan\Rules\DeadCode\UnusedPrivatePropertyRule ?

At the moment, the only way to use this module alongside phpstan and rector is to set a version constraint of phpstan to ~1.11.0 and rector to 1.2.4 (1.2.5 requires phpstan ^1.12.2)

I see no issue with removing the ReadWriteConfigPropertiesRule, given the removal of the documentation linked by @Cambis

[wernerkrauss]

Afair @config didn't have a meaning in Silverstripe, as every private static was and is seen as a config value. When I started using PHPStan that hint message helped me a lot.
If you decide to remove that class, then please add that hint in the documentation or a FAQ or whatever.

We could also write our own rule, pass processNode() to PHPstan and adjust the error array. What do you think about that?

[Cambis]

I'd be keen to remove the ReadWriteConfigPropertiesRule completely and modify the existing behaviour of ReadWritePropertiesExtension.

Currently the ReadWritePropertiesExtension checks for the @config annotation on a property. We can update this to just check if the property is private static and it doesn't have a the @internal annotation. Same behaviour as Silverstripe.

I made a branch with these changes a while back, so I can put in a PR to action this.

For a short term solution, you can still use this extension but create your own version of the phpstan.neon file and remove the ReadWriteConfigPropertiesRule from there.

# phpstan.neon

- # Rule informing the user to add the @config docblock
-    -
-        class: Syntro\SilverstripePHPStan\Rule\ReadWriteConfigPropertiesRule
-        arguments:
-            alwaysWrittenTags: %propertyAlwaysWrittenTags%
-            alwaysReadTags: %propertyAlwaysReadTags%
-            checkUninitializedProperties: %checkUninitializedProperties%
-       tags:
-           - phpstan.rules.rule

Otherwise you can use the extension I've been developing as an alternative. You can find it on packagist

[mleutenegger]

Finally back at it after a year-long hiatus. After running into the problem myself (finally 😬 ) and reading up on it, I have two things which should be considered.

First: As stated by @Cambis Silverstripe does not require the @config tag anymore, so the rule could actually be removed. But imho there is also a value in these tags, as they immediately state a property as something that changes something at runtime by recieving a value from a yml-file. This might not be easily inferred by just going with the private static rule and benefits working on the same codebase in a team.

Second: Also, the @config tag is used to remove the Static property Model\Person::$db is never read, only written. messages once correctly tagged.

The second Issue might be easily solved by just ignoring all private static properties, but this would also allow for actual mistakes / forgotten properties to slip by, because not all private static properties have to be config variables.

In the end, this boils down to the question: should this module require the @config tag in order to improve readability of code while not following Silverstripe doctrine, should it follow Silverstripe doctrine and completely ignore all private static properties and allowing unused properties to remain uncommented or should it hand the decision over to the developer via configuration.

As a quick fix, I will push a release which has the problematic rule removed, but still uses the @config tag to ignore the never read, only written. error / hint. Additionally, as @wernerkrauss requested, there is a Hint in the readme.

I'll leave this Issue opened for the time beeing, so there can be a discussion on the topic.