-
Notifications
You must be signed in to change notification settings - Fork 8.1k
[RFC] Allow Readonly Property Defaults #22588
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
Open
NickSdot
wants to merge
2
commits into
php:master
Choose a base branch
from
NickSdot:readonly-property-defaults
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+370
−8
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
36 changes: 36 additions & 0 deletions
36
Zend/tests/readonly_classes/readonly_with_property_default.phpt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| --TEST-- | ||
| Properties of a readonly class may have default values | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| readonly class Foo | ||
| { | ||
| public int $bar = 1; | ||
| public ?string $nullable = null; | ||
|
|
||
| public function __construct() | ||
| { | ||
| try { | ||
| $this->bar = 2; | ||
| } catch (Error $e) { | ||
| echo $e::class, ": ", $e->getMessage(), "\n"; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| $foo = new Foo(); | ||
| var_dump($foo->bar); | ||
| var_dump($foo->nullable); | ||
|
|
||
| try { | ||
| $foo->bar = 3; | ||
| } catch (Error $e) { | ||
| echo $e::class, ": ", $e->getMessage(), "\n"; | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| Error: Cannot modify readonly property Foo::$bar | ||
| int(1) | ||
| NULL | ||
| Error: Cannot modify readonly property Foo::$bar |
18 changes: 18 additions & 0 deletions
18
Zend/tests/readonly_classes/readonly_with_property_default_trait.phpt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| --TEST-- | ||
| Readonly class may use readonly trait property with default value | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| trait TDefault { | ||
| public readonly int $prop = 2; | ||
| } | ||
|
|
||
| readonly class C { | ||
| use TDefault; | ||
| } | ||
|
|
||
| var_dump(new C()->prop); | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| int(2) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
33 changes: 33 additions & 0 deletions
33
Zend/tests/readonly_props/readonly_with_default_asymmetric_visibility.phpt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| --TEST-- | ||
| Readonly property with default value and asymmetric visibility | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| class Test { | ||
| public readonly int $default = 1; | ||
| public private(set) readonly int $private = 2; | ||
| public protected(set) readonly int $protected = 3; | ||
| public public(set) readonly int $public = 4; | ||
| } | ||
|
|
||
| $test = new Test(); | ||
| var_dump($test->default, $test->private, $test->protected, $test->public); | ||
|
|
||
| foreach (['default', 'private', 'protected', 'public'] as $prop) { | ||
| try { | ||
| $test->$prop = 42; | ||
| } catch (Error $e) { | ||
| echo $e::class, ": ", $e->getMessage(), "\n"; | ||
| } | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| int(1) | ||
| int(2) | ||
| int(3) | ||
| int(4) | ||
| Error: Cannot modify readonly property Test::$default | ||
| Error: Cannot modify readonly property Test::$private | ||
| Error: Cannot modify readonly property Test::$protected | ||
| Error: Cannot modify readonly property Test::$public |
40 changes: 40 additions & 0 deletions
40
Zend/tests/readonly_props/readonly_with_default_inheritance.phpt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| --TEST-- | ||
| Readonly property with default value and inheritance | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| class ParentDefault { | ||
| public readonly int $prop = 1; | ||
| } | ||
|
|
||
| class ChildInherits extends ParentDefault {} | ||
|
|
||
| class ChildOverrides extends ParentDefault { | ||
| public readonly int $prop = 2; | ||
| } | ||
|
|
||
| class PrivateParent { | ||
| private readonly int $prop = 3; | ||
|
|
||
| public function getParentProp(): int { | ||
| return $this->prop; | ||
| } | ||
| } | ||
|
|
||
| class PrivateChild extends PrivateParent { | ||
| public readonly int $prop = 4; | ||
| } | ||
|
|
||
| var_dump(new ChildInherits()->prop); | ||
| var_dump(new ChildOverrides()->prop); | ||
|
|
||
| $privateChild = new PrivateChild(); | ||
| var_dump($privateChild->getParentProp()); | ||
| var_dump($privateChild->prop); | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| int(1) | ||
| int(2) | ||
| int(3) | ||
| int(4) |
29 changes: 29 additions & 0 deletions
29
Zend/tests/readonly_props/readonly_with_default_interface.phpt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| --TEST-- | ||
| Readonly property with default value satisfies get-only interface property | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| interface I { | ||
| public int $prop { get; } | ||
| } | ||
|
|
||
| class C implements I { | ||
| public readonly int $prop = 42; | ||
| } | ||
|
|
||
| var_dump(new C()->prop); | ||
|
|
||
| interface J { | ||
| public int $prop { get; set; } | ||
| } | ||
|
|
||
| // does not satisfy set | ||
| class D implements J { | ||
| public readonly int $prop = 42; | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECTF-- | ||
| int(42) | ||
|
|
||
| Fatal error: Set access level of D::$prop must be omitted (as in class J) in %s on line %d | ||
35 changes: 35 additions & 0 deletions
35
Zend/tests/readonly_props/readonly_with_default_reflection.phpt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| --TEST-- | ||
| Reflection for readonly property with default value | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| class Foo { | ||
| public readonly int $prop = 1; | ||
| public readonly ?string $nullable = null; | ||
| } | ||
|
|
||
| $rp = new ReflectionProperty(Foo::class, 'prop'); | ||
| var_dump($rp->isReadOnly()); | ||
| var_dump($rp->hasDefaultValue()); | ||
| var_dump($rp->getDefaultValue()); | ||
| var_dump(new ReflectionClass(Foo::class)->getDefaultProperties()); | ||
|
|
||
| $test = new Foo(); | ||
| try { | ||
| $rp->setValue($test, 2); | ||
| } catch (Error $e) { | ||
| echo $e::class, ": ", $e->getMessage(), "\n"; | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECT-- | ||
| bool(true) | ||
| bool(true) | ||
| int(1) | ||
| array(2) { | ||
| ["prop"]=> | ||
| int(1) | ||
| ["nullable"]=> | ||
| NULL | ||
| } | ||
| Error: Cannot modify readonly property Foo::$prop |
20 changes: 20 additions & 0 deletions
20
Zend/tests/readonly_props/readonly_with_default_trait_mismatch.phpt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| --TEST-- | ||
| Readonly trait property default value mismatch | ||
| --FILE-- | ||
| <?php | ||
|
|
||
| trait T1 { | ||
| public readonly int $prop = 1; | ||
| } | ||
|
|
||
| trait T2 { | ||
| public readonly int $prop = 2; | ||
| } | ||
|
|
||
| class C { | ||
| use T1, T2; | ||
| } | ||
|
|
||
| ?> | ||
| --EXPECTF-- | ||
| Fatal error: T1 and T2 define the same property ($prop) in the composition of C. However, the definition differs and is considered incompatible. Class was composed in %s on line %d |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the test is testing what you want.
$propshould bepublic(set)inD.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Tim, thanks for the review!
It's essentially a copy of here:
https://github.com/php/php-src/blob/master/Zend/tests/property_hooks/interface_get_set_readonly.phpt#L11
The intention was to confirm that having a default value does not change anything. You probably think it was not what I wanted to test because of the irrelevant error message? If so please check the comment in the existing test file above -- known issue, it seems.
The case you mean should be tested here:
https://github.com/php/php-src/pull/22588/changes#diff-a5f6cb0065c3c8fe569119380cdb62cb4a1a7b9813fcf3bb97723894cb262667R10
--
Fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that test predated asymmetric visibility. It should probably be adjusted to include
public(set)(or a second test added).No. That test is not testing the behavior of the interaction between
readonlyandset;in an interface.