JS is throwing an error SyntaxError: Unexpected token ILLEGAL when the ad slot machine name starts with a number.

On this line $js = 'googletag.slots.' . $tag->machinename . ' = googletag.defineSlot("' . $tag->adunit . '", ' . $tag->size . ', "' . $tag->placeholder_id . '")' . "\n";
When the machine name is 123, googletag.slots.123 is injected to the page, which throws the Unexpected token ILLEGAL error and breaks the ad.

CommentFileSizeAuthor
#1 dfp-slotname-js-error-2078187.patch1.02 KBmakangus

Comments

makangus’s picture

makangus commented
Status: Active » Needs review
StatusFileSize
new dfp-slotname-js-error-2078187.patch1.02 KB

One thought is to initialize the the slot array style, which seems to work for me.

bleen’s picture

bleen commented

I'm happy to go with #1 but Id love to have someone else confirm that it works... anyone? anyone?

adamzimmermann’s picture

adamzimmermann commented

I can confirm it works.

If the machine name is just a number like "123", I get this error: "Uncaught SyntaxError: Unexpected number".

If the machine name starts with a number and also contains letters like "123abc", I get this error: "Uncaught SyntaxError: Unexpected token ILLEGAL".

In both cases the patch above fixes the error.

My only question is, would it be better to just enforce machine names that don't begin with a number by tacking on some validation functionality and keep the object structure? The .install porting of old dart tags would have to be modified then also then. If so, I can try to put together a patch for this in the next couple days.

bleen’s picture

bleen commented

My only question is, would it be better to just enforce machine names that don't begin with a number by tacking on some validation functionality and keep the object structure?

Probably, yes .. but if DFP doesnt have this restriction on the backend then we shouldnt (if possible) here.

makangus’s picture

makangus commented

I thought about this, but this error only started with the commit in June #2001498: Ad slot definition and disable initial load setting. That means there could be sites that are using numbers as the ad slot name perfectly fine. Adding the restriction would suddenly stops those sites from being able to edit the slots without changing the machine name.

adamzimmermann’s picture

adamzimmermann commented

Ah. Both good points.

I haven't worked with any front end DFP js, as I am just working on the backend rewrite of the module for D6. (New issue will be posted about that soon once I get to a semi stable point). Anyways, so I have no idea if changing that to an array is going to cause developers to have to rewrite their front end js, or what the purpose of storing the tags in the googletag.slots variable even is. I originally fixed this issue by just removing that assignment, which appears to be how it was before the issue you linked to. However, the array fix definitely fixes the issue. I'm just not knowledgeable enough to know if it causes any additional problems.

<?php
// Removed.
// $js = 'googletag.slots.' . $tag->machinename . ' = googletag.defineSlot("' . $tag->adunit . '", ' . $tag->size . ', "' . $tag->placeholder_id . '")' . "\n";

// Added.
$js = 'googletag.defineSlot("' . $tag->adunit . '", ' . $tag->size . ', "' . $tag->placeholder_id . '")' . "\n";
?>
bleen’s picture

bleen commented
Status: Needs review » Fixed

agreed ... committed

http://drupalcode.org/project/dfp.git/commit/9acc91a

makangus’s picture

makangus commented

To answer adamzimmermann

I briefly tested it and it doesn't seem to break anything. For people who are using the slots variable, they would have something like this in their js code

var slot = googletag.slots.machinename;
pubads().refresh(slot)

Luckily, in JS you can access the object the same way googletag.slots.machinename even if it's initialized array style. So in our case, googletag.slots.machinename and googletag.slots['machinename'] should give you the same thing.

adamzimmermann’s picture

adamzimmermann commented

@makangus - Awesome. Thanks for clearing that up. Glad this is fixed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.