Re: Allow hooks in `readonly` promoted properties

Hey Tim, > On 4. Jun 2025, at 21:09, Tim Düsterhus <tim@bastelstu.be> wrote: > > Semantically once you involved inheritance it isn't that easy. It is allowed to override > an “unhooked” property with a hooked property and in the “Readonly Amendments” RFC we > already decided that inheriting from a readonly class by a > non-readonly class should not be valid. I added tests for this. The behaviour you expect seems confirmed. > > So if you would be allowed to override a readonly unhooked property with a hooked property that > has a get hook that is not a pure function, you would make the > property effectively mutable, which is something that users of the class can't expect. It > violates the history property of the Liskov substitution principle. > > Making this legal might also inhibit engine optimization. Currently if you know that a property > is readonly you can theoretically optimize: > > if ($object->foo !== null) { > do_something($object->foo); > } > > into: > > if (($foo = $object->foo) !== null) { > do_something($foo); > } > > to avoid reading $object->foo twice, which for example would > need to check visibility twice. Added Zend/tests/property_hooks/readonly_property_backed_inheritance_3.phpt Personally, I’d argue nothing unexpected happens here and everything is fair game. If we overwrite parents from a child it will naturally result in a changed get hook result. This, however, does not mean the actual class state has changed. Opinions? >> People often say “you can just do things”. So I did, and tried to contribute the code >> for your existing RFC text: >> https://github.com/php/php-src/pull/18757 >> Can it really be such a little change? I’d appreciate feedback from people more >> experienced than I am. Thanks! > > Your test cases really only scratch the surface of what should be tested. You are basically > just verifying that it compiles. In fact you are not even testing that reassigning the property is > disallowed, because the test fails due to a visibility error. In fact it appears that the > readonly check comes before the visibility check, which would imply > that the readonly doesn't have an effect: > https://3v4l.org/nqgpL You are right. I edited the tests accordingly. — Cheers, Nick