JavaScript Patch #3: JS Alter Operation

subscribe

#91250: JavaScript Patch #4: External Scripts

The external scripts part is at #91250: JavaScript Patch #4: External Scripts so we can remove it from the scope of this issue and concentrate on hook_js_alter.

Now that #315798: JavaScript Patch #2: Weight is in it's time to get this patch rolling. The patch should be fairly simple (fingers crossed). Deciding on the placement of the alter call will be more difficult.

Should it be in drupal_add_js or in drupal_get_js?

If we place the alter call in drupal_add_js:

• It will be called each time drupal_add_js is called with may be several times on a page.
• The returned js array from drupal_add_js will contain the alterations.

If we place the alter call in drupal_get_js:

• It will be called each time drupal_get_js is called. In core we would only see this called twice for the 2 regions (header and footer).
• The returned js array from drupal_add_js would not contain the alterations. Alteration would happen at presentation.

Thoughts?

subscriiiiibe. :)

Taking a shot at this

I don't know that I like the idea of making the key 'simpletest.js' rather than the full path. If we did this we would have to somehow get developers to follow a js file naming namespace. Although, it might be a good idea to enforce good namespacing with js file names. Imagine a case where 2 different modules called a file called script.js. How would you preform an alter on those 2 files differently?

We will run into an issue of modules and themes calling drupal_get_js that will need to be cleaned up. Imagine a module, for example OG which does this in drupal 6, that has a function og_preprocess_page call and in there calls drupal_add_js and drupal_get_js. In the order of the processing of these the drupal_get_js call isn't needed. If it's called before other hook_preprocess_page functions are called some drupal_add_js added files won't have the alter done on them.

Are there any use cases where we would run into a problem of calling the alter in drupal_get_js only once? I'm not saying the issues above are a stopper. We might want to include some usage examples to make sure contrib modules don't mess things up accidentally.

To put simply, we need some good documentation on this so developers don't misuse drupal_get_js.

Locale module is a perfect example of where we should be implementing this alter (rather than calling locale code directly from drupal_get_js). The attached patch adds this - it could use some more testing.

Here is what I saw from a quick pass of the patch

• Why, in locale_js_alter, is the if statement for type != 'setting' and 'inline' rather than = 'file'?
• function locale_js_alter is an implementation of hook_js_alter() not hook_javascript_alter(). The comment needs to be updated.
• In simpletest_js_alter we assume if simpletest.js is included that tableselect.js is included as well. Should we have that assumption or should we test for both simpletest.js and tableselect.js? If they are both present then set the simpletest.js weight to be just before tableselect.js.
• In the testAlter method where we test the altering script there should be a comment to refer to simpletest_js_alter for the expected altering. I stared at that for more than a few seconds before it struck me what was happening. If I didn't know about simpletest_js_alter the test wouldn't make sense.

Is the name drupal_get_js_defaults the correct name? The one place I found where a function returns defaults is _book_link_defaults (are there others?). On the other hand I don't see a drupal_get_* function returning defaults. See http://api.drupal.org/api/search/7/drupal_get_. Would a name like _js_defaults or _drupal_js_defaults be better?

This block doesn't read cleanly to me:

+ * Note that hook_js_alter(&$javascript) is called during this function call
+ * to allow alterations of the JavaScript during its presentation. Calls to
+ * drupal_add_js() from hook_js_alter() will reflect improperly on screen. The
+ * correct way to add JavaScript during hook_js_alter() is to add another
+ * element to the $javascript array, deriving from drupal_get_js_defaults().
+ * See locale_js_alter() for a demonstration of this.

Instead what if we use something like:

* Note that hook_js_alter(&$javascript) is called during this function call
* to allow alterations of the JavaScript during its presentation. Calls to
* drupal_add_js() from hook_js_alter() will not be add to the output 
* presentation. The correct way to add JavaScript during hook_js_alter()
* is to add another element to the $javascript array, deriving from 
* drupal_get_js_defaults(). See locale_js_alter() for an example.
* @see locale_js_alter()

I'll test the functionality tonight and make sure everything works as expected.

Looks good and passes test. Nice job Rob Loach and Grugnog2.

Submitting for re-testing after HEAD-breakage snafu this afternoon.

In the words of catch, this is a lovely, lovely patch filled with loveliness. Nice job using SimpleTest and Locale as the use cases for the hook you've introduced. And unit tests too! What's not to love? :)

Testing bot seems to be stuck, so I'm running the tests locally here overnight. Assuming that pans out okay, I'll give this another look tomorrow.

All in all, fantastic!

One head-scratcher though (even after reading #18 and #19):

+ * presentation. The correct way to add JavaScript during hook_js_alter()
+ * is to add another element to the $javascript array, deriving from
+ * _drupal_js_defaults(). See locale_js_alter() for an example of this.

It looks to me like _drupal_js_defaults() should not be "private", especially since we're recommending other modules call it directly (as locale.module does). It seems like the definition of a public API function if we're using it in 3rd party modules and outside of common.inc.

Excellent. :)

Committed to HEAD. Thanks! Docs please! :D