Skip to content
This repository has been archived by the owner on Feb 23, 2024. It is now read-only.

Avoid crash in InboxNotifications if woocommerce-admin is entirely disabled. #5954

Merged
merged 6 commits into from
Mar 2, 2022
Merged

Avoid crash in InboxNotifications if woocommerce-admin is entirely disabled. #5954

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5bef052
Avoid calling plugin_dir_url a lot
colinleroy Feb 22, 2022
0178cc7
Merge branch 'woocommerce:trunk' into trunk
colinleroy Feb 25, 2022
df9de0b
Fix crash if woocommerce admin is disabled
colinleroy Feb 25, 2022
eb0ed6e
Fix coding style
colinleroy Feb 25, 2022
278213e
Simplify code path
colinleroy Mar 1, 2022
2ef02f9
Remove unused initialization
colinleroy Mar 1, 2022
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
6 changes: 4 additions & 2 deletions src/InboxNotifications.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ public static function delete_surface_cart_checkout_blocks_notification() {
* Creates a notification letting merchants know about the Cart and Checkout Blocks.
*/
public static function create_surface_cart_checkout_blocks_notification() {

// If this is the feature plugin, then we don't need to do this. This should only show when Blocks is bundled
// with WooCommerce Core.
if ( Package::feature()->is_feature_plugin_build() ) {
Expand All @@ -74,9 +73,12 @@ public static function create_surface_cart_checkout_blocks_notification() {
return;
}

if ( apply_filters( 'woocommerce_admin_disabled', false ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm misunderstanding, but where is this filter added? What would make it return true/false? I see your PRs in WooCommerce, but I don't see anything that would change the value of this filter.

Would it be better to check for the existence of the WC_Data_Store class instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @opr ,
This filter can be added by plugins and/or themes. For example, this plugin adds it : https://wordpress.org/plugins/disable-dashboard-for-woocommerce/ (I dislike its name, as it's not "bloat", but optional features).
For now, this filter is not checked early, and if woocommerce_admin_disabled is set to true, it's only checked after construction of the Woocommerce Admin objects, so it's not a problem for this Woocommerce Block.
If my Woocommerce Core PR is accepted though, it'd allow to completely skip Admin's initialization, and that makes this Block crash on init.
Checking for the existence of WC_Data_Store is does just lines before my hunk, but it isn't enough, as the class exists but is not initialized.
For reference, here is the backtrace of the error that this PR fixes when woocommerce/woocommerce#31991 is applied :

PHP Fatal error: Uncaught Exception: Invalid data store. in /home/colin/www/wordpress/wp-content/plugins/woocommerce/includes/class-wc-data-store.php:107
Stack trace:
#0 /home/colin/www/wordpress/wp-content/plugins/woocommerce/includes/class-wc-data-store.php(139): WC_Data_Store->__construct()
#1 /home/colin/www/wordpress/wp-content/plugins/woocommerce/packages/woocommerce-blocks/src/InboxNotifications.php(77): WC_Data_Store::load()
#2 /home/colin/www/wordpress/wp-content/plugins/woocommerce/packages/woocommerce-blocks/src/Domain/Bootstrap.php(84): Automattic\WooCommerce\Blocks\InboxNotifications::create_surface_cart_checkout_blocks_notification()
#3 /home/colin/www/wordpress/wp-includes/class-wp-hook.php(305): Automattic\WooCommerce\Blocks\Domain\Bootstrap->Automattic\WooCommerce\Blocks\Domain{closure}()
#4 /home/colin/www/wordpress/wp-includes/class-wp-hook.php(331): WP_Hook->apply_filters()
#5 /home/colin/www/wordpress/wp-includes/plugin.php(474): WP_Hook->do_action()

HTH,
Colin

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Yes this helps, and yeah I see we already check the existence of the class, my bad.

So what if someone uses your changes suggested in your PR to add something to WC_DISABLED_PACKAGES but doesn't add a callback to handle this filter? Would it be useful for https://github.com/woocommerce/woocommerce/pull/31970/files to add these filters when checking what is contained in WC_DISABLED_PACKAGES?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @opr,
True, that's the inconvenient of this one https://github.com/woocommerce/woocommerce/pull/31970/files versus this one. woocommerce/woocommerce#31991. I think the second one is better in that it uses Wordpress' preferred mode of interacting with upstream code, using filters, and because of that PR here. I think I'll close the first one because of that :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the answer! OK then I think this is good to go, I'll approve 🥳

return;
}

$data_store = \WC_Data_Store::load( 'admin-note' );
$note_ids = $data_store->get_notes_with_name( self::SURFACE_CART_CHECKOUT_NOTE_NAME );

// Calculate store's eligibility to be shown the notice, starting with whether they have any plugins we know to
// be incompatible with Blocks. This check is done before checking if the note exists already because we want to
// delete the note if the merchant activates an ineligible plugin.
Expand Down