GH-230

[jemoon]

• SecureRenderer usage for inline scripts at checkout for Magento 2.4.7+ compatibility - possible fix for GH-230; self checked on luma based template

[hostep]

Be aware that the SecureHtmlRenderer class was introduced in version 103.0.0 of the magento/framework package (included in Magento 2.4.0 or higher).

So if this PR gets approved, we will need to reflect this in the composer.json of this module to make sure that people still using Magento 2.3.x can't upgrade to a new version where we use SecureHtmlRenderer.

Update: or alternatively, you can probably also check if the class SecureHtmlRenderer exists, if yes, use it, if not, output the script as-is.

[jemoon]

@hostep good point, thanks.

@jissereitsma what's Your opinion on that? Should I rewrite this PR in order to use additional ViewModel which will also check if we have SecureHtmlRenderer available or should I update composer dependencies within current PR or maybe You have other plans for solving this?

[hostep]

@jemoon: we have this PR installed as a patch on a 2.4.7 project, but I've noticed a bug here when we have products with names that contain single quotes. We then get a JS error: Uncaught SyntaxError: missing ) after argument list ...

It's very subtle, but you shouldn't wrap {$dataLayerJson} and {$dataLayerEventsJsonChunk} into single quotes, it wasn't done before your changes and that seemed to be for a good reason. Json output can just be parsed by javascript without having it in single quotes. Wrapping it in single quotes will lead to problems when the json content contain single quotes itself.

Could you update your PR and remove the surrounding single quotes? So like this:

@@ -15,11 +15,11 @@ $dataLayerJson = $dataLayerViewModel->getDataLayerAsJson();
 $dataLayerEventsJsonChunks = $dataLayerViewModel->getDataLayerEventsAsJsonChunks();
 $dataLayerEventsJsonChunksScript = '';
 foreach ($dataLayerEventsJsonChunks as $dataLayerEventsJsonChunk) {
-    $dataLayerEventsJsonChunksScript .= "pusher('{$dataLayerEventsJsonChunk}', 'push (initial event) [data-layer.phtml]');";
+    $dataLayerEventsJsonChunksScript .= "pusher({$dataLayerEventsJsonChunk}, 'push (initial event) [data-layer.phtml]');";
 }
 $scriptString = <<<SCRIPT
 require(['yireoGoogleTagManagerPush'], function (pusher) {
-    pusher('{$dataLayerJson}', 'push (initial page) [data-layer.phtml]');
+    pusher({$dataLayerJson}, 'push (initial page) [data-layer.phtml]');
     {$dataLayerEventsJsonChunksScript}
 });
 SCRIPT;

Thanks!

[jemoon]

@jemoon: we have this PR installed as a patch on a 2.4.7 project, but I've noticed a bug here when we have products with names that contain single quotes. We then get a JS error: Uncaught SyntaxError: missing ) after argument list ...

Could you update your PR and remove the surrounding single quotes?

@hostep done - PR updated, thanks for feedback...

[hostep]

Be aware that the SecureHtmlRenderer class was introduced in version 103.0.0 of the magento/framework package (included in Magento 2.4.0 or higher).

So if this PR gets approved, we will need to reflect this in the composer.json of this module to make sure that people still using Magento 2.3.x can't upgrade to a new version where we use SecureHtmlRenderer.

Update: or alternatively, you can probably also check if the class SecureHtmlRenderer exists, if yes, use it, if not, output the script as-is.

I have an update around what would be preferred here. I ran into an issue yesterday with the heredoc syntax introduced in this PR, on a Magento 2.3.7-p4 shop where it caused an issue with the html minifier of Magento, it would crash the entire frontend of the shop because the html minifier causes invalid php syntax due to the use of heredoc syntax being put on a single line, which is invalid. Magento fixed this bug in their html minifier in version 2.4.0 with https://github.com/magento/magento2/commit/41c3f6f4338326ee6efcf04a96555de9a0c3c436

So it's clear where to go to, the version constraints in the composer.json file should be updated to no longer allow newer versions of this module to be installed on Magento < 2.4.0 in my opinion. @jissereitsma do you agree?

[nige-one]

@hostep good point, thanks.

@jissereitsma what's Your opinion on that? Should I rewrite this PR in order to use additional ViewModel which will also check if we have SecureHtmlRenderer available or should I update composer dependencies within current PR or maybe You have other plans for solving this?

If you opt for the composer dep, please be aware that >= 2.4.6-p6 is also affected. What were they thinking...

[hostep]

Like @nige-one says, the CSP feature was backported to 2.4.6-p6, but also to versions 2.4.4-p9 and 2.4.5-p8

If we want the Yireo GTM module to stay compatible with Magento 2.3.x as well, then the heredoc syntax should be avoided and we should check if SecureHtmlRenderer class exists before using it.

Still awaiting opinion of @jissereitsma, I understand this is quite complex and the CSP implementation of Magento is very annoying to deal with...

[jissereitsma]

Thanks @nige-one @hostep @jemoon for chiming on this. I've been busy lately (priorities with my family) but eager to make this work, because I have waited for too long with this.

As of yet, I don't see anything wrong with the PR to make things work under Magento 2.4.7. So I will merge this PR first of all.

Next, the real discussed issue is compatibility with older versions. If I would be a bastard, I would simply go for raising the requirements of this module. However, I do know that this module is being used in a lot of older Magento instances, and I'm getting feedback from those people as well. So I would be eager to make sure that compatibility is still there.

One solution here would be the if-else check to see if $secureHtmlRenderer is there and otherwise output the original script.

Another solution would be to see if the $secureHtmlRenderer variable would be present in the HTML and otherwise just come up with it anyway. In other words, mocking the SecureHtmlRenderer if it is not there. I'm going to try out this solution.

As for the HEREDOC issue, I'm not sure how big this issue is. Perhaps the script-code can be rewritten later to live without it.

[jissereitsma]

@nige-one @hostep @jemoon This is what I had in mind: https://github.com/yireo/Yireo_GoogleTagManager2/commit/61716eabd268f0664cd61032037f4ca7f88ffb12 This keeps on working nicely with Magento environments where the SecureHtmlRenderer class is found, but adds a stubbed $secureRenderer for older versions.

Currently busy with running integration tests :)

[hostep]

Seems like this could work 👍

But for Magento shops older than 2.4.0, I would still recommend to rewrite the phtml files and not use heredoc syntax to avoid html minification to break these templates on older shops.