Skip to content

Improved readonly errors#22609

Open
NickSdot wants to merge 2 commits into
php:masterfrom
NickSdot:improve-readonly-errors
Open

Improved readonly errors#22609
NickSdot wants to merge 2 commits into
php:masterfrom
NickSdot:improve-readonly-errors

Conversation

@NickSdot

@NickSdot NickSdot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

This surfaced via #22588 see #22588 (comment).

There are two commits. The second one is redoing the first one. Tim asked me in the linked comment to add public(set) to a readonly property. There are definitely other tests doing that, and I went ahead with it. Though, it seems very weird to me that we would have to use public(set) to override the implicit protected(set) from readonly, just to then surface the actual error.

I am not going to pretend I understand all the complexity involved here... But the current state of the PR seems more logical to me because we no longer need public(set), and get actual helpful errors:

Fatal error: Readonly property C::$prop cannot implement set hook required by interface I::$prop in %s on
Fatal error: Readonly property C::$prop cannot implement set hook required by class A::$prop in %s on line %d

instead of only changing:

Fatal error: Set access level of C::$prop must be omitted (as in class I) in %s on line %d

to:

Fatal error: Set access level of readonly property C::$prop is protected(set), but must be public(set) (as in interface I) in %s on line %d

as the first commit did. This would make tests pass, but it's not honest because in combination with readonly the actual problem is not visibility. Not fully sure if it would be semantically defensible, but I think we could argue that public(set) in combination with readonly itself, on a promoted property, never should have worked and should have errored -- because it achieves nothing. Anyway, too late to change that because it would be breaking.

Fwiw, I know that a set interface property does not mean that the implementor must have a hook -- however, since readonly is implicit protected, for a promoted property the right conclusion seems to be to acknowledge that in this case the error must be related to hooks. As mentioned above, changing visibility to public(set) does not achieve anything other than reaching the actual error, and this will never change. However, it is possible that we will find a solution for allowing hooked readonly properties in the future -- which is another argument for having a hook related error, instead of a more generic one, until then

That said, it's not impossible that I am missing something, but changing the order as I did here seems sound.

Aside: interface_get_set_readonly.phpt was renamed interface_readonly_get_set_promoted.phpt, not deleted


@iluuu1994 you added the "error message should be improved". Can I please have your opinion on this?

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants