Skip to content

Provide a resource allowing to invalidate the cache based on the return value of a static method #25984

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

Closed
stof opened this issue Jan 31, 2018 · 6 comments

Comments

@stof
Copy link
Member

stof commented Jan 31, 2018

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no
Symfony version 4.1

The use case is supporting interface like the EventSubscriberInterface one, where the static method is called at compile-time to configure the container. In such case, changing the implementation of the static method must invalidate the container.

Currently, we have 2 choices for that:

  • invalidate the container any time the class definition changes, by using $container->addObjectResource. This is working, but it invalidates it much too often, as changes might be in other methods instead. This leads to poor DX (having to wait for the cache being rebuilt) when the class is likely to be edited for another place.
  • [Config] Handle Service/EventSubscriberInterface in ReflectionClassResource #25976 is hardcoding special support for EventSubscriberInterface and ServiceSubscriberInterface (which are the 2 interfaces using this pattern in the core) in the existing ReflectionClassResource, converting the usage to $container->getReflectionClass in related compiler passes. This has 2 drawbacks:
    • it is not reusable for external bundles having a similar need for their own interface (forcing them to go for the first option with bad DX)
    • it applies this invalidation to these classes even if they were not registered in the corresponding compiler passes (and so these interfaces were irrelevant for them) but they were still handled through $container->getReflectionClass in a different place (I recognize this is a bit of an edge case)

The idea here would be to provide a new kind of resource implementing the invalidation for this case, separate from the ReflectionClassResource. This resource should be configurable for different interfaces.
The case of the EventSubscriberInterface could then be migrated to using this new resource, avoiding the hardcoded handling of it in ReflectionClassResource.

@sroze
Copy link
Contributor

sroze commented Jan 31, 2018

What if we just isolate the generation of the signature behind an interface? We would allow the default implementation (i.e. service) to be decorated and therefore extended.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 31, 2018

The derived resource should also accurately report its "string" version, which is used to deduplicate resources.

@nicolas-grekas
Copy link
Member

About this deduplication, we should find a way to replace the usage of this ReflectionClassResource by the user's, so that we don't track the same class twice all the time.

@nicolas-grekas nicolas-grekas removed this from the 4.1 milestone Apr 20, 2018
nicolas-grekas added a commit that referenced this issue May 11, 2019
…bscribers change (weaverryan)

This PR was merged into the 4.2 branch.

Discussion
----------

[Messenger] Making cache rebuild correctly when message subscribers change

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #31409
| License       | MIT
| Doc PR        | not needed

An edge-case that's identical to `EventSubscriberInterface`: when the return value of `getHandledMessages()` changes, the container needs to be rebuilt.

If you're wondering why these checks aren't in their own resource class, see #25984 - it's something we probably should do, but haven't done yet.

Commits
-------

d88446b Making cache rebuild correctly with MessageSubscriberInterface return values
@carsonbot
Copy link

Thank you for this suggestion.
There has not been a lot of activity here for a while. Would you still like to see this feature?

@carsonbot
Copy link

Friendly ping? Should this still be open? I will close if I don't hear anything.

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants