Skip to content
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.

Sign up for GitHub

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

Jump to bottom

retconAttr wrap script tags in an unexpected head tag #25

Closed
juban opened this issue May 29, 2020 · 9 comments
Closed

retconAttr wrap script tags in an unexpected head tag #25

juban opened this issue May 29, 2020 · 9 comments
Labels
bug

Comments

@juban
Copy link

juban commented May 29, 2020

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
Copy link
Owner

mmikkel commented May 29, 2020

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
Copy link
Author

juban commented May 31, 2020

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
Copy link
Owner

mmikkel commented May 31, 2020

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

@mmikkel mmikkel added the bug label May 31, 2020
@juban
Copy link
Author

juban commented May 31, 2020

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

@juban
Copy link
Author

juban commented Jun 1, 2020

@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
Copy link
Owner

mmikkel commented Jun 1, 2020

@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
Copy link
Author

juban commented Jun 2, 2020

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
Copy link
Owner

mmikkel commented Jun 4, 2020

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

@mmikkel
Copy link
Owner

mmikkel commented Jul 19, 2020

Resolved in Retcon 2.2.0

@mmikkel mmikkel closed this as completed Jul 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mmikkel @juban