-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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
base: master
Are you sure you want to change the base?
Conversation
e35f83c
to
1a21998
Compare
1a21998
to
6840873
Compare
@@ -1,12 +1,73 @@ | |||
--TEST-- | |||
Hooked properties cannot be readonly | |||
Backed readonly property may have nice hooks |
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.
"nice"? 😄 Hooks are nice, but the term isn't descriptive in context.
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.
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.
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.
@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?
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.
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. 😄
wip
https://news-web.php.net/php.internals/127529
https://wiki.php.net/rfc/readonly_hooks