retconAttr wrap script tags in an unexpected head tag

juban commented 4 years ago

Given the following twig template

    {% filter retconAttr('script', { 'type': 'opt-in', 'data-type': 'application/javascript', 'data-name': 'google-analytics' }) %}
    <!-- Google Tag Manager --><script>(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start':
            new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0],
                                                      j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
        'https://www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f);
    })(window,document,'script','dataLayer','GTM-ZZZZZZ');</script>
    <!-- End Google Tag Manager -->
    {% endfilter %}

the resulting generated code will be

    <!-- Google Tag Manager --><head><script type="opt-in" data-type="application/javascript" data-name="google-analytics">(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start':
            new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0],
                                                      j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
        'https://www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f);
    })(window,document,'script','dataLayer','GTM-ZZZZZZ');</script>
    <!-- End Google Tag Manager -->
    </head>

instead of

    <!-- Google Tag Manager --><script type="opt-in" data-type="application/javascript" data-name="google-analytics">(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start':
            new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0],
                                                      j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
        'https://www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f);
    })(window,document,'script','dataLayer','GTM-ZZZZZZ');</script>
    <!-- End Google Tag Manager -->

Looks like the getHtml method is injecting the head tag when calling the saveHTML of the Mastermind HTML5 library (for some reason...). Changing the regex expression in getHtml method to strip the extra head tag seems to work, but could probably lead to some sides effects.

mmikkel commented 4 years ago

Hi @juban – that's odd, thanks for reporting. I'll take a look at it ASAP.

Sidenote: If that's your actual template, you could use the native attr filter instead of Retcon:

{% filter attr({ 'type': 'opt-in', 'data-type': 'application/javascript', 'data-name': 'google-analytics' }) %}
    <!-- Google Tag Manager --><script>(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start':
            new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0],
                                                      j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
        'https://www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f);
    })(window,document,'script','dataLayer','GTM-ZZZZZZ');</script>
    <!-- End Google Tag Manager -->
    {% endfilter %}

juban commented 4 years ago

Hi @mmikkel

Thank you for your fast feedback. Indeed, the native attr filter is working fine in that use case. Obviously, the example I gave you is pretty dumb and mostly to demonstrate the issue. (I'm mainly using retcon to alter a set of generated HTML markup). Anyway, I ran into that strange bug recently because my HTML markup was not validated due to redundant head tags. I would happily help you to figure out how to solve this if you need.

mmikkel commented 4 years ago

Hi @juban – I believe I might have a fix for this issue, if you'd be able to help me test it, that'd be great (the fix required a pretty fundamental change to how Retcon reads and writes HTML, so I won't push a release until I'm confident I didn't mess up something else).

To install the fix, do

composer require mmikkel/retcon:dev-dev#a25040575c3b18e3b1b55448e7359795c2b87e14

juban commented 4 years ago

@mmikkel I'll try this ASAP and let you know.

juban commented 4 years ago

@mmikkel Looks pretty good to me. Did some tests on my projects and the bug is gone and I didn't notice any regression. I'm not using anything beside the retconAttr filter though, I don't know if the fix could alter other retcon filters. Would you like I test some other specific use cases? Please let me know.

mmikkel commented 4 years ago

@juban Thank you for the feedback. The fix basically changes all the filters (as it's changing how Retcon constructs the DOMDocument, and how it reads out the final HTML). If you have the time to test additional filters for regression errors or other weird behaviour, I'd appreciate it – but no worries if you don't :)

juban commented 4 years ago

Hi @mmikkel,

I'll do my best ;) Just an idea: how about to setup some unit tests for that? If I have some time I'll try to set that up and I'll let you know.

mmikkel commented 4 years ago

@juban Sure, if you find some time to do that it'd be much appreciated!

mmikkel commented 4 years ago

Resolved in Retcon 2.2.0

mmikkel / Retcon-Craft

retconAttr wrap script tags in an unexpected head tag #25