Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:125775
Message-ID: <2A7CF24F-3AE3-4125-965F-C65431C42DFB@gmail.com>
Content-Type: multipart/alternative;
	boundary="Apple-Mail=_B0C0C92A-CB8F-4952-AC6A-AF0BA3FF0CA1"
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3776.700.51\))
Subject: Re: [PHP-DEV] Asymmetric visibility is a BC break
Date: Wed, 9 Oct 2024 18:20:37 +0200
In-Reply-To: <CAPyeZ4N8GMiLNVCDGxdofcy6txBhng_i2UKUGpiS5bnZ-YcYvA@mail.gmail.com>
Cc: php internals <internals@lists.php.net>
To: Valentin Udaltsov <udaltsov.valentin@gmail.com>
References: <CAPyeZ4N8GMiLNVCDGxdofcy6txBhng_i2UKUGpiS5bnZ-YcYvA@mail.gmail.com>
From: claude.pache@gmail.com (Claude Pache)


--Apple-Mail=_B0C0C92A-CB8F-4952-AC6A-AF0BA3FF0CA1
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8


> Le 9 oct. 2024 =C3=A0 17:01, Valentin Udaltsov =
<udaltsov.valentin@gmail.com> a =C3=A9crit :
>=20
> Hi, internals!
>=20
> Since writing https://externals.io/message/125740 I've realized that
> the major problem with aviz is actually simple but fundamental.
>=20
> In PHP <=3D8.0 this code is valid for an object of any user class:
>=20
> ```php
> class User
> {
>   public string $name =3D 'Bob';
> }
>=20
> $object =3D new User();
>=20
> foreach ($object as $property =3D> $value) {
>    $object->{$property} =3D 'Jack';
> }
> ```
>=20
> 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.
>=20
> Now let's jump to PHP 8.1+ with support for readonly properties.
>=20
> 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!
>=20
> In PHP 8.1+, the following User class suddenly breaks the code above:
>=20
> ```php
> class User
> {
>   public function __construct(
>       public readonly string $name =3D 'Bob',
>   ) {}
> }
>=20
> $object =3D new User();
>=20
> foreach ($object as $property =3D> $value) {
>   // Fatal error: Uncaught Error: Cannot modify readonly property =
User::$name
>   $object->{$property} =3D 'Jack';
> }
> ```
>=20
> 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".
>=20
> While not explicitly stated in changelogs, this was a BC break,
> because a changed semantic of smth that existed before is a BC break.
>=20
> 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
>=20
> I believe there are still many places where the concept of "public"
> needs to be adjusted to fully support readonly properties.
>=20
> 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:
>=20
> ```php
> final class User
> {
>   public function __construct(
>       public private(set) string $name =3D 'Bob',
>   ) {}
> }
>=20
> $object =3D new User();
>=20
> foreach ($object as $property =3D> $value) {
>   // Fatal error:  Uncaught Error: Cannot modify private(set)
> property User::$name from global scope
>   $object->{$property} =3D 'Jack';
> }
> ```
>=20
> 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.
>=20
> What can we do about it:
> 1. Explicitly introduce the concept of getter and setter visibility,
> preserve `ReflectionProperty::isPublic()` behavior from PHP <=3D8.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.
>=20
> --
> Best regards,
> Valentin


Hi Valentin,

There is no BC break, in the sense that code that worked under PHP 8.3 =
(and therefore use PHP 8.3 features only) will not break when run under =
PHP 8.4.

Of course, code that makes assumptions that are true when using PHP 8.3 =
features only, will need to be adapted as soon as PHP 8.4 features are =
used. This is unsurprising and expected.

Yes, it means that libraries that make use of Reflection are not =
automatically compatible with PHP 8.4+ without amendment. By nature, =
such libraries cannot claim to be compatible with arbitrary future =
versions of PHP.=20

That said, https://github.com/php/php-src/pull/16209 is interesting to =
have, but not mandatory. The current =
`ReflectionProperty::isPropertySet()` and =
`ReflectionProperty::isPrivateSet()` might be somewhat confusing at =
first, but they are sufficient in order to obtain the needed =
information.

=E2=80=94Claude



--Apple-Mail=_B0C0C92A-CB8F-4952-AC6A-AF0BA3FF0CA1
Content-Transfer-Encoding: quoted-printable
Content-Type: text/html;
	charset=utf-8

<html><head><meta http-equiv=3D"content-type" content=3D"text/html; =
charset=3Dutf-8"></head><body style=3D"overflow-wrap: break-word; =
-webkit-nbsp-mode: space; line-break: =
after-white-space;"><div><br></div><div><blockquote type=3D"cite"><div>Le =
9 oct. 2024 =C3=A0 17:01, Valentin Udaltsov =
&lt;udaltsov.valentin@gmail.com&gt; a =C3=A9crit :</div><br =
class=3D"Apple-interchange-newline"><div><div>Hi, =
internals!<br><br>Since writing https://externals.io/message/125740 I've =
realized that<br>the major problem with aviz is actually simple but =
fundamental.<br><br>In PHP &lt;=3D8.0 this code is valid for an object =
of any user class:<br><br>```php<br>class User<br>{<br> =
&nbsp;&nbsp;public string $name =3D 'Bob';<br>}<br><br>$object =3D new =
User();<br><br>foreach ($object as $property =3D&gt; $value) {<br> =
&nbsp;&nbsp;&nbsp;$object-&gt;{$property} =3D =
'Jack';<br>}<br>```<br><br>The same is true for Reflection API: once you =
check that `(new<br>ReflectionProperty(Foo::class, =
'property'))-&gt;isPublic()`, you can<br>safely read property and write =
to it from any scope.<br><br>Now let's jump to PHP 8.1+ with support for =
readonly properties.<br><br>A readonly property is two functionalities =
in one: write-once and<br>private set. This means that `public readonly =
$property` is actually<br>`public(get) private(set) readonly $property`. =
Although it is marked<br>as `public`, it is not public because it is not =
a symmetric public<br>property!<br><br>In PHP 8.1+, the following User =
class suddenly breaks the code above:<br><br>```php<br>class =
User<br>{<br> &nbsp;&nbsp;public function __construct(<br> =
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;public readonly string $name =3D =
'Bob',<br> &nbsp;&nbsp;) {}<br>}<br><br>$object =3D new =
User();<br><br>foreach ($object as $property =3D&gt; $value) {<br> =
&nbsp;&nbsp;// Fatal error: Uncaught Error: Cannot modify readonly =
property User::$name<br> &nbsp;&nbsp;$object-&gt;{$property} =3D =
'Jack';<br>}<br>```<br><br>In other words, the meaning of `public` has =
changed in PHP 8.1.<br>Before, it used to mean "symmetric", now it means =
"symmetric unless<br>readonly".<br><br>While not explicitly stated in =
changelogs, this was a BC break,<br>because a changed semantic of smth =
that existed before is a BC break.<br><br>Did it break anything? Of =
course it did! See:<br>- =
https://github.com/doctrine/orm/issues/10049<br>- =
https://github.com/symfony/symfony/pull/46840<br>- =
https://github.com/Ocramius/GeneratedHydrator/issues/656<br>- =
https://github.com/opis/closure/issues/129<br><br>I believe there are =
still many places where the concept of "public"<br>needs to be adjusted =
to fully support readonly properties.<br><br>Now in PHP 8.4 asymmetry =
will be made explicit and will allow users to<br>specify visibility for =
setters. However, the core issue =
remains<br>unresolved:<br><br>```php<br>final class User<br>{<br> =
&nbsp;&nbsp;public function __construct(<br> =
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;public private(set) string $name =3D =
'Bob',<br> &nbsp;&nbsp;) {}<br>}<br><br>$object =3D new =
User();<br><br>foreach ($object as $property =3D&gt; $value) {<br> =
&nbsp;&nbsp;// Fatal error: &nbsp;Uncaught Error: Cannot modify =
private(set)<br>property User::$name from global scope<br> =
&nbsp;&nbsp;$object-&gt;{$property} =3D 'Jack';<br>}<br>```<br><br>I'd =
like to draw your attention to the fact that aviz introduces a =
BC<br>break, despite saying "Backward Incompatible Changes: None. =
This<br>syntax would have been a parse error before." While the syntax =
is new,<br>it allows one to alter the old concept of public by changing =
set<br>visibility.<br><br>What can we do about it:<br>1. Explicitly =
introduce the concept of getter and setter visibility,<br>preserve =
`ReflectionProperty::isPublic()` behavior from PHP &lt;=3D8.0 and<br>add =
`ReflectionProperty::(get|set)Is(Public|Protected|Private)`<br>methods. =
I have explained all these ideas =
in<br>https://externals.io/message/125740 . If this option is chosen, =
aviz<br>will likely need to be reverted and reintroduced in PHP 8.5, =
since<br>we're already in the feature freeze period.<br>2. Proceed with =
the current approach, but clearly explain the BC break<br>in the =
changelog, and merge this =
PR<br>https://github.com/php/php-src/pull/16209 to mitigate =
reflection<br>issues as outlined in =
https://externals.io/message/125740.<br><br>--<br>Best =
regards,<br>Valentin<br></div></div></blockquote></div><br><div><br></div>=
<div>Hi Valentin,</div><div><br></div><div>There is no BC break, in the =
sense that code that worked under PHP 8.3 (and therefore use PHP 8.3 =
features only) will not break when run under PHP =
8.4.</div><div><br></div><div>Of course, code that makes assumptions =
that are true when using PHP 8.3 features only, will need to be adapted =
as soon as PHP 8.4 features are used. This is unsurprising and =
expected.</div><div><br></div><div>Yes, it means that libraries that =
make use of Reflection are not automatically compatible with PHP 8.4+ =
without amendment. By nature, such libraries cannot claim to be =
compatible with arbitrary future versions of =
PHP.&nbsp;</div><div><br></div><div>That said,&nbsp;<a =
href=3D"https://github.com/php/php-src/pull/16209">https://github.com/php/=
php-src/pull/16209</a>&nbsp;is interesting to have, but not mandatory. =
The current `ReflectionProperty::isPropertySet()` and =
`ReflectionProperty::isPrivateSet()` might be somewhat confusing at =
first, but they are sufficient in order to obtain the needed =
information.</div><div><br></div><div>=E2=80=94Claude</div><div><br></div>=
<div><br></div></body></html>=

--Apple-Mail=_B0C0C92A-CB8F-4952-AC6A-AF0BA3FF0CA1--