-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHP 8.2: dynamic properties are deprecated #3489
Comments
I like the option 6. |
Hello lovely people,
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:
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 |
@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 |
@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. 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. |
Right. If support for 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 (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.) |
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 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 |
Agree on option 6 being the current best way forward. |
running sniffers on PHP 8.1 I'm getting Option 6 looks good to go |
@andypost Not sure what you mean ? https://3v4l.org/sg5R0 |
@jrfnl interesting, probably it's a custom Drupal sniffers throwing it, I will dig it deeper tomorrow |
Sorry this message is from phpstan( |
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. |
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
I assume this means the Can you share the source of it? I could not find where that came from. I appreciate your time on this. |
The source is https://wiki.php.net/rfc/deprecate_dynamic_properties |
Thanks for the answer, but the rfc does not include plan to remove |
@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:
If that were to happen then it's at least a few years away and purely hypothetical at this time. |
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 |
Are there any news on the topic? |
@maxbethke PR #3629 is open implementing option 6 and is planned to be included in PHPCS 3.8.0. |
So, now what? How can I change my code below to fit the new rules?
|
@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. |
Is there any world where |
@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 If you see any other deprecation notices which are coming from PHPCS itself, please report them, so they can get fixed. |
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). |
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:
__get()
and__set()
methods.stdClass
(which implements the magic__get()
and__set()
methods).#[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 thephpcs: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: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 ?
#[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.
__get()
and__set()
methods to theSniff
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.
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
toSniffName 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.
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
toSniffName 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.
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.
... 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
#[AllowDynamicProperties]
attribute to theSniff
interface, for it to be inherited by implementing classes, is not an option as it will result in aFatal error: Cannot apply #[AllowDynamicProperties] to interface
.Proposal
All things considered, I'd like to propose implementing option 6 for the following reasons:
Opinions ?
The text was updated successfully, but these errors were encountered: