Woocommerce-admin: Build: Modernize PHP Classes for Autoloader

[timmyc]

See:

https://github.com/woocommerce/woocommerce-gutenberg-products-block/pull/633
https://github.com/woocommerce/woocommerce/pull/23957

With the work @mikejolley has done around loading in external modules ( REST API, Blocks ) into WooCommerce core, we need to prepare WooCommerce Admin for inclusion in Woo Core in a similar fashion.

This task involves touching most, if not all, of the PHP classes that exist in the project, and bringing them up to more modern PHP standards ( see the blocks PR above for an example ), and to prep them to be loaded under the new external package structure in core.

Mike has made an example plugin with how external features like this need to be structured.

Other considerations:

• How will our feature flag system work alongside this?

and /cc @mikejolley please layer in more details thoughts you might have 🙇

Child Issues

• [x] https://github.com/woocommerce/woocommerce-admin/issues/2711
• [x] https://github.com/woocommerce/woocommerce-admin/issues/2712
• [x] https://github.com/woocommerce/woocommerce-admin/issues/2713

[timmyc]

Related: I think #1320 can be satisfied with this modernization efforts too.

[psealock]

@mikejolley, do you have ideas on how to split this up into manageable chunks? The final Core pull request has a great checklist to go from in https://github.com/woocommerce/woocommerce/pull/23957. I will be digging further into this one over the next few days, but wanted to get your opinion first.

[mikejolley]

@psealock Your best bet is likely to split off the addition of the autoloader class and refactoring the main file to support it, and then splitting existing PHP files into groups so each group can be refactored at a time. For that, you'd likely need to git mv the file to the new src directory, add the namespaces/update the existing logic, and then bulk update usage of the class.

The refactoring of existing code will take the longest I'd expect.

How will our feature flag system work alongside this?

Put each feature in a class and then you only need to init the class if the feature flag is on. Seems the simplest approach.

[jeffstieler]

Are there any objections to reorganizing the Report API classes as follows?

Orders as example:

src/Reports/Orders/Controller.php
src/Reports/Orders/DataStore.php
src/Reports/Orders/Query.php
src/Reports/Orders/Stats/Controller.php
src/Reports/Orders/Stats/DataStore.php
src/Reports/Orders/Stats/Segmenting.php
src/Reports/Orders/Stats/Query.php

[rrennick]

WooCommerce core and other plugins I've worked with use functional as the parent folder

• src/admin/
• src/api/
• src/data-stores/

but organizing by data type will also work as long as others don't find it confusing.

[jeffstieler]

WooCommerce core and other plugins I've worked with use functional as the parent folder

Yep, which looks similar to the structure we have now.

but organizing by data type will also work as long as others don't find it confusing

Without protest, that's what I'll initially shoot for. When modifying an endpoint or creating a new one, it requires bouncing around in the codebase to create/edit each file. I've found that cumbersome, but perhaps others haven't.

[psealock]

I ❤️ this re-org. I always found navigating/finding the correct file difficult. This structure and rename lets me keep a sane sidebar in my editor 👍

[jeffstieler]

I think we can drop #2713

[psealock]

Nice work getting this one finished @jeffstieler

[hybridwebdev]

Yay, yet more breaking changes in 3.7. This further breaks the ability to use the WC cart class in WP's REST api. Previously in 3.6x you broke this, but it was possible to fix by using
add_filter( 'woocommerce_is_rest_api_request', 'somefucnction');

However in 3.7 this alone no longer fixes the issue. Now you're forced to essentially bootstrap WC by including WC_ABSPATH . 'includes/wc-cart-functions.php';

Do you guys not get that you're a widely used plugin across hundreds of thousands of sites, and that you can't just push breaking changes like this without some sort of heads up to the community?

Today alone this caused me an hour of grief and hair pulling as 1 of our clients sites updated and broke because of this update. And I foresee this causing issues on numerous other sites I maintain because of this update.

You owe it to your community to be transparent about these sort of things. If you're rolling out an update that is potentially breaking, announce it ahead of time. And if you DO release these updates, please be courteous in providing solutions to solve the issues these updates cause.

Seriously, been a long time WC user, but with how you guys have been handling updates recently, seriously considering looking at alternatives. Very unprofessional.

[mikejolley]

@hybridwebdev I think from the experience you're reporting it's clear that cart interaction via the REST API is not something core supports, and until it does (if it makes sense to do so) these problems may recur. I can understand your frustration if you've made a complex customisation for clients and had things break, but to respond to these points:

You owe it to your community to be transparent about these sort of things.
If you're rolling out an update that is potentially breaking, announce it ahead of time

• All updates are announced via dev blog and developing in the open, so probably more about discoverability than transparency.
• Breaking changes to _public_ APIs are announced (or avoided where possible) with backward compatibility layers in place.

Extending the REST API and relying on functionality that may or may not be loaded (cart, other core classes not used in REST) doesn't, at least to my understanding, constitute a public facing API. Cart is also not documented as something that is supported via the REST API out of the box - the documentation states the REST API is an admin facing API rather than a customer facing one. Therefore making a public announcement about this type of change possibly doesn't make much sense so long as unit tests and e2e continue to pass, and projects we test against continue to work. cc @peterfabian

FWIW we're looking at carts and the API as part of the blocks project so perhaps things will become easier, or the problems will be more apparent. Until then, if this did continue to be problematic, nothing stops you extending the WP REST API (rather than the Woo REST API) to add this functionality in a different way. Protections could also be added on your side to ensure that any dependencies exist, otherwise fallback to some other logic or alert you somehow which could be rolled into your own testing procedures ahead of updates.