This is a performance concern.

Is there a specific reason why this is done?

I looked at the code and there isn't a comment to go with it.

======

It is because drupal_add_js() does not support adding a script with both a src="" attribute and body. So Gigya uses drupal_add_html_head(), which gets rendered into <head> before CSS and scripts in most themes. (drupal_add_html_head() does not allow the "weight" to be set.)

CommentFileSizeAuthor
#23 interdiff-2020717-22-23.txt682 bytesrobcolburn
#23 gigya-socialize-js-footer-2020717-23.patch43.96 KBrobcolburn
#22 interdiff-2020717-14-22.txt43.28 KBrobcolburn
#22 gigya-socialize-js-footer-2020717-22.patch43.98 KBrobcolburn
#21 interdiff-2020717-14-21.txt40.48 KBrobcolburn
#21 gigya-socialize-js-footer-2020717-21.patch40.93 KBrobcolburn
#16 interdiff-2020717-14-16.txt649 bytesElijah Lynn
#16 gigya-socialize-js-footer-2020717-16.patch2.07 KBElijah Lynn
#14 interdiff-2020717-12-14.txt818 bytesElijah Lynn
#14 gigya-socialize-js-footer-2020717-14.patch1.86 KBElijah Lynn
#12 8152777-12-gigya-script_inclusion.patch1.97 KBrobcolburn
#9 8152777-9-gigya-script_inclusion.patch1.45 KBelliotttf
#8 8152777-7-gigya-script_inclusion.patch1.17 KBelliotttf
#5 moved-socialize_js-to-footer-2020717-4.patch1.83 KBAlex.Zabavsky
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gambaweb’s picture

gambaweb CreditAttribution: gambaweb commented

The only reason for that is the we need the gigya script to be loaded before we call its functions.
Could you please elaborate more way is this a concern for you?

torgosPizza’s picture

torgosPizza
he/him
Primary language English
Location Portland, OR
CreditAttribution: torgosPizza commented

@gambaweb, it's possible to use the 'weight' setting attribute (among others) to ensure the Gigya script is added in front of other scripts in the same scope. See the drupal_add_js() API documentation. Using the settings in this API function will allow the JS to get aggregated along with the rest of the site's Javascript.

I'd recommend the JS-addition functionality in the Gigya module(s) use this feature to help performance and cleanliness.

rupl’s picture

rupl
he/him
Primary language English
CreditAttribution: rupl commented

To elaborate on the performance concern, adding JS before CSS causes the web page to stop rendering, and it causes a massive degradation in the performance of a page load.

As was already mentioned, it is a much better practice to use the group and/or weight settings inside drupal_add_js(). Even better: define your main library using the Libraries API 7.x-2.x and libraries_load().

Regardless of your choice, using drupal_add_html_head() to add JavaScript should be avoided in all situations.

Take a look at the Modernizr module source code for an example. Modernizr has the need to be the first script (or really close to it), so that it can provide class names on the <html> tag which are used for presentation. The module uses a large negative number for both group and weight (in addition to explicitly defining a scope of 'header'), such that it normally appears first in a Drupal JS listing.

I am the maintainer of the Modernizr module and am happy to assist you in re-configuring the JS for the Gigya module so that it is more performant.

Alex.Zabavsky’s picture

Alex.Zabavsky CreditAttribution: Alex.Zabavsky commented

I am preparing a patch for you, Eric. Going to upload it shortly.

Alex.Zabavsky’s picture

Alex.Zabavsky CreditAttribution: Alex.Zabavsky commented
Issue summary: View changes
FileSize
moved-socialize_js-to-footer-2020717-4.patch1.83 KB
rupl’s picture

rupl
he/him
Primary language English
CreditAttribution: rupl commented
Status: Active » Needs work

This is a good start (nice job using hook_page_build()), but it might be detrimental to move the script that far down. He still needs the script before the other code, so using something similar to the following would work fine (code untested):


  $gigya_global = check_url(url($uri_prefix . '.gigya.com/JS/socialize.js?apikey=' . $gigya_apikey));
  drupal_add_js($gigya_global, array(
    'group' => JS_LIBRARY - 1,
    'weight' => -50,
    'scope' => 'header',
    'preprocess' => FALSE,
  ));

  $gigya_settings = json_encode($gigya_js_settings);
  drupal_add_js($gigya_settings, array(
    'group' => JS_LIBRARY - 1,
    'weight' => -49,
    'scope' => 'header',
    'preprocess' => FALSE,
    'type' => 'inline',
  ));
}

edit: I forgot the 'type' => 'inline' on the second call to add the settings. It's there now.

elliotttf’s picture

elliotttf CreditAttribution: elliotttf commented

The patch in #5 causes issues with the media module – specifically the media upload popup – because the footer piece of the page is unset in media_page_alter()

Updated patch that implements rupl's suggestions to follow.

elliotttf’s picture

elliotttf CreditAttribution: elliotttf commented
FileSize
8152777-7-gigya-script_inclusion.patch1.17 KB
elliotttf’s picture

elliotttf CreditAttribution: elliotttf commented
FileSize
8152777-9-gigya-script_inclusion.patch1.45 KB

Updating patch to use a relative protocol since protocol detection server side is not 100% reliable when reverse proxies are in play.

elliotttf’s picture

elliotttf CreditAttribution: elliotttf commented
Status: Needs work » Needs review
Bevan’s picture

Bevan CreditAttribution: Bevan commented
Category: Bug report » Feature request
Priority: Major » Normal
Issue summary: View changes
Status: Needs review » Needs work

This doesn't work because Gigya needs the settings to be in the same <script> tag that the src="" is loaded on. Otherwise it can not locate the JSON settings.

I think a better architecture would be something along the lines of:

  • In gigya_init(), call the first module that implements a new hook, E.g. hook_gigya_add_js()
  • If no implementation exists, do what it currently does.

Any patch should also document why drupal_add_html_head() is used in the default case. I have added this reason to the issue description.

However;

  • All/most of the JS code should be in an implementation of hook_page_build(), instead of hook_init(). That way it won't run on pages that return AJAX requests or process image styles and be more performant for those pages. It will also give many more options about how and where to add the script, since the entire page is available as structured data in hook_page_build().
  • A much better default would be to add all of the scripts together, in the correct order, to the end of the <body>, if the script is capable of executing successfully there. This would allow the page to render without being blocked by the DNS lookup, HTTP request, HTTP transfer (for unprimed browser caches) and JS execution of this JS file, which is much lower priority than the page's content. See also #784626: Default all JS to the footer, allow asset libraries to force their JS to the header

These last two points would probably mitigate the performance concerns of the original poster.

robcolburn’s picture

robcolburn CreditAttribution: robcolburn commented
Status: Needs work » Needs review
FileSize
8152777-12-gigya-script_inclusion.patch1.97 KB

This patch is not the best option ever, nor is it the most elegant. It just does the minimum to achieve the task and Bevan's comment.

We can debate semantics later. :)

Bevan’s picture

Bevan CreditAttribution: Bevan commented

This is a great start! There are a few style errors and maybe functional errors too. However my main concern is that it still does the heavy lifting on hook_init(). Is there any reason it can't be moved to hook_page_build()?

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn commented
FileSize
gigya-socialize-js-footer-2020717-14.patch1.86 KB
interdiff-2020717-12-14.txt818 bytes

Rerolling #12 to be relative to module root (git diff --relative > patch) + a few docblock improvements.

Maybe it can be improved more but it does work so I will leave my RTBC in the comments here.

robcolburn’s picture

robcolburn CreditAttribution: robcolburn commented

@Bevan,

Sorry, I stalled on answering your questions.

I totally agree with your comments. Getting out of hook_init() significantly helps backend performance, and getting scripts to footer significantly helps frontend performance. Unfortunately, this is a difficult to achieve goal. I tried for awhile, but kept hitting unexpected behavior.

* Some of the hook_init logic deals with user session/authentication, so it's difficult to move down to the page_build scope.
* The JS is very order dependent, so we need to be careful about it's execution order.
* It's difficult to piecemeal move things from hook_init to hook_page_build without breaking things.

Perhaps, we can think of this as a first step?

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn commented
FileSize
gigya-socialize-js-footer-2020717-16.patch2.07 KB
interdiff-2020717-14-16.txt649 bytes

I added a async and defer (for older browsers) to the socialize.js tag. It helps a bit on the performance end since jQuery waits for DOMContentLoaded to do it's stuff and we have a site that depends on that for bringing in images.

robcolburn’s picture

robcolburn CreditAttribution: robcolburn commented

@Elijah,
Does it continue to work okay with defer (esp in IE8)?

In particular, can we be sure that gigya will be loaded before window.onload fires? Because once onload fires, we're ok a way ticket through jQuery.ready and Drupal.behaviors to run the "glue" scripts in this module [1], which assume the presence of the gigya global object.

I believe async should be fine, but defer might get us in to trouble, since IE8 doesn't guarantee execution order, and the gigya script take seconds to load.

[1] Example http://cgit.drupalcode.org/gigya/tree/js/gigya_sharebar.js

Aside
------------
If you really need to delay some JavaScript, like say you needed to have most of your JS wait for the "frame buffered" event of from your 3rd party video player. Then you might consider this module JS Defer module we wrote. You need to add custom code to define your deferred scripts, and the event to listen for. But, after that you get a bunch of stuff kicked to the curb.
https://www.drupal.org/project/js_defer

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn commented

Have no idea right now, it appeared to be fine in my testing. Our QA team is going through it and I will report back any findings.

msound’s picture

msound CreditAttribution: msound commented

In continuation of Elijah's last comment: Our QA team did find an issue with Patch #16. It breaks Gigya share bar. We are moving back to patch #14.

@robcolburn, you are right. the `gigya` global object is not available when its needed.

robcolburn’s picture

robcolburn CreditAttribution: robcolburn commented

I've got an idea here, but haven't had time to go back, but perhaps if someone else does.

Gigya provides a ready callback "onGigyaServiceReady" which you define, then they execute once.
http://developers.gigya.com/010_Developer_Guide/84_Using_the_Client_API/...

We could add a little js to gigya.js

/js/gigya.js

window.onGigya = $.Deferred();
window.onGigyaServiceReady = function () {
  onGigya.resolve(gigya);
};

With that in place, we could update all the dependent parts of the script.

Before /js/gigya.behaviors.js

...
$('.friends-ui:not(.gigyaNotifyFriends-processed)', context).addClass('gigyaNotifyFriends-processed').each(function () {
  gigya.services.socialize.getUserInfo({callback:Drupal.gigya.notifyFriendsCallback});
  gigya.services.socialize.addEventHandlers({ onConnect:Drupal.gigya.notifyFriendsCallback, onDisconnect:Drupal.gigya.notifyFriendsCallback});
});
...

After /js/gigya.behaviors.js

...
onGigya.done(function(gigya) {
  $('.friends-ui:not(.gigyaNotifyFriends-processed)', context).addClass('gigyaNotifyFriends-processed').each(function () {
    gigya.services.socialize.getUserInfo({callback:Drupal.gigya.notifyFriendsCallback});
    gigya.services.socialize.addEventHandlers({ onConnect:Drupal.gigya.notifyFriendsCallback, onDisconnect:Drupal.gigya.notifyFriendsCallback});
  });
});
...

Being careful of anything that makes execution order assumptions.

robcolburn’s picture

robcolburn CreditAttribution: robcolburn commented
FileSize
gigya-socialize-js-footer-2020717-21.patch40.93 KB
interdiff-2020717-14-21.txt40.48 KB

Okay, here's an patch that essentially does what I just said, in particular:
* It provides `gigya` via promise rather than assumed synchronous global.
* Organizes JS in to hook_library to manage order.
* Updates related JS to use promise.
* Pushes much more logic into hook_page_build.

Before

gigya.whatever.doSomething();

After

Drupal.gigya.ready(function (gigya) {
  gigya.whatever.doSomething();
});

Major Caveats:
* I've only coded this, and haven't tested it at all yet.
* The new async logic is behind a variable `gigya_async`, which you can set in the admin. It's behind a variable because you may have custom code on your site that depends on the synchronous behavior (ex: https://www.drupal.org/node/2387495 ).

robcolburn’s picture

robcolburn CreditAttribution: robcolburn commented
FileSize
gigya-socialize-js-footer-2020717-22.patch43.98 KB
interdiff-2020717-14-22.txt43.28 KB

Fixing typos.

robcolburn’s picture

robcolburn CreditAttribution: robcolburn commented
FileSize
gigya-socialize-js-footer-2020717-23.patch43.96 KB
interdiff-2020717-22-23.txt682 bytes

Fix typo

Gigya’s picture

Gigya CreditAttribution: Gigya commented
Status: Needs review » Closed (works as designed)

Patches provided that may not suit all sites.

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn commented
Status: Closed (works as designed) » Needs work

@Gigya Could you please respond more appropriately? This is a performance issue. Performance affects all sites. Maybe the patches don't work right now but they could with some more work.

Please be more specific when closing this issue.

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn commented

@Gigya Also, just a friendly heads up that shared accounts are not permitted to comment on nodes/issues. Not sure if this is a shared account but seems that it may be possible given that your username is named after the Gigya product.

See the Drupal Terms of Service here https://www.drupal.org/terms

2. User accounts are intended for personal use by an individual.

3. If you are sharing your user account with multiple people (e.g. as your “official” organization account), you are not allowed to do the following using this account:

* commit code to Git repositories on the Website
* create any nodes except for organization, case study or project nodes
* comment on nodes

If you are sharing your user account with multiple people you ARE allowed to:

* create project nodes
* create organization nodes
* create case study nodes
* submit translations to localize.drupal.org

Elijah Lynn’s picture

Elijah Lynn
Location Portland, Oregon
CreditAttribution: Elijah Lynn commented
Status: Needs work » Needs review
sokrplare’s picture

sokrplare CreditAttribution: sokrplare commented

Bumping this issue up as it is an important one for our sites. We've had an internal ticket open since the start of this year waiting on this patch to get wrapped up.

@Gigya - what can we do to help get this into the module?

luciodiri’s picture

luciodiri CreditAttribution: luciodiri commented

Gigya minor version 4.8 now includes the module scripts via the more appropriate drupal_add_js script.
(instead of drupal_add_html_head)
See in 7.x-4.8 release on gigya.module line 74-96:

 //// Add all the js here, to speed page load w/ aggregation enabled.
  // Also add the basic JS needed for the module.
  $uri_prefix = !empty($_SERVER['HTTPS']) ? 'https://cdns' : 'http://cdn';
  $gigya_script_src = $uri_prefix . '.gigya.com/JS/socialize.js?apikey=' . $gigya_apikey;

  // add gigya settings script inline
  drupal_add_js(
    json_encode($gigya_js_settings),
    array(
      "type" => "inline",
      "weight" => 20
    )
  );
  //    add gigya external script call
  //    (after setting the global config script)

  drupal_add_js(
    $gigya_script_src,
    array(
      "type" => "external",
      "weight" => 21
    )
  );
robcolburn’s picture

robcolburn CreditAttribution: robcolburn commented

@luciodiri,
I'm glad Gigya is supporting more industry-standard ways of passing in variables (inline script vs embedded in external script tag.

That doesn't really address the core issue raised here. The Gigya module currently requires that the third-party Gigya script be placed at the HEAD of the document. However, this is considered best practice that all scripts (especially, third party scripts) be placed at the bottom of the document and/or setting the async attribute. This allows the browser to defer loading and processing of the script till after the DOM is loaded (when the user has control of the page again). Placing scripts in the HEAD, causes the browser to lock and stop downloading & rendering the remaining document until it downloads and processes the script. If the Gigya takes even 300ms to load, then that cuts down 300ms performance at the most critical time - initial page load.

So, the first thing that needs to be addressed is pushing the external Gigya script to the bottom of the page and/or setting the async attribute. Afterward, all of the associated Gigya behaviors can be pushed to the bottom of the page. However, if the external Gigya script is asynchronous this will cause a cascading issue on all the Gigya behaviors - whereby they expect the gigya variable to be available. To accommodate for that behavior, I re-factored the Gigya behaviors to know that the gigya variable will be supplied via a JS Promise (technically, jQuery.Deferred object because D7 behaviors are rooted in jQuery).

Further, the practice of promising the 3rd-party supplied gigya variable acts a safe-guard if the Gigya script becomes unavailable for any reason (Gigya outage, blocked by browser plugin, blocked by ISP/country firewall, network latency, etc…). Protecting the Drupal site and Drupal Behaviors sub-system by isolating down the Gigya functionality.

If the maintainers of the Gigya module believe that these concerns are important (front-end performance, front-end application security). Then the previously provided patch can be re-rolled on top of the changes to the module.

Personally, while I wrote a substantial portion of the currently proposed patch, it is not likely that I will assist in re-rolling the patch. The brand that I performed the patching for, was later burned by aforementioned Gigya security issues, and became wary of these performance issues as well. So, there is no business value for me to continue contributing to the Gigya module. Further, the module maintainers seem to ignore and/or reject the issue which is pretty de-motivating for a patch author.

luciodiri’s picture

luciodiri CreditAttribution: luciodiri commented

@robcolburn,
My comment referred to the fix in version 4.8 of the module, which gives the solution to the original topic raised by @ericduran. this fix has other implications which are important to the module functionality.
The further discussion and suggested patches regarding performance optimization, is a different and more extensive line of discussion. that is the reason that although the original topic was answered, I did not close the issue.
We very much appreciate any contribution and keep track of continuing this discussion and implementing when and what's possible.