PHP 8.2: dynamic properties are deprecated

jrfnl commented 3 years ago

Recently, the Deprecate dynamic properties RFC has been approved for PHP 8.2 and as I expected it will cause havoc, I've started doing some test runs against various repos, including PHPCS.

The test run against PHPCS only went to proof my suspicion as this deprecation (or rather the fatal error this will become in PHP 9.0) breaks currently supported functionality in PHPCS.

Context

The deprecation of dynamic properties basically means that any property which is not explicitly declared in a class, but is being get/set will now yield a "Creation of dynamic property ClassName::$propertyName is deprecated" deprecation notice, which will become a fatal error in PHP 9.0.

There are three exceptions to this:

Dynamic properties are still supported when the class implements the magic __get() and __set() methods.
Dynamic properties are still supported when a class extends stdClass (which implements the magic __get() and __set() methods).
Dynamic properties are still supported when a class is given the #[AllowDynamicProperties] attribute, though this attribute is expected to only be supported for a limited time (until ~PHP 9.0) as the intention is to eventually remove support for dynamic properties completely from PHP (with the exception of the above two situations).

What problem does this cause in PHPCS ?

In PHPCS, the value of public properties can be adjusted from within a ruleset and from within (test) files using the phpcs:set annotation.

While this feature is mostly used for adjusting the properties for individual sniffs, there are a number of (external) standards which use the same property in a multitude of sniffs.

As things are, PHPCS currently explicitly supports setting the value for a (public) property for all sniffs in a category/standard in one go, like so:

    <rule ref="PSR1">
        <properties>
            <property name="setforallsniffs" value="true" />
        </properties>
    </rule>

This is also documented and safeguarded as such via the RuleInclusionTest.

The deprecation/removal of support for dynamic properties breaks this functionality.

What are our options ?

Add the #[AllowDynamicProperties] attribute to every sniff. Impact: HIGH for sniff maintainers, no impact for users. Breaking change: No Stable: No. The attribute would also need to be added to every single sniff out in the wild in external standards, so this will break as soon as an external standard does not have the attribute. It will also break (again) when support for the attribute is removed from PHP.
Add the magic __get() and __set() methods to the Sniff interface. Impact: HIGH for sniff maintainers, no impact for users. Breaking change: Yes as every single sniff class, both internal and external, would need to implement these methods. This change could only be made for PHPCS 4.0, but will allow for standards to be cross-version compatible with PHPCS 3.x and 4.x without extra effort. Stable: Yes, for those standards which choose to support PHPCS 4.x and make this change.
Add a new AbstractSniff base class which includes the magic __get() and __set() methods. Impact: HIGH for sniff maintainers, no impact for users. Breaking change: Yes as the class declaration for every single sniff, both internal and external would need to be updated from SniffName implements Sniff to SniffName extends Sniff. This change could only be made for PHPCS 4.0 and will make it a lot more complicated for standards to be cross-version compatible with PHPCS 3.x and 4.x (for those who desire this). Stable: Yes, for those standards which choose to support PHPCS 4.x and make this change.
Make all sniffs extend stdClass. Impact: HIGH for sniff maintainers, no impact for users. Breaking change: No The class declaration for every single (abstract) sniff, both internal and external would need to be updated from SniffName implements Sniff to SniffName extends stdClass implements Sniff, but this is not enforced by PHPCS, so standard maintainers can implement this whenever they are getting ready to support PHP 8.2. This change would also be cross-version compatible with both PHPCS 3.x as well as 4.x (for those who desire this). Stable: Yes, for those standards which choose to make this change.
Remove support for setting properties for categories/standards in one go. Impact: none for sniff maintainers, MEDIUM impact for users. Any custom ruleset/standard which currently uses this feature will need to be adjusted and for those standards where this feature is commonly used, this will mean fiddly ruleset adjustments - the property would need to be explicitly set for every sniff using it, new sniffs added to a standard would not automatically receive the property anymore etc While this feature may not be used in a huge amount of standards, for those standards - and their users - where it is used, it will cause a lot of churn in continuous ruleset adjustments now and in the future. Breaking change: Yes A feature which has always been supported would now be removed. Stable: No, the need to continuously update rulesets whenever new sniffs using common properties come out, make this unstable for end-users.
Only set properties received from a ruleset when the property exists on a sniff. ... and throw an informative error when a ruleset attempts to set a property on a sniff which doesn't have that property. The error message should only be thrown when the property is being set on an individual sniff, not when the property is (attempted to be) set for a standard/category of sniffs. Impact: LOW/MEDIUM for sniff maintainers, LOW impact for users. Breaking change: Yes, sort of. While probably rare, there may be a few standards out there relying on "magic dynamic" properties which may or may not be available to the sniff depending on the ruleset under which a sniff is run. Those standards would break with this change, but that break can be mitigated by the standard maintainers by either adding the magic __get()/__set()/__isset() methods or making the property/properties explicit on each sniff. Stable: Yes, sort of and the informative error message for typos in properties would be in line with the "goal" of the PHP RFC.

What will not work

Adding the #[AllowDynamicProperties] attribute to the Sniff interface, for it to be inherited by implementing classes, is not an option as it will result in a Fatal error: Cannot apply #[AllowDynamicProperties] to interface.

Proposal

All things considered, I'd like to propose implementing option 6 for the following reasons:

The change would not be bound to PHPCS 4.x, which would put pressure on the roadmap for PHPCS 4.x as it would then be closely tied to the PHP 8.2 release schedule,
The change will yield improved error messages for users with typos in property names in custom ruleset, which is in line with the PHP RFC and would help end-users of PHPCS.
While it does have the potential to break (a limited set of) external standards, I suspect this will be exceedingly rare and that far more standards will benefit from the continued support for setting properties for a category/standard in one go.

Opinions ?

kukulich commented 3 years ago

I like the option 6.

Danack commented 3 years ago

Hello lovely people,

though this attribute is expected to only be supported for a limited time (until ~PHP 9.0)

That's not my understanding.

The RFC read to me as if that decision is deliberately not made yet, and would only be made at a later date:

We may still wish to remove dynamic properties entirely at some later point. Having the #[AllowDynamicProperties] attribute > will make it much easier to evaluate such a move, as it will be easier to analyze how much and in what way dynamic properties are used in the ecosystem.

So the current expectation should probably be for AllowDynamicProperties to exist for a reasonable amount of time, and it might go away at some point, but is a separate decision, that wouldn't be thought about until for a while.

Removing AllowDynamicProperties would also of course require another RFC. As the "Deprecate dynamic properties RFC" only just passed, it also seems unlikely that removing the AllowDynamicProperties annotation would pass.

mallardduck commented 3 years ago

@Danack - I expressed similar thoughts to @jrfnl on twitter, I appreciate someone with more experience sharing thoughts around that too. From my rough estimate PHP 9.0 is roughly 2+ years away. So unless my novice level understanding of PHP internals is wrong, any discussion of removing dynamic properties completely would be at least 2 years away. Does that seem right?

Even then it seems there would have to be a deprecation of AllowDynamicProperties (9.1 and at least another year) and full remove would probably have to be done in a major iteration potentially (10.x)? If so, there's no telling how many minor 9.x releases would be created right? So it could be additional years on top of that before 10.x comes. I ask because I'm genuinely curious about getting more involved in internals work, so hoping to understand the process better.

jrfnl commented 3 years ago

@Danack @mallardduck I appreciate your thoughts, but my experience is different.

Things which have been deprecated previously, will be removed in most cases on the next major - i.e. PHP 9.0. The fact that the attribute would also need to be removed is no reason for that not to happen as removing things can happen without consequence in majors and there's precedent for similar removals without prior deprecation as recently as in PHP 8.0.

Aside from that, adding the attribute would have to be done on all individual sniffs/abstracts sniffs if sniffs extend. And as there are plenty of external standards out there, there's bound to be a standard/sniff which will be "missed" in those updates. Guess where most end-users report problems ? Right. In this repo. Doesn't matter that it is a case of sending them to the right repo and closing the issue, it is still maintainer time being wasted.

I, for one, would much rather spend my time on solving this once and for all instead of generating more work for maintainers of external standards and for the maintainers of this repo, which will drag on for years to come.

nikic commented 3 years ago

@Danack - I expressed similar thoughts to @jrfnl on twitter, I appreciate someone with more experience sharing thoughts around that too. From my rough estimate PHP 9.0 is roughly 2+ years away. So unless my novice level understanding of PHP internals is wrong, any discussion of removing dynamic properties completely would be at least 2 years away. Does that seem right?

Even then it seems there would have to be a deprecation of AllowDynamicProperties (9.1 and at least another year) and full remove would probably have to be done in a major iteration potentially (10.x)? If so, there's no telling how many minor 9.x releases would be created right? So it could be additional years on top of that before 10.x comes. I ask because I'm genuinely curious about getting more involved in internals work, so hoping to understand the process better.

Right. If support for AllowDynamicProperties were to be dropped (which is speculative at this point), then that would happen via a deprecation in 9.x and removal in 10.0.

Of course, solving this in a way that does not require allowing dynamic properties for everything is always preferred, if not too hard to realize :)

jrfnl commented 3 years ago

Of course, solving this in a way that does not require allowing dynamic properties for everything is always preferred, if not too hard to realize :)

@nikic Please see my extensive write-up above: that would constitute removing an actively used and fully supported feature from PHPCS. IMO that's just not an option.

nikic commented 3 years ago

Of course, solving this in a way that does not require allowing dynamic properties for everything is always preferred, if not too hard to realize :)

@nikic Please see my extensive write-up above: that would constitute removing an actively used and fully supported feature from PHPCS. IMO that's just not an option.

Possibly a misunderstanding -- I was under the impression that option 6 is both acceptable in terms of BC and does not use AllowDynamicProperties.

(Ultimately I have no opinion on this issue, as I neither maintain nor use this project -- I just wanted to clarify the development process, so people do not make incorrect assumptions.)

mallardduck commented 3 years ago

Thank you for weighing in there @nikic - as I'm just going based on my interpretation of your words in the RFC. While I understand the concern of not just "kicking the can down the road" that @jrfnl is expressing. I appreciate you pointing out and being clear that any talk of removing that attribute in PHP 9.0 is purely speculative.

I would agree that overall option 6 seems like a good route if avoiding the use of AllowDynamicProperties is desired. For this use case specifically it does seem unfortunate that we cannot apply this attribute to an interface - since it seems like if we could do so here then then Sniff interface could have it applied there. Given that nothing is set in stone for PHP 8.2 yet I'm wondering if that would be possible?

I suppose there may be downsides to allowing the attribute to be used on interfaces. However I admit in this moment I can't really think of any critical ones. I guess if the class implements multiple interfaces with the trait it may be applied redundantly in that case? And overall it could be seen as odd inheriting that level of behavior from an interface. 🤔

On the flip side I've already created a rector/rector-src branch that includes a rule to remove the attribute. And can do so based on a configurable namespace based list of classes to check for removing the attribute.

So even though removing and deprecating this new attribute is purely speculation, and even though there's little chance this happens before PHP 10.x - this should help. IMO having those tools/rules should make Option 1 viable even if temporary. So if that is the selected choice, then this rule will help with those changes: https://github.com/mallardduck/rector-src/tree/php82-remove-allow-dynamic-attribute

gsherwood commented 3 years ago

Agree on option 6 being the current best way forward.

andypost commented 2 years ago

running sniffers on PHP 8.1 I'm getting Attribute class AllowDynamicProperties does not exist. so there should be a way to detect this attribute in earlier PHP versions

Option 6 looks good to go

jrfnl commented 2 years ago

@andypost Not sure what you mean ? https://3v4l.org/sg5R0

andypost commented 2 years ago

@jrfnl interesting, probably it's a custom Drupal sniffers throwing it, I will dig it deeper tomorrow

Ref https://www.drupal.org/pift-ci-job/2363989

andypost commented 2 years ago

Sorry this message is from phpstan(

jrfnl commented 2 years ago

I have opened PR #3629 to address this. The PR implements option 6 with some caveats. Please see the extensive write-up in the PR description for full details.

the-csaba commented 2 years ago

Hi there @jrfnl,

Thank you for the work on this project. It gives a lot of value to the whole PHP community.

I just stumbled on this issue because we are facing the exact dilemma. And we are thinking of adding the #[AllowDynamicProperties] attribute. But the explanation option 1 (adding #[AllowDynamicProperties]) includes this:

It will also break (again) when support for the attribute is removed from PHP.

I assume this means the #[AllowDynamicProperties] is a temporary solution.

Can you share the source of it?

I could not find where that came from.

I appreciate your time on this.

andypost commented 2 years ago

The source is https://wiki.php.net/rfc/deprecate_dynamic_properties

the-csaba commented 2 years ago

@andypost

The source is https://wiki.php.net/rfc/deprecate_dynamic_properties

Thanks for the answer, but the rfc does not include plan to remove [AllowDynamicProperties] in the future

mallardduck commented 2 years ago

@andypost & @om4csaba - As discussed above by Nikita there is no real plan to remove that attribute yet. However it could be proposed to be removed at any point someone wants to RFC that. As nikita says here:

Right. If support for AllowDynamicProperties were to be dropped (which is speculative at this point), then that would happen via a deprecation in 9.x and removal in 10.0. https://github.com/squizlabs/PHP_CodeSniffer/issues/3489#issuecomment-980794876

If that were to happen then it's at least a few years away and purely hypothetical at this time.

the-csaba commented 2 years ago

Hi @mallardduck,

Thank you for this.

I missed Nikita's comments, which confirm my assumption: no planned removal, i.e. stable as any feature in the language. Change may or may not introduce in the future.

I apologise for hijacking the conversation.

Cheers

maxbethke commented 1 year ago

Are there any news on the topic? Has a consensus been reached to implement Option 6? If yes, is an ETA possible?

jrfnl commented 1 year ago

@maxbethke PR #3629 is open implementing option 6 and is planned to be included in PHPCS 3.8.0.

IgorArnaut commented 1 year ago

So, now what? How can I change my code below to fit the new rules?

private $id;
private $naziv;
private $sifra;
private $cena;
private $kolicina;

public function __construct($id, $naziv, $sifra, $cena, $kolicina)
{
    $this->id = $id;
    $this->naziv = $naziv;
    $this->sifra = $sifra;
    $this->$cena = $cena;
    $this->kolicina = $kolicina;
}

jrfnl commented 1 year ago

@IgorArnaut This ticket is only about the impact of the dynamic property deprecation on the code base of PHP_CodeSniffer and external standards for PHPCS.

Your code doesn't look like PHPCS related code. Please open an issue in your own repo to discuss how to solve your issue there.

magnetik commented 1 year ago

Is there any world where error_reporting(E_ALL^E_DEPRECATED); is added in the phpcs bootstrap to avoid errors on PHP 8.2 ?

jrfnl commented 1 year ago

@magnetik I'm not sure I understand your question. As you can read in the above discussion, there is a PR open to solve the issue we identified regarding dynamic properties.

In the mean time, until that PR is in a release, if you run into this issue, you can run PHPCS with -d error_reporting=E_ALL~E_DEPRECATED to silence the notices.

If you see any other deprecation notices which are coming from PHPCS itself, please report them, so they can get fixed.

jrfnl commented 12 months ago

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).

squizlabs / PHP_CodeSniffer