Rendering JSON?

[ianstormtaylor]

Looking through the templates for the Javascript tags that get added with calls to the global analytics object, lots of them don't actually inline the JSON for properties, and instead use an object like so:

<?php echo $this->renderDataAsJsonVar("segment_analytics_addedtowishlist");?>
<script type="text/javascript">
    analytics.track("Wishlisted Product",
    segment_analytics_addedtowishlist.params);
</script>  

The one thing that worries me here is that it's going to make debugging which properties were actually rendered for a call a lot harder for our support team, or even for the users of the plugin themselves, since it adds an extra step after just View Source.

Would it be possible to instead do something like this: (pseudocode)

<?php $properties = $this->get("segment_analytics_addedtowishlist"); ?>
<script type="text/javascript">
    analytics.track("Wishlisted Product", <?php echo json_encode($properties); ?>);
</script>

An example from WordPress where we do something similar. Results in the actual JSON for properties being inline in the HTML, which makes it really easy to see what's going on there.

[ianstormtaylor]

Oh just realized this would make the log file mode even more awesome, since it would contain the actual properties and things too!

[astorm]

Happy to do this — however, I do think it leads to less readable code both from a phtml point of view, and from an end page source point of view. Putting the JSON into actual variables allows users, testers, and support folks to use the javascript debugger to view the actual properties in a sane formatted way and means we're not fiddling with specific properties too much in backend code.

However, it's a minor change. I'll drop it in place in the next update.

[ianstormtaylor]

Awesome, thanks! Good point about the downsides. I think they might get injected all in the same place? So we might mitigate that a bit, but it would definitely make support on our end a ton easier

[ianstormtaylor]

Actually just realized https://github.com/segmentio/analytics-magento/issues/23 might be a good reason to have a single track template, that takes an $event and $properties, so that the context can be easily added to all of them in one place

[astorm]

Just committed a change that outputs JSON objects as parameters where possible.

When the API calls for a scaler (i.e. string) to be send through we'll need to stick to the javascript variables approach. PHP/Magento has no built in way to sanitize a string for inclusion in Javascript — JSON encoding is the accepted best practice for getting a string from the server side to the client side.

A single track template is a bad idea — jamming everything together like that will be a code maintenance nightmare. See the other ticket/template for more context on that, but we're using the getContextJson method for that now.

[ianstormtaylor]

Can you explain that stringifying more? What's wrong with something like this:

$event = "event"; 
?>
<script>analytics.track(<?php json_encode($event) ?>);</script>

Which would properly output the string there?

[ianstormtaylor]

I also don't understand how having a single track template is a code maintenance nightmare in this case, we do it in WordPress just fine? Like so: https://raw.githubusercontent.com/segmentio/analytics-wordpress/master/templates/track.php — ignore the bit about the AJAX calls in there.

[astorm]

Scalers aren't part of the JSON specification — and without that specification it's not clear what escaping rules each different version of PHP applies to those strings. I've never liked how json_encdoe's does that, and my experience tells me to shy away from it to avoid XSS problems. If you think it's worth the risk let me know and I'll change things.

Re: the track template, having an individual template for each different track call makes it much easier for third party Magento developers to identify which event is happening where. A good percentage of Magento developers work purely with the phtml files. Additionally, it makes things easier for the extension maintainer (me) if/when the track calls start varying in what sort of parameters they need. My experience tells me this is the right way to build this out and I don't see what business goals would be met by such a refactoring. If you think it's worth my time to refactor this let me know.

[astorm]

Here's some more context on safely outputting scalar values with json_encode. There are Magento installations that go back to PHP 5.2, and PHP 5.2's json_encode doesn't have the constant flags mentioned in that Stack Overflow thread. We could (maybe) write out own parsing/cleaning routine, but I've seen too many edge cases creep in to that sort of code to feel comfortable launching recommending it.

[ianstormtaylor]

What's your sense for the percentage of installs that are on the old version that make things hard for us? I think we're going to want the strings to be there in the source, which also lets us remove all of the JS globals. If it's a smaller percentage of installs then we can potentially just drop support for them instead of having to add our own escaping?

[astorm]

Let's meet half way on this one — it sounds like having the parameters inline is super important for you, but I'm not shipping code with a known XSS vulnerability in it.

I've implemented a simple filter for string values — it users json_ecode and then whitelists known safe characters as opposed to trying to to strip out known bad ones. This provides adequate XSS protection, and lets you have your parameters inline.

[ianstormtaylor]

Nice, that sounds like a perfect solution. One thing, can we add : to the allowed characters? I can see people putting that in their event names as a way to show hierarchy. $ might also be a good one.

[astorm]

Added characters in latest pull request.