Allow drupal_add_js to load external Javascript files

this is required for some much more modules, too. for e.g. Google Analytics.

This patch tries to solve this by adding an extra scope to the javascript loaders

This patch looks like RTBC...

Additional something i found - it seems not possibly to add JS inline code in the closure variable. This is for e.g. required for Google Analytics and some other statistic modules. This seems currently not possible or i was only unsuccessful... 'header' and 'footer' works but 'closure' not.

It would certainly make referencing remote javascript urls cleaner inside drupal modules. So the feature request gets a +1 for me to use in the Google Analytics module.

There was a bug inside the patch. I fixed the ordering of JS files embedded into HTML and tested successful with Google Analytics under D51. Nevertheless the patch is build against today HEAD.

This is the example code for current Google Analytics module.

With the patch provided in #2 the order of JS ends with the following wrong code order:

This is now fixed with this patch and produces the following output. This loads first the external files with the functions possibly required by the types "inline" or "settings":

here is the missing part of my last posting:

This looks good to me.

Can't you work around this with drupal_set_html_head()? If so, I'd like to not commit this to 5.x, as it's an API change and 5.x modules that use it will break without it.

This was original build as a feature for D6 for not using a workaround like drupal_set_html_head()... if you'd like to commit to D5, well - we are happy, too. drupal_set_html_head() may work.

case 'external': could be placed after case 'inline':? Because it would be the rarest option used in drupal_add_js and this is just a tiny performance tweak :) Changing status..

Sorry, but you are wrong. Read my above posting, please. we first need to add JS code - no matter if - internal or external and then we must add inline code or settings code. if we do different, we cannot call a function from inline code that is inside the included JS files. therefor this order is a *must*.

Well, if you think so, you are incorrect. With this order you can't accomplish what you are trying to explain. You are not passing all the JS and options altogether to drupal_add_js where it decides which JS code to output first and what second. This order is just normal syntax.

I think the problems of adding inline scripts to closures and order of loading external and internal scripts is not the most important part of this issue.

So adding the new scope to a position in the switch that is fastest would be the best start, but I don't believe the few extra cycles that will save compared to my patch will matter that much.

if my code looks like

i get a function urchinTracker() is not defined error.

If i use my patch the order is done in the following way and i have NO function undefined error.

The order is therefor very important - if you have functions that are executed one page load, isn't it?

Yes it's important, and to accomplish that, you really don't need to patch drupal. Just use it like:

So, your point is not related here. :) And the patch seems to be fine too. :)

Yes it's important, and to accomplish that, you really don't need to patch drupal. Just use it like:

drupal_add_js(..., 'external');
drupal_add_js(..., 'inline');

So, your point is not related here. :) And the patch seems to be fine too. :)

Sorry i don't understand you. There is currently no drupal_add_js(..., 'external'); in drupal core. So we need to patch drupal. why are you writing we don't need to patch? We need to patch.

If you think the patch is fine set to RTBC, please. So we have a chance to get this into D6 core. D6 code freeze is only 2 weeks away... THX

I mean you dont have to patch anything to get the series of external or inline, up or down. Sure you have to patch to get the external option in. Hope you get it. Don't worry, whatever has to, will get in on time.

I created this workaround until the external is added to drupal_add_js.

I needed to add Amazon affiliate script at the end of my document similar to the earlier post that needed a Google Analytics post. I did it by creating a text file called affiliate.js. All it contained was:

Then I used a php include to include it at the bottom of my post:

include('sites/js/affiliate.js');

You can see what it does at: http://www.bradrice.com/drupal/node/8

I would still like to see the code updated to allow for external links, but this is a temporary workaround.

Here is a slightly extended patch which
1. adds documentation for the 'external' option to drupal_add_js()
2. fixes a small coding-style issue (was present before the patch)
3. allows the external files to be preprocessed optionally

I really hope we can get support for 'external' javascript into D6. Its a much needed feature IMO, esp. for integration modules.

@Steven's question 'Can't you work around this with drupal_set_html_head()?'

Here is why I think this is not a satisfying solution ...
- drupal_set_html_head does not check for duplicated includes (drupal_add_js does)
- drupal_set_html_head doesnt cascade with other js includes. It always adds the files at the beginning of the head section, even before jquery.js, etc. Thats especially bad if you have an external application to embed, which also depends on jquery. You must include jquery manually in this case and it will be added twice in the end.

@profix898: are you sure preprocess will work with this patch? I don't think so... maybe i missed something, but you need to send a HTTP request to an external box, save the content to disk and so on and so on... I think this is no good idea to integrate external scripts into your local scripts... this will break many external scripts, too. And finally if the remote site change the file your local cache is not updated and therefor may not work until regenerated.

calling external HTTP request is problematic... if the remote server is down your server may goes down while it starts with tons of hanging HTTP request to the JS file on the external site.

@hass: drupal_build_js_cache() simply uses $contents .= _drupal_compress_js(file_get_contents($path). ';'); to read the file and AFAIK file_get_contents() also works for remote files. I think it should work that way, yes. I also dont think this would break the external script. If the script changes frequently you cant still call drupal_add_js() with preprocessing disabled and the cache is not updated for local modified files either.

BUT I agree about the HTTP request being problematic. If the external source is unavailable the server will hang instead of the browser. So lets remove that option. Attached patch is quite similar to yours but fixes the coding-style issue and adds docs.

Why does the 'external' option allow an array to be passed in, when this is not done for local files? (at least according to the doxygen). This would be inconsistent.

As for the drupal_set_html_head() workaround, please read my comment carefully. The original post asks whether this a bug report or a feature request. My point was that since the bug fix requires an API change, it was unlikely to be backported to 5.x, but if there was a workaround available, that there would no real reason to do so.

@Stephen: as stated above - we are not talking about a Bugfix. This is a Feature Request for D6, nothing else.

Sorry, I was wrong :( The 'external' option does - of course - not allow an array to be passed. Changed the doxygen accordingly ...

I still think this is an important feature and should go in ...

For Google Analytics feature request in http://drupal.org/node/182334 we need this fixed and i'm bumping this up now to get into D6.

Updated patch for latest HEAD. No changes - only updated to remove hunks.

Simple decision

Subscribing...

Updated to HEAD and tested it by using the following contents inside a new node with the PHP Filter:

<?php
  // Add an external Javascript file
  drupal_add_js('http://maps.google.com/maps?file=api&amp;v=2&amp;key=GOOGLEKEYHERE', 'external');

  // Add the Javascript to test it
  drupal_add_js('window.onload = function() {
      if (GBrowserIsCompatible()) {
        var map = new GMap2(document.getElementById("map_canvas"));
        map.setCenter(new GLatLng(37.4419, -122.1419), 13);
      }
  }', 'inline');

  echo '<div id="map_canvas" style="width:500px; height:300px;"></div>';
?>

It displays a Google Map by using an external Javascript file from Google. You'll need a Google Maps API key generated here.

API changes are not accepted in Drupal 6, unless they fix bugs. This adds a new feature, so postponed. Just as it was with 5.x (see above), this came too late. Please make sure you get this into 7.x early.

Robby Rob If its not too much to ask, could you update the patch for the first RC? Too bad this one didn't make 6!

Subscribing. Why is it so hard to add such a seemingly simple feature? Don't get why this wasn't committed ages ago.

This patch will have to be moved to HEAD. It would also be great if somehow we got caching working on the external scripts.

Please make caching for external an extra patch. this one is RTBC

Discussion of adding caching for the external Javascript files can continue here: Cache External Javascript.

Patch is getting hunk failures.

Rerolled to HEAD...

To test:

1. Fresh Drupal install
2. Apply the patch
3. Enable the PHP input filter module at admin/build/modules
4. Create a new page at node/add/page using the new PHP Input Filter with the following code:
   
   <?php
  // Add an external Javascript file
  drupal_add_js('http://maps.google.com/maps?file=api&amp;v=2&amp;key=GOOGLEKEYHERE', 'external');

  // Add the Javascript to test it
  drupal_add_js('$(document).ready(function() {
      if (GBrowserIsCompatible()) {
        var map = new GMap2(document.getElementById("map_canvas"));
        map.setCenter(new GLatLng(37.4419, -122.1419), 13);
      }
  });', 'inline');

  echo '<div id="map_canvas" style="width:500px; height:300px;"></div>';
?>
5. Save the page and view it
6. You should see a Google Map

As you can see, in this code, we call drupal_add_js using the external flag, meaning it will load the Google Maps API externally without having to hold it on the local server.

@hass: drupal_build_js_cache() simply uses $contents .= _drupal_compress_js(file_get_contents($path). ';'); to read the file and AFAIK file_get_contents() also works for remote files.

No it doesn't. This is why Drupal implements its own HTTP client function with drupal_http_request and uses only this to get remote files. file_get_contents can be enabled for URLs, but most hosts shut this off for security, to ensure that you only access a URL when you really mean to.

I know the patch no longer supports preprocessing for remote files, but this is an important aspect of retrieving external data anywhere in core, so it bears repeating.

Or try this instead :

then in downloadscript.js:

That work-around is a horrible hack, and still results in the same issue at hand. Although it successfully allows us to use external scripts though drupal_add_js, it doesn't do it cleanly at all. We should be able to put the script URL right into the method call.

I think caching and preprocessing these external scripts should be handled in a seperate issue than what this patch is doing. The cache would download the referenced script to the files directory, and then reference the JS file in the files directory. This seems like a lot of work for one patch, so splitting it into two will help the situation. Could we get a review of this patch?

#39 works for me. The following test code was used in a PHP-evaluated node:

The jQuery forms extension is not in Drupal, so pre-patch (with the URL included as a relative path) the missing library will cause an error. Post-patch, clicking on the button will pop up the alert attached to it via ajaxForm.

Subscribing.

jQuery forms extension is not in Drupal

Whoops, correction: The jQuery forms extension is only loaded when a form requiring AHAH behavior is built. The test code above will not work if an AHAH form is present on the page - it will work without the patch just as it will with the patch.

So, has anyone reviewed this patch successfully? If so, can we get a RTBC?

I've tested it. Perhaps we can get a second review?

Updated to HEAD. Test still applies:

1. Fresh Drupal HEAD install
2. Apply the patch
3. Enable the PHP input filter module at admin/build/modules
4. Create a new page at node/add/page using the new PHP Input Filter with the following code:
   
   <?php
  // Add an external Javascript file
  drupal_add_js('http://maps.google.com/maps?file=api&amp;v=2&amp;key=GOOGLEKEYHERE', 'external');

  // Add the Javascript to test it
  drupal_add_js('$(document).ready(function() {
      if (GBrowserIsCompatible()) {
        var map = new GMap2(document.getElementById("map_canvas"));
        map.setCenter(new GLatLng(37.4419, -122.1419), 13);
      }
  });', 'inline');

  echo '<div id="map_canvas" style="width:500px; height:300px;"></div>';
?>
5. Save the page and view it
6. You should see a Google Map

The attached patch also caches the external Javascript files locally. Reviews would be great.

@Rob: caching of external files should go into an extra patch http://drupal.org/node/91250#comment-722222. See your own comment below this comment.

Then this patch should suffice for this issue...

obligatory sub comment

Similar thread can be found here: http://drupal.org/node/251578

Rob et al, it would be great if you could share your thoughts on #251578 at http://drupal.org/node/251578. It seems to fix this order, as well fix a problem with the order in which Javascript files are shown. We might conclude that this patch is a duplicate ... ?

Interesting discussion but everybody forget about collisions of variable names and security.

I ported the code over from the $type = 'external' code to just check if the path given is a valid absolute URL in the reinvention of drupal_add_js.