Forbid the addition of new magic class methods

[anton-vlasenko]

Is your feature request related to a problem?

Adding new magic methods such as __get and __set to WordPress Core should be discouraged unless necessary to fix existing dynamic class properties.
There is no good reason to use magic methods unless someone is writing an ORM library (which is not likely the case with WordPress Core).
PHP classes can almost always function just fine without them.

There have also been relatively recent examples of adding __get magic methods to new classes in WordPress versions 6.1 and 6.5 to provide backward compatibility. However, I'm not sure if that qualifies as a good reason to use magic methods in entirely new code.

Describe the solution you'd like

There could be a sniff that can take care of that (PrefixAllGlobals comes to mind).

class WP_Class {
    public function __get( $name ) { // This and similar magic methods should be detected and throw a linter error. Existing magic method usages should be allowed if necessary to fix existing dynamic class properties.
    }
}

I'm fine with this issue being closed without long explanations, as this idea may have already been explored and rejected.

Additional context (optional)

• I intend to create a pull request to implement this feature.

[jrfnl]

I think this ticket needs to be made more specific as there are more magic methods than mentioned above, including __construct(), __serialize() etc.

For the sake of discussion, let's presume the OP meant to limit the ticket to the magic __get()/__isset()/__set()/__unset() methods.

In that case, I'm in favour of the principle of this, as nearly all magic __get()/__isset()/__set()/__unset() methods I've seen implemented in WP Core are severely flawed and buggy, leading to significant problems.

Having said that, I do believe there should be (at least) one exception: final classes which don't extend.

While the magic methods implemented could still be flawed, at least there won't be any problems related to not dealing with inheritance correctly in final classes which don't extend.
The magic methods, in that case, the __get() and __isset() methods in combination with private properties, are a valid way to basically have "poor man"'s readonly properties (which we can't have properly until PHP 8.1 is the minimum). That is, of course, provided they are implemented correctly.

This exception should, IMO, not be encoded in the sniff, but should be handled via a separate error code, which can be excluded from a ruleset when desired.

It is also my opinion that this functionality should not be added to a pre-existing sniff, but should be encoded in a new sniff.
Adding this to the PrefixAllGlobals sniff is definitely out of the question as that sniff looks for something completely different.

I can also imagine a sister-sniff, which safeguards that magic methods when implemented, are always implemented in pairs for the following:

• __get() + __isset()
• __set) + __unset()
• __serialize() + __unserialize()
• __sleep() + __wakeup()
• (maybe more ?)

☝🏻 The above should probably also have an exception, i.e. when the only code within the magic method is a throw ... statement, the sister-method is not always required.

All in all, I think, this needs a proper, detailed discussion about the specs of any such sniffs. As things are, the ticket is too broadly formulated to be actionable for creating sniffs which will not be turned off by the majority.

While the discussion on the sniff can remain here, I also think that if the final specifications are generically useful enough, i.e. not too WP specific, the sniff(s) should be created in PHPCSExtra.

[anton-vlasenko]

For the sake of discussion, let's presume the OP meant to limit the ticket to the magic __get()/__isset()/__set()/__unset() methods.

This made me rethink what I actually meant. I will be more specific next time.

If a class has only the __get() magic method defined, the sniff should raise an error explaining that adding __get() to implement a read-only class property is a bad pattern and should be avoided.

There is no reason to use __get() to create a fake read-only class property as it can be replaced with a method that returns the same data.

Example: WP_Term::$data could be replaced with the WP_Term::get_raw_data() method or something similarly named (but of course this cannot be done due to BC concerns; it's just an example to explain what I mean).

While the magic methods implemented could still be flawed, at least there won't be any problems related to not dealing with inheritance correctly in final classes which don't extend.

The fact that the final class cannot be extended doesn't make introducing new magic methods more justified. For example. I see no reason to use __get() in the newly added PHP classes at all. A (fake) read-only class property can be replaced with a class method.

I can also imagine a sister-sniff, which safeguards that magic methods when implemented, are always implemented in pairs for the following:

That sounds great. I like the idea of having a complementary sniff that would enforce pairs of magic methods for existing classes (with the exception of the case with __get() that I described above).

[jrfnl]

There is no reason to use __get() to create a fake read-only class property as it can be replaced with a method that returns the same data.

While what you say is true, I don't agree that's always the best way.

Using __get() in a final class with (a limited, fixed list of) private properties for "fake readonly" allows for a future where the magic method can be removed and the property can be made properly public readonly, thus cleaning up the code base and without any need for changes on the side of the "code user", which can use $obj->property now and can still use that later.

Introducing a "getter" method, will, depending on the size of the class and the number of properties this applies to, litter the class with dozens of extra methods, which will become superfluous once the properties becomes public readonly, but those methods can never be removed as WP doesn't allow breaking changes.

I.e. using the "getter" pattern creates a huge burden of technical debt, which can never be fixed.

Even more so, if there would be some simple logic needed in the "getter"/__get() method as that can be replaced with property hooks as of PHP 8.4. Again, adding "getter" methods would prevent us from doing that clean up later.

[jrfnl]

Note: in any "normal" codebase, that, of course, wouldn't be a consideration, but it is for WP with its awkward "no-BC breaks" policy. With that in mind, a little thinking ahead, knowing what PHP features will be in reach before long, is not a bad thing.

[anton-vlasenko]

Introducing a "getter" method, will, depending on the size of the class and the number of properties this applies to, litter the class with dozens of extra methods, which will become superfluous once the properties becomes public readonly, but those methods can never be removed as WP doesn't allow breaking changes.

I see no reason to introduce new "fake" read-only properties, as class methods are better suited for retrieving data. I was referring only to new code, not existing "fake" read-only properties.

Of course, existing "fake" read-only properties likely don't need to be replaced with getter methods, and I absolutely agree that doing so would clutter the class.

I have no bias against using the readonly modifier when it becomes available for use in WordPress.

[jrfnl]

I see no reason to introduce new "fake" read-only properties, as class methods are better suited for retrieving data. I was referring only to new code, not existing "fake" read-only properties.

For new code, there is all the more reason not to clutter the codebase with nonsense getter methods.

Either way, none of this is helping to make the ticket actionable, as it is still way too vague.