-
Notifications
You must be signed in to change notification settings - Fork 11.7k
Introduce lazy object and proxy object support helpers
#57831
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: 12.x
Are you sure you want to change the base?
Conversation
|
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
8bd4c08 to
962e739
Compare
lazy object and proxy object support helpers
8dba754 to
36601d2
Compare
|
Amazing work, @timacdonald |
|
I hear that. I'm not sure at that point it might make the API / function usage feel like it has too much going on. I liked the simplicity of the current API with an escape hatch for more complex work as needed. No super strong feelings either way, though. |
|
I like the current API as well. Was just a thought that might work to just remove the need of manually calling the constructor. However, as you mentioned it will be one more thing for the helper to deal with, so I'm not sure if it's something you want to add. |
| * @param (\Closure(TValue): mixed)|int $callback | ||
| * @param int $options |
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.
Please don't use a random int:
| * @param (\Closure(TValue): mixed)|int $callback | |
| * @param int $options | |
| * @param (\Closure(TValue): mixed)|0|\ReflectionClass::SKIP_INITIALIZATION_ON_SERIALIZE $callback | |
| * @param 0|\ReflectionClass::SKIP_INITIALIZATION_ON_SERIALIZE $options |
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'd personally prefer to keep this as int so we don't need to update it every time a new option is added.
It would also mean that we could have issues if different PHP versions introduce different options that aren't supported by PHP versions the framework still supports.
I'd rather stick to what PHP itself uses.
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.
Then at least do ReflectionClass::*, which is not entirely correct, but still a lot more narrow than the wide open signed int
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.
Will leave that decision for Taylor.
| * @param array<string, mixed> $eager | ||
| * @return TValue | ||
| */ | ||
| function lazy($class, $callback = 0, $options = 0, $eager = []) |
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.
What if both $callback and $options are int?
lazy(MyClass::class); // $callback and $options both default to 0
lazy(MyClass::class, options: 42); // It will fail, because 0 cannot be called and newLazyGhost() does not accept 42 (only 0 and 8)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.
Don't hold it that way. Use the (yet to be) documented API.
It failing is a feature, not a bug.
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, but it would be better if it was explicitly validated so there could be a descriptive exception message. This can happen easily as the method signature doesn't clearly describe the options without consulting the docs.
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 we generally do this with other overloaded APIs. Marked as unresolved and will let Taylor chime in here, if he wants.
PHP's new lazy objects are very cool and useful for keeping the memory footprint low and improving performance in some use cases.
Unfortunately they are also super clunky to use.
Example of using the native API to create a ghost and a proxy:
Clunky?
Yes. Clunky.
__constructwhen creating a ghost vs returning a new instance when creating a proxy.$object->__constructfeels nasty.This PR introduces two new support helpers,
lazyandproxy, in hopes to make using these features more ergonomic.Re-worked example using the proposed helpers:
You may be thinking Hey, this is an older style Laravel API. We should be able to pass a single closure through like so:
If that is you, please hold back your feelings until the end. For now, I'll stick with the
lazy($class, $callback)API.Eagerly setting properties
Ghost objects allow you to eagerly set properties. Using the native APIs we would need the following:
The
lazyhelper allows you to pass eager values through as a named parameter using a hash map:Naming
I've opted for
lazyoverghostas a function name, even though PHP calls them ghosts.ghostmakes me feel like I'm gonna have to look up the difference between a ghost and a proxy every time I reach for them.lazyvsproxyfeels more clearer to me.lazyin the framework for similar things.Full API examples
Lazy
lazyis the go to when you are in full control of creating the object, e.g., if you would otherwise callnew Resultyourself, whether the class is in your namespace or a vendor namespace.Even with the more verbose escape hatch, it feels much more ergonomic to me. A comparison:
Proxy
proxyis what you would use for if you are not in control of instantiating the object and a 3rd party is in charge of creating the object. You can make make their instantiation logic lazy:Supplemental APIs
In similar APIs, Laravel has opted for determining the type from the closure. Although this is possible, I propose that we offer this as a secondary API, rather than the primarily documented API. I think it is fair to assume some people will attempt this API, based on previous Laravel experience, so it makes sense to support it. I'll outline below why I don't think it is a great primary API, though.
Given that majority of use-cases, in my opinion, are going to return an array of arguments, being forced to pass through the object as a variable to then never use does not feel nice:
Notice we never reference
$instanceor$proxyvariables in the closure. Sure, we could do take a leaf out of the JS book and use$_but it still feels off to me:I love other APIs that do this, but in all of those cases I want the thing I'm accepting. That isn't really the case with these helpers.
Comparing side-by-side for a vibes check:
The only time this particular API is an improvement on the suggested primary API is for the
lazyhelper when you need to call additional APIs when constructing, which also feels like a usage outlier:What if I only have a single argument to pass?
I went back and forth on this a bit, e.g., offering the ability to accept a single argument rather than an array:
If feel this adds ambiguity. What if I want to accept a single argument that is an array? You still need to wrap it in an array. Because of this, I felt keeping it simple and requiring a wrapping array was the best approach.