-
Notifications
You must be signed in to change notification settings - Fork 218
Add to Cart with Options Block: Remove global variable overwrite. #9457
Add to Cart with Options Block: Remove global variable overwrite. #9457
Conversation
The release ZIP for this PR is accessible via:
Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
Size Change: 0 B Total Size: 1.08 MB ℹ️ View Unchanged
|
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.
Heads up "Add to Cart Form" was renamed to "Add to Cart Options" in #9238, so please update the testing steps 🙏
![image](https://private-user-images.githubusercontent.com/20098064/238280719-4cffa917-2a8a-47c7-ad47-04fdcc061fcd.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQwNDg1MjcsIm5iZiI6MTcyNDA0ODIyNywicGF0aCI6Ii8yMDA5ODA2NC8yMzgyODA3MTktNGNmZmE5MTctMmE4YS00N2M3LWFkNDctMDRmZGNjMDYxZmNkLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MTklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODE5VDA2MTcwN1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTI1MmI1NjRkYjM1MzIxNzVjY2ZjM2M5MWUyMTlkMmFlNjNmZGQ1MDFhNDgxYTBiOGQ2NTMwZGE2ZDY0NWI0MWQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.P8y2yAnpPnlTbvgylRUy4sSHZmIeHs2ir5YKvJXlJDk)
Otherwise, it's testing well! Checked with different types of products: simple, variable, and grouped.
Thanks for your fast reaction to this issue! 🚀
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.
Thank you for working on this and fixing the issue so quickly, I can confirm that the following scenarios are working as expected:
✅ Works when we have just one Single Product block is added to a page/post
✅ Works when we multiple Single Product block are added to a page/post
✅ Works on the Single Product page
✅ Works on the Single Product page inside a Single Product block
✅ Works with Simple, Grouped, Variable, and External products
Regarding the code, I agree with Darren that we could leave the variable name as $product
instead of $single_product
. I would also change the title of this PR and the testing notes to Add to Cart with Options (as pointed by Karol).
Since there is nothing I see as being a blocker, I'm approving the PR 🚀
Thanks all for the reviews! 🙌
Good point, updated!
I did this change 100% on purpose, as I wanted to make it even more evident that this is a local variable (even though, in practice, the name doesn't impact the end result at all). Since folks here are more fond of |
Important NoteThe Add to Cart with Options block might not function as expected anymore within the Single Product block after this PR + the equivalent change to the Product Gallery block are merged: this is a temporary regression for this experimental block. Since no concerns were raised with the approach of relying on our altered version of those templates for rendering the Add to Cart with Options and the Product Gallery blocks opened for discussion on this PR, I'm opening two separate PRs to introduce our version of those and restore functionality for this block. cc: @nerrad , @gigitux , @thealexandrelara |
Ensure the
$product
global variable is not overwritten within theAdd to Cart with Options
block.Note about the usage of the Add to Cart with Options block within the Single Product block.
While this PR fixes the problem when the Add to Cart with Options block is used within the Single Product template, additional changes will be required to fully support its functionality within the (currently experimental) Single Product block, as all WooCommerce's relevant templates currently rely exclusively on the global
$product
variable for rendering content (e.g. https://github.com/woocommerce/woocommerce/blob/trunk/plugins/woocommerce/templates/single-product/add-to-cart/simple.php#L20-L24 ). With that said, I see two alternatives:$product
variable to do so.Between these two options, I'm more inclined to stick with the second one; as for the core of Woo, this change might not be reasonable considering the nature of the templates and their intended use-case: opening this topic for discussion in case folks have different ideas/suggestions/concerns. As soon as we make a decision, a new PR will be opened to address this use case.
Edit: I've opened a draft PR (#9458) with the fix for the Product Gallery Block, relying on this second approach as suggested here.
cc: @nerrad @danielwrobert @gigitux @thealexandrelara
Fixes #9453
User Facing Testing
WooCommerce Visibility
Performance Impact
Changelog