Skip to content

Restructure config value retrieval#42

Merged
schlessera merged 12 commits intomasterfrom
41-restructure-config-retrieval
Jan 25, 2018
Merged

Restructure config value retrieval#42
schlessera merged 12 commits intomasterfrom
41-restructure-config-retrieval

Conversation

@schlessera
Copy link
Member

@schlessera schlessera commented Jan 25, 2018

wp config list now produces a list of all config variables, constants and included files.

wp config get <key> now retrieves the value of the requested config key.

Note: Default behavior is to retrieve whatever key happens to fit, across both constants and variables. In case there's an ambiguous key that happens to exist as both a variable and a constant (which should be pretty rare), it will throw an error and ask for an explicit --type=<type> to disambiguate.

@schlessera schlessera added command:config-get Related to 'config get' command command:config-list Related to 'config list' command labels Jan 25, 2018
@schlessera schlessera added this to the 1.1.8 milestone Jan 25, 2018
@schlessera schlessera requested a review from a team January 25, 2018 14:52
@schlessera
Copy link
Member Author

Fixes #41

README.md Outdated
### wp config get

Gets variables, constants, and file includes defined in wp-config.php file.
Get the value of a specific variable or constant defined in wp-config.php
Copy link
Contributor

Choose a reason for hiding this comment

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

Just first glance notice lack of third-person singular here and in list!

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

*/
public function get( $_, $assoc_args ) {
public function list_( $args, $assoc_args ) {
$path = Utils\locate_wp_config();
Copy link
Contributor

Choose a reason for hiding this comment

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

Another trivial one, there's a space at the beginning of the line here before the tab, and in 4 other places /^ \t/

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Copy/paste issue because I copied from a previous implementation I had already previously done.

Copy link
Contributor

@gitlost gitlost left a comment

Choose a reason for hiding this comment

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

Fab stuff. Made a few points but 2 are ignorable and the one about the test can be done later.

Edit: I'll leave you to merge. Have to go off now for a while.

public function list_( $args, $assoc_args ) {
$path = Utils\locate_wp_config();
if ( ! $path ) {
WP_CLI::error( "'wp-config.php' not found." );
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe here and for get, add a similar spiel as in Runner::check_wp_version() about Pass --path=path/to/wordpress

Copy link
Member Author

Choose a reason for hiding this comment

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


$strict = Utils\get_flag_value( $assoc_args, 'strict' );
if ( $strict && empty( $args ) ) {
WP_CLI::error( 'The --strict option can only be used in combination with a filter.' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a test to config-list.feature to invoke this error?

Copy link
Member Author

Choose a reason for hiding this comment

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

continue;
}

if ( false === strpos( $value['key'], $filter ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you have tests for case but was wondering should this case-insensitive as not strict? (Or maybe add a case-insensitive flag).

Copy link
Member Author

Choose a reason for hiding this comment

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

Case-insensitivity will probably open up a huge can of worms. I'd prefer to keep everything case-sensitive for now.

@schlessera schlessera merged commit e3d6749 into master Jan 25, 2018
@schlessera schlessera deleted the 41-restructure-config-retrieval branch January 25, 2018 22:36
schlessera added a commit that referenced this pull request Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

command:config-get Related to 'config get' command command:config-list Related to 'config list' command

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants