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

Change the moment we check for activate/deactivate features #2410

Merged

Conversation

Rahmon
Copy link
Contributor

@Rahmon Rahmon commented Oct 14, 2021

Description of the Change

This PR changes the moment we check for activate/deactivate features updating the hook from init to admin_init on the class Features.

Alternate Designs

Benefits

Possible Drawbacks

The check for activate/deactivate features will not run on WP-CLI commands.

Verification Process

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

Closes: #2362

Changelog Entry

@Rahmon
Copy link
Contributor Author

Rahmon commented Oct 18, 2021

@felipeelia I changed a little bit the initial approach in this PR. Moving the functions handle_feature_activation and setup_features from the hook init to admin_init break some tests because inside those functions other functions are added to the hook init.

So, instead of moving to admin_init, I'm checking if is_admin() where we change the statuses of the features.

@@ -31,7 +31,6 @@ class Features {
* @since 2.1
*/
public function setup() {
// hooks order matters, make sure feature activation goes before features setup
Copy link
Member

Choose a reason for hiding this comment

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

This is actually a comment we should keep, shouldn't we @Rahmon ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I added back 😬

update_site_option( 'ep_feature_requirement_statuses', $new_requirement_statuses );
} else {
update_option( 'ep_feature_requirement_statuses', $new_requirement_statuses );
if ( is_admin() ) {
Copy link
Member

Choose a reason for hiding this comment

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

As is, this seems to be breaking WP-CLI index. These are the steps to reproduce the problem I'm seeing @Rahmon:

  1. Create a multisite, enable EP for the entire network and WooCommerce just on the main site
  2. Run wp elasticpress index --setup --network-wide --yes
  3. Visit /?s=woo-hoodie&post_type=product in the main site and see 3 products (found by SKU)
  4. Visit /wp-admin/plugins.php?plugin_status=all&paged=1&s in the second site (the one without WC)
  5. Run wp elasticpress index --setup --network-wide --yes again
  6. Visit /?s=woo-hoodie&post_type=product in the main site and see no product found

It seems #4 removes the WC feature (what would be expected, as WC is not available on that site), and then on #5 (as the WC feature is inactive) SKUs are not indexed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated to check for a WP-CLI context

@felipeelia felipeelia assigned Rahmon and unassigned felipeelia Oct 18, 2021
@Rahmon Rahmon assigned felipeelia and unassigned Rahmon Oct 18, 2021
@felipeelia felipeelia merged commit 5366ced into develop Oct 18, 2021
@felipeelia felipeelia deleted the chore/change-ep_feature_requirement_statuses-check branch October 18, 2021 17:55
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.

Persistant database updates for ep_feature_requirement_statuses on multisite
2 participants