Skip to content

Fix exception in menu list command if the format is ids #3074#3075

Merged
danielbachhuber merged 4 commits into
wp-cli:masterfrom
apertureless:3074
Jun 28, 2016
Merged

Fix exception in menu list command if the format is ids #3074#3075
danielbachhuber merged 4 commits into
wp-cli:masterfrom
apertureless:3074

Conversation

@apertureless
Copy link
Copy Markdown
Contributor

@apertureless apertureless commented Jun 28, 2016

Fixes #3074


🐛 Bug

Command: wp menu list --format=ids

If the format was ids, the whole menu array with wp_terms object got passed to the display_items() method. However the formatter could not convert the wp_terms object to a string.

🚑 Fix

Checked before passing the whole menu object array to the display_items method, if the format is ids.
If yes, grabbed out the term_ids and passed only an array of ids to the display_items() method.


Don't really know whats best practice here. You could also implode the $ids array directly, without passing it to display_items(). I checked other commands that have the --format=ids option, but they are a bit inconsistent. On some the ids get directly imploded and echo'ed out, on some they are passed to display_items(). But I think passing them to display_items() is better, if the display style get changed at some point.

The formatter could not iterate trough the WP_Term object.

Now it the format is 'ids' all term_ids getting grabbed from the array
and imploded in the display_items method
Add whitespace to match coding style
@danielbachhuber
Copy link
Copy Markdown
Member

Thanks for the PR, @apertureless. Can you include a test for this change too?

Don't really know whats best practice here. You could also implode the $ids array directly, without passing it to display_items(). I checked other commands that have the --format=ids option, but they are a bit inconsistent. On some the ids get directly imploded and echo'ed out, on some they are passed to display_items(). But I think passing them to display_items() is better, if the display style get changed at some point.

I agree — the previous implementations are inconsistent. Part of the challenge is that some resources have an inconsistent name for their 'id' attribute. What you've suggested makes sense to me.

@apertureless
Copy link
Copy Markdown
Contributor Author

Sure! Feature test added @danielbachhuber

@danielbachhuber danielbachhuber added this to the 0.24.0 milestone Jun 28, 2016
@apertureless
Copy link
Copy Markdown
Contributor Author

Ups 🙈 CI seems to fail.

I will rework the part with array_column. Seems that its accepting objects as an input only since PHP 7 .

My bad :c

array_column() only accepts an array of objects since php 7.0

For compatibility reasons with lower php versions, used array_map
instead
@apertureless
Copy link
Copy Markdown
Contributor Author

Okay, rewritten array_column to array_map. Now it should work with >= 5.3

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.

2 participants