Ina2a567d cache pools have been passed topublic: false by default.
The problem is that in 3.2 thecache:pool:clear command has been introduced and usesContainer::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.

carsonbot added Status: Needs Review Bug labels

Dec 7, 2016

Copy link

Contributor

Koc commentedDec 7, 2016

❤️ command as service

Copy link

Member

nicolas-grekas commentedDec 7, 2016

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.

Copy link

MemberAuthor

chalasr commentedDec 7, 2016

@nicolas-grekas I would like to go for your 3rd option but I believe adding this new method will be difficult since thePsr6CacheClearer doesn't reference pools by id (seePsr6CacheClearer::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 added Feature and removed Bug labels

Dec 7, 2016

nicolas-grekas added this to the3.x milestone

Dec 7, 2016

Copy link

Member

nicolas-grekas commentedDec 7, 2016

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.

Copy link

MemberAuthor

chalasr commentedDec 7, 2016

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

Status: needs work

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Dec 7, 2016

chalasr force-pushed theframework/cache-pools-default-public branch fromf7db3cc to435f5e9Compare

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 from3.1 tomaster

December 7, 2016 19:19

chalasr force-pushed theframework/cache-pools-default-public branch 9 times, most recently fromf2f4465 to54ee65bCompare

December 7, 2016 21:04

Copy link

MemberAuthor

chalasr commentedDec 7, 2016•
edited
Loading

I added~~a CachePoolClearer which is quite similar to thePsr6CacheClearer but~~ aCachePoolRegistry 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

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 theframework/cache-pools-default-public branch from54ee65b toe7bd1c3Compare

December 7, 2016 22:09

chalasr force-pushed theframework/cache-pools-default-public branch 2 times, most recently from12d7fda tod38ca9dCompare

December 9, 2016 14:02

Copy link

MemberAuthor

chalasr commentedDec 9, 2016•
edited
Loading

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

chalasr closed this

Dec 9, 2016

chalasr reopened this

Dec 9, 2016

nicolas-grekas reviewed

Dec 9, 2016

View reviewed changes

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

		$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)) {

Copy link

Member

nicolas-grekasDec 9, 2016

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

chalasrDec 9, 2016

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekasDec 9, 2016

Choose a reason for hiding this comment

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

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.

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

		thrownew \InvalidArgumentException(sprintf('Cache pool not found: %s.',$name));
		}

		$this->pools[$name]->clear();

Copy link

Member

nicolas-grekasDec 9, 2016

Choose a reason for hiding this comment

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

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

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

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

Copy link

Member

nicolas-grekasDec 9, 2016

Choose a reason for hiding this comment

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

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) byPsrCacheClearer::getPools() and handle validation with the resulting array.
WDYT?

Copy link

MemberAuthor

chalasrDec 9, 2016

Choose a reason for hiding this comment

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

Either we keephasPool()/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 :)

Copy link

Member

nicolas-grekasDec 9, 2016

Choose a reason for hiding this comment

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

let's go for hasPool :)

Copy link

MemberAuthor

chalasrDec 9, 2016

Choose a reason for hiding this comment

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

done!

chalasr force-pushed theframework/cache-pools-default-public branch fromd38ca9d to7522dcfCompare

December 9, 2016 16:04

Copy link

Member

nicolas-grekas commentedDec 9, 2016

👍
Status: reviewed

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Dec 9, 2016

chalasr force-pushed theframework/cache-pools-default-public branch 3 times, most recently fromdaac4e8 to6b94068Compare

December 10, 2016 10:51

[FrameworkBundle] Allow clearing private cache pools

b71df3f

chalasr force-pushed theframework/cache-pools-default-public branch from6b94068 tob71df3fCompare

December 10, 2016 10:52

Copy link

Contributor

ogizanagi commentedDec 10, 2016•
edited
Loading

Eithercache.default_clearer orcache.global_clearer needs to be akernel.cache_clearer, but not both, right?

Copy link

Member

nicolas-grekas commentedDec 10, 2016

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

Copy link

Contributor

ogizanagi commentedDec 10, 2016

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

Copy link

Contributor

ogizanagi commentedDec 10, 2016

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 dedicatedCachePoolRegistry?

Copy link

MemberAuthor

chalasr commentedDec 10, 2016

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).

Copy link

Contributor

ogizanagi commentedDec 10, 2016

Ok. Let's move forward.

👍

Copy link

Member

fabpot commentedDec 10, 2016

Thank you@chalasr.

fabpot merged commitb71df3f intosymfony:master

Dec 10, 2016

fabpot added a commit that referenced this pull request

Dec 10, 2016

feature#20810[FrameworkBundle] Allow clearing private cache pools i…

6751dc1

…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/aIna2a567d 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

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

nicolas-grekas added a commit that referenced this pull request

Dec 13, 2016

minor#20894[HttpKernel] Fix missing psr/cache dev requirement (chal…

5b91ec8

…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/aBecause#20810 added a test for the `Psr6CacheClearer` that mocks methods of the `CacheItemPoolInterface`. It should fix the travis build on master and deps=lowCommits-------bd59d75 [HttpKernel] Require psr/cache in dev

Copy link

Member

dunglas commentedDec 14, 2016•
edited
Loading

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).

Copy link

MemberAuthor

chalasr commentedDec 14, 2016

@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).

fabpot mentioned this pull request

May 1, 2017

Release v3.3.0-BETA1#22603

Merged

Labels

Feature Status: Reviewed

7 participants

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

chalasr commentedDec 7, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Koc commentedDec 7, 2016

Uh oh!

nicolas-grekas commentedDec 7, 2016

Uh oh!

chalasr commentedDec 7, 2016

Uh oh!

nicolas-grekas commentedDec 7, 2016

Uh oh!

chalasr commentedDec 7, 2016

Uh oh!

chalasr commentedDec 7, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

chalasr commentedDec 9, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedDec 9, 2016

Uh oh!

ogizanagi commentedDec 10, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commentedDec 10, 2016

Uh oh!

ogizanagi commentedDec 10, 2016

Uh oh!

ogizanagi commentedDec 10, 2016

Uh oh!

chalasr commentedDec 10, 2016

Uh oh!

ogizanagi commentedDec 10, 2016

Uh oh!

fabpot commentedDec 10, 2016

Uh oh!

dunglas commentedDec 14, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

chalasr commentedDec 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

chalasr commentedDec 7, 2016•
edited
Loading

chalasr commentedDec 7, 2016•
edited
Loading

chalasr commentedDec 9, 2016•
edited
Loading

ogizanagi commentedDec 10, 2016•
edited
Loading

dunglas commentedDec 14, 2016•
edited
Loading