Skip to content

Allow hooks for backed readonly properties #18757

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
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NickSdot
Copy link
Contributor

@NickSdot NickSdot commented Jun 4, 2025

@TimWolla TimWolla added the RFC label Jun 4, 2025
@NickSdot NickSdot force-pushed the allow-readonly-hooks branch 2 times, most recently from e35f83c to 1a21998 Compare June 6, 2025 01:17
@NickSdot NickSdot force-pushed the allow-readonly-hooks branch from 1a21998 to 6840873 Compare June 6, 2025 02:22
@@ -1,12 +1,73 @@
--TEST--
Hooked properties cannot be readonly
Backed readonly property may have nice hooks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"nice"? 😄 Hooks are nice, but the term isn't descriptive in context.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is close to what we want to confirm, but not quite. To match the RFC, we'd want a readonly Product class that contains public Category $category (possibly in the constructor?), then LazyProduct that extends it and adds a hook to do the lazy updating.

The intent here is that one can define a data model as just a boring readonly class, but then an ORM can code-generate a subclass that it will always use that handles the lazy initialization. That way it's all hidden from the author and user of the main class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Crell I pushed an update.

To be honest, I don't fully get this one. If something is generated, why the inheritance? Not trying to argue against though--I just don't get it, and hence have difficulties to be sure about what the test actually should look like. Any chance the update did land were you wanted it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a lot more what I was talking about.

The inheritance is for type compatibility. Consider you're writing a data-mapper ORM, like Doctrine. You want the developer to be able to define an object easily, without thinking about lazy loading or persistence. So they make a Product object. Because it's a read-model only (which is good; I've found separating the read model and write model is almost always a good decision), they make it readonly.

Then elsewhere in the system they have a function that takes a Product object as a parameter. They type it as Product and move on with life.

The ORM, though, wants to load things lazily. Say, references to other objects (like Category), or a list of other objects, or whatever. It can generate code that will do the lazy work in hooks (yay!), but then that function that expects Product won't accept it, because it's a LazyProduct rather than Product. The only way to resolve that is either to force interfaces on everything (which in this case is a needless burden) or to make the generated LazyProduct a child class of Product, so that it will pass type checks for Product. But then you're extending a readonly property and putting a hook on it, which you can't do.

... Until this PR. 😄

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.

3 participants