Skip to content
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

Allow to set a cache to store configuration #2373

Merged
merged 1 commit into from
Dec 31, 2021

Conversation

franmomu
Copy link
Collaborator

@franmomu franmomu commented Dec 12, 2021

Fixes #2365, #2227

Instead of relying on the metadata cache from the orm/odm, this PR adds a MappedEventSubscriber::setCacheItemPool() method to configure the cache service, in case it is not provided it uses an ArrayAdapter (this will invalidate existing caches).

For the bundle, a service can be created (allowing users to configure their own) and call setCacheItemPool in every definition as setAnnotationReader (https://github.com/stof/StofDoctrineExtensionsBundle/blob/a2bffca41974d1c968557b343e269a60a8d5ffa4/src/Resources/config/blameable.xml#L13-L15)

@codecov-commenter
Copy link

codecov-commenter commented Dec 12, 2021

Codecov Report

Merging #2373 (10bf2ff) into main (609a763) will increase coverage by 0.08%.
The diff coverage is 84.84%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #2373      +/-   ##
============================================
+ Coverage     80.91%   81.00%   +0.08%     
- Complexity     3150     3152       +2     
============================================
  Files           159      159              
  Lines          8149     8160      +11     
============================================
+ Hits           6594     6610      +16     
+ Misses         1555     1550       -5     
Impacted Files Coverage Δ
src/Mapping/MappedEventSubscriber.php 89.61% <81.81%> (+9.32%) ⬆️
src/Mapping/ExtensionMetadataFactory.php 91.78% <90.90%> (-0.87%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 609a763...10bf2ff. Read the comment docs.

// Cache the result, even if it's empty, to prevent re-parsing non-existent annotations.
$cacheId = self::getCacheId($className, $this->extensionNamespace);

if (null === $this->cachePoolItem) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this up? No point in computing a cache ID if the cache pool isn't set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks! in a previous version I was setting the configuration to the metadata cache when $this->cachePoolItem was null I forgot to move it again.

@franmomu franmomu marked this pull request as ready for review December 25, 2021 21:16
@phansys phansys added Needs Changelog note Bug A confirmed bug in Extensions that needs fixing. labels Dec 28, 2021
@franmomu franmomu force-pushed the use_cache branch 2 times, most recently from 474ca92 to 72b1e69 Compare December 28, 2021 23:51
@franmomu franmomu mentioned this pull request Dec 30, 2021
1 task
@franmomu franmomu changed the title Use own cache to store configuration Allow to set a cache to store configuration Dec 31, 2021
@franmomu franmomu force-pushed the use_cache branch 2 times, most recently from 7e70827 to 4039035 Compare December 31, 2021 18:14
@phansys phansys merged commit 20570a6 into doctrine-extensions:main Dec 31, 2021
@phansys
Copy link
Collaborator

phansys commented Dec 31, 2021

Thanks @franmomu!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A confirmed bug in Extensions that needs fixing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use our cache pool to store configurations
4 participants