Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This issue is a delegate of #251578: More flexible js/css ordering and an alter operation to add an "external" type to drupal_add_js so that you can reference files that arn't hosted locally. If JavaScript performance is enabled, it will download the file and host it locally.
This will have to wait until at least #315797: JavaScript Patch #1: $options is in.
Comment | File | Size | Author |
---|---|---|---|
#27 | 315801.patch | 7.7 KB | RobLoach |
#25 | 315801.patch | 7.71 KB | RobLoach |
#20 | 315801.patch | 7.69 KB | RobLoach |
#19 | 315801.patch | 7.77 KB | RobLoach |
#17 | 315801.patch | 7.73 KB | RobLoach |
Comments
Comment #1
mfer CreditAttribution: mfer commentedsubscribe
Comment #2
hass CreditAttribution: hass commented#91250: JavaScript Patch #4: External Scripts
Comment #3
mfer CreditAttribution: mfer commentedThe 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.
Comment #4
RobLoachThanks, Matt!
Comment #5
RobLoach#315798: JavaScript Patch #2: Weight will REALLY help this.
Comment #6
mfer CreditAttribution: mfer commentedNow 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:
If we place the alter call in drupal_get_js:
Thoughts?
Comment #7
mfer CreditAttribution: mfer commentedComment #8
webchicksubscriiiiibe. :)
Comment #9
Owen Barton CreditAttribution: Owen Barton commentedTaking a shot at this
Comment #10
RobLoachIt doesn't make much sense to call alter twice when the page is displaying because you might want to move some JavaScript from the 'footer' scope to the 'header' scope, so essentially, you'd be moving that script up to the top of the page twice unnecessarily. I propose using a static variable to check if the alter has been called. I also believe that altering the JavaScript after all JavaScript is added is more important then altering individual elements, because what if one JavaScript file needs to somehow reference another one in the array?
This patch calls drupal_alter() at the first call to drupal_get_js(), as well as adds
simpletest_js_alter(&$javascript)
, to stick simpletest.js before tableselect.js.Any thoughts on removing the relative paths in the $javascript array so that instead of having $javascript['modules/simpletest/simpletest.js'], we have $javascript['simpletest.js']? This would make finding and modifying JavaScript files throughout the array much easier. All it would require is calling basename() when sticking the items in the array.
Comment #11
mfer CreditAttribution: mfer commentedI 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.
Comment #12
RobLoachAlso, some tests would be good.
Comment #13
RobLoachComment #14
Owen Barton CreditAttribution: Owen Barton commentedLocale 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.
Comment #15
RobLoachGreat work there! I'm blind to not notice the locale example.
I noticed a documentation error in my code........
.... Should be something like:
"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()."
Comment #16
mfer CreditAttribution: mfer commentedHere is what I saw from a quick pass of the patch
Comment #17
RobLoachComment #18
mfer CreditAttribution: mfer commentedIs 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:
Instead what if we use something like:
I'll test the functionality tonight and make sure everything works as expected.
Comment #19
RobLoachComment #20
RobLoachTaking a look at other uses of @see in Drupal core, it seems they're all on different lines, and use the parenthesis.
Comment #21
mfer CreditAttribution: mfer commentedLooks good and passes test. Nice job Rob Loach and Grugnog2.
Comment #23
webchickSubmitting for re-testing after HEAD-breakage snafu this afternoon.
Comment #24
webchickIn 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.
Comment #25
RobLoachWebchick and her eagle eye noticed a couple of @see's without the ()......
Comment #26
quicksketchAll in all, fantastic!
One head-scratcher though (even after reading #18 and #19):
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.
Comment #27
RobLoachComment #28
webchickExcellent. :)
Committed to HEAD. Thanks! Docs please! :D
Comment #29
RobLoachhttp://drupal.org/node/224333#hook_js_alter
http://drupal.org/cvs?commit=154664
If you guys are better writers then me, then please feel free to update appropriately........ On a somewhat unrelated matter, I'd really like #314870: UNSTABLE-4 blocker: Add api.php for every core module so that hook documentation updates wouldn't require an entirely different commit.