Skip to content

[Serializer] Need to clear cache when updating Annotation Groups on Entities #40856

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[Serializer] Need to clear cache when updating Annotation Groups on Entities #40856

wants to merge 3 commits into from

Conversation

monteiro
Copy link
Contributor

@monteiro monteiro commented Apr 18, 2021 •
edited
Loading

Q A
Branch? 5.2
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #40034
License MIT
Doc PR symfony/symfony-docs#...

Sample project that mimics this behavior: https://github.com/monteiro/PR-40856

The solution proposed is just appending an hash of the metadata, so it gets refreshed every time there is a change.
Don't know if this impacts performance, but fixes the issue in development which does not refresh the metadata of the annotation during development.

@monteiro monteiro requested a review from dunglas as a code owner April 18, 2021 07:15
@carsonbot carsonbot added this to the 5.2 milestone Apr 18, 2021
@carsonbot carsonbot changed the title [serializer] refresh cache if metadata is not the same [Serializer] refresh cache if metadata is not the same Apr 18, 2021
@monteiro monteiro mentioned this pull request Apr 18, 2021
Closed
@monteiro monteiro changed the title [Serializer] refresh cache if metadata is not the same [Serializer] Need to clear cache when updating Annotation Groups on Entities Apr 18, 2021
digilist
digilist reviewed Apr 18, 2021
View reviewed changes
src/Symfony/Component/Serializer/Mapping/Factory/CacheClassMetadataFactory.php
@@ -51,14 +51,14 @@ public function getMetadataFor($value)
return $this->loadedClasses[$class];
}

$key = rawurlencode(strtr($class, '\\', '_'));
$metadata = $this->decorated->getMetadataFor($value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this basically circumventing the cache now that the metadata is loaded here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can switch to another implementation of debug is enabled on the framework bundle extension. Need some help here so we can fix this issue that only happens in dev.

@nicolas-grekas
Copy link
Member

Closing as this cannot be the correct implementation. In #40034, I'm suggesting to unregister the serializer.mapping.cache_class_metadata_factory service in debug mode. Would you like to send a PR doing so?

@jc-oclock
Copy link

Thanks here.

@nicolas-grekas could you guide us (me ^^) to where is registered this service. It is a part of Symfony I never dig into.

@monteiro monteiro mentioned this pull request Jul 3, 2021
Merged
@monteiro
Copy link
Contributor Author

monteiro commented Jul 3, 2021 •
edited
Loading

@jc-oclock , did a new PR with Nicolas suggestion about this: #41961

You can take a look. If you have any doubts let me know.

@monteiro monteiro deleted the serializer-not-updating-dev-mode branch July 3, 2021 15:43
fabpot added a commit that referenced this pull request Jul 4, 2021
@fabpot
bug #41961 [Serializer] Need to clear cache when updating Annotation …
018b109 
…Groups on Entities (monteiro)

This PR was squashed before being merged into the 5.3 branch.

Discussion
----------

[Serializer] Need to clear cache when updating Annotation Groups on Entities

| Q             | A
| ------------- | ---
| Branch?       | 5.3 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #40034 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT

Sample project that mimics this behavior: https://github.com/monteiro/PR-40856

The solution was suggested by `@nicolas`-grekas [here](#40856).

Commits
-------

a6829be [Serializer] Need to clear cache when updating Annotation Groups on Entities
@jc-oclock
Copy link

jc-oclock commented Jul 6, 2021 •
edited
Loading

@jc-oclock , did a new PR with Nicolas suggestion about this: #41961

You can take a look. If you have any doubts let me know.

Thanks a lot. I checked the PR, if I understand well :

  • The task of the class Symfony\Bundle\FrameworkBundle\DependencyInjection\FrameworkExtension is to register all the services available in the service container, by default.
  • The method registerSerializerConfiguration() is in charge of registering Serializer's services, from the config file located in the Resources folder, which registers the serializer.mapping.cache_class_metadata_factory service.
  • Your PR unregisters this service if debug is active on the app.

Is it ok ?

Thank you for bringing me light in a part that was still obscure to me 💡 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@digilist digilist digilist left review comments

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

Assignees
No one assigned
Labels
Bug Serializer Status: Needs Work
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
5 participants
@monteiro @nicolas-grekas @jc-oclock @digilist @carsonbot