More flexible js/css ordering and an alter operation

This is something we've needed ever since drupal_add_js() was added. We're able to circumvent many JS problems now (like upgrading jQuery) in Drupal 6 by using the hook_preprocess to alter the $scripts variable, but it's not solving the problem at the source. This patch has a lot of good stuff in it:

- hook_js_alter() is effectively added so that a module like jquery_update could keep sites up to date through an entire Drupal release cycle.
- I always look-up the params too, so I have to say this would definitely help me out when developing.

As for the # symbol, I'd be tempted to take it out. The # symbol is used in the FAPI specifically because we need to tell form elements apart from form properties. Because we'll never have a nested structure when adding these parameters, I feel that the # is unnecessary (similar to the l(), hook_menu(), and hook_link()).

Also, dmitri is the same thing planned for drupal_add_css? It needs this just as badly.

Btw, dmitri... in answer to your question about where to put unit tests, it looks like system.test for now. We'll later move these to files like system.common.inc.test once http://drupal.org/node/251473 is resolved.

Removing function parameters is quite IDE unfriendly. It is fine that some developers don't use IDEs and "cannot remember the parameters" but I don't think we need to cater to this audience. The audience of IDE using devs is just as large. An IDE remembers function parameters for you but has no chance of understanding these arrays without special care. I don't feel so strongly about this, but I do think we are turning off at least as many people as we win with this sort of change.

I also don't think the hash in front of the keys is desireable here but i know you did it in order to reuse the sort function of fapi. We should really make a sort function that can deal with non hash- prefixed keys.

Also setting the title to be more descriptive and less perjorative.

Correct me if I'm wrong, but the changes being done to drupal_add_js() are analogous to the changes that were done in l() during the 6.x dev cycle which resulted in *vastly* easier-to-grasp and legible function calls with a lot less code. If in 90% of the cases you only need one or two of the extra params at most, and the params are named logical things that one could reasonably guess whether one is using an IDE or not, I think it's fine if we move to the array() format. IDEs will get the PHPDoc of the function in-line anyway as it auto-completes, and be able to see what possible parameter keys the function accepts, so the IDE argument doesn't sway me all that much.

Subscribing so I can review it later. In general I do not object to rare parameters turning into an array to avoid the "47 defaults to get to the one you want" problem.

The latest patch still seems to be breaking the JS on the configure screen of the installer due to the empty script tags before the inline JS. This causes the password checker and clean url test to be unavailable.

Also subscribing so I can review later. And I agree with webchick that code legibility should also be taken into account.

I did a quick look at the patch, and noticed two things:

1. some inline comments don't have a trailing period.
2. it seems you are moving some functions, e.g. drupal_to_js(), this makes the patch bigger and less readable.

I'll review this thoroughly later.

An initial review:

- I think the $reset parameter on drupal_add_js() should be it's own parameter, not in $options['reset']. This keeps it consistent with other functions that keep static variables.

- This line sets off bells for me: // The first step is to format it nicely for altering. If we need to change the format for altering, we should just store that as the main storage mechanism. Reformatting the array every time a JS file is added is wasteful.

- I don't think the $options['remove'] flag should exist. You call drupal_add_js() to remove a file? I think the drupal_alter() and the $reset flag should be sufficient. If we're set on having this remove flag, a second drupal_remove_js() function should be created to facilitate removing files.

- Like recidive reports, this patch is unnecessarily moving around functions. We should reroll to make it change less lines of code.

Subscribing... It would be great to have the ability to reference external Javascript files, and cache them locally if JS aggregation is enabled. http://drupal.org/files/issues/91250_1.patch is a first hack implementation of this found in this thread.

drupal_add_js('http://maps.google.com/maps?file=api&v=2&key=GOOGLEKEYHERE', array('#type' => 'external));

Would add the reference if caching is disabled, and would cache the files locally if caching was enabled.

... I think this patch needs an update to HEAD, it's hitting some conflict fuzz.

I think there's some overlap with #173941: drupal_add_js() array should be modifiable. That one should probably be marked as a dupe of this.

As this weighting system is something i requested at pre D5 times and it was refused and i was told nobody need this - i subscribe here and hope this could get in now... don't forget weighting for CSS, please.

Themes have the ability to have scripts[] = 'myscript.js' attributes (scripts[]). Why not move this into the module scope as well, so module can always invoke the inclusion of Javascript files from their info file?

The same kind of thing could be done to drupal_add_css: Revamp drupal_add_css()

This patch still needs a large update to HEAD. Lots and lots of fuzz going on.

sub

I know this isn't my issue, but I took it upon myself to reroll this for HEAD so some more people can test this since it has been a while since any work was done on it. I used the patch in #16 as a start. I haven't made any changes except for fixing #17 and removed the unnecessary moving of existing functions. Also, a few drupal_add_js() calls weren't updated in the newest patch (locale.module, system.admin.inc), so I changed these to comply with the new function.

Also, line 2360 in common.inc in the patch used '#preprocess' as the index into $index while HEAD uses just 'preprocess'. I figured this was incorrect, so I changed it back to 'preprocess'.

Leaving as CNW as per #21.

Thanks a lot for the help, Bryant. I built upon Bryant's patch at #27 to add the ability to reference, as well as cache, external Javascript files. When adding a Javascript file, if you put in an absolute URL (e.g. http://maps.google.com/maps?file=api), it will reference the file remotely. If Javascript caching is enabled, it will download the contents of the file, and stick it into the aggregated Javascript file.

To test, create a new node, and stick the following code into the node, with the PHP input filter enabled:

  // Add an external Javascript file
  drupal_add_js('http://maps.google.com/maps?file=api');

  // 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>';

Try it out with JS aggregation both disabled, and enabled. The patch still seems to have problems when aggregation is enabled. Not quite sure what's wrong with it. We still also have to address the issues brought up in #21.

Unsubscribe.

Alright, so I think we've finally got something here. Everything seems to work as well as I can see, so no problems with aggregation or caching anymore. I have also addressed all issues in the way that dmitrig01 said he would in #21.

A few things to note, locale.module now uses hook_js_alter() as it should instead of being custom called by drupal_get_js(). This also gives us something to use to test this patch. Also, the array keys for the different types of files are now "file", "inline", and "setting" which are all singular. Using 'files' internally to store things of type 'file' was requiring some unnecessary code to do the conversion. This new way is also more consistent.

Regarding the second point from #20, the array used in altering is different from what's stored internally, but that conversion needs to happen anyway before being passed to theme_scripts(), and it is only done once anyways. I just left that as is.

Alright, attaching a file really is broken. Seems to be affecting a good number of posts, too.

I replaced some tab characters that you had in there, as well as cleaned up some documentation and code. Tested with the Google Maps example with aggregation both on and off and it worked very nicely. This is looking great!

Would it be possible for us to go one step further and compress the Javascript when it's aggregated? Like, remove unnecessary line breaks, remove comments, whitespace, etc. We already do this to CSS, so why not do it to Javascript as well? Take a look at Javascript Compressor for a demonstration.

I would think we should leave out compression for a follow up patch, especially if it requires external libraries. It shouldn't be too hard to add that in after this patch's committed. Compression can also break code easily if it doesn't strictly follow the semicolon requirements of JS.

Compression was tried for D6 and was left out because it was an order of magnitude harder than CSS compression to get working without breaking a lot of scripts. That's a separate issue unto itself and should not be handled here.

I closed out #173941: drupal_add_js() array should be modifiable as Crell recommended. I want to say that this is looking fantastic! It's everything I'd want adding/removing JS to be. We'll need to port this to CSS when we're done.

Some comments:
- drupal_remove_js() doesn't need an $options flag, though inside of it we can pass $options['remove'] = TRUE to drupal_add_js().
- Maybe we could make a few constants to clarify the -20, 0, and 20 values? Like JS_MODULE_WEIGHT, JS_CORE_WEIGHT, and JS_THEME_WEIGHT (I think leaving jQuery and drupal.js at hard-coded values is fine however). That in theory should increase the consistency of adding JS files, making things more predictable when you need to specially place a JS file.

@Susurrus Genius use of hook_js_alter() for use in locale.module!

This is really great code.

Compression should be a separate patch, we went through an issue over 100 comments long on compression breaking JS code.

Okay, I changed drupal_remove_js() so it now takes $data and $type, though $type defaults to 'file'. While removing JS is only practical for scripts of type 'file', I see no reason to explicitly limit it in the code (since the data for 'setting' and 'inline' is long and would have to be copied exactly, it'd be easier to use hook_js_alter() if possible).

I also changed the weights of drupal.js and jQuery.js to -100 and -99 respectively. They just don't need to be so large because that added distance really isn't a safety net, so why have it? The constants recommended by quicksketch in #35 have been added to common.inc. I liked the idea of constants because then you can just use JS_CORE_WEIGHT - 1 to load your script before all the core scripts.

Rob Loach: When you create patches in Eclipse, can you specify the Patch Root as being Project instead of Workspace? I have to manually edit all of yours before I can apply them because our projects are named differently.

A little more documentation cleanup was done also.

quicksketch: We'll need to port this to CSS when we're done.

Definitely, already made the issues! #266358: Revamp drupal_add_css().

quicksketch: drupal_remove_js() doesn't need an $options flag, though inside of it we can pass $options['remove'] = TRUE to drupal_add_js().

function drupal_remove_js($data) {
  return drupal_add_js($data, 'remove');
}

Hrmmmm, any thoughts?... Susurrus's new solution looks pretty good.

quicksketch: Maybe we could make a few constants to clarify the -20, 0, and 20 values? Like JS_MODULE_WEIGHT, JS_CORE_WEIGHT, and JS_THEME_WEIGHT (I think leaving jQuery and drupal.js at hard-coded values is fine however). That in theory should increase the consistency of adding JS files, making things more predictable when you need to specially place a JS file.

Great idea. For jQuery, if we had JS_ENGINE_WEIGHT, or something, it might be a good place to add other Javascript engines through third party modules if desired (e.g. Scriptaculous).

quicksketch: Compression should be a separate patch, we went through an issue over 100 comments long on compression breaking JS code.

Good one, Nate. I just re-open it as #119441: Compress JS aggregation.

Susurrus: Rob Loach: When you create patches in Eclipse, can you specify the Patch Root as being Project instead of Workspace? I have to manually edit all of yours before I can apply them because our projects are named differently.

Sure thing! Thanks for the note, I wouldn't have realized.

Okay, here's an updated patch. The JS tests weren't updated for all the changes that have happened lately. I think the tests do a good job of covering everything, I just hope they're written correctly and don't make assumptions that render the tests useless. It'd be great if someone else could look these over.

Also, I changed all the weights in core to use these new constants I added, which didn't happen in the last patch. There's no real reason to have JS_MODULE_WEIGHT, since that's what everything defaults to, but it's there anyways.

I've added JS_ENGINE_WEIGHT since there's really no reason not to right now, though I wonder how useful it actually is. I've named it JS_LIBRARY_WEIGHT as I think that makes more sense than ENGINE and suggests that various JS libraries can go there, not just huge libraries like scriptaculous, jQuery, etc.

Only a small suggestion... i would name drupal_remove_js() as drupal_del_js().

Just realized that if the request is made to aggregate an external JavaScript source fails, it outputs a PHP error. The request should then become something like:

          // Request the external Javascript resource.
          $request = drupal_http_request($path);
          if (isset($request->data)) {
            $contents .= $request->data . ';';
          }
          else {
            watchdog('javascript', 'Failed to aggregate JavaScript file from %js.', array('%js' => $path), WATCHDOG_ERROR);
          }

We could also somehow add the failed request to the < script > tag in the user's browser, but I'm not too sure how to modify that once we're already building the cache. Thoughts?

I'll try to look at this a little later. Hadn't even thought of an external JS request. That should also be added to the tests as requesting a random filename from drupal.org would work file I would think (I would say example.com, but Drupal.org would be more responsible).

Also, re #39 I'm very much against drupal_del_js as there's no need for obscure names for this function and it's not really deleting any JavaScript. Remove is a much better antonym to add in this case.

I've added caching for external files even when aggregation is turned off. This caching is done after hook_js_alter() so that other modules can still tie in to a script using it's original file path. After that, however, the script is cached locally.There is currently no way for that cache to be reset unless the JS cache is explicitly reset, though I think that's fine as that's the normal way it's handled.

I also added some tests for adding external files. They cover available and unavailable scripts and with caching enabled and disabled. Currently they aren't correct, though the first two pass. Javascript seems to be dealt with funny with SimpleTest, so I'm not sure of how to fix this right now, plus I'm tired. Maybe someone else can take a look at it?

Various thoughts from reading through the patch:

1) Why do we need weighting exactly? With the exception of jquery.js and drupal.js, everything else *should* be independent of each other, even if it "comes from core". Just like there is no "core" CSS anymore, there shouldn't really be "core" JS either, save for those two base libraries.

2) This is unclear:

+ *   - attributes
+ *       An array of attributes to be added to the script.  Run through 
+ *       drupal_attributes().

That makes it sound like I should run attributes through drupal_attributes(). "Gets run through..." would be better.

3) I don't get the remove directive. What's wrong with just unset()ing in the alter hook?

4) The docblock for drupal_to_js() has a multi-line summary. This is a bug. Please file a patch! :-)

5)

1) Weighting is because JavaScript objects and variables can be overridden. If you want to override a variable or not be overridden, you can just change the weight of your file versus needed to remove the offending script. There's various discussion about this in the several issues created about this same situation that are mentioned in this thread so I won't repeat them.

2) Will fix.

3) You can't always implement a hook; like for themes or theme functions or even if you wanted to do it on a single page. Many uses.

4) This patch supposed to be separate from this one?

Another version is forthcoming. I've been working a lot on the caching and aggregating parts as well as SimpleTests for these cases and I'm almost there.

I marked #217892: Extend drupal_add_js to allow for external scripts as a duplicate.

Susurrus: I also added some tests for adding external files.

They look great! Thanks again, Susurrus...

Crell: 4) The docblock for drupal_to_js() has a multi-line summary. This is a bug. Please file a patch! :-)

This should do:

/**
 * Converts a PHP variable into its Javascript equivalent.
 *
 * We use HTML-safe strings, i.e. with <, > and & escaped.
 * @param $var
 *   The variable to encode.
 * @return
 *   A string, which is a JavaScript-encoded version of $var.
 * @see drupal_json
*/

Here's an updated patch rolling in the changes in #45 as well as using the new common.test file for common.inc tests.

I have not yet finished the SimpleTests for the caching/aggregating parts as there are a few cases that I haven't tested yet. There needs to be various permutations of the caching, preprocessing, and external script availability in order to test all edge cases. I hope to finish writing up those tests today and get them working. Until then, if anyone else wants to hack away, here's the patch.

So, there's just two more tests to finish up that I didn't have the willpower to finish tonight. One makes sure scopes are respected and the other should test some edge cases in odd combinations of caching and aggregation settings.

The only issue I could see is if a file is unavailable and then it suddenly becomes available. I'm not sure how the system would react to that. I think one last test should try to attach a file that doesn't exist and then create the JS file and see if the cache is updated correctly. That wouldn't be too hard.

Anyways, all written tests pass so it would be great to get some reviews of both the code and the tests.

Undefined index: cache in includes/common.inc on line 2416.

Also not able to run tests because the groups are permanently collapsed.

Alright, here's a new patch. Fixes all of the issues from #48 and I've tested on various Drupal pages using JS and all of the tests pass as well. I haven't added any of the tests discussed in #47, but I will later today.

I've added all the tests discussed in #47, though two tests still aren't passing. That's for another day.

All tests now pass. So some things weren't updated properly in locale_alter_js(), which needed to be fixed. Settings weren't being handled properly, but that's now fixed. You can't replace JS settings now by passing a similar array with different values, you need to use drupal_remove_js() to do it.

There are no known bugs with this now, so it'd be great for people to play around with it.

Is there anything missing from this, other then a good code review?

Susurrus at #51: You can't replace JS settings now by passing a similar array with different values, you need to use drupal_remove_js() to do it.

Is this the design we want? It seems to me that if you run:

drupal_add_js(array('mysetting' => 50), 'setting');

... It should set the JavaScript value of mysetting to 50, not worrying about what it was before. Thoughts?

From code style view this needs work. Looks like the patch contains many "tabs", but drupal code style requires "two spaces".

So I've fixed the tabs and some other things locally, but there's still the issue of how to deal with replacing versus appending for settings.

An example of where appending is needed is in drupal_add_tabledrag(). This function is called multiple times and ends up needs to append values to an existing array for it to work. I'm not sure if this needs to be retooled, but it doesn't work if replacement is how adding settings work.

What I think needs to happen is that replacement is the default setting and then there is another options setting of 'append' which can be TRUE or FALSE. This would be used when values need to be appended and would only be applicable for settings.

Does this sound like how it should be done? I think there are many cases where both replacement and appending could be used so there should be the option for both. While this could be handled in the user's own code, I think it should be included in core, especially since appending is used in core. Replacement is not, but would be how contrib would alter JS settings easily.

So here it is using the appending method that drupal_add_js() has always used. hook_js_alter() should really be how people alter the JS available for the page.

I've fixed the tabs from #54 along with adding in some constants where they were needed.

I expect a double semicolon (";;") by this part of the code (untested). If we have a rule that every JS code need to be finished cleanly we don't need this.

+      if ($tmp) {
+        // Append a ';' after each JS file to prevent them from running
+        // together.
+        $contents .= $tmp . ';';
+      }

I still think the ';' there is a good idea. It adds little overhead and people "should" add semicolons at the end of the files, but that probably won't always happen. I don't think that needs to be removed.

No, this will make code invalid. Everyone developing JS code should be aware to develop code that is compliant and finish correctly. Remove this part, please.

In the interests of user friendliness, we should actually warn the coders that they've included invalid JS code rather than simply trying to think for them and close their semicolons or leave it alone and possibly broken. There's nothing quite as disappointing as including a downloaded file from a third -party javascript website only to have it break inexplicably without a known cause.

"Was it Drupal, or was it my downloaded file? Wait, it's gotta be Drupal's problem because this code runs just fine on my HTML-only testing site."

Let's not leave the users hangin'. Warn them that it failed, and why it failed.

@hass: Double semicolons are semantically correct. The double semicolon denotes a nop in between them. That's not a big deal.

@Senpai: Are you saying that if the last character isn't a semicolon, that the user should be warned about the last of a semicolon? I'm not sure what "failing" means specifically in your context.

It is good practice and jQuery developers tell you "everywhere" that you should finish everything with semicolons for the compressors. We have had the same discussion some time ago #183940: JS aggregation breaks valid Javascript when the D6 JS compression patch have been reverted back :-(. It's not the problem of Drupal if developers don't follow the dev rules if they are written in the upgrade handbooks. Please don't try to fix others carelessness and ignorance.

I'm not fixing anything, this code existed before this patch. I see absolutely no reason to remove it besides "we should". It might break things or other people exactly like Senpai said. Haas, why should this bemremoved if it might make things worse for contrib JS, especially when the overhead is so small? Reasons you've provided aren't very convincing.

If this code should support lazy developers who cannot read documentation I would at minimum add a regex that check of the closing letter is a semicolon. If not, add one semicolon otherwise don't add anything.

Nevertheless I'm sure this part of the code will never get committed as I expect the core developers will not support lazy developers. Not my decision, but my 2 cents. :-)

Regardless of how clean Drupal and jQuery developers make their own JS files, we're powerless to stop how unclean the vast majority of JS is out there that end-users are going to want to put on their websites and use with this operation. Drupal therefore has to cater to the worst possible code, or else it will be blamed for causing bugs that crop up from crappy JS code being included in pages.

Susurrus said:

Are you saying that if the last character isn't a semicolon, that the user should be warned about the last of a semicolon? I'm not sure what "failing" means specifically in your context.

What Webchick said in #65 above is exactly the reason I'm advising that Drupal, as the web application, be smart enough to warn the users when they've included crappy code into the system. If the code is pure, and properly closed with a final semicolon, then Drupal doesn't have to do anything. If the included JS code is not properly formatted, then Drupal has to warn the user that their code is not valid and might break something, and not include the crappy code.

In this manner, Drupal will remain smart, error-free, and refuse to cater to bad programmers, thus forcing them to get smarter without doing it for them.

I think that this should he left to a follow up issue as there is already enough code change in this patch. This approach is not new in this patch and I see no reason for it to change in this issue, as that is really feature creep by reading this issue's title.

I believe that the double semi-colan phenomenon was the only thing holding this issue back from a code review. It is true that this patch is changing a lot of stuff having to do with JavaScript, and it would be great to delegate some of the work to other issues. Hass, would you mind creating the bonus issue of removing the additional semi-colan, or warning the user they're experiencing bad-JavaScript-itus?

At #53, I asked whether or not setting an already existing JavaScript variable must be removed before being replaced.

drupal_add_js(array('mysetting' => 50), 'setting');

If we want to set mysetting to 50, we'd have to remove the value, and then re-add it. If we just use what I provided above, the value is added to an array. Adding values to an array is a must, however, like Susurrus mentioned at #55. What if we added a new operation that overwrote the JavaScript setting?

drupal_add_js(array('mysetting' => 50), 'replace setting');

Not sure of the naming, but you get the idea....

@Rob Loach: As of right now I think this patch should stay as feature rich as it is and we should get it in. hook_js_alter() already allows for altering the setting array. By calling drupal_get_js() with no options, you can also get the entire array. You can pass it back to drupal_get_js() by using the RESET parameter and whatever array structure is passed to it, that becomes the current array. So this is definitely possible now, just not the most convenient if you don't use the hook.

Cool! Guess we just need a good review, as well as the creation of the bonus issues.

It'd be great to get someone to review this. Seems like the tests don't want to work on my machine because of drupal_http_requests() to localhost returning -10060 and I'm not really sure what that's about, probably something with my setup.

However this did need a reroll and a small bugfix.

Subscribing (I'll review as soon as I get a chance)

The patch needs a re-roll. I'm hitting a huge amount of conflicts when applying. CVS really sucks at merging, do you guys think we could move this to a GIT repository or something kept up against head with this patch in a branch? Thoughts?

Subscribing, although I'm not at all a fan of these ginormous patches that "fix" everything all at once. :\ Makes them really hard to review. :(

Speaking of "hard to review", I just noticed that Dmitri also posted #237566: Automated JavaScript unit testing framework....

Failed attempt at a re-roll........ I checked out the repository from when the patch was uploaded, applied the patch, and then did a CVS update trying to go around conflicts that came up....

Honestly, I think it'd be best if we can split this up into multiple smaller patches: http://webchick.net/please-stop-eating-baby-kittens. That'd make it much easier to review, and we can commit them much more quickly. It sounds like this is at least 3 patches: a JS weighting system, a JS alter system, and a general re-work of drupal_add_js().

If all of these have to be grouped together into a monster patch, then there better be a really good reason. :P

Amen!

Some fixes:

Index: update.php
===================================================================
RCS file: /cvs/drupal/drupal/update.php,v
retrieving revision 1.258
diff -u -r1.258 update.php
--- update.php	9 Sep 2008 13:33:19 -0000	1.258
+++ update.php	18 Sep 2008 00:35:26 -0000
@@ -284,7 +284,11 @@
 
 function update_batch() {
   global $base_url;
-
+  $site_offline = variable_get('site_offline', FALSE);
+  if ($site_offline == FALSE) {
+    variable_set('site_offline', TRUE);
+  }
+  
   $operations = array();
   // Set the installed version so updates start at the correct place.
   foreach ($_POST['start'] as $module => $version) {
@@ -308,6 +312,9 @@
   );
   batch_set($batch);
   batch_process($base_url . '/update.php?op=results', $base_url . '/update.php');
+  if ($site_offline == FALSE) {
+    variable_set('site_offline', FALSE);
+  }
 }

The second if ($site_offline... should be:

if ($site_offline == TRUE) {
  variable_set('site_offline', FALSE);
}

Also should it be === and not ==?

Whoops, mixed up two different patches. Either way, we'll have to split up this patch as webchick mentioned.

Does anyone have a suggest path forward for breaking this patch up into smaller patches?

folks should look at #315100: Allow to add JS/CSS libraries (sets of files, settings, and dependent libraries) as well, as the work there will be impacted/have an effect from here as well.

I think splitting it up into three different issues makes sense:

• #315797: JavaScript Patch #1: $options
• #315798: JavaScript Patch #2: Weight
• #315801: JavaScript Patch #3: JS Alter Operation
• #91250: JavaScript Patch #4: External Scripts

@Rob Loach - after reading through the patch I was thinking the same split. Looks good.

@Rob: remember we have had a extra case for "External Scripts"... #91250: JavaScript Patch #4: External Scripts

YAY!! Thanks, guys! I'll be eagerly monitoring the RTBC queue for these. :)

#315797: JavaScript Patch #1: $options has made some progress. Add #210447: Introduce Javascript compression to the list of awesome drupal_add_js JavaScript enhancements.

#315797: JavaScript Patch #1: $options needs to be reviewed. Anyone have a few minutes to look it over? I think it's RTBC but we need a review from someone who didn't work on it. This is the first step in getting this issue completed.

I think that we have two distinct APIs - JS and CSS - that are very similar, and that we are allowing to significantly diverge, both in terms of interface and internal implementation. These should be sharing *more* code, not less, in my opinion.

Anyway, I did a several hours of work on this while I was traveling (#315797: JavaScript Patch #1: $options got committed in the meantime) that I feel sets a direction for this. Please take a look at the direction of this patch, that (I hope) people can agree offers massively simplified and unified code for both CSS and JS.

The main changes are that the JS and CSS is passed around in a consistent fashion, that the production of output as well as locale, and other tweaks are separated out and that we avoid building arrays structured in a format specific hierarchy. Please note that it is not complete (still need to update the aggregation code to accept the new format), and I am *not* necessarily suggesting we commit in one big lump unless people are totally inspired. However, I think that perhaps we can use this as a target and refactor the APIs and other code in this direction, such that once the interfaces are improved we can push the underlying code in this direction.

Fixed patch paths

Here is an patch updated to the latest HEAD. I have updated some tests too (I added the tests from 315798 for good measure) - there are still a couple of failures to debug.

The more I look at this the more committable I think this is - we have already done the major API changes (they just need minor tweaks in this patch), and the rest of the patch is not really that big compared to many other patches. Overall it actually removes more code than it adds, which is a good sign, considering that weights, altering and theming features for both JS and CSS are added :)

I guess we should fix the issue title here...

This is quite the monster patch. Per webchick, we need to break things up into smaller patches. See #78 or http://webchick.net/please-stop-eating-baby-kittens. That's why we targeted the route in #84.

Before we decide to go down this path as opposed to what's outlined in #84 I'd prefer to get input from Dries or Webchick.

@Grugnog2 - can you, please, break out the API change in your patch into a separate issue that doesn't try to take on everything at once? One that just handles the API change and not everything in one patch.

If you can link to that issue here we can manage the change is a smaller and easier to grep fashion.

This issue was broken out into a number of individual issues and they have all been completed and committed to core. Additional issues can be opened as nice new d.o issues.

On the JavaScript front, we're quite good, on the CSS front, however, we have:

• #259368: Allow Inline CSS in drupal_add_css
• #356963: Allow CSS to be altered
• #92877: Add numeric weight to drupal_add_css

For CSS we should not forget #228818: IE: Stylesheets ignored after 31 link/style tags.