Skip to content

Explore: implement a basic autoloader #4260

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

daledupreez
Copy link
Contributor

@daledupreez daledupreez commented Apr 24, 2025

Fixes STRIPE-<linear_issue_id>
Fixes #<github_issue_id>

Changes proposed in this Pull Request:

This PR explores adding a basic autoloader for the plugin largely as a way to avoid unnecessary file loads across the plugin. I considered using some prefix matching before building the classmaps, but I decided against it as the overhead for building the classmaps is one-time, whereas performing multiple string prefix checks for every class we hit (including other classes) feels like it would quickly add up.

Another small change I included in this PR is to remove class instantiation side-effects from the class files so the effect is clearer from outside the class. I made changes so we're now auto-loading all classes, and I've moved the code that instantiated the classes from the respective class files to the main plugin file.

Testing instructions

There should be no change in behaviour as a result of this PR, as we're simply changing how we load the files containing the classes. Our unit tests and e2e tests should give us a basic safety net for the core autoloader functions, but it's possible that some edge cases exist.


  • Covered with tests (or have a good reason not to test in description ☝️)
  • [N/A] Tested on mobile (or does not apply)

Changelog entry

  • This Pull Request does not require a changelog entry. (Comment required below)
Changelog Entry Comment

Comment

Post merge

@daledupreez daledupreez self-assigned this Apr 24, 2025
Copy link

📈 PHP Unit Code Coverage Report

Package Line Rate Health
includes/abstracts/abstract-wc-stripe-payment-gateway.php 48%
includes/admin/class-wc-stripe-inbox-notes.php 28%
includes/class-wc-gateway-stripe.php 28%
includes/class-wc-stripe-autoloader.php 6%
includes/class-wc-stripe-customer.php 40%
includes/class-wc-stripe-helper.php 59%
includes/compat/trait-wc-stripe-subscriptions.php 20%
includes/payment-methods/class-wc-stripe-express-checkout-helper.php 21%
includes/payment-methods/class-wc-stripe-upe-payment-gateway.php 49%
woocommerce-gateway-stripe.php 30%
Summary 44% (7274 / 16420)

@daledupreez daledupreez marked this pull request as ready for review April 30, 2025 15:13
@daledupreez daledupreez requested review from a team and malithsen and removed request for a team April 30, 2025 15:13
Copy link
Contributor

@malithsen malithsen left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for pulling this together! The new autoloader cleans up our bootstrap nicely and should shave off unnecessary file loads. In the future, maybe we can explore moving to Composer with PSR-4 to get automatic class mapping and caching out of the box, it might require a large effort to restructure/rename our classes though.

Comment on lines +51 to +53
require self::$classmap[ $class_lower ];

return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I was about to suggest adding a check to see if the file exists, to avoid fatal errors when there is a mapping to a non-existent file. However, I suppose throwing a fatal error is clearer than logging an error, which could be overlooked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants