-
Notifications
You must be signed in to change notification settings - Fork 11.4k
[13.x] RFC: Lazy Services - New #[Lazy] Attribute #55645
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
If you move the redis connection out of the constructor and only establish a connection when a method that will interact with redis is actually called, then everything would work as you’d expect without the “invisible” downside of having to dig into the underlying class to see the attribute and understand its implications on the Laravel container. Laravel container is already extremely powerful and factory-driven in the sense that only when a class needs to be instantiated it incur overhead, so I don’t think this is a positive change to the framework overall. |
In your controller example, one could use method injection instead of a constructor injection: public function show(RedisService $redisService, int $productId): JsonResponse
{
$product = $redisService->getProduct($productId);
// …
} Furthermore, I agree with @deleugpn, and can't figure out places where this would bring more value, than what we already get from Laravel's container. |
$instance = $reflector->newLazyProxy(function () use ($concrete, $instances) { | ||
return new $concrete(...$instances); | ||
}); |
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.
You could do just
$instance = $reflector->newLazyProxy(function () use ($concrete, $instances) { | |
return new $concrete(...$instances); | |
}); | |
$instance = $reflector->newLazyProxy(fn () => new $concrete(...$instances)); |
@@ -1058,8 +1059,16 @@ public function build($concrete) | ||
|
||
array_pop($this->buildStack); | ||
|
||
if (!empty($reflector->getAttributes(Lazy::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.
You could just directly check this
if (!empty($reflector->getAttributes(Lazy::class))) { | |
if ($reflector->getAttributes(Lazy::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.
Does this change bring anything beyond syntax? I’m just asking out of curiosity.
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.
Not much. Technically, a function call is more expensive than a comparison, but this should mostly be optimized away in practice. One slight advantage would be a clear communication that we are working with an array here and don't have to check any structure's emptiness.
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.
@olekjs empty function is a trap for future bugs.
We have a rule in place to never use empty function because of that.
empty('0') is true for example.
I would agree that Lazy Objects support would be great, especially for services that require a huge set up when instanced. The main problem I have with Lazy objects is testing. Are these completely compatible with Mockery? |
@@ -15,7 +15,7 @@ | ||
} | ||
], | ||
"require": { | ||
"php": "^8.3", | ||
"php": "^8.4", |
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.
Laravel 13 will depends on PHP 8.3 as minimum.
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.
Alrighty, in that case we can consider version 14.x, but any potential merging should wait until 13.x is released - is that correct? :)
@deleugpn Thank you for the feedback :) I partially agree with you. However, I don’t fully understand what you mean by "only when a class needs to be instantiated it incurs overhead" - could you elaborate? @rodrigopedra Thanks for the feedback as well. It's great that you and @deleugpn provided an alternative approach to the example - it makes sense. However, it's just a simple example. I'm also considering a case where we have a class overloaded with dependencies, such as Carbon. Let’s be honest - Carbon is a heavy class and sometimes it can even freeze the IDE. I’m referring to lazy loading for such a class (of course, this is just a quick example): class ArticleService
{
public function __construct(
public Carbon $createdAt,
public Carbon $publishedAt,
// ...
) {
}
} @DarkGhostHunter could you explain where the claim that "The main problem I have with Lazy objects is testing" comes from? :D I don’t see any issue with testing - a Lazy Object gets initialized upon interaction and behaves like a regular object. You can see that in the tests in this PR - take a look, there’s an assertInstanceOf on the lazy object. If I misunderstood the question, please clarify or provide an example - I’ll try to test it and share some feedback ;) |
Nevermind, I though they weren't testeable. Keep up the good work. |
Never heard of this problem before, to be honest.
I get the idea. What I don't get is the benefits compared to what the container already does. It already runs a factory function only when needed. And you can use method injection, or the But maybe I am missing something. Let's wait on the maintainers. |
I don't feel a need for this feature, but maybe it will be better to define it in the constructor of the class where dependency is required (for example in the controller) and not in dependency class? Otherwise you can't define it in third party classes. |
Can you elaborate a bit more about why we couldn't support it at the parameter level? |
In short
A new attribute #[Lazy] has been introduced, which defers the initialization of a class until it is actually used.
Example
Let’s assume we have a service that creates a connection to Redis.
The above service is used in a controller:
In short. I have a service that handles the Redis connection. I have a controller that handles two endpoints. The first returns a list of products (Redis is not used). The second returns a product object – Redis is used as a cache.
In the current form, there is no Lazy Services support. Traditionally, I make a request to the product listing endpoint:
You can see that even though Redis is not used in the product listing endpoint, Redis is still initialized.
Now let’s add the previously mentioned “#[Lazy]” attribute to the service.
As you can see above. I added the special “#[Lazy]” attribute, which marks the class as Lazy, meaning it will be a Lazy Object in the container until the class is used.
Let’s see what happens now:
Send a request to the products endpoint skips loading Redis, why? Because it’s in the container as a Lazy Object. Only using Redis – in this case, fetching a single product – will trigger Redis to load.
Discussion and usage
In 2018 there was a proposal to implement Lazy Service in Laravel. On that occasion, a discussion arose:
As mentioned in the linked discussion above, Lazy Services are already implemented in:
First, a lot has changed since 2018, and second, it was just a proposal. In the meantime, Lazy Objects have been natively supported by PHP since version 8.4.
I decided to create a working POC and possibly start a discussion.
Challenges and future development opportunities
Here’s the Proof of Concept, and I’d appreciate your feedback. Feel free to join the discussion about this feature and whether there is a place for it in Laravel.