-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Fix non-ASCII coupon search #56319
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
Fix non-ASCII coupon search #56319
Conversation
I checked the failing e2e tests and they occur on |
Testing GuidelinesHi @frosso , Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
e1cad97
to
ae4cdbf
Compare
I think I switched the context to something else yesterday and forgot to push ae4cdbf as well. Now it's here. |
0697e22
to
6122a36
Compare
6122a36
to
eb006c9
Compare
f6c90dd
to
b3a1814
Compare
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 left a few more comments about the new implementation, but the main one is about the possibility that filters applied onto woocommerce_coupon_code
wouldn't be applied to woocommerce_coupon_code_without_lower
- I'd like to hear your thoughts on that point.
After discussing in p1741951466805729-slack-C8X6Q7XQU and f2f, we picked the initial version of the implementation, which removes the @frosso thanks for the reviews so far, may I ask you to review & test it? |
@frosso after #56319 (comment) on Friday, I also added 3f2624c today in the morning because I realized that it wouldn't hurt to have an extra sanitization when comparing values too. I hope it didn't interfere with your review process. Thanks! |
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.
* @return bool | ||
*/ | ||
function wc_is_same_coupon( $coupon_1, $coupon_2 ) { | ||
return wc_strtolower( $coupon_1 ) === wc_strtolower( $coupon_2 ); |
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.
(minor) I wonder about the utility of a function that says it's comparing coupons, when in reality it's just comparing two strings converted to lowercase. Are you just trying to get a consistent comparison without the overhead of WC_Coupon
?
I was thinking that one alternative could be to use the WC_Coupon
core class - something like this:
function wc_is_same_coupon( $coupon_1, $coupon_2 ) {
$coupon_obj_1 = new WC_Coupon( $coupon_1 );
$coupon_obj_2 = new WC_Coupon( $coupon_2 );
return $coupon_obj_1->get_code() === $coupon_obj_2->get_code();
}
That way, you don't need to call wc_strtolower
each time (and you could possibly keep the filter).
But there might be some overhead with the usage of WC_Coupon
🤷
In terms of naming convention, I noticed that there seem to be a slight preference towards the wc_[object]_is*
convention - so maybe consider the naming wc_coupon_is_equal()
? But again, up for discussion, I'd like to hear your thoughts 🤷
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.
Thanks for the thoughts!
The coupon class is slightly more preferable to me too, and we could move to using the state of the class and doing something like
public function is_coupon_code_equal_to( $coupon_code ) {
return wc_strtolower( $this->get_code() ) === wc_strtolower( $coupon_code );
}
in WC_Coupon
, but I haven't done so because there's also WC_Order_Item_Coupon
class for order flows and I didn't want to repeat the logic when there's an utility file. I acknowledge that there are existing places that are duplicated but I didn't want to follow this behavior.
In terms of naming convention, I noticed that there seem to be a slight preference towards the wc_[object]_is* convention
I was considering that too, but I found helper functions following the wc_is_*
pattern to be more prevalent:
wc_is_order_status()
wc_is_valid_url()
wc_is_attribute_in_product_name()
wc_is_webhook_valid_topic()
wc_is_webhook_valid_status()
wc_is_current_account_menu_item()
wc_is_external_resource()
wc_is_active_theme()
wc_is_wp_default_theme_active()
wc_is_file_valid_csv()
many of wc_is_*
are also helper functions performing some basic operations of objects/collections/types.
I think having said that plus the fact that new changes will require additional round of testing, I think keeping things as they are would be my favorite option, but I will keep the PR open for a bit in case you will want to chat more. Thank you!
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.
The coupon class is slightly more preferable to me too, and we could move to using the state of the class and doing something like [...] in WC_Coupon, but I haven't done so because there's also WC_Order_Item_Coupon class for order flows and I didn't want to repeat the logic when there's an utility file. I acknowledge that there are existing places that are duplicated but I didn't want to follow this behavior.
That's also a great suggestion - unfortunate about WC_Order_Item_Coupon
.
I was considering that too, but I found helper functions following the
wc_is_*
pattern to be more prevalent
I was basing my observation off of this:
(wc|woocommerce)_(is|has)_[a-z]*
: 97 results, 29 unique instances
(wc|woocommerce)_[a-z]*_(is|has)_[a-z_-]*
: 110 results, 37 unique instances
This includes both functions and filters, of course.
But either way - not a massive difference in terms of results :) Thank you for the consideration!
Submission Review Guidelines:
Changes proposed in this Pull Request:
Closes #32536
When PHP attempts to
strtolower()
for non-ASCII chars, the converted value is corrupted which is why the search for coupons with titles like e.g.VÅR20
failed. This PR removes the lowercase, similarly to how it was done in #25314.How to test the changes in this Pull Request:
0️⃣ Preparation steps
latin1_swedish_ci
collation setAlso define the collation and charset constants in
docker/wordpress/wp-config.php
1️⃣ Adding coupons
SummerSale2025
)VÅR20
2️⃣ Applying coupons
On the shortcode
VÅR20
coupon and confirm it's appliedVÅR20
and confirm it's removedvÅr20
coupon and confirm case-insensitivity as the coupon should be applied tooSummerSale2025
coupon and confirm it's appliedSUMMERSALE2025
coupon and confirm that you see the "Coupon code already applied!" message under the input field3️⃣ Removing coupons
Staying on the shortcode
4️⃣ Other pages
5️⃣ Manual order creation & coupon management
VÅR20
coupon and confirm it's appliedVÅR20
and confirm it's removedvÅr20
coupon and confirm case-insensitivity as the coupon should be applied tooSummerSale2025
coupon and confirm it's appliedSUMMERSALE2025
coupon and confirm that you see the "Coupon code already applied!" message under the input field6️⃣ APIv1 tests
POST /wp-json/wc/v1/coupons
Here's the request body to take as an example
MÅQ25
mÅq25
7️⃣ APIv2 tests
POST /wp-json/wc/v2/coupons
Here's the request body to take as an example
DÅQ25
dÅq25
8️⃣ APIv3 order test
POST /wp-json/wc/v3/orders
request with a different case of a coupon included in the request body and confirm, that for all of them the request is successfulExample (additionally try with other combinations of existing coupon code e.g. `vÅR20` and `VÅR20` or `vÅr20`
9️⃣ WooCommerce Analytics
Testing that has already taken place:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment