When WooCommerce Subscription is installed with folder name other than woocommerce-subscriptions, WCPay might return fatal error

[shendy-a8c]

Describe the bug

When WooCommerce Subscriptions is installed to a folder with name other than woocommerce-subscriptions, eg wp-content/plugins/woocommerce-subscriptions-4.6.0, this code block that checks if WCS is installed won't think WCS is installed, so WCPay will try to load its own subscriptions-core and since both WCS and WCPay are loading their subscriptions-core, store will get fatal error.

To Reproduce

1. Install and activate WCPay.
2. Install and activate WooCommerce Subscriptions to a folder named wp-content/plugins/woocommerce-subscriptions-unusual. You can unzip the package and move the folder to wp-content/plugins folder.

Actual behavior

Notice the Fatal error: Cannot declare class WCS_Core_Autoloader.

Expected behavior

Shouldn't result in fatal error and all functionalities should work.

Additional context

Slack: p1671110964952559-slack-C02BW3Z8SHK.

[haszari]

Raising priority to high, I think we should prioritise tidying this up. If custom folder name is not supported, we should fail gracefully and document the requirement. Ideally we'd support this cleanly (if practical).

Next steps here:

1. Confirm what is expected/correct behaviour of WooCommerce extensions with custom folder name. Is a custom folder name supported or not recommended?
2. Investigate if it's feasible for us (WC Subscriptions) to support custom folder name.
3. Decide on plan of action based on the above & implement. Options:
   • Fix the loader code to better handle arbitrary folder name for WCS.
   • Prevent the fatal error from firing, and ensure we gracefully handle this situation. Consider updating docs to guide merchants who experience this problem.

This is another symptom of the fact that we are shipping a library (subscriptions core), and implementing our own library loading code manually. Long term it would be good to investigate options for loading subscriptions core using standard techniques (e.g. composer and/or autoloader) rather than hacking a custom solution.

[haszari]

This is another symptom of the fact that we are shipping a library (subscriptions core), and implementing our own library loading code manually. Long term it would be good to investigate options for loading subscriptions core using standard techniques (e.g. composer and/or autoloader) rather than hacking a custom solution.

FYI @jrodger keen for some input or ideas from architects team on the best ways to share and load subscriptions-core code. cc @james-allan @mattallan as y'all have relevant experience here (FYI, no need to take action)

[jrodger]

If memory serves there was an intentional decision not to use the Jetpack autoloader for this, it would be good to dig up the reasoning for that if we'd like to revisit it. I'm afraid my quick search is coming up empty, I don't suppose anyone involved in that original plan can point me in the right direction?

[haszari]

If memory serves there was an intentional decision not to use the Jetpack autoloader for this, it would be good to dig up the reasoning for that if we'd like to revisit it. I'm afraid my quick search is coming up empty, I don't suppose anyone involved in that original plan can point me in the right direction?

@mattallan can you track down an answer or info (on P2)? I figure you're closest to the decisions & plan for subs-core, and you're review rotation now too anyway :)

[RadoslavGeorgiev]

@haszari is this issue on Helix's radar? We're prioritizing the WCPay backlog, and we're not sure if you are going to handle it.

[haszari]

Thanks for the reminder @RadoslavGeorgiev 😀

This has fallen back in the priorities, since it's an edge case. However I think the reasoning above stands – the way we load subscriptions-core library should be more robust.

Tagging in @james-allan (porter) or @mattallan (subs-core expert) – can you comment with some options for how we might address this, or track down rationale for not using an autoloader (e.g. Jetpack)?

@RadoslavGeorgiev if Gamma has bandwidth to take a look, we'd be appreciative.

[james-allan]

or track down rationale for not using an autoloader (e.g. Jetpack)?

I don't know if this is the original rationale but this quote from the Jetpack autoloader readme is kinda a deal breaker for our needs.

"This is a custom autoloader generator that uses a classmap to always load the latest version of a class." src: https://github.com/Automattic/jetpack-autoloader#a-custom-autoloader-for-composer

Subscriptions-functionality from subscription-core is only used by WC Subscriptions or WC Payments - not both at once. That imo is an important distinction. The Jetpack autoloader is more useful in situations where you have a library being used by multiple plugins where the APIs are shared.

Action Scheduler is a good candidate for this kind of thing. All sorts of plugins may make use of Action Scheduler APIs and using the latest one consistently across all plugins is important.

But, because subscriptions-core is only loaded by WC Subscriptions or WC Payments - depending on which plugin is active - we don't actually want the most recent version to be loaded, we want the version of subscriptions-core that the subscription-enabling plugin is compatible with using - ie the one it is packaged with.

If we wanted to move to a Jetpack autoloader model we would probably need to introduce compatible versioning eg. WC Subscriptions x.y.z is compatible with subscriptions-core a.b.c -> d.f.g. Because if someone doesn't update WC Subscriptions for a while and then installs/activates WC Payments, all of a sudden WC Subscriptions is running the latest version of subscriptions-core and they might not be compatible.

This could also lead to strange behaviour where subscription functionality changes after installing WC Payments. With that in mind and because the subscriptions-core library isn't loaded by both plugins at once creating a conflict, which is what the Jetpack autoloader is designed to resolve, we didn't really need it.

---

can you comment with some options for how we might address this

I haven't tested this myself but I think the solution here is to loosen the is_plugin_active() check.

is_plugin_active() requires you to pass the plugin slug in the following format dir/plugin-main-file.php (eg woocommerce-subscriptions/woocommerce-subscriptions.php). If the plugin is installed at woocommerce-subscriptions-5.4.0/woocommerce-subscriptions.php, this condition fails and causes the error.

We can loosen this by just checking if the woocommerce-subscriptions.php main plugin file is loaded by a plugin at any dir. eg

• Loop over get_option( 'active_plugins' );
• Split each plugin slug by the /
• Check if woocommerce-subscription.php is active.

[haszari]

Thanks @james-allan - that makes sense to me, subs-core is loaded in parallel (unversioned, each client plugin loads its own copy). I can see that implementing versioning of the "library" APIs is not practical at this stage – the api surface is too large (since this was essentially pulled out of WC S as is).

Making the check more lenient sounds like a good pragmatic solution.

I am still curious about other standardised ways of managing this library code. It does feel like we're manually managing this and if we can use something standard that might help us move faster.

[shendy-a8c]

@james-allan's suggestion aligns with what @mattallan suggested here p1671075205151369/1670982533.558779-slack-C7U3Y3VMY, ie loosening is_plugin_active() to just look for woocommerce-subscriptions.php (not including the plugin's folder name.