Re: Asymmetric visibility is a BC break

On Wed, Oct 9, 2024, 9:02 a.m. Valentin Udaltsov < udaltsov.valentin@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. >