[FrameworkBundle] Allow clearing private cache pools in cache:pool:clear #20810

chalasr · 2016-12-07T10:42:23Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	n/a

In a2a567d cache pools have been passed to public: false by default.
The problem is that in 3.2 the cache:pool:clear command has been introduced and uses Container::get($pool); for clearing them, but the service can't be found since it is private.

So I propose to add a global clearer being able to clear any pool.

Koc · 2016-12-07T10:55:25Z

❤️ command as service

nicolas-grekas · 2016-12-07T11:02:29Z

Changing the default behavior is a BC break so not possible. Solution 1. is simplest, yet it would make sense to allow clearing any pool with the command I agree. I'd prefer a 3rd option: use the cache pool clearer service to clear by name (method to be added on it to allow so.)
Not a bug though, but a new feature.

chalasr · 2016-12-07T12:02:28Z

@nicolas-grekas I would like to go for your 3rd option but I believe adding this new method will be difficult since the Psr6CacheClearer doesn't reference pools by id (see Psr6CacheClearer::addPool()). Any clue? Otherwise, solution 2 looks the only working solution for this feature.

Btw, could you please re-label this as a feature? Thanks

nicolas-grekas · 2016-12-07T12:25:12Z

Relabeling done.
Not sure about command as a service.
Since each pool can have a different cache clearer, I think we should not reuse the existing cache.default_clearer for that purpose but create a new service (another Psr6CacheClearer enhanced to fit the need?) dedicated to this use case.

chalasr · 2016-12-07T12:35:58Z

Thanks for the hints, let's see what it gives.

Status: needs work

chalasr · 2016-12-07T21:15:48Z

I added ~~a CachePoolClearer which is quite similar to the Psr6CacheClearer but~~ a CachePoolRegistry referencing pools by id, marked as final and internal. Not sure it is the best way to do it, but it make us able to clear any private cache pool from its id whithout changing the current behavior for cache clearers.

Status: needs review

chalasr · 2016-12-09T14:20:07Z

@nicolas-grekas changes done, I think we are almost good here

nicolas-grekas · 2016-12-09T15:13:06Z

src/Symfony/Component/HttpKernel/CacheClearer/Psr6CacheClearer.php

-        $this->pools[] = $pool;
+        @trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0. Pass an array of pools indexed by name to the constructor instead.', __METHOD__), E_USER_DEPRECATED);
+
+        if (!in_array($pool, $this->pools, true)) {


this block is unrelated to the feature of the PR, should be removed

It's about keeping BC. If someone rely on addPool(), the pool should still be cleared when calling clear(), right? The use of in_array is to avoid registering the same pool twice in this case. Maybe you've a better one?

I wouldn't care much: either people use the new way, or the deprecated one. We don't have to handle people doing both IMO.

nicolas-grekas · 2016-12-09T15:13:51Z

src/Symfony/Component/HttpKernel/CacheClearer/Psr6CacheClearer.php

+            throw new \InvalidArgumentException(sprintf('Cache pool not found: %s.', $name));
+        }
+
+        $this->pools[$name]->clear();


fir the shake of completeness: return $this->pools[$name]->clear();

nicolas-grekas · 2016-12-09T15:23:24Z

src/Symfony/Bundle/FrameworkBundle/Command/CachePoolClearCommand.php

+            if ($pool instanceof CacheItemPoolInterface) {
+                $pool->clear();
+            } else {
+                $globalClearer->clearPool($id);


Now I understand why you needed hasPool :)
This can throw in the middle of a clean because of a missing pool.
Then I propose to replace clearPool($pool) by PsrCacheClearer::getPools() and handle validation with the resulting array.
WDYT?

Either we keep hasPool()/clearPool() or we get all the pools and let the caller doing the filtering logic as you propose, it's debatable but I would prefer keep the clearer being a clearer rather than an accessor. As you prefer :)

let's go for hasPool :)

nicolas-grekas · 2016-12-09T17:28:17Z

👍
Status: reviewed

ogizanagi · 2016-12-10T11:21:44Z

Either cache.default_clearer or cache.global_clearer needs to be a kernel.cache_clearer, but not both, right?

nicolas-grekas · 2016-12-10T11:25:39Z

@ogizanagi which is the case: https://github.com/symfony/symfony/pull/20810/files#diff-1754c5372532c2b324d500472dd5df66R96

ogizanagi · 2016-12-10T11:28:40Z

Indeed. Forgot about the fact tags are not inherited anyway when using a parent definition. 👍

ogizanagi · 2016-12-10T11:32:12Z

However, I found it confusing to reuse the same class for the global cache clearing, even if it's not wired as a kernel cache clearer. What were the arguments against a dedicated CachePoolRegistry?

chalasr · 2016-12-10T11:54:16Z

There's no need for accessing pools but only clearing them and like this we can keep pools really private (not accessible from a public service).

ogizanagi · 2016-12-10T11:56:01Z

Ok. Let's move forward.

👍

fabpot · 2016-12-10T14:15:50Z

Thank you @chalasr.

…n cache:pool:clear (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [FrameworkBundle] Allow clearing private cache pools in cache:pool:clear | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a In a2a567d cache pools have been passed to `public: false` by default. The problem is that in 3.2 the `cache:pool:clear` command has been introduced and uses `Container::get($pool);` for clearing them, but the service can't be found since it is private. So I propose to add a global clearer being able to clear any pool. Commits ------- b71df3f [FrameworkBundle] Allow clearing private cache pools

…asr) This PR was merged into the 3.3-dev branch. Discussion ---------- [HttpKernel] Fix missing psr/cache dev requirement | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes, tests are broken | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Because #20810 added a test for the `Psr6CacheClearer` that mocks methods of the `CacheItemPoolInterface`. It should fix the travis build on master and deps=low Commits ------- bd59d75 [HttpKernel] Require psr/cache in dev

dunglas · 2016-12-14T06:36:58Z

Maybe that this PR breaks tests on master (https://travis-ci.org/symfony/symfony/jobs/183706820) but I don't get exactly what happens (I cannot reproduce in local).

chalasr · 2016-12-14T07:16:51Z

@dunglas tests are broken on 3.2 as well but this is on master only, so it is not the responsible. I tend to think that your first attempt #20906 was the good, but that tests will stay broken because we released it (maybe I'm wrong, I can't explain it myself).

carsonbot added Status: Needs Review Bug labels Dec 7, 2016

nicolas-grekas added Feature and removed Bug labels Dec 7, 2016

nicolas-grekas added this to the 3.x milestone Dec 7, 2016

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 7, 2016

chalasr force-pushed the framework/cache-pools-default-public branch from f7db3cc to 435f5e9 Compare December 7, 2016 19:17

chalasr changed the title ~~[Framework] Cache pools must be public for using cache:pool:clear~~ [HttpKernel] Allow clearing private cache pools in cache:pool:clearer Dec 7, 2016

chalasr changed the title ~~[HttpKernel] Allow clearing private cache pools in cache:pool:clearer~~ [FrameworkBundle][HttpKernel] Allow clearing private cache pools in cache:pool:clearer Dec 7, 2016

chalasr changed the base branch from 3.1 to master December 7, 2016 19:19

chalasr force-pushed the framework/cache-pools-default-public branch 9 times, most recently from f2f4465 to 54ee65b Compare December 7, 2016 21:04

carsonbot added Status: Needs Review and removed Status: Needs Work labels Dec 7, 2016

chalasr changed the title ~~[FrameworkBundle][HttpKernel] Allow clearing private cache pools in cache:pool:clearer~~ [FrameworkBundle][HttpKernel] Allow clearing private cache pools in cache:pool:clear Dec 7, 2016

chalasr force-pushed the framework/cache-pools-default-public branch from 54ee65b to e7bd1c3 Compare December 7, 2016 22:09

chalasr force-pushed the framework/cache-pools-default-public branch 2 times, most recently from 12d7fda to d38ca9d Compare December 9, 2016 14:02

chalasr closed this Dec 9, 2016

chalasr reopened this Dec 9, 2016

nicolas-grekas reviewed Dec 9, 2016

View reviewed changes

chalasr force-pushed the framework/cache-pools-default-public branch from d38ca9d to 7522dcf Compare December 9, 2016 16:04

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 9, 2016

chalasr force-pushed the framework/cache-pools-default-public branch 3 times, most recently from daac4e8 to 6b94068 Compare December 10, 2016 10:51

[FrameworkBundle] Allow clearing private cache pools

b71df3f

chalasr force-pushed the framework/cache-pools-default-public branch from 6b94068 to b71df3f Compare December 10, 2016 10:52

fabpot merged commit b71df3f into symfony:master Dec 10, 2016

chalasr deleted the framework/cache-pools-default-public branch December 10, 2016 14:19

chalasr mentioned this pull request Dec 13, 2016

[HttpKernel] Fix missing psr/cache dev requirement #20894

Merged

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle] Allow clearing private cache pools in cache:pool:clear #20810

[FrameworkBundle] Allow clearing private cache pools in cache:pool:clear #20810

chalasr commented Dec 7, 2016 •

edited

Loading

Koc commented Dec 7, 2016

nicolas-grekas commented Dec 7, 2016

chalasr commented Dec 7, 2016

nicolas-grekas commented Dec 7, 2016

chalasr commented Dec 7, 2016

chalasr commented Dec 7, 2016 •

edited

Loading

chalasr commented Dec 9, 2016 •

edited

Loading

nicolas-grekas Dec 9, 2016

chalasr Dec 9, 2016

nicolas-grekas Dec 9, 2016

nicolas-grekas Dec 9, 2016

nicolas-grekas Dec 9, 2016

chalasr Dec 9, 2016

nicolas-grekas Dec 9, 2016

chalasr Dec 9, 2016

nicolas-grekas commented Dec 9, 2016

ogizanagi commented Dec 10, 2016 •

edited

Loading

nicolas-grekas commented Dec 10, 2016

ogizanagi commented Dec 10, 2016

ogizanagi commented Dec 10, 2016

chalasr commented Dec 10, 2016

ogizanagi commented Dec 10, 2016

fabpot commented Dec 10, 2016

dunglas commented Dec 14, 2016 •

edited

Loading

chalasr commented Dec 14, 2016

[FrameworkBundle] Allow clearing private cache pools in cache:pool:clear #20810

[FrameworkBundle] Allow clearing private cache pools in cache:pool:clear #20810

Conversation

chalasr commented Dec 7, 2016 • edited Loading

Koc commented Dec 7, 2016

nicolas-grekas commented Dec 7, 2016

chalasr commented Dec 7, 2016

nicolas-grekas commented Dec 7, 2016

chalasr commented Dec 7, 2016

chalasr commented Dec 7, 2016 • edited Loading

chalasr commented Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Dec 9, 2016

ogizanagi commented Dec 10, 2016 • edited Loading

nicolas-grekas commented Dec 10, 2016

ogizanagi commented Dec 10, 2016

ogizanagi commented Dec 10, 2016

chalasr commented Dec 10, 2016

ogizanagi commented Dec 10, 2016

fabpot commented Dec 10, 2016

dunglas commented Dec 14, 2016 • edited Loading

chalasr commented Dec 14, 2016

chalasr commented Dec 7, 2016 •

edited

Loading

chalasr commented Dec 7, 2016 •

edited

Loading

chalasr commented Dec 9, 2016 •

edited

Loading

ogizanagi commented Dec 10, 2016 •

edited

Loading

dunglas commented Dec 14, 2016 •

edited

Loading