-
Notifications
You must be signed in to change notification settings - Fork 222
[ECP-9715] Multishipping feature is broken on develop10 #2982
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
Conversation
# Conflicts: # Helper/Requests.php # Model/Api/AdyenDonations.php # Test/Unit/Model/Api/AdyenDonationCampaignsTest.php # Test/Unit/Model/Api/AdyenDonationsTest.php
# Conflicts: # Helper/Data.php
# Conflicts: # AdminMessage/VersionMessage.php # Gateway/Request/CheckoutDataBuilder.php # Helper/PaymentMethods.php # Helper/PaymentResponseHandler.php # Helper/PlatformInfo.php # Model/Api/PaymentRequest.php # Test/Unit/Helper/DataTest.php # Test/Unit/Model/Ui/AdyenCcConfigProviderTest.php # Test/Unit/Model/Ui/AdyenGenericConfigProviderTest.php
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.
Hello @khushboo-singhvi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team,
Gemini here, providing a summary of this pull request. The main goal of this PR, as indicated by the title and description, is to fix the broken multishipping feature on Magento 2.4.8 and later versions. This issue arose due to stricter Content Security Policy (CSP) headers enforced by Magento, which require inline scripts to use nonces or hashes. The fix involves adapting the relevant templates to use Magento's SecureHtmlRenderer to correctly render JavaScript blocks in a CSP-compliant manner.
In addition to the core CSP fix, the PR includes several smaller changes across various files, including minor refactors, cleanup, and adjustments to Adyen-specific JavaScript components.
Highlights
- Fix Multishipping CSP Compliance: The primary change involves updating the
billing.phtmlandabstract-form.phtmltemplates used in the multishipping checkout flow. Inline<script>tags are replaced with PHP code that utilizesMagento\Framework\View\Helper\SecureHtmlRendererto render the JavaScript content, ensuring compliance with stricter CSP headers in Magento 2.4.8+. - Adyen JavaScript Updates: Several Adyen-specific JavaScript files have been modified. This includes a minor robustness improvement in the
currencyHelper.jsby explicitly converting the amount to a Number, removing theonCancelhandler from theadyen-checkout.jsmodel, and removing theurl-builderdependency and a modal show call in theadyen-cc-method.js. - Minor Refactors and Cleanup: The pull request also includes a small refactor in
Helper/PointOfSale.phpchanging a method call fromdataHelpertoplatformInfo, removes a trailing newline in a layout XML file, and cleans up commented-out code and whitespace in templates.
Changelog
Click here to see the changelog
- Helper/PointOfSale.php
- Changed the method call for
padShopperReferencefrom$this->dataHelperto$this->platformInfo.
- Changed the method call for
- view/frontend/layout/multishipping_checkout_billing.xml
- Removed a trailing newline character at the end of the file.
- view/frontend/templates/checkout/multishipping/billing.phtml
- Added
@var Magento\Framework\View\Helper\SecureHtmlRenderer $secureRendererto the docblock. - Replaced inline
<script>tags forwindow.checkoutConfigand billing address population with PHP code using$secureRenderer->renderTagfor CSP compliance. - Replaced the inline
<script>tag for Adyen payment methods initialization with PHP code using$secureRenderer->renderTag. - Removed commented-out code (
<?php /* Payment methods forms list */ ?>). - Adjusted whitespace.
- Added
- view/frontend/templates/form/multishipping/abstract-form.phtml
- Added
@var \Magento\Framework\View\Helper\SecureHtmlRenderer $secureRendererto the docblock. - Replaced the inline
<script>tag for UI component layout initialization with PHP code using$secureRenderer->renderTagfor CSP compliance. - Adjusted whitespace and escaping for JavaScript variables.
- Added
- view/frontend/web/js/helper/currencyHelper.js
- Added explicit
Number()conversion to theamountvariable before callingtoFixedin theformatAmountfunction for improved robustness.
- Added explicit
- view/frontend/web/js/model/adyen-checkout.js
- Removed the
handleOnCancelparameter from thecreatefunction. - Removed the
onCanceloption from the Adyen Checkout configuration object passed to the Adyen SDK.
- Removed the
- view/frontend/web/js/view/checkout/summary/agreements-assigner-mixin.js
- Fixed a typo in a comment (
twicechanged totwice).
- Fixed a typo in a comment (
- view/frontend/web/js/view/payment/method-renderer/adyen-cc-method.js
- Removed the import and usage of
Magento_Checkout/js/model/url-builder. - Removed the line
let popupModal = self.showModal();before callingadyenPaymentService.paymentDetails.
- Removed the import and usage of
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Inline script tags,
CSP says 'No, no, no!',
Renderer saves the day.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
This pull request primarily focuses on addressing Content Security Policy (CSP) issues by integrating SecureHtmlRenderer for inline scripts in .phtml template files, which is a crucial update for compatibility with stricter CSP headers in newer Magento versions. The implementation of these CSP-related changes appears to be correct and follows best practices.
However, there's a significant concern regarding a change in Helper/PointOfSale.php that seems to introduce a critical error by calling a method on an object where it doesn't exist. This, along with an outdated unit test, needs immediate attention. Additionally, there's a concern about the removal of copyright and license information from a file header.
Other changes include a robustness improvement in a JavaScript helper and minor code cleanups.
Summary of Findings
- Critical Method Call Error: In
Helper/PointOfSale.php,platformInfo->padShopperReference()is called, butPlatformInfo.phpdoes not define this method. This will likely cause a fatal error. The associated unit test also appears outdated. - Missing Copyright/License Header: The detailed copyright, license, and author information was removed from the header of
view/frontend/templates/checkout/multishipping/billing.phtml. This should be reviewed to ensure it wasn't an accidental deletion of important legal information. - JavaScript Robustness Improvement: In
view/frontend/web/js/helper/currencyHelper.js,amountis now explicitly converted toNumberbeforetoFixed()is called, enhancing robustness. - CSP Compliance (Not commented due to severity settings): The core changes in
.phtmlfiles to useSecureHtmlRendererfor CSP compliance are well-implemented and address the PR's main objective. - Minor Code Cleanups (Not commented due to severity settings): Several minor improvements were made, including newline fixes in XML, whitespace adjustments, removal of unneeded comments, and cleanup of unused JavaScript dependencies/variables. These are generally positive for code hygiene.
Merge Readiness
This pull request has a critical issue in Helper/PointOfSale.php that must be addressed before merging. The call to a non-existent method on $this->platformInfo will likely lead to runtime errors, and the unit test for this functionality also needs correction. Additionally, the removal of the copyright header in billing.phtml should be clarified.
While the CSP-related changes are positive, the identified critical issue prevents this PR from being merge-ready. I am unable to approve this pull request in its current state. Please address the critical and medium severity issues, after which further review can be conducted by the appropriate maintainers.
|



Description
Multishipping was broken on develop-10 because Magento began enforcing stricter CSP headers on M2.4.8; Now inline scripts must use nonces or hashes and it fails without SecureHtmlRenderer
With this PR adapting SecureRenderer for all the block templates
Tested scenarios
Fixes