I like this feature 👍!

However, to me, it's better to spit this command into two: cache:pool:get and cache:pool:set. With get-set, we don't know exactly if the command get or set or how to set how to get...

About the output, I'm not convinced of the big green block. I prefer to have a small text like with messages of the second version in blue or green and have the dumped value before with a blank line.
Example:

$ php bin/console cache:pool:set my_key my_value
(In green) Cache item was successfully saved in "cache.app" "my_key"
(In blue) Cache item "cache.app" "my_key" is:

(Dumped value)

$ ...

I agree the cache key should just be a SymfonyStyle::section instead of a success. So if we go with multiple keys support, each section would be a cache key followed by their value.

@YaFou @ogizanagi defined 2 separate commands instead
for the display, i change to a simple green text

@ogizanagi i do not understand the So if we go with multiple keys support, wdym by this?

@ogizanagi i do not understand the So if we go with multiple keys support, wdym by this?

If we have to distinct get and set commands, the get one could accept multiple values at once:

$ bin/console cache:pool:get cache.app key1 key2 key3

key1
----

(Dumped value)


key2
----

(Dumped value)

key3
----

(Dumped value)

for the display, i change to a simple green text

My preference would go for simply outputting the result (or use sections (like above example) if we have a get command accepting multiple keys).
In the context of get, this [INFO] line does not bring much value as it only repeats command arguments. If you known what you're doing, this is useless.
In the context of set: using success was relevant to confirm an action was made.

@ogizanagi thx for the review, all are fixed/improved

i also added 2 tests, based on an existing one
(https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Tests/Functional/CachePoolsTest.php)
and relying on Redis as storage
should I use instead the arrayadapter (in memory) one?

many thanks

tests failure are unrelated:

1) Symfony\Component\Security\Http\Tests\Firewall\UsernamePasswordFormAuthenticationListenerTest::testHandleNonStringUsernameWith__toString with data set #0 (true)
Error: Class 'Symfony\Component\Security\Http\Tests\Firewall\GetResponseEvent' not found

i also added 2 tests [...]
and relying on Redis as storage
should I use instead the arrayadapter (in memory) one?

I indeed think we should not rely on redis for these test cases.
I'd suggest to not rely on a web/kernel test case neither, but simply instantiate the command yourself (see \Symfony\Component\Form\Tests\Command\DebugCommandTest as an example)

Btw, to me, this command can be shipped in the Cache component instead of the FrameworkBundle, so anyone using the component standalone could register these commands on their own.

tests failure are unrelated:

those are fixed on master, you can rebase.

@ogizanagi I moved the commands inside the Cache component

@nicolas-grekas when testing with $allPools, the var content is

^ array:6 [
  "cache.app" => null
  "cache.system" => null
  "cache.validator" => null
  "cache.serializer" => null
  "cache.annotations" => null
  "cache.property_info" => null
]

so it is not useful here, I've done like the WorkflowDumpCommand to lazy call the right service if existing from the container

Todo: if ok, add tests as commented above
Status: need review

when testing with $allPools, the var content is [...]

That's a bit more complex, otherwise the cache pool clearer wouldn't be able to do its job. I suppose it gets a service locator injected + the list of pool it contains. Then it can do it's job. We should be able to borrow this.

I've done like the WorkflowDumpCommand to lazy call the right service if existing from the container

This won't work, because pools are private services, isn't it?

@nicolas-grekas services cache.app and cache.system are public and so work here
i think those are the one the commands should deal with, and not interact with others "more internal"

also inside the CachePoolClearCommand there is a usage of the same logic $kernel->getContainer()->get($id) so i fear not understanding your

otherwise the cache pool clearer wouldn't be able to do its job

thx

services cache.app and cache.system are public and so work here

yes but all the other pools are not public and are perfectly legit to be used here, especially all the ones specified in the user config (I agree for system pools, they're not really intended for this command.

inside the CachePoolClearCommand there is a usage of the same logic $kernel->getContainer()->get($id) so

yes, but that's just a fallback when $this->poolClearer->hasPool($id) returns false

BTW, I just noticed that all the other cache:pool command are in FrameworkBundle. The new ones should be there too IMHO.

@nicolas-grekas ok i will revert the file move
and check how to inject/retrieve differenlty the pools here
thx

Do we really want these 2 commands in core? I'm asking the question as we are trying to not clutter the list of default commands and these new commands seem to cover a very edge case (which can easily be done via a custom command if needed).

@fabpot thank you for your pov here
according to the existing cache:pool:delete (inside core FrameworkBundle), adding those make the whole DX experience available at will

Do we really want these 2 commands in core?

unsure if the question is for me and/or others reviewers :)
i would personaly use them inside some automated scripts when deploying for example (to set a cache item for request listener to deactivate some feature while deploying)

seeing the reactions on the PR makes me think it can be usefull inside core

cheers!

Thanks for the PR @noniagriconomie. I'm closing as I agree with @fabpot. Your use case can be implemented with a simple command in your app. I'm also closing because the implementation works only with public pools. Not supporting private ones will create confusion (and supporting private pools is some work that doesn't look worth it compared to @fabpot's comment.)

@nicolas-grekas understood no problem, just wanted to give a try on the related issue :)

thanks anyway :)

Hi, I originally open the feature request, but I don't really understand why you don't give it a chance.
I can create the command by myself in my application but allow everybody to have the same feature was my intention.
@fabpot for the edge case, in a production ready system, get a fast feedback of the running system can solve many problems and reduce the time used to debug the configuration problems
@nicolas-grekas , what's the meaning of public vs private pools ? the method used for cache:delete cannot be used ?

Anyway, thanks for your try @noniagriconomie and for the time used to review

meaning of public vs private pools ?

cache pools are services, and services can be public or private. I'm referring to this concept.

the method used for cache:delete cannot be used ?

dunno, someone should give it a try.

But @fabpot's objection will remain. We could deprecate cache:pool:delete actually.