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. (wip) #399

Conversation

baizmandesign
Copy link

Hi @danielbachhuber,

Here's the work-in-progress pull request that you asked me to submit. Please see our comment thread on wp-cli/ideas for the history of our exchange regarding challenges creating tests for this new flag.

Please let me know if I can help in any other way.

Saul

PS. The contents of features/theme-list.feature can be ignored and likely needs to be completely rewritten. My plan was to work in features/plugin-list.feature and get that right first, then I was going to copy and paste any working code and replace "plugin" with "theme" where needed in features/theme-list.feature. But as you know things didn't proceed that far.

@baizmandesign baizmandesign requested a review from a team as a code owner February 14, 2024 23:18
@@ -0,0 +1,56 @@
Feature: List WordPress plugins

Scenario: Refresh update_plugins transient when listing plugins with --force-check flag
Copy link
Member

Choose a reason for hiding this comment

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

@baizmandesign What do you think of this Behat test refactor?

Copy link
Author

@baizmandesign baizmandesign May 2, 2024

Choose a reason for hiding this comment

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

I ran the behat tests, and they worked for me! Very exciting. A couple comments:

  • I'm unclear why the wp plugin list --fields=name,status,update command needs to be run twice after update-transient.php is executed.
  • Is it an issue that the version of the Hello Dolly plugin, 1.7.2, is hard-coded in the test?

if ( true === (bool) Utils\get_flag_value( $assoc_args, 'force-check', false ) ) {
delete_site_transient( $this->upgrade_transient );
}

Copy link
Member

Choose a reason for hiding this comment

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

What should happen if someone uses --force-check and --skip-update-check at the same time?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @danielbachhuber,

That's a good question. As currently written, --force-check is toothless if --skip-update-check is also passed to the command.

Here are a couple of proposals for resolving this matter (assuming we're solely talking about plugins, as an example):

  1. Let the code for both functions execute—delete the plugin transient and skip the call to wp_update_plugins()—and issue a warning: "Plugin updates were requested to be force-checked and were not checked due to the --skip-update-check argument."
  2. Make it so that the two options are mutually exclusive: you can run either one or the other. Running both results in an error: "Plugins cannot be both force-checked and skipped. Choose one."
  3. Let one command always "over-rule" the other. For example, we might decide that --skip-update-check should always prevail if both are present. It can proceed quietly (and not delete the plugin transient) or issue a warning ("Plugin updates were not force-checked due to the --skip-update-check argument."). On the other hand, we could decide that --force-check always over-rules --skip-update-check.

I think I'm partial to option 2 but can be convinced of choosing one of the other options (or alternatives unmentioned above).

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

@baizmandesign Option 2 works!

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, @danielbachhuber! I'll get to work on implementing the error and the theme version of the flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants