Update Console Tool Usage for Cache and Index Operations

[davidalger]

Fixes bug in cache management console commands where --all is not applied by default

• The commands cache:flush, cache:clear, cache:enable and cache:disable now properly default to use --all when no types explicitly given
• Unit tests for cache management commands have been made simpler, resulting in less duplicate test code
• Unit tests added to verify --all is applied by default

[vancoz]

Hello @davidalger, thank you for your input. From my point of view this improvement is questionable.
Right now according to our documentation --all option is not mandatory and has no default value, you are going to add exception - after the change if type not defined --all option will have default value "true".

Please, let me check this behavior with our PO.

[davidalger]

@vancoz Thanks for the note. I do realize that it's effectively changes existing behavior of cache:flush. I'm calling it a bug because the help text says it should behave that way this PR causes it to now behave, and also brings it inline with the existing behavior of similar commands.

The following is current behavior of cache:flush with nothing specified:

dalger:10:03 PM:/www/mage2.dev develop $ bin/magento cache:flush
Flushed cache types:

Note that it says "Flushed cache types" The folks I've been working closely with on Magento 2 for the last month and a half find this very confusing, and if it's going to do nothing, it should ask you to specify cache types to flush.

How does it bring it inline with similar commands? The index:reindex one is a good example. It has a similar check which implies the --all where no index types are passed in the arguments list:

dalger:10:03 PM:/www/mage2.dev develop $ bin/magento index:reindex
Category Products index has been rebuilt successfully in 00:00:00
Product Categories index has been rebuilt successfully in 00:00:00
Product Price index has been rebuilt successfully in 00:00:00
Product EAV index has been rebuilt successfully in 00:00:02
Stock index has been rebuilt successfully in 00:00:00
Catalog Rule Product index has been rebuilt successfully in 00:00:10
Catalog Product Rule index has been rebuilt successfully in 00:00:10
Catalog Search index has been rebuilt successfully in 00:00:25

And why would I call this a bug fix vs an enhancement? Simple. Read the --help output and you'll notice is reads "if omitted, all caches will be affected" which documents an implicit --all

dalger:10:04 PM:/www/mage2.dev develop $ bin/magento cache:flush --help
Usage:
 cache:flush [--all] [--bootstrap="..."] [types1] ... [typesN]

Arguments:
 types                 List of cache types, space separated. If omitted, all caches will be affected

Options:
 --all                 All cache types

I hope you can see both why I fixed this, as well as that it is not deviating from existing practice, but pulling the command in-line with existing and normally expected behavior. If there is documentation on devdocs that indicates otherwise, I'd be happy to update that as well.

[vancoz]

Hi @davidalger, I think with this approach (clear all types if any type is specified) we do not need --all option at all.
Also I have checked indexer:reindex command, and I found that --all option is redundant, because it clears all caches if list of particular indexes not provided. So looks like --all option uses only in command definition.

Btw, we need to have discussion about this and in case if we decide to change behavior my proposal is to remove --all option and make decision what to clear based on types only. Also would be good to remove --all option from indexer:reindex command or fix current behavior.

What do you thunk about that?

Thank you.

[buskamuza]

Hi @davidalger , thanks for pointing to the issue.
My fear is that skipping the type may be a mistake and it will lead to cleaning all caches or reindexing all indexes. I believe, it may be dangerous, if used in production. But another question is whether it will be used in an environment, where such operation is dangerous?
I would better fix the help message and the behavior of the commands, so cache commands would say something in case no type is specified.

[davidalger]

@vancoz The --all option should remain, if only intended as an internal switching mechanism and/or explicit flag used for scripting purposes. I do not want to see this option removed. If it is removed, you will have to have functional switching logic in the execution body of commands vs being implemented at a more surface level.

@buskamuza This is a developer tool in an developer only environment. It is no more dangerous than allowing them SSH access where they could accidentally run rm -rf and delete all the store files. If someone with SSH access ran a cache:flush on an environment under such load that it caused problems…it's either an honest mistake or he doesn't belong in there to begin with. I appreciate the concern, but folks working with sites and needing to flush all the caches on a regular basis really don't like typing out needless flags to get the optimal default behavior. As a point of reference, flushing all cache types is the default behavior of the n98-magerun command that many Magento developers are accustomed to. It is formed habit to type cache:flush with no arguments; changing that habit means undue pain.

Beyond that though, if a command line tools is considered dangerous because the default behavior of a command might impact performance if accidentally run on a large production site under heavy load, then more concerning to me would be these giant buttons in the admin…where folks are working who really can't be expected to know that a cache flush could immediately tank performance when under heavy load.

[vancoz]

Hello @davidalger, we have response from PO team, and they decided to remove --all option from cache:flush and index:reindex commands.

Execution logic should be based on parameters, if array of particular types/caches is empty - the default behavior should be flush/reindex all.

Would you like to change it in scope of this PR?

[davidalger]

@vancoz I'll update the PR accordingly for both commands. Thanks for taking the time internally to determine forward direction on this

[davidalger]

@vancoz I've updated the PR, build is passing and I believe it's ready for your review. Following the updates I've made this PR effects the following:

• Updates cache commands to operate on all cache types when none have been explicitly specified
• Strips the --all argument from both cache and index commands
• Updates index commands to include better error messaging when invalid index types are specified as arguments (it now prints a list of valid types in like manner to cache:* commands)
• Unit tests for cache and index commands updated to account for change(s) in behavior
• Unit tests for cache management commands have been made simpler, resulting in less duplicate test code
• Integration application install procedure updated to imply all types vs using --all when disabling caches

[ghost]

Thanks, we'll update documentation in sync with this PR.

[vancoz]

@davidalger thank you very much. Let me review changes and accept it 👍

[vancoz]

Internal ticket MAGETWO-40553