Skip to content

Conversation

@marksabbath
Copy link
Contributor

This is a HackDay PR 🎉

This PR should cover wp-cli/ideas#153

@marksabbath marksabbath requested a review from a team as a code owner November 11, 2023 04:33
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @marksabbath ! A few things to fix up.

}

if ( ! isset( $where['blog_id'] ) ) {
WP_CLI::error( 'Could not find a site with the user provided.' );
Copy link
Member

Choose a reason for hiding this comment

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

We should present an empty list here, instead of erroring.

}

if ( isset( $assoc_args['site__user_in'] ) ) {
$user = get_user_by( 'login', $assoc_args['site__user_in'] );
Copy link
Member

Choose a reason for hiding this comment

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

_in implies the argument supports a CSV of users. Should we accommodate that here?

Also, we should use WP_CLI\Fetchers\User instead of directly calling get_user_by(). It will handle the "invalid user" case for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Il'll take a look at WP_CLI\Fetchers\User.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the code to use WP_CLI\Fetchers\User and also updated the name of the flag to --user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danielbachhuber I needed to change the flag to site_user since it was not properly working with user. I guess it conflicts with some global flag or something.

Copy link
Member

Choose a reason for hiding this comment

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

I needed to change the flag to site_user since it was not properly working with user. I guess it conflicts with some global flag or something.

@marksabbath Yep, it does. site_user is a fine alternative.

@danielbachhuber
Copy link
Member

@marksabbath Are you still planning to finish this up?

@marksabbath
Copy link
Contributor Author

@marksabbath Are you still planning to finish this up?

Yes! I'll make sure to spend some time this week.

@danielbachhuber danielbachhuber added this to the 2.6.2 milestone Feb 6, 2024
@danielbachhuber danielbachhuber added command:site Related to 'site' command command:site-list Related to 'site list' command labels Feb 6, 2024
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

Looks great, @marksabbath ! Thanks for your work on this.

I added another test case with b016207

@danielbachhuber danielbachhuber merged commit 2b5d5d5 into wp-cli:main Feb 6, 2024
@schlessera schlessera changed the title Add filter site__user_in on wp site list Add filter site__user_in on wp site list Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

command:site Related to 'site' command command:site-list Related to 'site list' command

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants