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

For reference, current policy on BC-breaks is here:
https://github.com/php/policies/blob/main/release-process.rst

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

Hey Larry,

First, thank you for this feature. Second, attributes and annotations are a moot point. Adding attributes didn't render annotations broken and is still (unfortunately) used today in many codebases running 8.3. So, in other words, the deprecation of annotations is a framework/codebase/user-level issue and totally unrelated to attributes and PHP as a language. While attributes helped with that deprecation, it wasn't caused by them—people could have deprecated annotations without ever having attributes.

According to Wikipedia (https://en.wikipedia.org/wiki/Backward_compatibility)

In compilers https://en.wikipedia.org/wiki/Compiler, [and interpreters] backward compatibility may refer to the ability of a compiler [or interpreter] for a newer version of the language to accept source code of programs or data that worked under the previous version.

I think this is all we need: just a proper definition. Looking at the issue at play, we can conclude that it isn't a BC break — code that worked in 8.3 will continue to work in 8.4. Will things have to be updated (or in my case, rewritten basically from scratch)? Sure! But there, it becomes a framework/codebase/user-level issue on whether they will remain backwards compatible with older versions of PHP.

— Rob

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

After discussing it and thinking about it for a few days, I agree it's
not a BC break. Existing code should be fine, and larger frameworks
that tend to rely on reflection trickery and such also tend to need
updates for newer PHP. The expectations don't really change.

I don't think we need to rush a solution in for 8.4 as long as we can
implement one with the existing APIs in userland. As Larry mentioned,
readonly (and hooks) already made the previous assumptions invalid by
8.1. As long as we can provide a polyfill and documentation (which
should also cover the readonly case in 8.1 too), I'm OK with deferring
isReadable/isWritable to 8.5.

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

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