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

If the Legacy REST API plugin is auto-installed once, do not auto-install a second time. #47563

Merged
merged 6 commits into from
May 20, 2024

Conversation

barryhughes
Copy link
Member

@barryhughes barryhughes commented May 16, 2024

Currently:

  • When WooCommerce is updated, we check to see if the Legacy REST API is enabled.
  • If it is enabled, we automatically install the Legacy REST API plugin (for continuity reasons).
  • If the Legacy REST API plugin is subsequently removed, then on the next WooCommerce update we still perform the same check and will re-install (the legacy plugin) if needed.

This change makes it so automated installation only happens once. Therefore, if the plugin is subsequently removed merchants will A) need to manually re-install it if desired B) won't have to manually remove it on subsequent plugin updates.

To achieve this, these code changes are implemented:

  1. PluginInstaller will now maintain a new option, woocommerce_history_of_autoinstalled_plugins, in addition to the already existing woocommerce_autoinstalled_plugins. Both contain the same information, the difference is that the information in the new option is never deleted (the information in the old option is deleted if the plugin is manually reinstalled).
  2. PluginInstaller::maybe_install_legacy_api_plugin will now check if the Legacy REST API plugin is listed in either of the options (the new one should be enough, we check the old one too for robustness) and if so the plugin won't be installed (the woocommerce_skip_legacy_rest_api_plugin_auto_install filter still works as expected).
  3. A migration is also added to create the new option from the contents of the old one.

How to test the changes in this Pull Request:

Testing the usual flow

Setup:

  1. Start with a clean installation of WordPress.
  2. Install WooCommerce 8.8.0.
  3. You may benefit from a tool (WooCommerce Beta Tester, or WP CLI) that makes it easy to switch between versions.

Special note for those using Jurassic Ninja: if you start with the latest version of WooCommerce, that's fine, but make sure you:

  • Downgrade to 8.8.0.
  • Reset the installed version information stored in the database. Using WP CLI:
wp option set woocommerce_version 8.8.0

From there:

  1. Navigate to WooCommerce ▸ Settings ▸ Advanced ▸ Legacy API and enable the Legacy REST API.
  2. Update to WooCommerce 8.9.0. You should find that the Legacy REST API plugin has been automatically installed (consider refreshing the plugin admin list to see this):
Screenshot 2024-05-16 at 16 21 24
  1. Deactivate and delete the Legacy REST API plugin.
  2. Now upgrade to this branch. You may wish to do this by manually uploading the zip produced by the build live branches action and overwriting the existing copy of WooCommerce.
  3. The Legacy REST API plugin should not have been re-installed.

Testing the manual reinstall flow

From the current state:

  1. Deactivate and delete the Legacy REST API plugin, then reinstall and activate it manually, then deactivate and delete again.
  2. Run wp option get woocommerce_autoinstalled_plugins, it should say that the option doesn't exist.
  3. Run wp option get woocommerce_history_of_autoinstalled_plugins, you should get the plugin information.
  4. Trick WooCommerce into believing it's in an older version, so reinstalling the same version will trigger the install workflow again: wp option set woocommerce_version 8.8.0
  5. Reinstall WooCommerce from the zip of this branch: again, the Legacy REST API plugin should not have been re-installed.

🔌 woocommerce.zip (pre-built plugin zip for this branch—last updated 2024-05-17 13:30 UTC)

@github-actions github-actions bot added the plugin: woocommerce Issues related to the WooCommerce Core plugin. label May 16, 2024
Copy link
Contributor

github-actions bot commented May 16, 2024

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

@barryhughes barryhughes requested review from a team, jorgeatorres and Konamiman and removed request for a team May 16, 2024 23:39
@barryhughes barryhughes marked this pull request as ready for review May 16, 2024 23:39
Copy link
Contributor

github-actions bot commented May 16, 2024

Hi @jorgeatorres, @Konamiman,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

Copy link
Contributor

@Konamiman Konamiman left a comment

Choose a reason for hiding this comment

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

We can't use the woocommerce_autoinstalled_plugins option for this, because the information in this option is deleted if the plugin is reinstalled manually. We need to use a different option, something like woocommerce_history_of_autoinstalled_plugins.

This option holds the same data as woocommerce_autoinstalled_plugins,
the difference is that the data in this new option is never deleted.
It's also never updated, it just holds the information of the
first autoinstall for each plugin.
@alvarothomas alvarothomas added this to the 8.9.0 milestone May 17, 2024
@github-actions github-actions bot removed this from the 8.9.0 milestone May 17, 2024
Copy link
Contributor

@vedanshujain vedanshujain left a comment

Choose a reason for hiding this comment

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

LGTM!

@Konamiman Konamiman self-requested a review May 17, 2024 14:13
Copy link
Member

@jorgeatorres jorgeatorres left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member Author

@barryhughes barryhughes left a comment

Choose a reason for hiding this comment

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

Thanks for the amendments!

☑️ 8.8.0 → 8.9.0 (legacy plugin installed, then deleted) → this branch (legacy plugin not installed a second time)

☑️ 8.8.0 → straight to this branch (legacy plugin installed)

📝 Noting I am currently reliant on testing via JN, as I'm currently having some build problems with the mono-repo locally.


@Konamiman there was a side discussion about simplifying this down, so that the auto-installation logic is simply contained in a regular migration (in wc-update-functions.php).

This might avoid us having to track the auto-installation history, because (typically) that migration function will only be executed once, whenever a user upgrades to or past the specified version. Could this reduce complexity?

I'm also quite happy to move ahead with the current approach, so we can expedite inclusion in the upcoming release—but wanted to run this by you.

@vedanshujain vedanshujain merged commit 660b2bc into trunk May 20, 2024
26 of 27 checks passed
@vedanshujain vedanshujain deleted the fix/avoid-repeated-legacy-api-installation branch May 20, 2024 13:29
@github-actions github-actions bot added this to the 9.0.0 milestone May 20, 2024
@github-actions github-actions bot added the needs: analysis Indicates if the PR requires a PR testing scrub session. label May 20, 2024
@alvarothomas alvarothomas modified the milestones: 9.0.0, 8.9.0 May 20, 2024
github-actions bot pushed a commit that referenced this pull request May 20, 2024
…tall a second time. (#47563)

* If the Legacy REST API plugin is auto-installed once, do not auto-install a second time.

* Remove unused private constant (PluginInstaller records the info we need in a sitewide option).

* Introduce the woocommerce_history_of_autoinstalled_plugins option

This option holds the same data as woocommerce_autoinstalled_plugins,
the difference is that the data in this new option is never deleted.
It's also never updated, it just holds the information of the
first autoinstall for each plugin.

* Add a migration to create woocommerce_history_of_autoinstalled_plugins

* More robust check of previous autoinstall of the plugin

* Change migration to run in 8.9.1 instead of 9.0.0

---------

Co-authored-by: Nestor Soriano <konamiman@konamiman.com>
alvarothomas pushed a commit that referenced this pull request May 20, 2024
* If the Legacy REST API plugin is auto-installed once, do not auto-install a second time. (#47563)

* If the Legacy REST API plugin is auto-installed once, do not auto-install a second time.

* Remove unused private constant (PluginInstaller records the info we need in a sitewide option).

* Introduce the woocommerce_history_of_autoinstalled_plugins option

This option holds the same data as woocommerce_autoinstalled_plugins,
the difference is that the data in this new option is never deleted.
It's also never updated, it just holds the information of the
first autoinstall for each plugin.

* Add a migration to create woocommerce_history_of_autoinstalled_plugins

* More robust check of previous autoinstall of the plugin

* Change migration to run in 8.9.1 instead of 9.0.0

---------

Co-authored-by: Nestor Soriano <konamiman@konamiman.com>

* Prep for cherry pick 47563

---------

Co-authored-by: Barry Hughes <3594411+barryhughes@users.noreply.github.com>
Co-authored-by: Nestor Soriano <konamiman@konamiman.com>
Co-authored-by: WooCommerce Bot <no-reply@woocommerce.com>
@rodelgc rodelgc added needs: internal testing Indicates if the PR requires further testing conducted by Solaris status: analysis complete Indicates if a PR has been analysed by Solaris and removed needs: analysis Indicates if the PR requires a PR testing scrub session. labels May 22, 2024
@nigeljamesstevenson nigeljamesstevenson added fix release: 8.9.1 fix release: 8.6.1 Temporary label for fix release metrics retrieval and removed fix release: 8.9.1 fix release: 8.6.1 Temporary label for fix release metrics retrieval labels May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix release: 8.9.1 needs: internal testing Indicates if the PR requires further testing conducted by Solaris plugin: woocommerce Issues related to the WooCommerce Core plugin. status: analysis complete Indicates if a PR has been analysed by Solaris
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants