[PHP 8.2] Add rule for AllowDynamicProperties attribute

[mallardduck]

This rector rule accounts for the proposed RFC to deprecate implicit dynamic properties in PHP 8.2. Per discussion on twitter this rule would add the new #[AllowDynamicProperties] attribute to every class - unless it already has this attribute applied.

[samsonasik]

I think this should be configurable, and make default to false (or make to specific classes only maybe?) to avoid unnecessary change, you can implements ConfigurableRectorInterface for it, eg:

rector-src/rules/Php74/Rector/Property/TypedPropertyRector.php

Line 50 in 82de366

final class TypedPropertyRector extends AbstractRector implements ConfigurableRectorInterface, MinPhpVersionInterface

with set defalut value:

rector-src/rules/Php74/Rector/Property/TypedPropertyRector.php

Line 55 in 82de366

public const CLASS_LIKE_TYPE_ONLY = 'class_like_type_only';

rector-src/rules/Php74/Rector/Property/TypedPropertyRector.php

Line 65 in 82de366

private bool $classLikeTypeOnly = false;

the class will need a configure() method:

rector-src/rules/Php74/Rector/Property/TypedPropertyRector.php

Lines 190 to 194 in 82de366

	public function configure(array $configuration): void
	{
	$this->classLikeTypeOnly = $configuration[self::CLASS_LIKE_TYPE_ONLY] ?? false;
	$this->privatePropertyOnly = $configuration[self::PRIVATE_PROPERTY_ONLY] ?? false;
	}

[mallardduck]

@samsonasik - I'm unsure if this rector rule should even actually be a default for PHP 8.2 overall. Or one that you would manually add to your config since it's not the de-facto solution for PHP 8.2/9.0 updates.

The change of the RFC realistically has two main paths to address the BC deprecation and pending BC break. The way I see it this is the simple and easy solution, but not necessarily the "best" solution. Using a rule that detects dynamic prop usage and adds properly typed properties would be more "forward thinking" for the RFCs intent.

---

Note: This PR was going based on my interpretation of the chat I had with @afilina on twitter. So I'm mostly going on her wealth of experience and wisdom there - granted I could have misunderstood.

Initially I thought the rule should do what it can to detect when dynamic props are used and only add the attribute then. However Anna suggested just making he rule add it to all classes, see here: https://twitter.com/afilina/status/1459312382895964166

So with that in mind maybe I shouldn't have staged this rule in the Php82 space at all?

[mallardduck]

Given the intended purpose of this rector rule I'm thinking I should have created it under either: Transform/Rector/Class_ or Transform/Rector/Attribute.

[afilina]

@mallardduck

Should this rector be part of a default 8.2 upgrade? In my opinion, it shouldn't. There are different upgrade paths. The team may or may not want to address tech debt in their code at the same time as they upgrade to the latest PHP. This means that they may either opt for this quick fix, or a more involved approach using a combination of this other rector, automated tests and manual fixes.

Should we attempt to detect dynamic properties before setting the attribute? We may have this as an option, but not as the default behavior. Using static analysis, we cannot accurately guess that a class will have a dynamic property created.

[mallardduck]

Great! Thank you @afilina - i've refactored the PR a bit to put this under the Transform/Rector/Class_ rules. And reverted the creation of a PHP 8.2 specefic rule set.

Also I adjusted the name of the rector to AddAllowDynamicPropertiesAttributeRector as that felt more fitting.

[samsonasik]

I think under Transform is ok 👍

[mallardduck]

As I've been working on adjusting for feedback I've been thinking about the RFC a lot. I've re-read it and come to a new understanding for cases that this attribute shouldn't need to be applied.

Ignoring the fact that we can't reliably detect when a dynamic property is used and only apply to a class then. And the fact that a follow up PR should add configuration options to allow/exclude paths/classes from being refactored by the rule.

The main two cases this new Attribute is not necessary is:

• When the class, or its ancestor, already has the attribute applied to it.
• When the class, or its ancestor, define a __set magic method.

The first case technically covers the cases of "a class that extends stdClass". As reviewing the RFC i see it is the only internal/bundled class marked with this attribute. Meaning the rule to check for the attribute should find the attribute on stdClass as well, meaning the one checking for stdClass is a bit redundant.

---

However that in mind, I think we still need "3 checks" in rector as checking for stdClass as an ancestor of the current node is easily the most simple check to do. Mainly because we can get a list of all ancestors and check if stdClass is in the array.

In contrast it seems more complex to recursively get all parent nodes and check those for __set methods or the new attribute.

With that in mind, @samsonasik is there a helper/resolver I can use to retrieve the current nodes parent class (as a node) - then we can pass that back into the method to check for attribute/__set?

[villfa]

After reading this message from internals and looking at this piece of code, I've found another case where the attribute is not needed: the method __set() can be implemented by using a trait.

Do you think the current implementation handle it already?

[mallardduck]

found another case where the attribute is not needed: the method __set() can be implemented by using a trait.

Do you think the current implementation handle it already?

I believe i covered that under the "class, or ancestor, defines magic __set" method. While not technically an "ancestor" having that method via a trait falls on the same bucket. I believe the use of reflection to check for __set method should catch if a class has set from itself, a parent or via a trait.

Edit: butterfinger closed the PR while writing this on my phone.

[samsonasik]

Thank you @mallardduck , the RFC seems accepted, I guess it can be merged while open for new improvements ( configurable per-namespace or all, more fixture test, etc)