ReflectionProperty::isPrivateSet() not working as expected

[vudaltsov]

Description

The following code:

<?php

final class A
{
    private mixed $private;
    private private(set) mixed $privatePrivateSet;
    protected private(set) mixed $protectedPrivateSet;
    public private(set) mixed $publicPrivateSet;
}

var_dump(
    new ReflectionProperty(A::class, 'private')->isPrivateSet(),
    new ReflectionProperty(A::class, 'privatePrivateSet')->isPrivateSet(),
    new ReflectionProperty(A::class, 'protectedPrivateSet')->isPrivateSet(),
    new ReflectionProperty(A::class, 'publicPrivateSet')->isPrivateSet(),
);

Resulted in this output:

bool(false)
bool(false)
bool(true)
bool(true)

But I expected this output instead:

bool(true)
bool(true)
bool(true)
bool(true)

PHP Version

PHP 8.4.0-dev (cli) (built: Sep 24 2024 14:38:59)

Operating System

MacOS

[vudaltsov]

I understand, why this happens. private private(set) mixed $prop equals private mixed $prop. And Reflection is designed to say isPrivateSet() -> false for private mixed $prop. However from my POV this is kind of strange and inconsistent. isPrivateSet() now has two responsibilities: to check that set visibility is explicit and to check whether it is private.

[vudaltsov]

Here's another strange example:

final class A
{
    public readonly mixed $publicReadonly;
    protected readonly mixed $protectedReadonly;
}

var_dump(
    new ReflectionProperty(A::class, 'publicReadonly')->getModifiers() & ReflectionProperty::IS_PROTECTED_SET,
    new ReflectionProperty(A::class, 'publicReadonly')->isProtectedSet(),
    new ReflectionProperty(A::class, 'protectedReadonly')->getModifiers() & ReflectionProperty::IS_PROTECTED_SET,
    new ReflectionProperty(A::class, 'protectedReadonly')->isProtectedSet(),
);

gives

int(2048)
bool(true)
int(0)
bool(false)

I would expect

int(2048)
bool(true)
int(2048)
bool(true)

[iluuu1994]

I do get where you're coming from. As you have mentioned, this is intentional. private private(set) is not really meaningful, so the private(set) is removed implicitly. Additionally, the flag is not set for private, because is allows skipping asymmetric visibility checks. Reflection gives an accurate view into what is happening under the hood. This is also why there is no isPublicSet(), because that flag doesn't exist internally, if it is public(set) then it's just public.

What we should definitely do is provide a unified method to check whether the property is readable/writable from some scope, as described here, as there are multiple ways in which the property could or could not be readable/writable:

https://github.com/php/php-src/issues/15309#issuecomment-2306830508

[vudaltsov]

Thank you for the explanations! As a frequent reflection user I am really interested in this topic.

Reflection gives an accurate view into what is happening under the hood.

Is it a technical limitation? I mean, what if we do the opposite: hide the implementation details and expose a more comprehensive intuitive API? For example, is(Public|Private|Protected) for get and is(Public|Private|Protected)Set for set? This will be fully backward compatible with php <8.4.

What we should definitely do is provide a unified method to check whether the property is readable/writable from some scope

Yes, this is great, but do we need is(Provate|Protected)Set() then?

[iluuu1994]

hide the implementation details and expose a more comprehensive intuitive API? For example, is(Public|Private|Protected) for get and is(Public|Private|Protected)Set for set?

Well, this would still be somewhat clunky. I.e. if I want to check whether something is writable in protected scope, I would have to check isPublicSet() || isProtectedSet(). Edit: I guess it could do !isPrivateSet(). But as mentioned in my other comment, there are other potential reasons that something isn't writable, e.g. readonly or a missing set hook on virtual properties.

Yes, this is great, but do we need is(Provate|Protected)Set() then?

Something like PHPStan might want to know why a property isn't writable.

[vudaltsov]

Alright, then I propose to add isReadable, isWritable in PHP 8.4 (is it possible without an RFC?), otherwise ReflectionProperty is a huge pain for hydrators and other similar tools. Just to check that property is writable from global scope, you have to do isPublic() && !isReadonly() && !isPrivateSet() && !isProtectedSet() && (!isVirtual() || hasHook(PropertyHookType::Set)).

isReadable and isWritable can have a $scope parameter, similar to the $newScope in Closure::bind():

class ReflectionProperty
{
    // ...

    /**
     * @param null|class-string|object|'static' $scope null — global scope, 'static' — current scope
     */
    public function isReadable(null|string|object $scope = 'static'): bool {}

    /**
     * @param null|class-string|object|'static' $scope null — global scope, 'static' — current scope
     */
    public function isWritable(null|string|object $scope = 'static'): bool {}
}

[iluuu1994]

Alright, then I propose to add isReadable, isWritable in PHP 8.4 (is it possible without an RFC?)

Sadly, neither are possible, given that RC1 has been released. But it should be easy to polyfill in userland in the meantime. We can ping @php/release-managers-84 to see if they are willing to make an exception, but we have just voted to disallow such changes after RC.

[SakiTakamachi]

Yes, this feature should be accepted in 8.5/9.0.

[iluuu1994]

I will throw together an implementation tomorrow. The RMs can make a final decision then. It should indeed not be complicated, but if we don't follow our policies, why have them. :slightly_smiling_face:

[iluuu1994]

I now created an implementation here: https://github.com/php/php-src/pull/16209 It wouldn't be a risky merge, as these are completely isolated functions. But again, it's up to the RMs to decide.

[vudaltsov]

Thank you, @iluuu1994. I am still concerned about the current ReflectionProperty behavior. I even think that it breaks BC. See my email to internals: https://externals.io/message/125740.

[iluuu1994]

I've seen. I do not agree with changing isPublic and friends. It does not accurately reflect the implementation, and I don't think it's even intuitively how people understand it, given the syntax we have chosen. If the syntax were something like var $prop { public get; private set; } then it would make a bit more sense. But both syntax and implementation wise, we do not have a get visibility. The get visibility is dictated by the visibility of the entire property. The set visibility can be further restricted.

A BC break is certainly worse than a new feature, so if we decided something needs to be done about this, merging this PR would be much preferable.

[vudaltsov]

I do not agree with changing isPublic and friends.

I actually propose to keep their semantics, not to change it. Consider this function:

function setNonReadonlyPublicProperty(object $object, string $property, mixed $value): void
{
    $reflection = new ReflectionProperty($object, $property);

    if ($reflection->isPublic() && !$reflection->isReadOnly()) {
        $object->{$property} = $value;
    }
}

It works fine in PHP 8.3 for any possible property:

class A
{
    public string $a;
}

setNonReadonlyPublicProperty(new A, 'a', 'test'); // OK

However, there exists a property in PHP 8.4 for which the function throws:

class A
{
    public private(set) string $a;
}

setNonReadonlyPublicProperty(new A, 'a', 'test'); // Cannot modify private(set) property A::$a from global scope

This is because in the current PHP 8.4 build the meaning of isPublic() has changed. It's a BC break. If there exists a hydrator that works similar to setNonReadonlyPublicProperty() function, it will suddenly not support some new PHP 8.4 code.

This actually happened in PHP 8.1 when readonly properties were introduced, because suddenly checking isPublic() became not sufficient to write to those properties. I've seen these issues in Doctrine and Symfony Serializer back then.

we do not have a get visibility

I am sorry, I don't get this. The word "asymmetric" implies that there are two sides which are different. "symmetric" means that both sides are equal. In our case these sides are "get" and "set", there's nothing else here. When I see public private(set) $prop, I realize it is public(get) private(set) $prop. In presence of private(set) there is no other meaning of public, rather than a property is public for reading. When I see public $prop, I realize it is public(get) public (set) $prop. So, while implementation wise things are a little bit different, usage wise public private(set) means exactly two things: property is public for reading and private for writing.