The Wayback Machine - https://web.archive.org/web/20210705205209/https://github.com/php/php-src/pull/7089
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement readonly properties #7089

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Implement readonly properties #7089

wants to merge 4 commits into from

Conversation

@nikic
Copy link
Member

@nikic nikic commented Jun 2, 2021

@nikic nikic added the RFC label Jun 2, 2021
@@ -154,6 +154,7 @@ static YYSIZE_T zend_yytnamerr(char*, const char*);
%token <ident> T_PRIVATE "'private'"
%token <ident> T_PROTECTED "'protected'"
%token <ident> T_PUBLIC "'public'"
%token <ident> T_READONLY "'readonly'"

This comment has been minimized.

@iluuu1994

iluuu1994 Jun 16, 2021
Member

Can this be added to semi_reserved?

Zend/zend_object_handlers.c Show resolved Hide resolved
@matthieu88160
Copy link

@matthieu88160 matthieu88160 commented Jun 16, 2021

Hi all,
Shouldn't this feature be extended to the class constant rather than properties only?

@iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Jun 16, 2021

@matthieu88160 Constants are by definition readonly. Not sure how that would make sense. Also note that the mailing list is the primary tool for feedback when it comes to specification.

@matthieu88160
Copy link

@matthieu88160 matthieu88160 commented Jun 16, 2021

@matthieu88160 Constants are by definition readonly. Not sure how that would make sense. Also note that the mailing list is the primary tool for feedback when it comes to specification.

My bad, I was thinking modification was allowed.

@iluuu1994
Copy link
Member

@iluuu1994 iluuu1994 commented Jun 16, 2021

@nikic Here are two more tests for useful use-cases of overriding a readonly property:

--TEST--
Visibility can change in readonly property
--FILE--
<?php

class A {
    protected readonly int $prop;

    public function __construct() {
        $this->prop = 42;
    }
}
class B extends A {
    public readonly int $prop;
}

$a = new A();
try {
    var_dump($a->prop);
} catch (\Error $error) {
    echo $error->getMessage() . "\n";
}

$b = new B();
var_dump($b->prop);

?>
--EXPECT--
Cannot access protected property A::$prop
int(42)

--TEST--
Can override readonly property with attributes
--FILE--
<?php

#[Attribute]
class FooAttribute {}

class A {
    public readonly int $prop;

    public function __construct() {
        $this->prop = 42;
    }
}
class B extends A {
    #[FooAttribute]
    public readonly int $prop;
}

var_dump((new ReflectionProperty(B::class, 'prop'))->getAttributes()[0]->newInstance());

?>
--EXPECT--
object(FooAttribute)#1 (0) {
}
$rp = new ReflectionProperty(Test::class, 'ro');
var_dump($rp->isReadOnly());
var_dump(($rp->getModifiers() & ReflectionProperty::IS_READONLY) != 0);

This comment has been minimized.

@mvorisek

mvorisek Jun 24, 2021
Contributor

setValue by reflection should be tested here and allowed

This comment has been minimized.

@iluuu1994

iluuu1994 Jun 24, 2021
Member

ReflectionProperty::setValue() can bypass the requirement that initialization occurs from the scope where the property has been declared. However, reflection cannot modify a readonly property that has already been initialized.

Allowing setValue on properties that have already been set breaks the assumption that the value can never change. This assumption is useful for not only the user but also the optimizer.

This comment has been minimized.

@mvorisek

mvorisek Jun 24, 2021
Contributor

But php is known for it's beaty that non-constants can be changed during runtime which makes debugging and hacking much easier.

This comment has been minimized.

@kocsismate

kocsismate Jun 24, 2021
Member

Nikita's standpoint has always been that we shouldn't allow such loopholes, and I also agree with him - Ilya pointed out the reasoning very clearly. PHP used to allow a lot of shady things in the past, but it's less and less true these days.

@reyadkhan
Copy link

@reyadkhan reyadkhan commented Jul 2, 2021

Method parameters can be readonly:
foo(readonly string $bar){}

@kolardavid
Copy link

@kolardavid kolardavid commented Jul 5, 2021

Hey guys, I would like to add my few objections you might want to consider:

  1. Personally, I would allow unset() on readonly property, which would unlock it for writing again. This can be done only in private scope and can be useful. It would then work as controlled lockable property within scope, which has knowledge about the object. Also it would be consistent with typed properties, which can be unset, which returns them back in state, where reading throws exception (in oppose to writing).
  2. PHP is imho missing simple function to check if typed or readonly property is initialized, which might be harder to safely detect if reading/writing will throw an exception or is permitted. People usually use isset(), which gives false negative if typed/readonly is already set to explicit null. It would be probably helpful to add is_initialized() function. You can't even use property_exists() for this, since it returns (correctly) true for uninitialized properties.
  3. I would personally vote for not using readonly keyword, rather use writeonce/immutable/locked. readonly imho does not describe the usage that well as the other. Also, I would reserve readonly for other purpose:
  4. readonly would be imho much more useful for different scenario, where property is writable within private scope and readonly outside of private scope. Actually I believe I would use this scenario even more than the currently proposed readonly feature. It would safely expose private properties to outside without risk of modification. It would be basically syntactic sugar for private property and public getter without setter (the above proposed behavior of being able to unset and set it again would be more close to this scenario, even though it would always require unset before set to be safe, which is annoying). I can imagine 2 ways of behavior:
    a. private readonly int $prop; would mean that property is writable in private scope and readonly in protected and public scope (in which case public readonly would be non-practical).
    b. public readonly int $prop; would mean the same, but changes meaning of visibility specifier (in which case private readonly would be non-practical)
    Basically the only difference would be if private/protected/public would specify where is readonly visible and it would be always writable only in private scope (which would work similarly to current readonly behavior, but you will be unable to make it writable from protected scope) or it would be always publicly visible, but you would specify if it is writable in private/protected/public scope. That would be imho more practical, but would be confusing and required explanation, because it would make visibility modifiers works differently than usual.

Of course there is option to merge both behaviors by adding modifier to readonly - so having public readonly working as readonly from outside, writable from inside and public readonly writeonce as current readonly proposal, making both things more consistent. The downside is that readonly writeonce is longer to write, but makes most sense to me.

Thank you for your attention and considering this opinions :)

@kocsismate
Copy link
Member

@kocsismate kocsismate commented Jul 5, 2021

Hey guys, I would like to add my few objections you might want to consider:

Please note that the feature is currently in voting, so any change should be warranted very much.

Points 3 and 4 have already been discussed to death, I believe the RFC provides background information why the readonly keyword (rather than writeonce) was chosen, and it also talks about the relation of "readonly properties" to the "asymmetric visibility" feature you also described.

Implementing an is_initialized() function could make sense, but it's IMO also orthogonal to this feature, since it doesn't only affect readonly properties. Also, it's already possible to detect if a property has been initialized via reflection.

Point 1 would render the feature broken, since any property value could be changed in private scope at any time, simply by unsetting it first.

@nikic
Copy link
Member Author

@nikic nikic commented Jul 5, 2021

is_initialized() was recently discussed and declined (https://externals.io/message/114607).

@kolardavid
Copy link

@kolardavid kolardavid commented Jul 5, 2021

OK guys, thanks for your reply and sorry to bother you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7 participants