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

Added --minor and --patch CLI option in wp theme update #393

Merged
merged 9 commits into from Feb 5, 2024

Conversation

up1512001
Copy link
Contributor

@up1512001 up1512001 commented Jan 20, 2024

Fixes #394

Added missing CLI option for --minor and --patch into wp theme update command list.

Output Before Adding CLI Options for --minor & --patch

Screenshot 2024-01-20 at 17 21 30

Output After Adding CLI Options for --minor & --patch

Screenshot 2024-01-20 at 17 21 55

@up1512001 up1512001 requested a review from a team as a code owner January 20, 2024 11:40
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, @up1512001 !

Can you include some functional tests on this PR?

@swissspidy
Copy link
Member

OK so there are a couple of reasons why the tests are not currently working.

First, this PR right now only allows these two new args to be passed, but does not actually change the code to do anything with those args. Support for theme item types needs to be added in these two places:

if ( 'plugin' === $this->item_type
&& ( $minor || $patch ) ) {
$type = $minor ? 'minor' : 'patch';
$insecure = (bool) Utils\get_flag_value( $assoc_args, 'insecure', false );
$items_to_update = self::get_minor_or_patch_updates( $items_to_update, $type, $insecure, true );
}

private function get_minor_or_patch_updates( $items, $type, $insecure, $require_stable ) {

I just addressed this in 895e6ad

Second, and most importantly, the WordPress.org API by default does not actually return information about available versions for themes.

Here's the API response for Hello Dolly: https://api.wordpress.org/plugins/info/1.2/?action=plugin_information&request%5Blocale%5D=en_US&request%5Bslug%5D=hello-dolly

Here's the API response for Twenty Twelve: https://api.wordpress.org/themes/info/1.2/?action=theme_information&request%5Blocale%5D=en_US&request%5Bslug%5D=twentytwelve

See how there's no versions field.

To address this, we first need to add support for fetching additional fields. After those code modifications, this PR will work as expected.

See wp-cli/wp-cli#5893 for where I add this support.

@up1512001
Copy link
Contributor Author

Hi @swissspidy @danielbachhuber may I know any update regarding wp-cli/wp-cli#5893 PR and when can I expect feedback for my PR?

@swissspidy
Copy link
Member

This PR is already in good shape. Once wp-cli/wp-cli#5893 is merged, this PR only requires a dependency update and then it should be good to go.

I just pinged the other committers on wp-cli/wp-cli#5893 to review it.

@up1512001
Copy link
Contributor Author

up1512001 commented Feb 1, 2024

Hey @swissspidy I just checked the new theme update logic as I understand instead of getting info from style.css we will now fetch from the theme archive for its info am I right?
Thanks for updating it.

@swissspidy
Copy link
Member

Not sure what you mean, this doesn't really change how themes are looked up or anything.

The one thing I noticed while testing this patch is that filter_item_list() stored themes with their full directory path as the key. However, plugins are only stored with their slug. This caused problems when filtering the theme transient, as it turns out that didn't really work before, because we were using the full directory path and not the theme slug. That's why there are these changes to use get_stylesheet() instead of get_stylesheet_directory() for example.

Another interesting find was that the theme update transient is a list of arrays, whereas for plugins it's a list of objects apparently. So the PR now also caters for that.

As you can see, all existing tests are still passing and the new tests helped uncover these flaws.

@up1512001
Copy link
Contributor Author

Hey, @danielbachhuber as wp-cli/wp-cli#5893 this PR is merged can you please update this PR status?
Thank You.

@swissspidy swissspidy merged commit 5104a4b into wp-cli:main Feb 5, 2024
36 checks passed
@swissspidy swissspidy added this to the 2.1.18 milestone Feb 5, 2024
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.

Add --minor and --patch to wp theme update
3 participants