Remove docs recommendation to load AS with negative priority

[capuderg]

Hi,

I'm not sure if opening a GH issue first would be better, than editing the docs directly... but here I am :)

In our plugin, we were requiring the AS library via the plugins_loaded action with priority -10. This requiring/loading of AS is not working as expected. I think the loading goes into the "Theme usage" use case, if there are no other plugins registering AS and this caused a fatal PHP error once we tested it with WooCommerce 5.5 Nightly (Trunk) -> which has the newer AS 3.2.0 version in it (our plugin is using 3.1.6 for now).

The Fatal error was:

PHP Fatal error:  Cannot declare class ActionScheduler, because the name is already in use in /.../wp-content/plugins/woocommerce/packages/action-scheduler/classes/abstracts/ActionScheduler.php on line 10

A simple fix was to require the AS library as soon as our plugin initializes (before the plugins_loaded hook is fired). So that's why I think the removed part of this doc (this PR) is needed.

[barryhughes]

@capuderg mind sharing a minimal snippet to help me reproduce this?

For instance, with a plugin as follows (deliberately simple, I just wanted to understand where/when the problem occurs) and with WooCommerce also active and loading its own copy of Action Scheduler, I don't see an issue—regardless of how the test plugin's directory is named (ie, whether I make this load before or after WooCommerce):

<?php
/**
 * Plugin name: Test AS Init Scenarios
 * Description: https://github.com/woocommerce/action-scheduler/pull/713/
 */

function as_713() {
    require_once __DIR__ . '/action-scheduler/action-scheduler.php';
}

add_action( 'plugins_loaded', 'as_713', -10 );

(Probably obvious, but for clarity I'll note that Action Scheduler is included per the path you see in the require statement.)

What would I need to add to replicate?

[capuderg]

Hmm...

Our plugin loads after WooCommerce. I used the WooCommerce Nightly (Trunk) build which uses AS 3.2 and our plugin loaded the AS 3.1.6 version.

The only difference was that we had the require_once statement inside a class method load_action_scheduler, so it would be like: add_action( 'plugins_loaded', [ $this, 'load_action_scheduler' ], -10 ); but that doesn't make a difference...

Anyway, your example is correct and it didn't work for us -> caused a fatal error when WooCommerce required AS 3.2.0 after our plugin already loaded 3.1.6. Could you please test with this scenario (+ rename the plugin, so it will run after WooCommerce)?

Could it be that this condition was met: https://github.com/woocommerce/action-scheduler/blob/master/action-scheduler.php#L48 because the plugins_loaded hook did already fire?

[barryhughes]

I did already test loading it before and after WooCommerce (by changing the plugin directory name from a-test/ to z-test/) ... but I didn't test with AS 3.1.6 (my test plugin used 3.2.0). Let me see if that makes a difference.

[barryhughes]

Yep that would appear to be relevant: the problem happens when the test plugin includes 3.1.6 (but doesn't occur with 3.2.0). We'll check into this; it could be catching out other plugin combinations (including things like WooCommerce plus WooCommerce Subscriptions, depending on the respective versions).

[capuderg]

I think the plugin with AS 3.1.6 and is loaded after WooCommerce with AS 3.2.0 has a different function name (with version) here: https://github.com/woocommerce/action-scheduler/blob/master/action-scheduler.php#L28 and so it can go on and then run code inside this if block: https://github.com/woocommerce/action-scheduler/blob/master/action-scheduler.php#L48 because the plugins_loaded hook is running at that time and the condition is met?

…

On Wed, Jun 16, 2021 at 7:09 PM Barry Hughes ***@***.***> wrote: Yep that would appear to be relevant: the problem happens when the test plugin includes 3.1.6 (but doesn't occur with 3.2.0). We'll check into this; it could be catching out other plugin combinations (including things like WooCommerce plus WooCommerce Subscriptions, depending on the respective versions). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#713 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACKDTVVIP3KOPF45CERMB7LTTDLD5ANCNFSM46ZQOYNQ> .

[barryhughes]

Yep, that sounds right. I put together a fix, because even if we were to change the documentation going forward there could be plugins out there that are already following the plugins_loaded strategy for initializing Action Scheduler.

[capuderg]

@barryhughes yeah, that makes sense (a fix in AS rather than doc change), you can close this PR then.

Thank you for your quick investigation! 👍

[barryhughes]

OK, will go ahead and close (and thanks both for flagging this, and taking the time to talk it through) 👍🏽