CSP migration#113
Conversation
This will run several tools to optimize your code, find more about CSP Read our documentation for [theme](https://docs.hyva.io/hyva-themes/writing-code/csp/index.html) and [checkout](https://docs.hyva.io/checkout/hyva-checkout/devdocs/csp/index.html) changes - @todo! means **work todo**, please review - @done! means **no work todo**, just informing about the change - requirement `hyva-themes/magento2-theme-module` (`^1.3.11`) @done! - requirement `hyva-themes/magento2-hyva-checkout` (`^1.3`) @done!
|
@JeroenBoersma thanks for this! Would you know if this also solves #111? @Adyen when will this be checked/accepted? |
|
Hello @JeroenBoersma, Thank you for this massive contribution and for your effort to make the module compatible with the latest version of the Hyvä Checkout module. We will review and merge the code if everything goes well. Best Regards, |
|
@candemiralp thanks for the update. Any ETA on this? |
|
Hello @Aquive, Thank you, there is no ETA at the moment (yet) but we are discussing this internally. I will update everyone soon. Best Regards, |
| function adyenHyvaPaymentModal() { | ||
| return Object.assign( | ||
| hyva.modal(), | ||
| ,{ |
There was a problem hiding this comment.
This line triggers a syntax error.
In Firefox, the following console error is logged for this line:
Uncaught SyntaxError: expected expression, got ','
In Chome, the following console error is logged for this line:
Uncaught SyntaxError: Unexpected token ','
There was a problem hiding this comment.
Removing the comma on this line fixes this syntax error, but reveals another one: on the next line. On that line, a comma should be added.
There was a problem hiding this comment.
pushed and fixed, thanks @aadmathijssen for blending in
There was a problem hiding this comment.
Thanks Jeroen, I can confirm that this is resolved. However, another syntax error is present on the next line. I left a separate comment for that.
| return Object.assign( | ||
| hyva.modal(), | ||
| { | ||
| isLoading: false |
There was a problem hiding this comment.
This line triggers a syntax error.
In Firefox, the following console error is logged for this line:
Uncaught SyntaxError: missing } after property list
* this file is fully overwritten, which makes it impossible if Hyva will add a new feature, or of another payment method is listening for the original method.
JeroenBoersma
force-pushed
the
csp-compatibility-generated
branch
from
df0b98c to
c13b080
Compare
|
So, we got some feedback from clients saying that still some stuff was off, which made me revisit the problem... Last time I had a comma to much, now somewhere else I was lacking a comma. Sorry for that. Next step was looking at the rest of the feedback, as it turns out, Adyen overwrites Because all the things are loaded via AlpineJS we can drop the eventListener approach, as the payment method is only loaded when it is active, this reduces a lot of the complexity it had. I asked our partner to test and the first feedback that it does work as expected. I did the same with a testing account and that also seems to work just fine! |
|
Great work @JeroenBoersma thanks for all the effort! I would have also loved it to test the extension. But since we need to go online asap I had to downgrade to Hyva Checkout v1.2.0, otherwise I would have tested it. Thanks again! |
|
|
|
Hello @JeroenBoersma, Thank you for your great contribution to make our compatibility module CSP compliant. This PR has been merged and a new version will be released soon. Best Regards, |
5 participants
Summary
I checked all code against the with the new standard, which meant moving all non-global JS into a global JS file.
Added AlpineJS components to initialize the payment methods on request (if magewire->methodCode is correct)
Read our documentation for theme and checkout changes
Updated requirements in
composer.jsonhyva-themes/magento2-theme-module(^1.3.11)hyva-themes/magento2-hyva-checkout(^1.3)Tested scenarios
Tested the code with a test account, didn't run all payment methods yet.
Still needs manual testing
Fixed issue: #109
notes
I've seen the other PR in here which didn't reworked all the files, this time we did.