webmatch / WbmTagManager

Shopware 5 Plugin for Google Tag Manager integration and dataLayer configuration
27 stars 18 forks source link

Fix for wrong body tag position #13

Closed christopherholst closed 6 years ago

dneustadt commented 6 years ago

Thanks for your contribution. This topic has caused some concerns so I'd like to express my opinion on why I don't think that a regular expression is the way to go.

It's generally agreed upon that it's a horrible, horrible idea to parse HTML with regex. That's why I opted to get rid of any wildcards in one of the last versions and decided to use the existing noscript tag of the bare Shopware theme. There are tons of discussions and articles on the net on why regex and html are a dangerous combination. E.g. https://blog.codinghorror.com/parsing-html-the-cthulhu-way/

There's a flaw in your solution too. See: https://ideone.com/PYdm7P A body tag without any attributes will fail to be parsed as intended. And in the past there already have been cases where users had removed and added attributes, line breaks within the tag and so on. Now you could add a case for that. But where does it stop? How many more edge cases are there?

Now Google says the noscript tag needs to be inserted immediately after the opening body tag. That wording is straight up misleading. It makes it sound like it's a rule not to be broken. In reality it makes no difference. Especially since the existing noscript tag we're using now is still just a couple elements down the opening body tag. And what's inside the tag is an iframe and the content of an iframe will be loaded asynchronously anyway. So going with the regex method and taking the risk of breaking markup by missing some weird edge case is not worth it in my opinion. Especially when you consider that there is probably no technical benefit and how few people are browsing with javascript deactivated beyond that.

Please don't get me wrong, your contribution is still very much appreciated. It's just for the reasons I mentioned beforehand, that I might have to decline your pull request.

dneustadt commented 6 years ago

Just to strengthen my point here's another weird yet plausible edge case. Having > within the value of an attribute. https://ideone.com/eshyGm

dneustadt commented 6 years ago

Ultimately, if you absolutely need to have the noscript tag follow the opening body tag, you can extend your template accordingly.

frontend/index/index.tpl

{extends file='parent:frontend/index/index.tpl'}

{block name='frontend_index_after_body'}<noscript></noscript>{$smarty.block.parent}{/block}
christopherholst commented 6 years ago

Hey, regex is not the nicest solution, it's clear. However, in the current (2.1.8) versions you get, for example, in the search console no more release for this integration. Google rejects this position. With the fix in 2.1.9 it works again.