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] Add command to delete an item from a cache pool #26223

Closed
wants to merge 4 commits into
base: master
from

Conversation

@pierredup
Contributor

pierredup commented Feb 19, 2018

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 TBD

Currently there is no way to clear a specific item from a cache pool (except programatically), the entire pool needs to be cleared.
Especially during development, when implementing caching, it is useful to delete a specific key to test functionality. Clearing the entire pool, means that everything will need to be cached again, adding unnecessary execution time.

I propose adding a new command, cache:pool:delete to delete a specific item from a cache pool

$io = new SymfonyStyle($input, $output);
$pool = $input->getArgument('pool');
$key = $input->getArgument('key');
$cachePool = $this->getApplication()->getKernel()->getContainer()->get($pool);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 19, 2018

Member

This means the pool has to be public.
What about going through the Psr6CacheClearer instead?

}
if (!$cachePool->deleteItem($key)) {
return $io->error(sprintf('Cache item "%s" could not be deleted', $key));

This comment has been minimized.

@iltar

iltar Feb 20, 2018

Contributor

The warning and error functions do not return a status code, however, I would imagine that at least the error should return a status code other than 0.

This comment has been minimized.

@pierredup

pierredup Feb 20, 2018

Contributor

I'm aware that they don't return a status code, but for me this is just cleaner than

$io->warning(sprintf('Cache item "%s" does not exist in cache pool "%s"', $key, $pool));

return;

This comment has been minimized.

@pierredup

pierredup Feb 20, 2018

Contributor

For the error I think it might be better to rather throw an exception, which should then have a proper error return code set

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 21, 2018

Member

go for an exception then :)

*
* @author Pierre du Plessis <pdples@gmail.com>
*/
final class CachePoolDeleteCommand extends Command

This comment has been minimized.

@Destroy666x

Destroy666x Feb 20, 2018

Since this can only delete an item as specified in descriptions (key is required), wouldn't CachePooltemDeleteCommand be more accurate? Also, there's a tyop in the comment above.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 22, 2018

Member

This matches the name of the command, and is good enough IMHO as nothing else than an item can be deleted.

This comment has been minimized.

@Destroy666x

Destroy666x Feb 22, 2018

This matches the name of the command

I was thinking about renaming both.

as nothing else than an item can be deleted.

Not everyone may know that though and can have different expectations. But as long as it's documented well, it's ok, I guess.

$cachePool = $this->poolClearer->getPool($pool);
if (!$cachePool->hasItem($key)) {
return $io->warning(sprintf('Cache item "%s" does not exist in cache pool "%s"', $key, $pool));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 21, 2018

Member

I'd suggest to turn this into a notice, and to explicitly return instead ($io->warning doesn't return anything AFAIK)

}
if (!$cachePool->deleteItem($key)) {
return $io->error(sprintf('Cache item "%s" could not be deleted', $key));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 21, 2018

Member

go for an exception then :)

return $io->error(sprintf('Cache item "%s" could not be deleted', $key));
}
$io->success(sprintf('Cache item "%s" was successfully deleted.', $key));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 21, 2018

Member

I'd suggest to log only in verbose mode (same above)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 21, 2018

Member

hum, maybe not actually :)

*
* @author Pierre du Plessis <pdples@gmail.com>
*/
final class CachePooltemDeleteCommand extends Command

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 21, 2018

Member

missing I before tem, nice typo :)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 21, 2018

Member

but this doesn't match the name of the command
cache:pool:delete LGTM, so the previous name was OK IMHO

$cachePool = $this->poolClearer->getPool($pool);
if (!$cachePool->hasItem($key)) {
$io->warning(sprintf('Cache item "%s" does not exist in cache pool "%s".', $key, $pool));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Feb 22, 2018

Member

should be a notice

This comment has been minimized.

@pierredup

pierredup Feb 22, 2018

Contributor

Right, forgot about this. Thanks

@nicolas-grekas

with one minor comment

{
$this
->setDefinition(array(
new InputArgument('pool', InputArgument::REQUIRED, 'The cache pool to delete an item from'),

This comment has been minimized.

@chalasr

chalasr Feb 22, 2018

Member

The cache pool from which to delete an item?

pierredup added some commits Feb 22, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 2, 2018

Thank you @pierredup.

nicolas-grekas added a commit that referenced this pull request Mar 2, 2018

feature #26223 [FrameworkBundle] Add command to delete an item from a…
… cache pool (pierredup)

This PR was squashed before being merged into the 4.1-dev branch (closes #26223).

Discussion
----------

[FrameworkBundle] Add command to delete an item from a cache pool

| 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        | TBD

Currently there is no way to clear a specific item from a cache pool (except programatically), the entire pool needs to be cleared.
Especially during development, when implementing caching, it is useful to delete a specific key to test functionality. Clearing the entire pool, means that everything will need to be cached again, adding unnecessary execution time.

I propose adding a new command, `cache:pool:delete` to delete a specific item from a cache pool

Commits
-------

fd43e81 [FrameworkBundle] Add command to delete an item from a cache pool
private $poolClearer;
public function __construct(Psr6CacheClearer $poolClearer)

This comment has been minimized.

@xabbuh

xabbuh Mar 9, 2018

Member

Do we really want to inject the cache clearer to then use it to fetch a pool? Shouldn't we rather inject the pools directly?

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

@pierredup pierredup deleted the pierredup:cache-commands branch May 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment