Asymmetric visibility is a BC break

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:
-https://go.dev/doc/go1compat
-https://peps.python.org/pep-0387/

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

Hello,

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.

As much as I like this new feature and I am more than thankful for the
work behind it, if a test in codes using a x.y version of php works but
fails in x.y+1, it is a BC break, no matter how we look at it. A php
dependency targeting x.* is very common. While it tends to be used less
frequently as the amount of issues increase, that's not necessarly a
good thing.

In some cases it is a necessary evil (extension deprecated adding
warnings, security fix requiring a bc break, f.e.).

However, I am very doubtful here. And I do not know if it can be
avoided while keeping the new behaviors.

All in all, it would be great to at least agree that there is a BC
break issue, so it can be addressed according, whatever the final
decision is.

best,
Pierre

I think folks are operating with different definitions of "BC Break".

Consider:

// Foreign.php

class Foreign {
public function __construct(public string $name) {}
}

// My code:

$f = new Foreign('beep');
$rProp = (new ReflectionObject($f))->getProperty('name');
if ($rProp->isPublic()) {
$f->name = 'boop';
}

Under PHP 8.0, this code works.
Upgrading to PHP 8.1, this code still works exactly the same. Therefore, there is no BC break.
Upgrading to PHP 8.4, this code still works exactly the same. Therefore, there is no BC break.

Now, someone edits Foreign.php in 8.1 to read:

class Foreign {
public function __construct(public readonly string $name) {}
}

Now, my code will fail, because isPublic() does not guarantee "is writeable" anymore.

Is this a BC break in PHP?

I can see the argument either way, personally, but I tend toward no. One PHP version to the next should always be safe to upgrade an existing code base. Just swap out the PHP binary and it should still work the same. We want that. However, once you start modifying any part of the code (including dependencies), all bets are off.

Suppose some library switched from using annotations to using attributes. Any other code that was looking for those annotations is now broken, but it would be ridiculous to accuse attributes of having broken BC in PHP. Or ancient PHP 4 code that just assumed all properties were public (which they were in PHP 4), now confronted with PHP 5 code that has private properties. Is that a PHP BC break? Not at all.

And again, the relevant change here happened in PHP 8.1. This is only tangentially related to 8.4 at all, because isPublic() has not implicitly meant is-writeable anymore for three years.

--Larry Garfield

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: https://github.com/php/doc-en/pull/3828

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: https://github.com/php/php-src/pull/16209

(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: https://github.com/php/php-src/issues/15309) 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

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.

Then perhaps we should change the name of the thread, since the title of
this is rather confusing.

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
https://github.com/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.

There are some major problems with this ideal:

1. Often, a partially working library (because of a bug that only occurs
   with new PHP features) is of more use than one that doesn't work at all
   because of version constraints

2. With upper version constraints if any of the projects you depend on
   aren't on the ball it will hold you back from upgrading PHP versions.
   You are now only as up-to-date as the slowest of your dependencies
   (Regardless whether the upper version constraint is warranted because of
   an impending bug)

3. Any project that already has the typical php >= 8.2 constraint can
   NEVER use upper version constraints:

If you decide to add an upper version constraint eg. php >= 8.3 < 8.5
then years down the line with php >= 10.1 < 10.3 everyone who updates
to 10.3 will still be installing the years old >= 8.2 version which is
the latest one they are "compatible" with.

This will cause far more breakage if the library isn't up to date than
without the upper version constraint. I don't know if it's technically
possible to force push new tags with a changed composer.json for all old
versions of a library but just typing that out sounds scary.