-
Notifications
You must be signed in to change notification settings - Fork 212
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
base: develop
Are you sure you want to change the base?
Conversation
This reverts commit aae0eb3. Reverting as the autoloader is now case-insensitive.
📈 PHP Unit Code Coverage Report
|
There was a problem hiding this 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.
require self::$classmap[ $class_lower ]; | ||
|
||
return true; |
There was a problem hiding this comment.
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.
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.
Changelog entry
Changelog Entry Comment
Comment
Post merge