NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[FrameworkBundle] Allow configuring taggable cache pools#26934

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:masterfromnicolas-grekas:fwb-cache-tag

May 29, 2018

Merged

[FrameworkBundle] Allow configuring taggable cache pools#26934

nicolas-grekas merged 1 commit intosymfony:masterfromnicolas-grekas:fwb-cache-tag

May 29, 2018

Conversation

Copy link

Member

nicolas-grekas commentedApr 15, 2018•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#25903
License	MIT
Doc PR	symfony/symfony-docs#9652

This PR adds a new configuration option for cache pools:

framework:cache:pools:app.taggable_pool:tags:trueapp.taggable_pool_with_separate_store_for_tags:tags:app.my_pool_for_tags

nicolas-grekas added this to the4.1 milestone

Apr 15, 2018

carsonbot added Status: Needs Review FrameworkBundle Feature labels

Apr 15, 2018

nicolas-grekas force-pushed thefwb-cache-tag branch from8f0b6bd to88f22a2Compare

April 15, 2018 14:51

Copy link

Contributor

ostrolucky commentedApr 15, 2018

What is your opinion on making this default?

Copy link

MemberAuthor

nicolas-grekas commentedApr 15, 2018

Tags involve extra round-trips so this cannot be the default IMHO.

Copy link

Member

weaverryan commentedApr 16, 2018•
edited
Loading

Wooooo! So much easier than currently :). It looks liketags: true (the simplest config) just re-uses the same adapter for tagging. I like that. What do you think abouttaggable: true - it makes it sound a bit more like we're adding a "behavior"?

And, is there a way that a user could add tagging to theapp cache. I still think many users (for small-ish apps) will use that one pool.

Update: About the taggableapp cache - it looks like#26929 adds this, without any config. That makes even more sense.

Copy link

MemberAuthor

nicolas-grekas commentedApr 16, 2018•
edited
Loading

taggable: true

A service id can also be given here, where tags will be stored. This would then look like:
taggable: some_pool_for_tags. If you're OK, I'm OK :)

add tagging to the app cache

not needed as you spotted in#26929: just type-hintTaggableCacheInterface and done with autowiring (or manually injectcache.app.taggable)

Copy link

Member

weaverryan commentedApr 16, 2018

A service id can also be given here, where tags will be stored. This would then look like:
taggable: some_pool_for_tags. If you're OK, I'm OK :)

Ah, of course! I think we should keeptags then

nicolas-grekas modified the milestones:4.1,4.2

Apr 20, 2018

Copy link

MemberAuthor

nicolas-grekas commentedApr 20, 2018

Moving to 4.2.
ping @symfony/deciders

xabbuh reviewed

Apr 20, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/CachePoolClearerPass.php Outdated

		$pools[$id] =newReference($id);
		foreach ($clearer->getArgument(0)as$name =>$ref) {
		if ($container->hasDefinition($id = (string)$ref)) {
		$pools[$name] =newReference($id);

Copy link

Member

xabbuhApr 20, 2018

Choose a reason for hiding this comment

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

I found this change a bit hard to read as$id is used in both loops. Can we solve that by renaming one of them?

Copy link

MemberAuthor

nicolas-grekasApr 20, 2018

Choose a reason for hiding this comment

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

updated

xabbuh reviewed

Apr 20, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

		$definition =newChildDefinition($pool['adapter']);

		if ($pool['tags']) {
		if ($config['pools'][$pool['tags']]['tags'] ??false) {

Copy link

Member

xabbuhApr 20, 2018

Choose a reason for hiding this comment

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

Do we really want to fail silently if you misconfigure a cache pool service for the tags by having a typo there?

Copy link

MemberAuthor

nicolas-grekasApr 20, 2018

Choose a reason for hiding this comment

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

I don't think we can do otherwise: you should be allowed to reference a service you created manually.
For the "typo" case, this will create a Reference to a missing service, so you'll still get an error in the end.

nicolas-grekas force-pushed thefwb-cache-tag branch from88f22a2 tod0ba134Compare

April 20, 2018 11:13

nicolas-grekas force-pushed thefwb-cache-tag branch 3 times, most recently frome6be2bf toca4ccb7Compare

May 19, 2018 07:38

carsonbot removed the Status: Needs Review label

May 21, 2018

weaverryan approved these changes

May 21, 2018

View reviewed changes

carsonbot added the Status: Reviewed label

May 21, 2018

Copy link

MemberAuthor

nicolas-grekas commentedMay 28, 2018

vote pending @symfony/deciders

michaelcullum approved these changes

May 28, 2018

View reviewed changes

nicolas-grekas force-pushed thefwb-cache-tag branch fromca4ccb7 to7651c60Compare

May 29, 2018 12:27

[FrameworkBundle] Allow configuring taggable cache pools

896be4c

nicolas-grekas force-pushed thefwb-cache-tag branch from7651c60 to896be4cCompare

May 29, 2018 13:12

nicolas-grekas merged commit896be4c intosymfony:master

May 29, 2018

nicolas-grekas added a commit that referenced this pull request

May 29, 2018

feature#26934[FrameworkBundle] Allow configuring taggable cache poo…

143628f

…ls (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[FrameworkBundle] Allow configuring taggable cache pools| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#25903| License       | MIT| Doc PR        |symfony/symfony-docs#9652This PR adds a new configuration option for cache pools:```yamlframework:     cache:         pools:            app.taggable_pool:                tags: true            app.taggable_pool_with_separate_store_for_tags:                tags: app.my_pool_for_tags```Commits-------896be4c [FrameworkBundle] Allow configuring taggable cache pools

nicolas-grekas deleted the fwb-cache-tag branch

May 31, 2018 18:36