[PHP-DEV] Asymmetric visibility is a BC break

Hi, internals!

Since writing Asymmetric visibility Reflection API problems - Externals 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:

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:

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:
- PHP readonly properties and inheritance cause error on hydration · Issue #10049 · doctrine/orm · GitHub
- [ExpressionLanguage] Revert readonly flag on ConstantNode::$isNullSafe by fmata · Pull Request #46840 · symfony/symfony · GitHub
- Cannot hydrate readonly properties · Issue #656 · Ocramius/GeneratedHydrator · GitHub
- Is there a workaround serializing readonly properties? · Issue #129 · opis/closure · GitHub

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:

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
Asymmetric visibility Reflection API problems - Externals . 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
Implement ReflectionProperty::is{Readable,Writable}() by iluuu1994 · Pull Request #16209 · php/php-src · GitHub to mitigate reflection
issues as outlined in Asymmetric visibility Reflection API problems - Externals.

--
Best regards,
Valentin

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:

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:

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:

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:

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.

I have to admit I understood nothing from your email, but I got
curious about some of your words.

A readonly property is two functionalities in one: write-once and

private set.

What do you mean it is private set? Readonly only means the property
is writable once. It does not affect its visibility.

While the syntax is new,

it allows one to alter the old concept of public by changing set
visibility.

Isn't that the whole point of asymmetric visibility? If you use the
new syntax, you can change what public means. Code that doesn't use it
will function as it did before. What is the BC break then?

Le 9 oct. 2024 à 17:01, Valentin Udaltsov udaltsov.valentin@gmail.com a écrit :

Hi, internals!

Since writing Asymmetric visibility Reflection API problems - Externals 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:

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:

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:

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:

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
    Asymmetric visibility Reflection API problems - Externals . 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
    Implement ReflectionProperty::is{Readable,Writable}() by iluuu1994 · Pull Request #16209 · php/php-src · GitHub to mitigate reflection
    issues as outlined in Asymmetric visibility Reflection API problems - Externals.


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.

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.

—Claude

On 09.10.2024 19:08, Kamil Tekiela <tekiela246@gmail.com> wrote:

I have to admit I understood nothing from your email, but I got
curious about some of your words.

Hi, Kamil! I tried my best :slight_smile: Thank you for your interest!

> A readonly property is two functionalities in one: write-once and
private set.

What do you mean it is private set? Readonly only means the property
is writable once. It does not affect its visibility.

It in fact does affect visibility. For instance, you cannot write
uninitialized public property from global scope:

The same fact is mentioned in the aviz RFC:
https://wiki.php.net/rfc/asymmetric-visibility-v2#:~:text=The%20readonly%20flag%2C%20introduced%20in%20PHP%208.1%2C%20is%20really%20two%20flags%20in%20one

--
Valentin

On 09.102024 at 19:20 Claude Pache <claude.pache@gmail.com> wrote:

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.

Hi, Claude!

Thank you for the explanation. I now get why aviz does not break BC :slight_smile:
Until now, I was worried that we were missing something important.

That said, Implement ReflectionProperty::is{Readable,Writable}() by iluuu1994 · Pull Request #16209 · php/php-src · GitHub 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.

Yes, they are sufficient, but very difficult to work with. Just to
check that property is writable from global scope, you have to do
`isPublic() && !isReadonly() && !isPrivateSet() && !isProtectedSet()
&& (!isVirtual() || hasHook(PropertyHookType::Set))`.
See our discussion with Ilija:

--
Valentin

On Thu, Oct 10, 2024, at 15:32, Valentin Udaltsov wrote:

On 09.102024 at 19:20 Claude Pache <claude.pache@gmail.com> wrote:

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.

Hi, Claude!

Thank you for the explanation. I now get why aviz does not break BC :slight_smile:

Until now, I was worried that we were missing something important.

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.

Yes, they are sufficient, but very difficult to work with. Just to

check that property is writable from global scope, you have to do

`isPublic() && !isReadonly() && !isPrivateSet() && !isProtectedSet()

&& (!isVirtual() || hasHook(PropertyHookType::Set))`.

See our discussion with Ilija:

https://github.com/php/php-src/issues/16175#issuecomment-2389966021

Valentin

Hello all,

I am still struggling to understand how this isn’t a BC break when it most obviously is. Sure, code that worked on 8.3 will continue to work on 8.4. In that case, we could have argued that my function autoloading RFC didn’t have a BC break, because proper implementations wouldn’t have broken. To me, this is along the same lines. A “proper” implementation won’t break, but there may be subtle ways that “improper” implementations will break and thus it should be considered a BC break.

— Rob

On Fri, Oct 11, 2024, at 12:20, Jonathan Vollebregt wrote:

A “proper” implementation won’t break, but there may be subtle ways that “improper” implementations will break and thus it should be considered a BC break.

This thread is fallaciously equating breaks in third-party libraries

when changing consumer code, with breaks just by updating PHP.

If I’m in PHP 8.1+ and I pass an object into a library and all goes

well, then I change a property to readonly and get an error, that’s not

PHP making a breaking change by allowing me to use readonly. That’s an

outdated library (and me) breaking my code.

I’ve had my reflection code break on backwards compatible changes loads

of times, but every time it required the user to make a change to their

code first.

Valentin’s list of examples proves this point. They worked fine on 8.1

until people changed code to add readonly. That’s not a BC break. Same

deal for aviz.

I guess what I am saying is that we probably need a proper definition of “BC break”. IMHO, adding a php version check to do something is probably a BC break. Serializers will need a version check, thus it is a BC break.

— Rob

On Fri, Oct 11, 2024 at 3:34 AM Rob Landers rob@bottled.codes wrote:

On Fri, Oct 11, 2024, at 12:20, Jonathan Vollebregt wrote:

A “proper” implementation won’t break, but there may be subtle ways that “improper” implementations will break and thus it should be considered a BC break.

This thread is fallaciously equating breaks in third-party libraries

when changing consumer code, with breaks just by updating PHP.

If I’m in PHP 8.1+ and I pass an object into a library and all goes

well, then I change a property to readonly and get an error, that’s not

PHP making a breaking change by allowing me to use readonly. That’s an

outdated library (and me) breaking my code.

I’ve had my reflection code break on backwards compatible changes loads

of times, but every time it required the user to make a change to their

code first.

Valentin’s list of examples proves this point. They worked fine on 8.1

until people changed code to add readonly. That’s not a BC break. Same

deal for aviz.

I guess what I am saying is that we probably need a proper definition of “BC break”. IMHO, adding a php version check to do something is probably a BC break. Serializers will need a version check, thus it is a BC break.

— Rob

Is this not something than has a really standard and consistent definition?

“Code which runs correctly on the previous version of the project, if it is not updated, will also run correctly and provide the same output on the next version.”

Backwards compatible has never, in any work I’ve done through my entire career, meant something like “if you take old code and then update it to the new version incorrectly, it doesn’t work”… that seems… obvious?

What exactly is the claim being made here? Because it sounds like the claim is very much that second “definition”.

Jordan

On Mon, 14 Oct 2024 at 01:28, Jordan LeDoux <jordan.ledoux@gmail.com>:

Backwards compatible has never, in any work I've done through my entire career, meant something like "if you take old code and then update it to the new version incorrectly, it doesn't work"... that seems... obvious?

What exactly is the claim being made here? Because it sounds like the claim is very much that second "definition".

Jordan

Hi, Jordan!

The problem is that in practice most of the PHP libraries consider
themselves to be compatible with newer PHP versions.

For instance, Symfony PropertyInfo uses `"php": ">=8.2"` constraint in
its `composer.json`. However, it is not compatible with PHP 8.4, I've
just created an issue: [PropertyInfo] PropertyInfo is not compatible with PHP 8.4 · Issue #58556 · symfony/symfony · GitHub

The end user will be the victim, because `composer require
symfony/property-info` will happily install property-info v7.1.4 for
PHP 8.4, but it's not gonna work.

--
Valentin

On 14/10/2024 01:02, Valentin Udaltsov wrote:

The problem is that in practice most of the PHP libraries consider
themselves to be compatible with newer PHP versions.

For instance, Symfony PropertyInfo uses `"php": ">=8.2"` constraint in
its `composer.json`.

That seems like a problem they have created for themselves. It seems an error to me to declare software forward-compatible with PHP versions that do not yet exist and thus have clearly not been tested against. Being as it is an error, we shouldn't consider it impinges on PHP's definition of a BC break.

Cheers,
Bilge

On Mon, 14/10/2024 04:07, Bilge <bilge@scriptfusion.com> wrote:

On 14/10/2024 01:02, Valentin Udaltsov wrote:
> The problem is that in practice most of the PHP libraries consider
> themselves to be compatible with newer PHP versions.
>
> For instance, Symfony PropertyInfo uses `"php": ">=8.2"` constraint in
> its `composer.json`.

That seems like a problem they have created for themselves. It seems an
error to me to declare software forward-compatible with PHP versions
that do not yet exist and thus have clearly not been tested against.
Being as it is an error, we shouldn't consider it impinges on PHP's
definition of a BC break.

Cheers,
Bilge

Hi, Bilge!

I think that PHP should then clearly explain what is a BC break and
what isn't on a separate php.net page.
And even explain what php version constraints are safe for Composer libraries.

Some languages have such a document:
- Go 1 and the Future of Go Programs - The Go Programming Language
- PEP 387 – Backwards Compatibility Policy | peps.python.org

--
Valentin

On Sun, Oct 13, 2024 at 5:03 PM Valentin Udaltsov <udaltsov.valentin@gmail.com> wrote:

On Mon, 14 Oct 2024 at 01:28, Jordan LeDoux <jordan.ledoux@gmail.com>:

Backwards compatible has never, in any work I’ve done through my entire career, meant something like “if you take old code and then update it to the new version incorrectly, it doesn’t work”… that seems… obvious?

What exactly is the claim being made here? Because it sounds like the claim is very much that second “definition”.

Jordan

Hi, Jordan!

The problem is that in practice most of the PHP libraries consider
themselves to be compatible with newer PHP versions.

For instance, Symfony PropertyInfo uses "php": ">=8.2" constraint in
its composer.json. However, it is not compatible with PHP 8.4, I’ve
just created an issue: https://github.com/symfony/symfony/issues/58556

The end user will be the victim, because composer require symfony/property-info will happily install property-info v7.1.4 for
PHP 8.4, but it’s not gonna work.


Valentin

How does a library that uses code that will not even compile (like readonly or private(set)), but claims to not require a PHP version that uses the syntax, suddenly make a BC problem for the language? Composer allows libraries to set minimum PHP versions for releases. Any time you update your libraries, you may have to update your code which uses it. That’s just part of how libraries work.

Jordan

On Mon, 14/10/2024 05:01, Jordan LeDoux <jordan.ledoux@gmail.com>:

On Sun, Oct 13, 2024 at 5:03 PM Valentin Udaltsov <udaltsov.valentin@gmail.com> wrote:

On Mon, 14 Oct 2024 at 01:28, Jordan LeDoux <jordan.ledoux@gmail.com>:
> Backwards compatible has never, in any work I've done through my entire career, meant something like "if you take old code and then update it to the new version incorrectly, it doesn't work"... that seems... obvious?
>
> What exactly is the claim being made here? Because it sounds like the claim is very much that second "definition".
>
> Jordan

Hi, Jordan!

The problem is that in practice most of the PHP libraries consider
themselves to be compatible with newer PHP versions.

For instance, Symfony PropertyInfo uses `"php": ">=8.2"` constraint in
its `composer.json`. However, it is not compatible with PHP 8.4, I've
just created an issue: [PropertyInfo] PropertyInfo is not compatible with PHP 8.4 · Issue #58556 · symfony/symfony · GitHub

The end user will be the victim, because `composer require
symfony/property-info` will happily install property-info v7.1.4 for
PHP 8.4, but it's not gonna work.

--
Valentin

How does a library that uses code that will not even compile (like `readonly` or `private(set)`), but claims to not require a PHP version that uses the syntax, suddenly make a BC problem for the language? Composer allows libraries to set minimum PHP versions for releases. Any time you update your libraries, you may have to update your code which uses it. That's just part of how libraries work.

Jordan

First of all, I have already agreed above that PHP does not have a BC
break here. Now we are discussing the potential problems in the PHP
ecosystem and how they could be mitigated.

Composer allows libraries to set minimum PHP versions for releases.

I think you've got the problem wrong. The problem is about the maximum
version, not the minimum one.

Consider the Symfony issue I've just reported:

Symfony PropertyInfo requires `php >= 8.2`. And it is written in PHP
8.2 syntax. So it can be safely installed and used in PHP 8.2 and 8.3.
In other words it's forward compatible. However, the `php >= 8.2`
constraint also allows installing it in PHP 8.4. And as it turned out,
PropertyInfo is not ready for 8.4 syntax (see my reproducer
GitHub - vudaltsov/symfony-property-access-php84). In my
opinion, this is a problem, because a PHP 8.4 user can install the
library without any obstacles and only later find out that `private
(set)` properties do not work as expected there. Ideally
`symfony/property-info` should not be installable until it ensures to
be compatible with all the PHP 8.4 features. So, its PHP constraint
should be `>=8.2 && <8.4`. And then `>=8.2 && <8.5`, once the tests
pass for PHP 8.4 features.

--
Best regards,
Valentin

On Sun, Oct 13, 2024, at 9:37 PM, Valentin Udaltsov wrote:

First of all, I have already agreed above that PHP does not have a BC
break here. Now we are discussing the potential problems in the PHP
ecosystem and how they could be mitigated.

Ilija and I have discussed this issue a bit.

The first issue is that isPublic() technically means "does this property have the public flag set," and nothing more. Prior to 8.1, that implicitly also meant "can the property be read and written to from public scope," because of how properties worked. (And same for isProtected().) That implicit assumption became invalid in 8.1 with readonly, which stealth introduced limited and not fully designed asymmetric visibility as well as properties that could not be set multiple times from any scope. Full aviz in 8.4 doesn't change that. It just makes the previous assumption change more apparent. The fact that no one seems to have reported it as an issue until now suggests it's not a particularly widespread problem. In practice, if someone is using reflection to determine the visibility of a property, they'll be writing to it through reflection as well if at all.

The best solution here is probably to just clarify the docs, which I will do as part of the aviz docs that I have already been working on. cf: Asymmetric Visiblity and Final properties by Crell · Pull Request #3828 · php/doc-en · GitHub

The second issue is that the behavior of isProtectedSet() / isPrivateSet() was not as clearly defined in the RFC as it should have been. That's on us for not being clearer, as we apologize for the oversight.

Those methods follow the low level pattern of isPublic() , that is, they just report of a given flag is set, not what the implications of that flag in various contexts are. That is consistent with the rest of the reflection API, so we feel that is best left as-is.

That still means the "so can I read/write this property or not?" question has no simple operation for it. Again: it never did, we just kinda sorta had it indirectly and implicitly. For that we feel the best answer, as well as least disruptive given we're in RCs, is dedicated methods as Ilija has already described that take all property behavior and context into account. (isReadable and isWriteable.)

As a reminder, the concept is:

$rProp->isReadable($obj); // Can this property on $obj be read from the calling scope?
$rProp->isReadable($obj, 'static'); // Same as previous.
$rProp->isReadable($obj, null); // Can this property on $obj be read from global scope?
$rProp->isReadable($obj, Foo::class); // Can this property on $obj be read from code inside class Foo?

$rProp->isWriteable($obj); // Can this property on object $obj be written from the calling scope?
$rProp->isWriteable($obj, 'static'); // Same as previous.
$rProp->isWriteable($obj, null); // Can this property on object $obj be written from global scope?
$rProp->isWriteable($obj, Foo::class); // Can this property on object $obj be written from code inside class Foo?

cf: Implement ReflectionProperty::is{Readable,Writable}() by iluuu1994 · Pull Request #16209 · php/php-src · GitHub

(The use of null to indicate global scope is borrowed from Closure::bind(), which does the same.)

These methods do runtime analysis to see if a property should be readable/writeable. Specifically:

isReadable()
* Checks that the property is readable from the passed scope
* Checks that the property is initialized (i.e. not typed and never written to)
* Checks that the property is not virtual or has a get hook
isWritable()
* Checks that the property is writable (respecting symmetric and asymmetric properties) from the passed scope
* Checks that the property is not readonly, is not yet initialized, or is reinitializable (__clone)
* Checks that the property is not virtual or has a set hook

Of note, this does not absolutely guarantee that a read/write will succeed. There's at least two exceptions: One, some PHP built-in classes have effectively immutable properties but do not use `readonly` or `private(set)`. Those would not be detected here, until and unless they are updated to use the now-available mechanisms. (See, eg: ReflectionClass can't detect readonly properties for mysqli · Issue #15309 · php/php-src · GitHub) The other is that a get or set hook may throw an exception under various circumstances. There is no way to evaluate that via reflection, so it's a gap that will necessarily always be there.

Whether those methods are OK to add in the RC phase or if they should be left to early 8.5, and if they would need a formal RFC, is up to the RMs to decide. RMs, what is your preference?

--Larry Garfield

On Mon, 14/10/2024 at 06:01, Larry Garfield <larry@garfieldtech.com> wrote:

On Sun, Oct 13, 2024, at 9:37 PM, Valentin Udaltsov wrote:

> First of all, I have already agreed above that PHP does not have a BC
> break here. Now we are discussing the potential problems in the PHP
> ecosystem and how they could be mitigated.

Ilija and I have discussed this issue a bit.

The first issue is that isPublic() technically means "does this property have the public flag set," and nothing more. Prior to 8.1, that implicitly also meant "can the property be read and written to from public scope," because of how properties worked. (And same for isProtected().) That implicit assumption became invalid in 8.1 with readonly, which stealth introduced limited and not fully designed asymmetric visibility as well as properties that could not be set multiple times from any scope. Full aviz in 8.4 doesn't change that. It just makes the previous assumption change more apparent. The fact that no one seems to have reported it as an issue until now suggests it's not a particularly widespread problem. In practice, if someone is using reflection to determine the visibility of a property, they'll be writing to it through reflection as well if at all.

The best solution here is probably to just clarify the docs, which I will do as part of the aviz docs that I have already been working on. cf: Asymmetric Visiblity and Final properties by Crell · Pull Request #3828 · php/doc-en · GitHub

The second issue is that the behavior of isProtectedSet() / isPrivateSet() was not as clearly defined in the RFC as it should have been. That's on us for not being clearer, as we apologize for the oversight.

Those methods follow the low level pattern of isPublic() , that is, they just report of a given flag is set, not what the implications of that flag in various contexts are. That is consistent with the rest of the reflection API, so we feel that is best left as-is.

That still means the "so can I read/write this property or not?" question has no simple operation for it. Again: it never did, we just kinda sorta had it indirectly and implicitly. For that we feel the best answer, as well as least disruptive given we're in RCs, is dedicated methods as Ilija has already described that take all property behavior and context into account. (isReadable and isWriteable.)

As a reminder, the concept is:

$rProp->isReadable($obj); // Can this property on $obj be read from the calling scope?
$rProp->isReadable($obj, 'static'); // Same as previous.
$rProp->isReadable($obj, null); // Can this property on $obj be read from global scope?
$rProp->isReadable($obj, Foo::class); // Can this property on $obj be read from code inside class Foo?

$rProp->isWriteable($obj); // Can this property on object $obj be written from the calling scope?
$rProp->isWriteable($obj, 'static'); // Same as previous.
$rProp->isWriteable($obj, null); // Can this property on object $obj be written from global scope?
$rProp->isWriteable($obj, Foo::class); // Can this property on object $obj be written from code inside class Foo?

cf: Implement ReflectionProperty::is{Readable,Writable}() by iluuu1994 · Pull Request #16209 · php/php-src · GitHub

(The use of null to indicate global scope is borrowed from Closure::bind(), which does the same.)

These methods do runtime analysis to see if a property should be readable/writeable. Specifically:

isReadable()
* Checks that the property is readable from the passed scope
* Checks that the property is initialized (i.e. not typed and never written to)
* Checks that the property is not virtual or has a get hook
isWritable()
* Checks that the property is writable (respecting symmetric and asymmetric properties) from the passed scope
* Checks that the property is not readonly, is not yet initialized, or is reinitializable (__clone)
* Checks that the property is not virtual or has a set hook

Of note, this does not absolutely guarantee that a read/write will succeed. There's at least two exceptions: One, some PHP built-in classes have effectively immutable properties but do not use `readonly` or `private(set)`. Those would not be detected here, until and unless they are updated to use the now-available mechanisms. (See, eg: ReflectionClass can't detect readonly properties for mysqli · Issue #15309 · php/php-src · GitHub) The other is that a get or set hook may throw an exception under various circumstances. There is no way to evaluate that via reflection, so it's a gap that will necessarily always be there.

Whether those methods are OK to add in the RC phase or if they should be left to early 8.5, and if they would need a formal RFC, is up to the RMs to decide. RMs, what is your preference?

--Larry Garfield

Hi, Larry!

Thank you very much for this response. I agree with every point.

I have only one comment about the methods' signatures. They should not
require an object, because properties can be static or the developer
might want to check writable/readable without instantiating an object
(in a code generator, for instance).
Then it makes sense to make `$object` the 2nd parameter. This will
also be consistent with Closure::bind().

isReadable(?string $scope = 'static', ?object $object = null): bool
isWritable(?string $scope = 'static', ?object $object = null): bool

--
Best regards,
Valentin

On 14/10/2024 02:51, Valentin Udaltsov wrote:

Hi, Bilge!

I think that PHP should then clearly explain what is a BC break and
what isn't on a separate php.net page.
And even explain what php version constraints are safe for Composer libraries.

Some languages have such a document:
-Go 1 and the Future of Go Programs - The Go Programming Language
-PEP 387 – Backwards Compatibility Policy | peps.python.org

Absolutely! After some 16 years using PHP, I only recently became aware BC breaks are acceptable in minor versions. For anyone spending significant time in open source, I think there is a tendency to become semver-minded and naturally assume PHP follows the same; it does not.

Cheers,
Bilge

On 14/10/2024 10:03, Bilge wrote:

On 14/10/2024 02:51, Valentin Udaltsov wrote:

Hi, Bilge!

I think that PHP should then clearly explain what is a BC break and
what isn't on a separate php.net page.
And even explain what php version constraints are safe for Composer libraries.

Some languages have such a document:
-Go 1 and the Future of Go Programs - The Go Programming Language
-PEP 387 – Backwards Compatibility Policy | peps.python.org

Absolutely! After some 16 years using PHP, I only recently became aware BC breaks are acceptable in minor versions. For anyone spending significant time in open source, I think there is a tendency to become semver-minded and naturally assume PHP follows the same; it does not.

Cheers,
Bilge

For reference, current policy on BC-breaks is here: policies/release-process.rst at main · php/policies · GitHub

On 14/10/2024 11:17, AllenJB wrote:

For reference, current policy on BC-breaks is here: policies/release-process.rst at main · php/policies · GitHub

That policy clearly states BC breaks are only allowed in major versions, which flies in the face of what I read:

"Breaking changes are allowed in minor releases, if they go through the RFC process. We don't follow semantic versioning." <https://chat.stackoverflow.com/transcript/message/57515936&gt;

Now I don't know what to believe. *shrugs*

Kind regards,
Bilge

On Monday, 14 October 2024 at 12:22, Bilge <bilge@scriptfusion.com> wrote:

On 14/10/2024 11:17, AllenJB wrote:

> For reference, current policy on BC-breaks is here:
> policies/release-process.rst at main · php/policies · GitHub

That policy clearly states BC breaks are only allowed in major versions,
which flies in the face of what I read:

"Breaking changes are allowed in minor releases, if they go through the
RFC process. We don't follow semantic versioning."
PHP - 2024-07-20

Now I don't know what to believe. shrugs

Kind regards,
Bilge

Yeah, this is wrong.
Every single minor version of PHP dating all the way back to PHP 4.0.0 has had BC breaks.
The thing is, I don't even know what:

Source compatibility should be kept if possible, while breakages are allowed

means here.

Best regards,

Gina P. Banyard