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

[FrameworkBundle] Allow configuring taggable cache pools #26934

Merged
merged 1 commit into from
May 29, 2018

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Apr 15, 2018

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: true
            app.taggable_pool_with_separate_store_for_tags:
                tags: app.my_pool_for_tags

@ostrolucky
Copy link
Contributor

What is your opinion on making this default?

@nicolas-grekas
Copy link
Member Author

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

@weaverryan
Copy link
Member

weaverryan commented Apr 16, 2018

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 about taggable: 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 the app cache. I still think many users (for small-ish apps) will use that one pool.

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

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Apr 16, 2018

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-hint TaggableCacheInterface and done with autowiring (or manually inject cache.app.taggable)

@weaverryan
Copy link
Member

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 keep tags then

@nicolas-grekas nicolas-grekas modified the milestones: 4.1, 4.2 Apr 20, 2018
@nicolas-grekas
Copy link
Member Author

Moving to 4.2.
ping @symfony/deciders

$pools[$id] = new Reference($id);
foreach ($clearer->getArgument(0) as $name => $ref) {
if ($container->hasDefinition($id = (string) $ref)) {
$pools[$name] = new Reference($id);
Copy link
Member

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
Member Author

Choose a reason for hiding this comment

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

updated

$definition = new ChildDefinition($pool['adapter']);

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

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
Member Author

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
Copy link
Member Author

vote pending @symfony/deciders

@nicolas-grekas nicolas-grekas merged commit 896be4c into symfony:master May 29, 2018
nicolas-grekas added a commit that referenced this pull request May 29, 2018
…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#9652

This PR adds a new configuration option for cache pools:
```yaml
framework:
     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 nicolas-grekas deleted the fwb-cache-tag branch May 31, 2018 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants