PSR4 & Class Map Autoloading #704
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nmolham-godaddy
requested review from
agibson-godaddy
and removed request for
agibson-godaddy
nmolham-godaddy
requested review from
agibson-godaddy,
itambek-godaddy,
nikolas4175-godaddy and
ajaynes-godaddy
There was a problem hiding this comment.
Nice work!
Since we're changing the namespace of a few classes (other than the usual version change), do you think this needs to be in a major version?
Or to make it backwards compatible we could set up class aliases just for the namespaces that we changed. For example, the Country_Helper class:
- namespace SkyVerge\WooCommerce\PluginFramework\v5_13_1;
+ namespace SkyVerge\WooCommerce\PluginFramework\v5_13_1\Handlers;
|
@agibson-godaddy Good catch, I forgot I changed the namespace for Usually a namespace change like that should be a major version release as it is a breaking change, but in this case, this class (as far as I can tell) has been introduced recently (by you I believe) and been only used in one plugin. Do you think it worth the additional effort of adding a class alias? |
|
@nmolham-godaddy There were actually two namespace changes here:
If it were just the context checker, then I think I'd agree with you. But |
|
Good catch @agibson-godaddy, I got them mixed up as you concluded. I will setup class aliases for them. Done 3fb89fa let me know what you think I am playing around not with using |
|
Just testing the latest changes on Memberships plugin (https://github.com/gdcorp-partners/woocommerce-memberships/pull/1222) and it looks pretty good, no issues or error so far. It required tiny change, other than that, looks good to me |
|
I did a dump of composer loader final computed PSR4 and class mapping in runtime, and find a very promising results. As far as I can tell, all framework classes counted for and mapped correctly to proper class files 🕺 🕺 🕺 🕺 🕺 🕺 🕺 |
# Conflicts: # tests/unit/Helpers/CountryHelperTest.php # tests/unit/Payment_Gateway/PaymentFormContextCheckerTest.php # woocommerce/Handlers/Country_Helper.php # woocommerce/payment-gateway/Blocks/Gateway_Blocks_Handler.php # woocommerce/payment-gateway/PaymentFormContextChecker.php
# Conflicts: # tests/unit/Helpers/CountryHelperTest.php # tests/unit/Payment_Gateway/PaymentFormContextCheckerTest.php # woocommerce/Handlers/Country_Helper.php # woocommerce/payment-gateway/Blocks/Gateway_Blocks_Handler.php # woocommerce/payment-gateway/PaymentFormContextChecker.php
|
Replace base PR with the new release branch PR #705 |
5 tasks
|
Completed authorize[.]net gateway testing. Seems to work just fine! |
|
When I have both authorizenet and memberships activated at the same time, I get this error:
EDIT: fixed 68da0a5 |
agibson-godaddy
approved these changes
Aug 29, 2024
There was a problem hiding this comment.
Excellent work @nmolham-godaddy ! I've done QA with both plugins separately, and with them activated at the same time. I believe everything is working as expected.
Not merging just so you can have a chance to look at my changes above. But I think we can go ahead and merge this.
|
Cool, thanks @agibson-godaddy for doing the QA and catching this bug caused by the class aliases, I definitlly didn't count for that one. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Setup and configure PSR4 autoloading as well as class map existing matching class files.
Release: #705
QA
Before merge