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

Add get-mapping WP-CLI Subcommand #2414

Merged
merged 8 commits into from
Oct 19, 2021
Merged

Add get-mapping WP-CLI Subcommand #2414

merged 8 commits into from
Oct 19, 2021

Conversation

felipeelia
Copy link
Member

This is merely a takeover of #2378.

Description of the Change

Using the wp elasticpress WP-CLI command, I am able to create and retrieve any or all indexes. However, I can put mappings only, but not retrieve.

This PR adds a new subcommand, wp elasticpress get-mapping, that allows for retrieving the current mappings:

wp elasticpress get-mapping

By passing an optional index name, the command will return the mapping for that index only:

wp elasticpress get-mapping --index-name=ep-examplecom-post-1

Alternate Designs

If I have access to WP-CLI, but don't have this subcommand, I could do the following:

wp shell

wp> wp_remote_retrieve_body( ElasticPress\Elasticsearch::factory()->remote_request( '[<index>/]_mapping' ) );

That is unneccesarily complicated.

Benefits

More complete API/feature set. Easier to use than wp shell command.

Possible Drawbacks

I don't see any.

Verification Process

I tested this locally, both with and without passing an index name.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

#2379

Changelog Entry

### Added
- New WP-CLI subcommand for retrieving mappings: `wp elasticpress get-mapping [--index-name]`. Props [@tfrommen](https://github.com/tfrommen) via [#2378](https://github.com/10up/ElasticPress/pull/2378).

* @param array $assoc_args Associative CLI args.
*/
public function get_mapping( $args, $assoc_args ) {
$index_names = (array) isset( $assoc_args['index-name'] ) ? $assoc_args['index-name'] : $this->get_index_names();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to chek if it's not empty ! empty( $assoc_args['index-name'] ). Using (array) isset( $assoc_args['index-name'] ) is always returning true. PHP is converting to array the result of isset: [ 0 => false ].

Consequently, join is receiving a NULL value and emitting a Warning.

public function get_mapping( $args, $assoc_args ) {
$index_names = (array) isset( $assoc_args['index-name'] ) ? $assoc_args['index-name'] : $this->get_index_names();

$path = join( ',', $index_names ) . '/_mapping';
Copy link
Contributor

Choose a reason for hiding this comment

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

When we pass an index name (string), we shouldn't call join because it expects an array.

@Rahmon Rahmon removed their assignment Oct 18, 2021
@felipeelia
Copy link
Member Author

Good catch @Rahmon, do you mind giving another look?

@felipeelia felipeelia assigned Rahmon and unassigned felipeelia Oct 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants