search

railslove / rack-tracker

Tracking made easy: Don’t fool around with adding tracking and analytics partials to your app and concentrate on the things that matter.
https://www.railslove.com/open-source
MIT License
648 stars 121 forks source link

Set wildcard to non-greedy for GTM body insertion #107

Closed JeremiahChurch closed 6 years ago

JeremiahChurch commented 6 years ago

Change introduced in #86 causes issues with mismatches of closing brackets (single elements vs separately closed elements <input/> vs <div></div>) - removing the greediness cleanly matches just the <body> tag - regardless of what's contained in it.

You can see the problem with this behavior in rubular <body.*> vs <body.*?> - use the below HTML - the first without the question mark will return most (but not all!) of the HTML - with the question mark just the body tag is returned in the match.

sample HTML code used to repro issue:

<html lang="en" style="font-size: 10px"><head></head><body class="app"><div class="portlet box blue" id=""><div class="portlet-title"><div class="tools"></div><div class="caption">Edit Location</div><div class="actions">&nbsp;&nbsp;&nbsp;</div></div><div class="portlet-body"><form novalidate="novalidate" class="simple_form portlet-body form edit_service_location" id="edit_location_920" action="/en/locations/920" accept-charset="UTF-8" method="post"><input name="utf8" type="hidden" value="✓"><input type="hidden" name="_method" value="patch"><input type="hidden" name="authenticity_token" value="d05xw=="><div class="form-group text optional "><label class="control-label text optional" for="service_location_comment">Comment</label><textarea class="form-control text optional" name="service_location[comment]" id="service_location_comment">
</textarea></div></form></div></div></body></html>

Our users reported the issue when some (but not all, based on luck of the HTML) of our textarea elements would end up with the google tag manager code in addition to whatever would normally be there. any other mismatching html tags and the code would just have been in a random div on the page - no problem.

DonSchado commented 6 years ago

Hi @Tongboy, interesting, we didn't catch that. thank you for fixing it 👍

bumi commented 6 years ago

oh nooo. why am I so bad at regexp?! shit... thanks @Tongboy for debugging and fixing this.

we should add a spec for this, can you add that? also we should probably change the regexp for the head tag also?

JeremiahChurch commented 6 years ago

I'll take a look at adding a spec.

Not sure of the value of changing the head regex to match. Only non displayed elements are in the head so the only reason to do that would to ensure the js is inserted in a deterministic location (always before everything else, not sure that would cause any problems?) Right now it will be towards the bottom of the head tag and the exact location will change based on HTML tag elements.

The only sure benefit I could see to match the head behavior would be a potential speed increase. the regex match pattern would always be much shorter...

DonSchado commented 6 years ago

@Tongboy please bundle the new release 1.6.0 :)

bumi commented 6 years ago

@DonSchado we should also do a patch release with this one

DonSchado commented 6 years ago

ah, 1.5.1 you mean, yeah good idea