Skip to content
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

Introduce wp theme mod list #100

Merged
merged 8 commits into from May 29, 2018

Conversation

2 participants
@montu1996
Copy link
Contributor

montu1996 commented May 27, 2018

Added new command wp theme mod list.

@montu1996

This comment has been minimized.

Copy link
Contributor Author

montu1996 commented May 27, 2018

@schlessera can you please review this PR?

* +------------------+---------+
*
*/
public function list( $args = array(), $assoc_args = array() ) {

This comment has been minimized.

@schlessera

schlessera May 28, 2018

Member

list is a protected keyword in more recent versions of PHP, so you cannot use it as a method name.

Use list_ instead, and then add the @subcommand list annotation to the docblock, similar to how it is done here: https://github.com/wp-cli/extension-command/blob/master/src/Plugin_Command.php#L1006-L1008

$assoc_args['all'] = 1;
}
// If no format was given ,set to default 'table'.
if ( ! array_key_exists( 'format', $assoc_args ) ) {

This comment has been minimized.

@schlessera

schlessera May 28, 2018

Member

Setting the default --format should be unnecessary, as the default is already provided in https://github.com/wp-cli/extension-command/pull/100/files#diff-03f3f9875c58703b8a8855ca3e5ca77eR138 and will be enforced by the system.

@@ -121,6 +121,52 @@ public function get( $args = array(), $assoc_args = array() ) {
}
/**
* Gets all theme mods.

This comment has been minimized.

@schlessera

schlessera May 28, 2018

Member

Should be Gets a list of theme mods. to match similar commands.

*
* ## OPTIONS
*
* [<mod>...]

This comment has been minimized.

@schlessera

schlessera May 28, 2018

Member

This one should be removed, as the filtering is overridden by the --all flag.

@@ -0,0 +1,38 @@
Feature: Manage WordPress theme mods list

This comment has been minimized.

@schlessera

schlessera May 28, 2018

Member

The tests should be written with a list of theme mods that actually contain values, otherwise the tests could just succeed by random chance.

This comment has been minimized.

@schlessera

schlessera May 28, 2018

Member

You can use something like this to pre-populate the list:

Background:
  Given a WP install
  And I run `wp theme mod set key_a value_a`
  And I run `wp theme mod set key_b value_b`

This comment has been minimized.

@montu1996

montu1996 May 28, 2018

Author Contributor

@schlessera Done. can you please check again? 😃

Mitesh added some commits May 28, 2018

@schlessera
Copy link
Member

schlessera left a comment

Only 2 small changes, and then this is good for merging...

-
key: key_b
value: value_b
"""

This comment has been minimized.

@schlessera

schlessera May 29, 2018

Member

Code Style: Always end files with a new line to avoid text processing issues on exotic platforms.

This comment has been minimized.

@montu1996

montu1996 May 29, 2018

Author Contributor

Done.....

This comment has been minimized.

@montu1996

montu1996 May 29, 2018

Author Contributor

Done

* @subcommand list
*/
public function list_( $args = array(), $assoc_args = array() ) {
if ( ! array_key_exists( 'all', $assoc_args ) ) {

This comment has been minimized.

@schlessera

schlessera May 29, 2018

Member

Now that I think about this some more, I'd prefer to have it just always forcefully set $assoc_args['all'] to 1 here. If you check for existence of the key first, a user could do something like this, which would produce an error:

wp theme mod list --no-all

So please just remove the if ( ! array_key_exists( ... ) { } that wraps the assignment.

@schlessera schlessera added this to the 1.1.10 milestone May 29, 2018

Mitesh

@schlessera schlessera merged commit 941e7e3 into wp-cli:master May 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@schlessera

This comment has been minimized.

Copy link
Member

schlessera commented May 29, 2018

Thanks for the pull request, @montu1996 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.