Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:125772
Precedence: bulk
MIME-Version: 1.0
Date: Wed, 9 Oct 2024 18:01:12 +0300
Message-ID: <CAPyeZ4N8GMiLNVCDGxdofcy6txBhng_i2UKUGpiS5bnZ-YcYvA@mail.gmail.com>
Subject: [PHP-DEV] Asymmetric visibility is a BC break
To: php internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
From: udaltsov.valentin@gmail.com (Valentin Udaltsov)

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