On Wed, Oct 9, 2024, 9:02 a.m. Valentin Udaltsov < udaltsov.valen...@gmail.com> wrote:
> Hi, internals! > > Since writing https://externals.io/message/125740 I've realized that > the major problem with aviz is actually simple but fundamental. > > In PHP <=8.0 this code is valid for an object of any user class: > > ```php > class User > { > public string $name = 'Bob'; > } > > $object = new User(); > > foreach ($object as $property => $value) { > $object->{$property} = 'Jack'; > } > ``` > > The same is true for Reflection API: once you check that `(new > ReflectionProperty(Foo::class, 'property'))->isPublic()`, you can > safely read property and write to it from any scope. > > Now let's jump to PHP 8.1+ with support for readonly properties. > > A readonly property is two functionalities in one: write-once and > private set. This means that `public readonly $property` is actually > `public(get) private(set) readonly $property`. Although it is marked > as `public`, it is not public because it is not a symmetric public > property! > > In PHP 8.1+, the following User class suddenly breaks the code above: > > ```php > class User > { > public function __construct( > public readonly string $name = 'Bob', > ) {} > } > > $object = new User(); > > foreach ($object as $property => $value) { > // Fatal error: Uncaught Error: Cannot modify readonly property > User::$name > $object->{$property} = 'Jack'; > } > ``` > > In other words, the meaning of `public` has changed in PHP 8.1. > Before, it used to mean "symmetric", now it means "symmetric unless > readonly". > > While not explicitly stated in changelogs, this was a BC break, > because a changed semantic of smth that existed before is a BC break. > > Did it break anything? Of course it did! See: > - https://github.com/doctrine/orm/issues/10049 > - https://github.com/symfony/symfony/pull/46840 > - https://github.com/Ocramius/GeneratedHydrator/issues/656 > - https://github.com/opis/closure/issues/129 > > I believe there are still many places where the concept of "public" > needs to be adjusted to fully support readonly properties. > > Now in PHP 8.4 asymmetry will be made explicit and will allow users to > specify visibility for setters. However, the core issue remains > unresolved: > > ```php > final class User > { > public function __construct( > public private(set) string $name = 'Bob', > ) {} > } > > $object = new User(); > > foreach ($object as $property => $value) { > // Fatal error: Uncaught Error: Cannot modify private(set) > property User::$name from global scope > $object->{$property} = 'Jack'; > } > ``` > > I'd like to draw your attention to the fact that aviz introduces a BC > break, despite saying "Backward Incompatible Changes: None. This > syntax would have been a parse error before." While the syntax is new, > it allows one to alter the old concept of public by changing set > visibility. > > What can we do about it: > 1. Explicitly introduce the concept of getter and setter visibility, > preserve `ReflectionProperty::isPublic()` behavior from PHP <=8.0 and > add `ReflectionProperty::(get|set)Is(Public|Protected|Private)` > methods. I have explained all these ideas in > https://externals.io/message/125740 . If this option is chosen, aviz > will likely need to be reverted and reintroduced in PHP 8.5, since > we're already in the feature freeze period. > 2. Proceed with the current approach, but clearly explain the BC break > in the changelog, and merge this PR > https://github.com/php/php-src/pull/16209 to mitigate reflection > issues as outlined in https://externals.io/message/125740. > > -- > Best regards, > Valentin > Tbh we should consider voting to get rid of it, the costs are starting to outweigh the benefits (which aren't super clear to me). Cheers. >
