Avoid crash in InboxNotifications if woocommerce-admin is entirely disabled. #5954
Conversation
With Woocommerce's PR woocommerce/woocommerce#31991 applied, if woocommerce admin is disabled via the woocommerce_admin_disabled filter, the InboxNotifications block could crash.
There was a problem hiding this comment.
Thanks for creating this PR @colinleroy! This part of the codebase is far from my area of expertise, so I will probably request a second review from somebody else, but for now, I left one question.
src/InboxNotifications.php
Outdated
| $data_store = \WC_Data_Store::load( 'admin-note' ); | ||
| $note_ids = $data_store->get_notes_with_name( self::SURFACE_CART_CHECKOUT_NOTE_NAME ); | ||
|
|
||
| if ( ! apply_filters( 'woocommerce_admin_disabled', false ) ) { |
There was a problem hiding this comment.
I wonder if we should simply return early if woocommerce_admin_disabled is true. Something along these lines:
if ( apply_filters( 'woocommerce_admin_disabled', false ) ) {
return;
}If WC Admin is disabled, I don't think we need to continue running the logic that creates the Notification, because it will never show up for users, right?
There was a problem hiding this comment.
I think so, indeed :)
Aljullu
added
block: cart
There was a problem hiding this comment.
Thanks for updating this PR @colinleroy! Changes look good to me, but I will deffer final approval to somebody else.
@opr can you or somebody else from Rubik take a look at this one since it mostly affects C&C blocks?
There was a problem hiding this comment.
Thanks for doing this @colinleroy - please have a look at my inline comment, it's possible I have misunderstood something so please correct me if I have! 😁
| @@ -74,9 +73,12 @@ public static function create_surface_cart_checkout_blocks_notification() { | |||
| return; | |||
| } | |||
|
|
|||
| if ( apply_filters( 'woocommerce_admin_disabled', false ) ) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
Thanks for the answer! OK then I think this is good to go, I'll approve 🥳
|
Thank you folks too! Now I'll wait for my Core PR to get some attention, and I'm happy it won't break Blocks if/when merged :) |
Hi,
I am currently experimenting with completely disabling Woocommerce admin and/or Woocommerce blocks, for performance reasons, by entirely removing the instantiation of the main packages classes' init() from Woocommerce core.
This works well, but I noticed that if the InboxNotifications::create_surface_cart_checkout_blocks_notification() function is called without the WooCommerce Admin->Composer->Package->init() have been called, the InboxNotifications block fails to initialize, because it uses a WC-Admin class.
The crash does not happen with the current release of WooCommerce core, of course, but will if one of my PRs is approved: woocommerce/woocommerce#31991 or woocommerce/woocommerce#31970
Testing
Automated Tests
Manual Testing
Changelog
Prevent crash in InboxNotifications if woocommerce-admin has not been loaded at all.