I am currently working on getting ads up and running on a site, but noticed that the current state of the 7.x-1.x-dev branch doesn't particularly print the correct markup for the dfp ads on the page (at least based on their documentation: https://support.google.com/dfp_sb/answer/4623874?hl=en).

On my end, I only got the ads to work on the test page, but on none of the front-facing themes themes, even on the ones supported by Drupal core. After doing a little bit of reading, I've kind of got the impression that due to some changes in browser security and potential API changes, the current code may not be up to standard and may not reliably serve ads properly.

Installed the module, but no ads are working on my site regardless of the parameters I put in

CommentFileSizeAuthor
#30 interdiff-2629154-25-30.txt3.19 KBscottalan
#30 dfp-js-2869225-2-D7.patch20.25 KBscottalan
#25 interdiff-2629154-20-25.txt919 bytesscottalan
#25 update_to_dfp_ad_code-2629154-25.patch19.34 KBscottalan
#23 interdiff-2629154-20-23.txt537 bytesscottalan
#23 update_to_dfp_ad_code-2629154-23.patch19.38 KBscottalan
#20 update_to_dfp_ad_code-2629154-20.patch19.42 KBscottalan
#18 update_to_dfp_ad_code-2629154-18.patch19.55 KBscottalan
#16 dfp_googletag-2629154-15-7.patch18.84 KBscottalan
#10 Screen Shot 2017-03-16 at 18.04.51.png44.37 KBmarcelovani
#7 dfp-update_standards-2629154-7.patch7.37 KBmarcelovani
#6 dfp-update_standards-2629154-6.patch7.37 KBmarcelovani
#4 dfp-update_standards-2629154-2-7.patch7.3 KBpcho
#3 dfp-update_standards-2629154-1-7.patch7.3 KBpcho

Comments

pcho created an issue. See original summary.

bleen’s picture

bleen commented

Canyou give more specifics about what changes you think need to be made? There are ~1200 sites using this module and if ads weren't working because the module couldn't connect to DFP properly then a LOT of people would be complaining of problems

pcho’s picture

pcho commented
StatusFileSize
new dfp-update_standards-2629154-1-7.patch7.3 KB

I've written a patch to address this. If the current module still works, you may not need to apply this. This is a blocker for me, so I might have to use this going forward.

You can view the diffs in a more prettier format here: https://github.com/pchop2/drupal_dfp/pull/1/files

Note: This was originally a fork, but I don't think the changes are drastic enough to warrant it be a refactor.

Another note: Patch also has changes to the way we import the gpt.js file from the old method, rewriting the spaces to the output $js variable, and additional documentation to make the code more readable.

pcho’s picture

pcho commented
StatusFileSize
new dfp-update_standards-2629154-2-7.patch7.3 KB

I have one minor update to my patch, which fixes an issue with breakpoints in the js. I am deferring loading that stuff to later.

pcho’s picture

pcho commented

For a more in depth explanation, when I installed the module originally, I noticed that a lot of the method calls were not placed in the proper scope. In addition, I could not find any evidence that the googletag object was actually read all the ads being compiled in the array prior to the push() method.

The third party vendor gave me code that slightly deviates from what the module sets up. Though I am not expecting this patch gets merged due to my edge case, I put it out there just in case someone else might have ran into the same problems I did due to code compliance.

This is what I got as sample code:

<!DOCTYPE HTML> 
<html lang="en-us">
<head>
<meta http-equiv=""Content-type"" content=""text/html; charset=utf-8""> 
<title>Widgets Magazine</title> 
<style type=""text/css"" media=""screen""> 
</style>

<script type='text/javascript'>
  var googletag = googletag || {};
  googletag.cmd = googletag.cmd || [];
  (function() {
    var gads = document.createElement('script');
    gads.async = true;
    gads.type = 'text/javascript';
    var useSSL = 'https:' == document.location.protocol;
    gads.src = (useSSL ? 'https:' : 'http:') +
      '//www.googletagservices.com/tag/js/gpt.js';
    var node = document.getElementsByTagName('script')[0];
    node.parentNode.insertBefore(gads, node);
  })();
</script>

<script>
   var networkCode = 1234;
   var topLevelAdUnit = "acme";
   var s1 = "homepage";
   var adUnit = topLevelAdUnit + "/" + s1;
   var slotName = "/" + networkCode + "/" + adUnit;
   var pid = "homepage";
   var pgtype = "landing";
   var breakpoint = "desktop";
   var test = "[value]"; //only if "?test=[value]" is appended to URL
  googletag.cmd.push(function() {
    googletag.defineSlot(slotName, [[970, 90], [970, 250]] "div-id-for-970x90-top").addService(googletag.pubads())
     .setTargeting("pos", "top");
    googletag.defineSlot(slotName, [[300, 250], [300, 600]] "div-id-for-300x250-top")
     .addService(googletag.pubads())
     .setTargeting("pos", "top");
    googletag.defineSlot(slotName, [300, 250], "div-id-for-300x250-mid")
     .addService(googletag.pubads())
     .setTargeting("pos", "mid");
    googletag.defineSlot(slotName, [300, 250], "div-id-for-300x250-bot")
     .addService(googletag.pubads())
     .setTargeting("pos", "bot");
    googletag.defineOutOfPageSlot(slotName, "div-id-for-interstitial-ad")
     .addService(googletag.pubads())
     .setTargeting("pos", "interstitial");
      googletag.pubads().setTargeting("s1",s1);     
      googletag.pubads().setTargeting("pid",pid);
      googletag.pubads().setTargeting("pgtype",pgtype);
      googletag.pubads().setTargeting("breakpoint",breakpoint);
      googletag.pubads().setTargeting("test",test);
      googletag.pubads().enableSingleRequest();
      googletag.enableServices();
   });
</script>
  
  
  

</head>
<body>


<div id="div-id-for-970x90-top">
</div>
<div id="div-id-for-300x250-top">
</div>
<div id="div-id-for-300x250-mid">
</div>
<div id="div-id-for-300x250-bot">
</div>
<div id="div-id-for-interstitial-ad">
</div>
 <script type="text/javascript">
  googletag.cmd.push(function() {
        googletag.display("div-id-for-970x90-top");
        googletag.display("div-id-for-300x250-top");
        googletag.display("div-id-for-300x250-mid");
        googletag.display("div-id-for-300x250-bot");
        googletag.display("div-id-for-interstitial-ad");
       });
  </script> 

</body>
</html>
marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented
Status: Active » Needs review
StatusFileSize
new dfp-update_standards-2629154-6.patch7.37 KB

Re-rolling the patch

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented
StatusFileSize
new dfp-update_standards-2629154-7.patch7.37 KB
marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented
Assigned: Unassigned » marcelovani
Status: Needs review » Postponed

Trying to make CI show the button that said something like 'Do not wait for the test to pass'

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented
Status: Postponed » Needs review
marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented
Issue summary: View changes
StatusFileSize
new Screen Shot 2017-03-16 at 18.04.51.png44.37 KB

The last submitted patch, 6: dfp-update_standards-2629154-6.patch, failed testing.

The last submitted patch, 3: dfp-update_standards-2629154-1-7.patch, failed testing.

The last submitted patch, 4: dfp-update_standards-2629154-2-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 7: dfp-update_standards-2629154-7.patch, failed testing.

scottalan’s picture

scottalan commented

I'm posting this patch here since this is where this refactor began.

I'm taking pieces of the original patch but also decided to move most of the inline JS to files.

This has been tested on a site that serves lots of ads. Let me know what you think.

Scott

scottalan’s picture

scottalan commented
StatusFileSize
new dfp_googletag-2629154-15-7.patch18.84 KB

and the patch...forgot to attach it

scottalan’s picture

scottalan commented

Will be pushing a new patch. I didn't realize that there were some dfp formatters that needed to be updated as well to handle the changes I've made.

e.g., 

$formatted_sizes[] = '[' . implode(', ', $formatted_size) . ']';
// to
$formatted_sizes[] = $formatted_size;

return count($formatted_sizes) == 1 ? $formatted_sizes[0] : $formatted_sizes;

and a few other things as well.

scottalan’s picture

scottalan commented
StatusFileSize
new update_to_dfp_ad_code-2629154-18.patch19.55 KB

Here's an updated patch. This does not contain any changes to tests but just wanted to get this up so others can test it.

This takes some of what was being done here https://github.com/pchop2/drupal_dfp.

For example taking VAST ads into consideration. This also adds the ability to save a GTM (Google Tag Manger) key that will automatically add GTM to the site.

scottalan’s picture

scottalan commented
Status: Needs work » Needs review
scottalan’s picture

scottalan commented
StatusFileSize
new update_to_dfp_ad_code-2629154-20.patch19.42 KB

I'm not sure what's going on but it seems that git apply doesn't like the fact that I've added a `js` directory and two files.

It applies cleanly using:

curl https://www.drupal.org/files/issues/update_to_dfp_ad_code-2629154-20.patch | patch -p1

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented

Hi Scottalan

I am a bit confused with the patch. I am not saying it is wrong or anything, because I did not test it.
But it seems that what you are suggesting is to use Google Tag Manager with DFP.

Should not this be a separate issue?

Also, I was wondering if its possible to deliver DFP ads using https://www.drupal.org/project/google_tag
more info here https://georgegarside.com/blog/google/doubleclick-publishers-google-tag-...

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented
Assigned: marcelovani » Unassigned
scottalan’s picture

scottalan commented
StatusFileSize
new update_to_dfp_ad_code-2629154-23.patch19.38 KB
new interdiff-2629154-20-23.txt537 bytes

Minor update. Needed to modify logic for targeting.

I added an interdiff to show the change.

scottalan’s picture

scottalan commented

@marcelovani

I'm sure it's possible to use https://www.drupal.org/project/google_tag but seemed like a lot of bloat when you can simply add a snippet of code, reference: https://developers.google.com/tag-manager/quickstart, and just add the container ID for your account. Just seemed like a nice, simple add-on.

As far as moving all the javascript to Drupal settings, it seemed like the logical thing to do. I'm not saying it's "wrong" but I prefer to have all JS in files and let Drupal settings do what it was meant to do. If you notice, I did not add the "User defined JS (1 & 2)". I'm also not a fan of allowing JS (nor PHP) in textfields in the UI. Moving all the JS to settings with weighting allows a developer to easily inject JS where (weighted) they want.

Let me give you a specific use case we ran into: The ad agency requested some special sizing parameters solely based on browser dimensions for banner ads. It was much easier to move the front-end logic being built in PHP functions to stand-alone files so I could manipulate what was needed. Apparently, another dev was trying to use the "User defined" JS fields to do this but it seemed messy to me. I was able to create a very simple JS file, weight it appropriately, and create a global variable that was accessible. Could this be done with the textfield...sure, but like I said, not a fan of "evalish" code in textfields.

I'm curious as to why this module chose to build the JS inline through string concatenation vs using Drupal settings to begin with, maybe just a preference? Either way, I had already setup most of the variable needed for Drupal settings in a custom implementation anyway so I thought I would just complete the task and post it back here as a patch.

I was thinking maybe this could just be the start of a 7.x-2.x branch or something. I realize without the eval textfields this would break backwards compatibility for those that are actually using them.

I'd be more than glad to make any changes you deem necessary to the patch I've provided if you want to go that direction and use it.

Thanks,

Scott

scottalan’s picture

scottalan commented
StatusFileSize
new update_to_dfp_ad_code-2629154-25.patch19.34 KB
new interdiff-2629154-20-25.txt919 bytes

Missed the same fix for 'breakpoints'. Adding a patch and another interdiff that shows both fixes.

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented

Hi Scottalan

Totally valid points.

I agree with creating a new 2.x branch too.

I was wondering if we should start a new ticket to propose this, what do you think?

scottalan’s picture

scottalan commented

@marcelovani,

Sounds like a plan. Do you want to create the new ticket with the information you would like to see in it and then we could just link to the latest patch here or I could just upload to the new ticket.

marcelovani’s picture

marcelovani
Primary language Portuguese, Brazil
Location London
commented

You can create it and explain your proposed solution as you did on #24
Then add the link on this issue for reference.

scottalan’s picture

scottalan commented

Will do.

I'm working through any potential bugs and then I'll create a new ticket.

scottalan’s picture

scottalan commented
StatusFileSize
new dfp-js-2869225-2-D7.patch20.25 KB
new interdiff-2629154-25-30.txt3.19 KB

I'll go ahead and add the latest patch here with an interdiff but I created a new issue here: https://www.drupal.org/node/2869225 with the full patch.

The last submitted patch, 16: dfp_googletag-2629154-15-7.patch, failed testing.

The last submitted patch, 18: update_to_dfp_ad_code-2629154-18.patch, failed testing.

The last submitted patch, 20: update_to_dfp_ad_code-2629154-20.patch, failed testing.

The last submitted patch, 23: update_to_dfp_ad_code-2629154-23.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 30: dfp-js-2869225-2-D7.patch, failed testing.

vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
Status: Needs work » Closed (outdated)

Thank you for your contributions.
Drupal 7 is no longer supported.
Closing issue as outdated.