-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DependencyInjection] [WIP] add a #[Memoize] method attribute #47099
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
Conversation
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 like this.
I left some comments before going sleep :)
*/ | ||
public function __invoke(string $className, string $method, array $arguments): string | ||
{ | ||
return hash('sha256', $className.$method.serialize($arguments)); |
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.
Just remember. You don't know what is inside in $arguments variable. May be anything, for example a closure instance that's impossible to serialize here.
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 totally agree. The KeyGenerator
is configurable to allow user to handle this kind of specific cases.
namespace Symfony\Component\DependencyInjection\Attribute; | ||
|
||
#[\Attribute(\Attribute::TARGET_CLASS)] | ||
class Memoizable |
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.
IMO we can skip this class as attribute, useless double check in compiler pass :)
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 used it to make a first filter instead of retrieving all services' method attributes.
By the way, this attribute could contain default configuration like TTL or cache pool.
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.
Yes, good idea. ;)
What about just using HttpCacheKernel? |
You're right, HttpCacheKernel is better if you want to cache a Response. |
Thanks for the proposal. |
Another note: once #47236 is merged, it should be possible to replace proxy-manager by code generation based on reflection + |
Thanks for the review! Indeed, this PR will bring a lot of code to maintain. |
Closing then. Thank you @Jean-Beru and congrats! 👶 |
Memoization is a common technique to avoid heavy computation by caching previous returned value.
This is a proposal to add
#[Memoize]
method attribute to Symfony in order to return a cached response using Cachecomponent. Actually more a POC than PR.
Proposed Usage:
Questions:
Before going further (refactoring, adding tests, etc.), it could be a good thing to get some comments because
each solution could take a lot of time to implement :
PR. Final services won't be supported used in this PR due to ProxyManager.
methods implemented to retrieve/call public properties/methods.
AccessInterceptorValueHolderFactory
if ProxyManager bridge is installed used in this PR.AccessInterceptorValueHolderGenerator
to generate the class and using a customPhpDumper
to save. Something similar as it's done for lazy services.Is it new?
A bundle already exists about memoization: https://github.com/RikudouSage/SymfonyMemoizeBundle but this PR differs: