Allow asynchronous loading of GPT

I'm trying to get my hands around exactly how this will affect an existing site. In other words, if an existing site changed nothing except updating the module what would happen (assuming they have async checked)? Believe it or not I do not have a DFP account with which to test this, so please be really detailed with what this change looks like to the end user and to the site admin who will be fired if ads stop working the way they used to...

Below are a couple of small things...

1. +++ b/dfp.admin.inc
   @@ -71,6 +71,27 @@ function dfp_admin_settings($form, $form_state) {
   +    $options['async'] = t('Script tag with async and defer attributes');
   

   What happens if someone turns off advagg in the future?

2. +++ b/dfp.admin.inc
   @@ -71,6 +71,27 @@ function dfp_admin_settings($form, $form_state) {
   +    '#description' => t('Select the asynchronous loading method of the . Inline snippet is the default and recommended by Google. "Async and defer" method is only available if <a href="!url">advagg module</a> is enabled.'),
   

   Can you make "Recommended by google" a link to that page

This patch is no longer relevant as Google's documentation on what the structure should be has now changed. You can see the current recommendation here:
https://support.google.com/dfp_sb/answer/1651549?hl=en#async

With the current module setup there is no way to have the DFP script loading async and therefore it becomes a render blocking issue for performance. This is something that does need to be addressed.

There is further discussion on how this should be patched for the DFP module in this issue thread:
https://www.drupal.org/node/2629154

Hi Chris,

Do I understand that atm the module does not support async loading?

We are having some issues and I've detected that the site takes a lot to load, in part because of the ads being loaded. In an async scenario I would expect tyke site to be loaded, and then the ads in a different thread, not blocking the whole page Load and displaying the loading icon as loaded a lot earlier (although the ads are still being loaded)

Am I wrong?

Thanks

sorry, answering myself, async is provided in gpt module. I agree with https://www.drupal.org/node/1322754#comment-6535548 this should have not be a new module, introduces confusion.

Hello Chris:

Following your suggestion about the current recommendation:
https://support.google.com/dfp_sb/answer/1651549?hl=en#async

We have this patch working in our project.

Best

I appreciate that the maintainers of this module didn't initially want this in the module but by all current UX standards and even Google's own standards (exemplified by their AMP project) I think this issue should be considered heavily again. @rescandon thank you for taking another stab at this. I'm going to see if I can test this tonight. Could the maintainers please take a look at the patch in #11 to see if it addresses the expressed concerns? Thank you.

I've never been particularly against this feature I just dont want to introduce it if it might screw with someone's existing installation. I asked this question:

I'm trying to get my hands around exactly how this will affect an existing site. In other words, if an existing site changed nothing except updating the module what would happen (assuming they have async checked)?

And got one response that it shouldn't affect an existing user (see #4). I would love for someone to confirm that.

The patch in #11 looks like it adds async with minimal other changes. However, the default value for the dfp_async_load variable is consistent between the settings page and the rendering code.

Here is a version of the patch in #11 with a few minor changes/fixes. The new async loading defaults to off to avoid issues with existing installs.

It's also rooted in the project directory instead of the drupal root so that it's easier to apply.

I think that #16 looks pretty good. I'd like for someone else to RTBC it before I'd commit it though.

+++ b/dfp.module
@@ -681,6 +688,7 @@ function _dfp_js_global_settings() {
+  $options['async'] = _dfp_js_async_load();

I'm not sure how this works, I don't think drupal_add_js() supports it? I made this work with $options['attributes']['async'] = 'async' and the core patch from #1664602: Allow attributes to be passed to drupal_add_[css|js] (SRI)

Ah, i see now you were relying on advagg to provide that feature, I also removed the module_exists() check on that as I'm not using advagg.

If this patch requires advagg or a core patch, thats a whole different animal. In that case I would MUCH prefer that this was implemented in a separate module that depends on advagg rather than add a new dependency here.

Thoughts?

If it was me, i'd probably postpone this issue on the core one, once the core issue is committed we can safely commit a patch here until that point, others can just use those patches available?

Depending on advagg doesn't seem like a great solution. We could consider depending on https://www.drupal.org/project/async_js if you really want to fix it quicker.

Re-rolled the patch

Hello all,

Google has updated it's GPT API again which has added an async attribute to the script tag as well as removing enableAsyncRendering() call. I have applied these changes to a fresh version of 7.x-1.x.

Re-rolling the patch written by Peps84

All right,
Fixed tests and added some extra tests too.
Attaching new patch and interdiff against peps84 patch.

Working as expected for me in 7.x-1.x

This patch doesn't apply, ignore this attached file.

I've found in the documentation that enableAsyncRendering() doesn't need to be removed as even if it's the default option it can still be used to override previous configs.

https://developers.google.com/doubleclick-gpt/reference#googletag.PubAds...

Re-rolled patch #30

We could be loading the `gpt.js` with some javascript like so:

```
$gads_js = << (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);
})();
dfp;

// Include a script tag for the Google Tag Services.
$options['type'] = 'external';
$options['weight']++;
drupal_add_js($gads_js, $options);
```

This way we have async also on the script tag getting gpt.js. What do you think? We wouldn't need the advagg module?

I've re-rolled the patch. I also fixed adding the async attribute per the DFP Documentation Async Example. The async attribute is only necessary for adding the gpt.js script. Additionally, per the example, the gpt.js script should be the first of the DFP scripts, so I added weight and group to accomplish this.

Re-rolled patch from #39.

When using the AdvAgg Modifier module to group together async loaded JS in the header, the gtp.js script is added after the inline JS. Even though the ads are loaded fine with this order, it might be good to force the gtp.js script to be added before the inline JS, so it can start loading as soon as possible. This patch adds the 'scope_lock' => TRUE option for the gtp.js script, so it's loaded before the inline JS, even when using AdvAgg Modifier module.

Interdiff for #40 and #41

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