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.

Sign up for GitHub

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
merged 9 commits into from
Oct 18, 2021
Merged

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

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
1ecc579
Update to use admin_init hook
Rahmon Oct 14, 2021
bce6f63
Update to add hook if it's not AJAX
Rahmon Oct 14, 2021
76d884f
Revert "Update to add hook if it's not AJAX"
Rahmon Oct 14, 2021
e9ddfa8
Update to hook on WPCLI contexts
Rahmon Oct 14, 2021
534ebba
Revert to use init hook for handle_feature_activation and setup_features
Rahmon Oct 15, 2021
c9630f8
Check if it's admin request before try to handle status change on
Rahmon Oct 15, 2021
9d95965
Add comment that was removed
Rahmon Oct 18, 2021
fd20a0e
Check if we're in a WP-CLI context to check for features statuses
Rahmon Oct 18, 2021
9868f20
Merge branch 'develop' into chore/change-ep_feature_requirement_statu…
Rahmon Oct 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions includes/classes/Features.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 😬

add_action( 'init', array( $this, 'handle_feature_activation' ), 0 );
add_action( 'init', array( $this, 'setup_features' ), 0 );
}
Expand Down Expand Up @@ -196,10 +195,12 @@ public function handle_feature_activation() {
$new_requirement_statuses[ $slug ] = (int) $status->code;
}

if ( defined( 'EP_IS_NETWORK' ) && EP_IS_NETWORK ) {
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

if ( defined( 'EP_IS_NETWORK' ) && EP_IS_NETWORK ) {
update_site_option( 'ep_feature_requirement_statuses', $new_requirement_statuses );
} else {
update_option( 'ep_feature_requirement_statuses', $new_requirement_statuses );
}
}

/**
Expand Down Expand Up @@ -231,7 +232,7 @@ public function handle_feature_activation() {
* If a requirement status changes, we need to handle that by activating/deactivating/showing notification
*/

if ( ! empty( $old_requirement_statuses ) ) {
if ( is_admin() && ! empty( $old_requirement_statuses ) ) {
foreach ( $new_requirement_statuses as $slug => $code ) {
$feature = $this->get_registered_feature( $slug );

Expand Down