NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

pierredup wants to merge4 commits intosymfony:masterfrompierredup:cache-commands

Closed

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

pierredup wants to merge4 commits intosymfony:masterfrompierredup:cache-commands

Conversation

Copy link

Contributor

pierredup commentedFeb 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

carsonbot added Status: Needs Review FrameworkBundle Feature labels

Feb 19, 2018

pierredup force-pushed thecache-commands branch from06148c8 to26a049eCompare

February 19, 2018 11:12

nicolas-grekas added this to the4.1 milestone

Feb 19, 2018

nicolas-grekas reviewed

Feb 19, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/CachePoolDeleteCommand.php Outdated

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

Copy link

Member

nicolas-grekasFeb 19, 2018

Choose a reason for hiding this comment

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

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

pierredup force-pushed thecache-commands branch from26a049e to56dd080Compare

February 19, 2018 12:29

linaori reviewed

Feb 20, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/CachePoolDeleteCommand.php Outdated

		}

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

Copy link

Contributor

linaoriFeb 20, 2018

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

pierredupFeb 20, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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;

Copy link

ContributorAuthor

pierredupFeb 20, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekasFeb 21, 2018

Choose a reason for hiding this comment

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

go for an exception then :)

Destroy666x reviewed

Feb 20, 2018

View reviewed changes

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

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

Copy link

Destroy666xFeb 20, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekasFeb 22, 2018

Choose a reason for hiding this comment

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

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

Copy link

Destroy666xFeb 22, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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.

nicolas-grekas requested changes

Feb 21, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/CachePoolDeleteCommand.php Outdated

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

Copy link

Member

nicolas-grekasFeb 21, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

src/Symfony/Bundle/FrameworkBundle/Command/CachePoolDeleteCommand.php Outdated

		}

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

Copy link

Member

nicolas-grekasFeb 21, 2018

Choose a reason for hiding this comment

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

go for an exception then :)

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

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

		$io->success(sprintf('Cache item "%s" was successfully deleted.', $key));

Copy link

Member

nicolas-grekasFeb 21, 2018

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekasFeb 21, 2018

Choose a reason for hiding this comment

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

hum, maybe not actually :)

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

Feb 21, 2018

pierredup force-pushed thecache-commands branch 3 times, most recently from3451432 todccd939Compare

February 21, 2018 12:59

nicolas-grekas reviewed

Feb 21, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/CachePooltemDeleteCommand.php Outdated

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

Copy link

Member

nicolas-grekasFeb 21, 2018

Choose a reason for hiding this comment

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

missingI beforetem, nice typo :)

Copy link

Member

nicolas-grekasFeb 21, 2018

Choose a reason for hiding this comment

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

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

Add command to delete an item from a cache pool

89355ba

pierredup force-pushed thecache-commands branch fromdccd939 to89355baCompare

February 21, 2018 14:13

nicolas-grekas reviewed

Feb 22, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/CachePoolDeleteCommand.php Outdated

		$cachePool = $this->poolClearer->getPool($pool);

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

Copy link

Member

nicolas-grekasFeb 22, 2018

Choose a reason for hiding this comment

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

should be a notice

Copy link

ContributorAuthor

pierredupFeb 22, 2018

Choose a reason for hiding this comment

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

Right, forgot about this. Thanks

nicolas-grekas approved these changes

Feb 22, 2018

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

with one minor comment

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

Feb 22, 2018

Change warning to notice

e556003

chalasr approved these changes

Feb 22, 2018

View reviewed changes

chalasr reviewed

Feb 22, 2018

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/CachePoolDeleteCommand.php Outdated

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

Copy link

Member

chalasrFeb 22, 2018

Choose a reason for hiding this comment

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

The cache pool from which to delete an item?

pierredup added2 commits

February 22, 2018 15:40

Change pool description

6cfae6f

Fix typo

0e00b76

Copy link

Member

nicolas-grekas commentedMar 2, 2018

Thank you@pierredup.

nicolas-grekas closed this

Mar 2, 2018

nicolas-grekas added a commit that referenced this pull request

Mar 2, 2018

feature#26223[FrameworkBundle] Add command to delete an item from a…

1f7b9f0

… 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        | TBDCurrently 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 poolCommits-------fd43e81 [FrameworkBundle] Add command to delete an item from a cache pool