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 --force-check flag to wp plugin list and wp theme list. #426

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

baizmandesign
Copy link

Hi @danielbachhuber,

In the process of updating my fork, I somehow accidentally closed the existing pull request for this feature.

Here's the code and tests. Please let me know if you have any feedback!

Saul

@baizmandesign baizmandesign requested a review from a team as a code owner May 7, 2024 05:25
@ernilambar
Copy link
Member

@baizmandesign Please make sure all tests except PHP 8.4 are passing in Actions.

Signed-off-by: Saul Baizman <saul@baizmandesign.com>
@baizmandesign
Copy link
Author

Hi, @ernilambar. I added another commit, and it failed three tests: 2 related to PHP 8.4 and one test due to a GitHub API rate limit issue. Is there a way to re-run the one failed test unrelated to PHP 8.4?

@swissspidy
Copy link
Member

PHP 8.4 tests are expected to fail, PHP 8.4 is still in alpha and WP-CLI is not yet compatible. Feel free to ignore those.

@baizmandesign
Copy link
Author

Thanks for the info, @swissspidy. Is there a way to re-run the one test unrelated to PHP 8.4 that failed?

@baizmandesign baizmandesign force-pushed the feature/plugin-theme-force-check-183 branch from 5043fba to 9c85840 Compare May 14, 2024 14:44
@baizmandesign
Copy link
Author

Hi, @ernilambar. I've made the changes you requested. Please let me know if there's anything else I need to do. Thanks!

@BrianHenryIE
Copy link
Member

Looks good to me; tests run locally.

I like the When I run wp eval-file update-transient.php trick.

@baizmandesign
Copy link
Author

Hi, guys. Any update on the status of this pull request? Thanks!

@ernilambar ernilambar requested a review from swissspidy June 3, 2024 16:40
@swissspidy swissspidy requested a review from a team June 3, 2024 16:57
@mrsdizzie
Copy link
Member

I notice that we use --force in most other commands for similar behavior. Would it be better to name this --force and not --force-check to match existing commands?

@swissspidy
Copy link
Member

It's a good question. For commands like install or update, the intention of --force seems clear.

But for wp plugin/theme list, only --force seems too ambiguous to me.

@BrianHenryIE
Copy link
Member

--force makes sense in terms of something being written which is already known. It's more of a push word, whereas the point of this PR is to pull/fetch the latest information. I found myself using --refresh in a premium plugin licence update command I was writing.

I do see --skip-check elsewhere, so --force-check makes sense as its opposite. Also worth considering that --flag automatically comes with --no-flag (utils.php) (Configurator.php).

--force-check is probably fine here.

In terms of premium plugins, this does now suggest that when the update_plugins transient value does not have the premium plugin's update status, it should fetch it from its remote server and not to use cached information (which is fine, just not so explicit before).

@mrsdizzie mrsdizzie added this to the 2.1.24 milestone Dec 19, 2024
@mrsdizzie
Copy link
Member

Tempting to just remove the flag and make this the default behavior if we're also having the discussion of a 3.0 release that can include 'breaking' changes wp-cli/wp-cli#6026

I know personally I always want this command to do a live check if I'm running it directly from the command line and have been bitten in the past looking for a security update that doesn't show because it wasn't there the last time wp itself checked

@swissspidy
Copy link
Member

Having both force-check and skip-update-check is certainly confusing 🤔

@mrsdizzie
Copy link
Member

Having both force-check and skip-update-check is certainly confusing 🤔

I agree it feels weird to have both a --force-check and --skip-update-check (I don't really understand the benefit of --skip-update-check but I know we can't remove it now). It implies the state of updates is really just an unknown which doesn't seem right for this tool. This is another reason to have it just always check for updates (imo), and the current flag could prevent that if wanted.

I understand why WordPress core shouldn't do an api call for every visit to the plugins page, but I'm not sure wp cli needs that same restriction (and I think most people using wp cli would prefer the --force behavior).

If we don't want to make a force check the default behavior I think we just need to accept this new flag as awkward but merge this PR. Personally I've gotten used to the more awkward wp transient delete --all; wp plugin list so it would be an improvement on that :)

@baizmandesign
Copy link
Author

@mrsdizzie I'm the author of this PR, and I would love the default behavior of WP CLI to be to always check for the latest versions of plugins and themes and that this --force-check flag wasn't necessary at all. It's confusing (and frustrating) that running the list commands only occasionally checks for the newest versions of plugins and themes—whose behavior is unstated and whose "occasional" frequency is unknown. I do not recall how I eventually learned that to retrieve the most up-to-date plugin and theme versions, I had to delete transients, but I recall this step was not obvious.

Adding this functionality as the default behavior would not be a breaking change (I don't think), and I suspect it would prevent a small amount of confusion for a subset of WP CLI users.

@swissspidy
Copy link
Member

WP-CLI has typically avoided changing default behavior when it comes to cases like this with transients/cache. So in many cases the user needs to manually do that. (see also this related discussion)
This situation here is similar to wp core check-updates , where wp_version_check() defaults to a transient unless forced. But wp_update_plugins() and wp_update_themes() don't have such a flag. One needs to manually delete the transients. These commands have skip-update-check though, but that doesn't make sense when omitting it doesn't actually force the check. I would consider that a bug.

So to me it makes sense to change the default behavior to force (i.e. delete the transient) unless skip-update-check is used.

@mrsdizzie
Copy link
Member

Could probably just put a delete_site_transient( $this->upgrade_transient ); inside here above the call_user_func

// Force WordPress to check for updates if `--skip-update-check` is not passed.
if ( false === (bool) Utils\get_flag_value( $assoc_args, 'skip-update-check', false ) ) {
call_user_func( $this->upgrade_refresh );
}

Interesting that comment suggests to me that this should already be happening

@swissspidy
Copy link
Member

Yeah that's where I would put it 👍

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.

None yet

6 participants