A patch straight from #1542344: Use AMD for JS architecture that declare all JS files as library and add the relevant dependencies to all scripts.

SebCorbin should be credited, he's the one who wrote everything, I'm just putting it in a patch.

This will greatly simplify issues like #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page as well as prepare core for a module-oriented JS.

Basically, we shouldn't be using drupal_add_js anymore, all scripts should be declared in library_info since pretty much all of them have dependencies on drupal and jQuery. As D8 is focused on mobile, it's critical to be able to add JS that is not implicitly adding jQuery and drupal.js to the page.

The naming isn't totally consistent, feedback welcome. There is still one drupal_add_js left in the color module. not sure how best to fix it yet.

Files: 
CommentFileSizeAuthor
#113 core-js-explicit-dependencies-1737148-113.patch74.52 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 40,337 pass(es).
[ View ]
#111 core-js-explicit-dependencies-1737148-111.patch74.25 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 18,297 pass(es), 447 fail(s), and 403 exception(s).
[ View ]
#107 core-js-explicit-dependencies-1737148-107.patch74.26 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 20,387 pass(es), 497 fail(s), and 882 exception(s).
[ View ]
#107 interdiff.txt3.55 KBnod_
#93 js-deps-1737148-86.patch74.54 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 40,703 pass(es).
[ View ]
#91 core-js-explicit-dependencies-1737148-91.patch73.88 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,700 pass(es), 3 fail(s), and 1 exception(s).
[ View ]
#86 js-deps-1737148-86.patch74.54 KBSebCorbin
PASSED: [[SimpleTest]]: [MySQL] 40,552 pass(es).
[ View ]
#86 interdiff.txt596 bytesSebCorbin
#85 js-deps-1737148-85.patch77.34 KBSebCorbin
PASSED: [[SimpleTest]]: [MySQL] 40,552 pass(es).
[ View ]
#85 interdiff.txt645 bytesSebCorbin
#83 js-deps-1737148-82.patch77.47 KBSebCorbin
PASSED: [[SimpleTest]]: [MySQL] 40,552 pass(es).
[ View ]
#78 js-deps-1737148-78.patch77.59 KBSebCorbin
PASSED: [[SimpleTest]]: [MySQL] 40,571 pass(es).
[ View ]
#78 interdiff.txt966 bytesSebCorbin
#76 js-deps-1737148-76.patch77.49 KBSebCorbin
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#76 interdiff.txt1.95 KBSebCorbin
#63 js-deps-1737148-63.patch75.52 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 40,563 pass(es).
[ View ]
#63 js-deps-1737148-63.interdiff.txt2.65 KBlarowlan
#61 js-deps-1737148-61.patch75.92 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 40,365 pass(es).
[ View ]
#61 interdiff.txt3.15 KBAlbert Volkman
#58 js-deps-1737148-58.patch75.85 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 40,358 pass(es).
[ View ]
#58 interdiff.txt3.36 KBAlbert Volkman
#55 js-deps-1737148.55.patch76.23 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 40,369 pass(es).
[ View ]
#55 js-deps-1737148.55.interdiff.txt939 byteslarowlan
#43 core-js-explicit-dependencies-1737148-41.patch74.89 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,352 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#36 core-js-explicit-dependencies-1737148-36.patch74.81 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,233 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
#32 js-deps-1737148.32.patch75.77 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 40,245 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#32 js-deps-1737148.interdiff.txt6.4 KBlarowlan
#28 core-js-explicit-dependencies-1737148-28.patch74.35 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,240 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#27 interdiff.txt19.42 KBnod_
#25 core-js-explicit-dependencies-1737148-25.patch74.08 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,114 pass(es), 6 fail(s), and 763 exception(s).
[ View ]
#23 drupal-1737148-23.patch73.02 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 40,232 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
#23 interdiff.txt2.9 KBtim.plunkett
#18 core-js-explicit-dependencies-1737148-18.patch73.03 KBnick_schuch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-explicit-dependencies-1737148-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#16 core-js-explicit-dependencies-1737148-16.patch73.01 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,236 pass(es), 12 fail(s), and 0 exception(s).
[ View ]
#14 core-js-explicit-dependencies-1737148-14.patch58.42 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,215 pass(es), 31 fail(s), and 4 exception(s).
[ View ]
#12 core-js-explicit-dependencies-1737148-12.patch58.3 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#12 core-js-explicit-dependencies-1737148-12-no-locale.patch57.63 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#9 core-js-explicit-dependencies-1737148-9.patch52.62 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 core-js-explicit-dependencies-1737148-4.patch52.67 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#3 core-js-explicit-dependencies-1737148-2.patch52.68 KBnod_
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#2 drupal-1737148-2.patch45.41 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#2 interdiff.txt822 bytestim.plunkett
core-js-explicit-dependencies.patch45.42 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 40,079 pass(es), 2 fail(s), and 7 exception(s).
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new822 bytes
new45.41 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Part of locale_library_info_alter() was off.

StatusFileSize
new52.68 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

All right, awesome new patch that actually fix #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page.

There are a few things related to ajaxPageState moved around, this should be using #1573020: Use data-* for CSS and JS files list to begin with so that we don't even need to do that in PHP.

StatusFileSize
new52.67 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Thanks, rerolled with changes from #2

Priority:Normal» Major

Bumping since it takes care of a nasty bug.

Priority:Major» Normal

I *think* my review over at #1542344-44: Use AMD for JS architecture has already been incorporated?

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-4.patch, failed testing.

Priority:Normal» Major
Status:Needs work» Needs review

Putting back to major and NR since testbot is going crazy.

Yes the review has been addressed by SebCorbin before I made the patch, check the AMD sandbox log :p

StatusFileSize
new52.62 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-9.patch, failed testing.

Not sure why its failing exactly, but a note for the next reroll:

+++ b/core/modules/locale/locale.moduleundefined
@@ -879,3 +878,33 @@ function _locale_rebuild_js($langcode = NULL) {
}
+
+
+/**
+ * Implements hook_library_info().
+ */
+function locale_library_info() {

This should come before locale_library_info_alter(). Also, adds an extra blank line by mistake.

Status:Needs work» Needs review
StatusFileSize
new57.63 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new58.3 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Added more dependencies.

On patch without the local changes to see if that's the only thing crashing the tests.

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-12-no-locale.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new58.42 KB
FAILED: [[SimpleTest]]: [MySQL] 40,215 pass(es), 31 fail(s), and 4 exception(s).
[ View ]

All right, should just have the tests to fix.

SebCorbin found out that variable_get was called during install, before DB was set, Added a check for the MAINTENANCE_MODE . Apologies for blaming testbot :)

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-14.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new73.01 KB
FAILED: [[SimpleTest]]: [MySQL] 40,236 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

Less test failures.

Status:Needs review» Needs work

Quick comments review. Quite a lot of comments to explain tricky edge cases, great! :)

+++ b/core/includes/common.incundefined
@@ -3946,23 +3900,50 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+  // Don't add those settings if there is no other JS on the page.

Remove "those", no?

+++ b/core/includes/common.incundefined
@@ -3946,23 +3900,50 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // Sort the JavaScript so that it appears in the correct order.

Sort the JavaScript files so that they appear in the correct order.

+++ b/core/includes/common.incundefined
@@ -3946,23 +3900,50 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // later Ajax request can be rendered using the same theme.

s/Ajax/AJAX/

+++ b/core/includes/common.incundefined
@@ -3946,23 +3900,50 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // Skip if no files were added to the page or jQuery.extend() will overwrite

s/or/otherwise/

+++ b/core/includes/common.incundefined
@@ -3946,23 +3900,50 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // that drupal_get_js() is running, so add the setting to this output as well

s/setting/settings/

+++ b/core/modules/color/color.moduleundefined
@@ -246,6 +244,7 @@ function theme_color_scheme_form($variables) {
+  // TODO transform to add library

s/TODO/@todo/

Status:Needs work» Needs review
StatusFileSize
new73.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-js-explicit-dependencies-1737148-18.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Updated comments as per #17.

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-18.patch, failed testing.

You don't include settings if they are no other JS on the page, but what about arbitrary inline JS or potential external JS being included?

well, it just works™

Everything is managed by libraries so to get settings, you need to drupal_add_library the drupal library, otherwise, no drupal.js and no Drupal.settings. The rest works. But yeah we still have to keep drupal_add_js around for now. That said you can add this type of JS in a library hook too.

All right, thanks.

Status:Needs work» Needs review
StatusFileSize
new2.9 KB
new73.02 KB
FAILED: [[SimpleTest]]: [MySQL] 40,232 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

It seems the last patch was made by editing the previous patch directly, and the grammar fixes were made to both the deletions and insertions. Which doesn't work.

Straight reroll with interdiff against #16.

Status:Needs review» Needs work

The last submitted patch, drupal-1737148-23.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new74.08 KB
FAILED: [[SimpleTest]]: [MySQL] 40,114 pass(es), 6 fail(s), and 763 exception(s).
[ View ]

Green :D

All right changed a couple of things:

- drupal.settings needs to be added as a dependency explicitly as well, that's the same mess otherwise.
- there is a js_alter hook in system to add the default configuration for the JS setting data, it's messy to try to do that in drupal_add_js

This fixes #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page the right way.

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-25.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new19.42 KB

Here you go

StatusFileSize
new74.35 KB
FAILED: [[SimpleTest]]: [MySQL] 40,240 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Should get rid of the warnings at least, added an isset() in locale_library_info()

I moved things around too much, the ajax thing is broken.

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-28.patch, failed testing.

+++ b/core/includes/common.incundefined
@@ -3946,23 +3888,52 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // Don't add settings if there is no other JS on the page.

s/JS/JavaScript/

+++ b/core/includes/common.incundefined
@@ -3946,23 +3888,52 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    if (!empty($items['settings'])) {

The comment says "if there is no other JavaScript". But here it's checking for other JavaScript *settings* specifically.
I think the comment is right and the code is wrong. The original code also just checked for $items not being empty, not $items['settings'].

However, if I look at drupal.ajax, which is where ajaxPageState would be used (primarily), then I see that it explicitly lists a dependency on drupal.settings. That means this check should also work.

I'd love a clarification here :)

+++ b/core/includes/common.incundefined
@@ -3946,23 +3888,52 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+      // files removed below are still considered "used" and prevented from being

"removed below"? Not within this function; so it's better to refer to the specific function in which this is happening.

+++ b/core/includes/common.incundefined
@@ -3946,23 +3888,52 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+      // Skip if no files were added to the page otherwise jQuery.extend() will overwrite

Wrap at col 80.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.phpundefined
@@ -68,13 +68,7 @@ class JavaScriptTest extends WebTestBase {
-    $this->assertTrue(array_key_exists('core/misc/jquery.js', $javascript), t('jQuery is added when a file is added.'));
-    $this->assertTrue(array_key_exists('core/misc/drupal.js', $javascript), t('Drupal.js is added when file is added.'));

Maybe we should assertFalse ()these?

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.phpundefined
@@ -139,6 +136,7 @@ class JavaScriptTest extends WebTestBase {
+    drupal_add_library('system', 'drupal.settings');
     // Only the second of these two entries should appear in Drupal.settings.
     drupal_add_js(array('commonTest' => 'commonTestShouldNotAppear'), 'setting');
     drupal_add_js(array('commonTest' => 'commonTestShouldAppear'), 'setting');

So drupal.settings gets added because we're adding settings that would otherwise not appear.

Can't we automatically add drupal_add_library() whenever drupal_add_js(array(), 'setting') is called?

Or do we intentionally not want to do this to enforce "good behavior"?

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.phpundefined
@@ -496,14 +507,14 @@ class JavaScriptTest extends WebTestBase {
+    // Note: This test installs tracker module.

s/tracker/language/? :)

+++ b/core/modules/system/system.moduleundefined
@@ -1212,6 +1212,51 @@ function _system_batch_theme() {
+  // Drupal specific javascript.

Drupal-specific JavaScript.

+++ b/core/modules/system/system.moduleundefined
@@ -1212,6 +1212,51 @@ function _system_batch_theme() {
+  $libraries['drupal.settings'] = array(
+    'title' => 'Drupal Settings',
+    'version' => VERSION,
+    'js' => array(
+      array(
+        'data' => array(
+          'basePath' => base_path(),
+          'scriptPath' => $scriptPath,
+          'pathPrefix' => $pathPrefix,
+          'currentPath' => current_path(),
+        ),
+        'type' => 'setting',
+        'scope' => 'header',
+        'group' => JS_SETTING,
+        'every_page' => TRUE,
+        'weight' => 0,
+        'browsers' => array(),
+      ),
+    ),
+    'dependencies' => array(
+      array('system', 'jquery'),
+      array('system', 'drupal'),
+    ),
+  );

I LOVE THIS!

+++ b/core/modules/system/system.moduleundefined
@@ -1831,12 +1924,106 @@ function system_library_info() {
+  // Add configuration only if there are settings added to the page.

I think you should extend this comment and state that this is ONLY necessary for JS that is added through drupal_add_js(), i.e. for JS that is not added as a lib, because JS libs should specify a dependency on drupal.settings, in which case this hook implementation is not necessary at all.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1703,3 +1703,27 @@ function taxonomy_entity_query_alter($query) {
+   * Implements hook_library_info().

Unnecessary/unwanted leading spaces.

Assigned:Unassigned» larowlan

tackling Wim's comments and last two failing tests.

StatusFileSize
new6.4 KB
new75.77 KB
FAILED: [[SimpleTest]]: [MySQL] 40,245 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Attached patch fixes three of the failing tests.
Adds new util function drupal_get_content_request_type which is wrapper to DIC to get the current request type.
Previous patch didn't load/update $settings['ajaxPageState'] because no other js settings existed for the ajax framework tests, during lazy load.
New patch adds a check to see if it's an ajax request, if so $settings['ajaxPageState'] is always added in drupal_get_js (i.e. of course we need to load javascript in an ajax response).
Interdiff attached too.

Status:Needs work» Needs review

Letting the test-bot have a look.
Note there is still an issue with the lazy loading of scripts.
You can see response.data =

"<script type="text/javascript" src="http://d8.taco/core/modules/system/system.js?m92uip"></script>↵"

In the ajax insert command, however it is adding an empty <div> to the page instead of the script in it's own right.
The problem lines are in ajax.js around here:
    // We don't know what response.data contains: it might be a string of text
    // without HTML, so don't rely on jQuery correctly iterpreting
    // $(response.data) as new HTML rather than a CSS selector. Also, if
    // response.data contains top-level text nodes, they get lost with either
    // $(response.data) or $('<div></div>').replaceWith(response.data).
    var new_content_wrapped = $('<div></div>').html(response.data);
    var new_content = new_content_wrapped.contents();

After this runs, new_content_wrapped only has an empty text child element: i.e. new_content_wrapped.html() = '<div></div>'

Assigned:larowlan» Unassigned

Status:Needs review» Needs work

The last submitted patch, js-deps-1737148.32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new74.81 KB
FAILED: [[SimpleTest]]: [MySQL] 40,233 pass(es), 3 fail(s), and 3 exception(s).
[ View ]

#33 is a known issue #736066: ajax.js insert command wraps content in a div

Here's me hoping :)

Status:Needs review» Needs work
Issue tags:-mobile, -JavaScript clean-up

The last submitted patch, core-js-explicit-dependencies-1737148-36.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+mobile, +JavaScript clean-up

The last submitted patch, core-js-explicit-dependencies-1737148-36.patch, failed testing.

re #33 is a known issue, true but without this patch the lazy loading of script tests work so I think we're missing something

Status:Needs work» Needs review
StatusFileSize
new74.89 KB
FAILED: [[SimpleTest]]: [MySQL] 40,352 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Yeah got to find out what it's about, it seems to work fine for me. The script is actually added to the page.

In the meantime, reroll.

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-41.patch, failed testing.

Everything I said in my review #30 was addressed, except for the items below.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.phpundefined
@@ -68,13 +68,7 @@ class JavaScriptTest extends WebTestBase {
-    $this->assertTrue(array_key_exists('core/misc/jquery.js', $javascript), t('jQuery is added when a file is added.'));
-    $this->assertTrue(array_key_exists('core/misc/drupal.js', $javascript), t('Drupal.js is added when file is added.'));

Maybe we should assertFalse() these?

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.phpundefined
@@ -139,6 +136,7 @@ class JavaScriptTest extends WebTestBase {
+    drupal_add_library('system', 'drupal.settings');
     // Only the second of these two entries should appear in Drupal.settings.
     drupal_add_js(array('commonTest' => 'commonTestShouldNotAppear'), 'setting');
     drupal_add_js(array('commonTest' => 'commonTestShouldAppear'), 'setting');

So drupal.settings gets added because we're adding settings that would otherwise not appear.

Can't we automatically add drupal_add_library() whenever drupal_add_js(array(), 'setting') is called?

Or do we intentionally not want to do this to enforce "good behavior"?

I expclicitly don't want that. It's was part of the reason why the drupal_add_js code was weird. If you want settings, declare it.

#46: Okay :) I personally agree, I'm just trying to approach it from all angles. But, in that case, a small explanatory comment would be nice, because you won't see this happening in many locations (since everything/as much as possible should move towards hook_library()).

Regarding the test failures:

  • It seems this is caused by the fact that the necessary tables don't exist at all. Locally, I can reproduce one of the two failures on qa.drupal.org, but I also have two failures from SimpleTest itself: Failed to find test tables to drop.
  • When I revert this patch, I still get the two failures within SimpleTest itself

Hence it seems the problem lies in Locale module/SimpleTest module. The discrepancy between what I see on a fresh local install vs. what qa.drupal.org says is actually worrisome, I think. I've requeued the testing to hopefully get the same result.

Status:Needs work» Needs review
Issue tags:-mobile, -JavaScript clean-up

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-41.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+mobile, +JavaScript clean-up

The last submitted patch, core-js-explicit-dependencies-1737148-41.patch, failed testing.

nod_: do you have any more ideas? Does my research from #47 give you any ideas what might be causing this?

I too grappled with the locale fails.
If you do exactly what the test is doing in the UI, it passes - you get the exported file.
But the test is getting the 'nothing to export' message from line 248 of locale.bulk.inc.
Perhaps it's a profile thing, if I get time today I'll try and install the testing profile and test in the UI.

Tried with the testing profile installed, get "Nothing to export" doing the thing from the UI.

Status:Needs work» Needs review
StatusFileSize
new939 bytes
new76.23 KB
PASSED: [[SimpleTest]]: [MySQL] 40,369 pass(es).
[ View ]

Too damn efficient - the earlier patches resulted in no calls to drupal_add_js so _locale_parse_js_file() never ran, so locales_source was empty.
New patch fetches admin/config/regional/language (which includes tabledrag js so db is at least primed) before exporting PO file.

haha awesome. Now we just need to clean it up or are you still seeing the issue with the JS files added during ajax calls?

Status:Needs review» Needs work

@larowlan: GREAT job! :)

Final review before marking as RTBC. Looks very good :)

+++ b/core/includes/common.incundefined
@@ -533,6 +533,35 @@ function drupal_get_destination() {
+function drupal_get_request_content_type() {
@@ -3946,23 +3917,54 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // this is an ajax request.

s/ajax/AJAX/

+++ b/core/includes/common.incundefined
@@ -3946,23 +3917,54 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+      // Provide the page with information about the theme that's used, so that a

Just beyond col 80?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleExportTest.phpundefined
@@ -114,6 +114,10 @@ class LocaleExportTest extends WebTestBase {
+    // Load an admin page with JavaScript so drupal_add_library fires at least

s/drupal_add_library/drupal_add_library()/

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleExportTest.phpundefined
@@ -114,6 +114,10 @@ class LocaleExportTest extends WebTestBase {
+    // once and _locale_parse_js_file() gets to run at least once so that
+    // locales_source table gets populated with something.

s/so that locales_source/so that the locales_source/

+++ b/core/modules/simpletest/simpletest.moduleundefined
@@ -566,3 +566,25 @@ function simpletest_mail_alter(&$message) {
+function simpletest_library_info() {

Missing the "Implements hook_library_info()." comment.

+++ b/core/modules/system/system.moduleundefined
@@ -1216,6 +1216,51 @@ function _system_batch_theme() {
+  // Drupal specific JavaScript.

s/Drupal specific/Drupal-specific/

+++ b/core/profiles/testing/testing.infoundefined
@@ -2,6 +2,5 @@ name = Testing
-hidden = TRUE

I think this is an accidental change? We *don't* want to change this!

Status:Needs work» Needs review
StatusFileSize
new3.36 KB
new75.85 KB
PASSED: [[SimpleTest]]: [MySQL] 40,358 pass(es).
[ View ]

Re-roll with @Wim Leers requested fixes.

Thanks Wim and Albert, good catch on the hidden flag on the testing profile.

Status:Needs review» Needs work

This looks almost RTBC! Just some clean-up points.

+++ b/core/includes/common.incundefined
@@ -533,6 +533,35 @@ function drupal_get_destination() {
+ * @return string
+ *   The request type

This should be @return string|false, and the description should clarify what FALSE signifies. It should also end with a trailing full stop

+++ b/core/includes/common.incundefined
@@ -533,6 +533,35 @@ function drupal_get_destination() {
+    if ($container->has('request') &&
+        $container->has('content_negotiation')) {

Combine this into one line

+++ b/core/modules/locale/locale.moduleundefined
@@ -374,7 +374,7 @@ function locale_js_alter(&$javascript) {
       if (!in_array($filepath, $parsed)) {
@@ -431,8 +431,7 @@ function locale_library_info_alter(&$libraries, $module) {
@@ -431,8 +431,7 @@ function locale_library_info_alter(&$libraries, $module) {
     // locale.datepicker.js should be added in the JS_LIBRARY group, so that
     // this attach behavior will execute early. JS_LIBRARY is the default for
     // hook_library_info_alter(), thus does not have to be specified explicitly.
-    $datepicker = drupal_get_path('module', 'locale') . '/locale.datepicker.js';
-    $libraries['jquery.ui.datepicker']['js'][$datepicker] = array();
+    $libraries['jquery.ui.datepicker']['dependencies'][] = array('locale', 'drupal.locale.datepicker');
     $libraries['jquery.ui.datepicker']['js'][] = array(
       'data' => array(
         'jquery' => array(
@@ -882,6 +881,38 @@ function _locale_rebuild_js($langcode = NULL) {
@@ -882,6 +881,38 @@ function _locale_rebuild_js($langcode = NULL) {
}
/**
+ * Implements hook_library_info().
+ */
+function locale_library_info() {
+  $libraries['drupal.locale.admin'] = array(
+    'title' => 'Locale',
+    'version' => VERSION,
+    'js' => array(
+      drupal_get_path('module', 'locale') . '/locale.admin.js' => array(),

This would make sense to have locale_library_info() directly before locale_library_info_alter() in the code.

Status:Needs work» Needs review
StatusFileSize
new3.15 KB
new75.92 KB
PASSED: [[SimpleTest]]: [MySQL] 40,365 pass(es).
[ View ]

Thanks @tim.plunkett

Status:Needs review» Needs work

Final review. Three style nitpicks. One actual — yet minor — regression.

+++ b/core/modules/locale/locale.pages.incundefined
@@ -255,8 +255,8 @@ function locale_translate_edit_form($form, &$form_state) {
-  $form['#attached']['js'] = array(
-    $path . '/locale.admin.js',
+  $form['#attached']['library'] = array(
+    array('locale', 'drupal.locale.admin')
   );

We should change this from ['library'] = array(array('foo')) to ['library'][] = array('foo').

+++ b/core/modules/statistics/statistics.moduleundefined
@@ -103,10 +103,8 @@ function statistics_permission() {
function statistics_node_view(Node $node, $view_mode) {
   if (!empty($node->nid) && $view_mode == 'full') {
-    $node->content['#attached']['js'] = array(
-      drupal_get_path('module', 'statistics') . '/statistics.js' => array(
-        'scope' => 'footer'
-      ),
+    $node->content['#attached']['library'] = array(
+      array('statistics', 'drupal.statistics'),
     );
     $settings = array('nid' => $node->nid);
     $node->content['#attached']['js'][] = array(
@@ -462,3 +460,23 @@ function statistics_preprocess_block(&$variables) {
@@ -462,3 +460,23 @@ function statistics_preprocess_block(&$variables) {
     $variables['attributes']['role'] = 'navigation';
   }
}
+
+/**
+ * Implements hook_library_info().
+ */
+function statistics_library_info() {
+  $libraries['drupal.statistics'] = array(
+    'title' => 'Statistics',
+    'version' => VERSION,
+    'js' => array(
+      drupal_get_path('module', 'statistics') . '/statistics.js' => array(),
+    ),
+    'dependencies' => array(
+      array('system', 'jquery'),
+      array('system', 'drupal'),
+      array('system', 'drupal.settings'),
+    ),
+  );
+

'scope' => 'footer' was lost here.

We should change this from ['library'] = array(array('foo')) to ['library'][] = array('foo').

+++ b/core/modules/translation/translation.moduleundefined
@@ -116,8 +116,8 @@ function translation_permission() {
-  $form['#attached']['js'] = array(
-    drupal_get_path('module', 'translation') . '/translation.js',
+  $form['#attached']['library'] = array(
+    array('translation', 'drupal.translation'),

We should change this from ['library'] = array(array('foo')) to ['library'][] = array('foo').

Status:Needs work» Needs review
StatusFileSize
new2.65 KB
new75.52 KB
PASSED: [[SimpleTest]]: [MySQL] 40,563 pass(es).
[ View ]

Fixing issues flagged at #62

That is very awesome :)

I'm very tempted to RTBC it :p It'd be great to have that committed this week, this is the most important frontend performance-related patch ever.

Please give credit to SebCorbin first for this patch he did the hard work of making all the library_info declarations.

SebCorbin++ nod_++ RTBC?

And the other people involved++ :)

Just need confirmation the new drupal_get_request_content_type function is done correctly, I don't know is that's using the container properly or not. Looks like it but I can't say.

I don't like this function, I'm asking myself why is that in the global scope, and will it help other pieces of API than the JS/CSS helpers? It seems to be a wrapper on top of the Request to fetch something that the Request object should probably capable of giving it us in a more direct way, without the need of a wrapper.

can you try it out please?

/me don't even know where to start looking.

Right now, I can't, I'm @work and I don't have a complete environement for this, but I manage to find some time in the weekend I'd try. EDIT: The first place to start is maybe open a support request with the WSCCI tag explaining that you wrote this wrapper function and asking if it exists a better way to fetch this value. I'm thinking that the WSCCI team will probably answer you quickly.

pounard, are you saying this should be a public method on the request object?
If so I can tackle but won't be for a day or so.

Fwiw I started out without the wrapper but the amount of logic required in the if statement was unwieldly so figured a global wrapper might be useful.

I'm not saying that it must be a method of the Request object, but that there may be a more direct way to obtain it, that's why I propose a support request to the WSCCI team and not a feature request.

I'll try to talk to @Crell about this.

Status:Needs review» Needs work

I talked to @Crell. First of all he said that we should *never* use static caching for Symfony stuff.

He told us we should do the following:

$container = drupal_container();
$type = $container->get('content_negotiation')->getContentType($container->get('request'));

But soon, we'll be able to do the following:

$type = drupal_container()->get('content_negotiation')->getContentType();

So we should add a @todo to make sure we do that clean-up later (but @Crell first has to make that possible first).

All right should be an easy one :) someone reroll?, that'll take it very very close to rtbc :)

Assigned:Unassigned» SebCorbin
Status:Needs work» Needs review
StatusFileSize
new1.95 KB
new77.49 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Status:Needs review» Needs work

The last submitted patch, js-deps-1737148-76.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new966 bytes
new77.59 KB
PASSED: [[SimpleTest]]: [MySQL] 40,571 pass(es).
[ View ]

Damn, I forgot about the install state...

wow, someone rtbc that stuff, it's the most important patch for JS. Ever. (funnily enough, there is no js changes :p)

Status:Needs review» Reviewed & tested by the community

RTBC.

RTBC HELL YEAH!

I've also reviewed this many times and think its the right approach. RTBC++

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new77.47 KB
PASSED: [[SimpleTest]]: [MySQL] 40,552 pass(es).
[ View ]

Re-rolled as http://drupalcode.org/project/drupal.git/commitdiff/34e4ef48118d1a4d9155... modified toolbar.module

Also removed unused $module_path variable in toolbar.module

Can you post an interdiff?

StatusFileSize
new645 bytes
new77.34 KB
PASSED: [[SimpleTest]]: [MySQL] 40,552 pass(es).
[ View ]

@tim.plunkett here's a re-roll patch with a patch interdiff against #78 (no changes except for the settings that conflicted)

StatusFileSize
new596 bytes
new74.54 KB
PASSED: [[SimpleTest]]: [MySQL] 40,552 pass(es).
[ View ]

And here's a new one, with unused $module_path removed in toolbar.module, and already stated dependency to jquery.cookie.
interdiff is against #85

Reroll ok for me.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

Awesome patch! I really like it.

I reviewed this patch in-depth and found a couple of issues with it. I did not read comments on this issue, in order to try to stay as ignorant for reviewing the final implementation.

+++ b/core/modules/block/block.admin.inc
@@ -82,8 +82,8 @@ function block_admin_display_prepare_blocks($theme) {
+  $form['#attached']['library'][] = array('system', 'drupal.tableheader');
+  $form['#attached']['library'][] = array('block', 'drupal.block');

Let's remove the "drupal." prefix from registered libraries. This was introduced, because System module was more or less the only module in core that ships with any libraries and we wanted to separate Drupal stuff in the /misc folder from jQuery and other stuff.

However, in this larger scope the "drupal." prefix doesn't really make sense. I think that we should only use the "drupal." prefix for stuff in the /misc directory. For everything else it is unnecessary.

+++ b/core/modules/block/block.module
@@ -1085,3 +1085,22 @@
+function block_library_info() {
+++ b/core/modules/book/book.module
@@ -1454,3 +1454,23 @@
+function book_library_info() {

The new hook_library_info() implementations have been added to the bottom of .module files, whereas it is (not a standard but) common practice to put the "central" info hook implementations to the top.

I do not want to hold up this patch on that, but it would be great if we could quickly adjust that before commit - otherwise, let's do a follow-up patch/issue.

+++ b/core/modules/block/block.module
@@ -1085,3 +1085,22 @@ function block_language_delete($language) {
+    'dependencies' => array(
+      array('system', 'jquery'),
+      array('system', 'drupal'),
+    ),
+++ b/core/modules/book/book.module
@@ -1454,3 +1454,23 @@ function book_menu_subtree_data($link) {
+    'dependencies' => array(
+      array('system', 'jquery'),
+      array('system', 'drupal'),
+      array('system', 'drupal.form'),
+    ),

drupal_add_library() resolves nested dependencies already, so I don't quite understand why we're specifying the full stack of dependencies instead of just the "highest" dependency?

Removing the deeper dependencies doesn't affect how this patch works, so I'm happy if we clean this up in a follow-up issue.

+++ b/core/modules/locale/locale.module
@@ -374,7 +374,7 @@ function locale_js_alter(&$javascript) {
   foreach ($javascript as $item) {
-    if ($item['type'] == 'file') {
+    if (isset($item['type']) && $item['type'] == 'file') {

'type' should always be set for every JS item. I do not see where and why this would be changed in this patch. Is this additional check for 'type' being set an artifact of earlier debugging or approaches?

+++ b/core/modules/system/system.module
@@ -1216,6 +1216,51 @@ function _system_batch_theme() {
+  // url() generates the script and prefix using hook_url_outbound_alter().
+  // Instead of running the hook_url_outbound_alter() again here, extract
+  // them from url().
+  // @todo Make this less hacky: http://drupal.org/node/1547376.
+  $scriptPath = $GLOBALS['script_path'];
+  $pathPrefix = '';
+  url('', array('script' => &$scriptPath, 'prefix' => &$pathPrefix));
+  // Drupal settings.
+  $libraries['drupal.settings'] = array(
+    'title' => 'Drupal Settings',
+    'version' => VERSION,
+    'js' => array(
+      array(
+        'data' => array(
+          'basePath' => base_path(),
+          'scriptPath' => $scriptPath,
+          'pathPrefix' => $pathPrefix,
+          'currentPath' => current_path(),
+        ),
+        'type' => 'setting',
+        'scope' => 'header',
+        'group' => JS_SETTING,
+        'every_page' => TRUE,
+        'weight' => 0,
+        'browsers' => array(),
+      ),
+    ),
+    'dependencies' => array(
+      array('system', 'jquery'),
+      array('system', 'drupal'),
+    ),
+  );

This cannot be contained in hook_libraries_info(), since it is incompatible with caching of the registered libraries. hook_libraries_info() is a static declaration/registration of available libraries provided/shipped by modules and not supposed to change on every request.

It looks like this change actually belongs into #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page instead.

+++ b/core/modules/system/system.module
@@ -1357,13 +1444,16 @@ function system_library_info() {
-  $libraries['farbtastic'] = array(
+  $libraries['jquery.farbtastic'] = array(
     'title' => 'Farbtastic',
     'website' => 'http://code.google.com/p/farbtastic/',

The farbtastic library name change is irrelevant for this issue and should be reverted. I don't think our code is actually the code of that Google code repo (development on that seems to have stopped in 2009/2010), although I could be mistaken. In any case, this rename seems out of scope for this issue.

+++ b/core/modules/system/system.module
@@ -1835,12 +1928,109 @@ function system_library_info() {
+function system_js_alter(&$javascript) {
+  // Add configuration only if there are settings added to the page. This is
+  // only necessary for JavaScript that is added through drupal_add_js(),
+  // JavaScript that is added as a library would nominate its dependency on
+  // drupal.settings.
+  if (!empty($javascript['settings'])) {
+    $javascript['settings'] += array(
+      'type' => 'setting',
+      'scope' => 'header',
+      'group' => JS_SETTING,
+      'every_page' => TRUE,
+      'weight' => 0,
+      'browsers' => array(),
+    );
+  }
+}

Lazy-injecting the base information for settings in a hook_js_alter() implementation doesn't fly for me. I guess this change is the cause for the isset($item['type']) check in locale_js_alter(), which is ultimately going to be only one of many consequences. This handling of defaults has to be carried out by drupal_get_js().

This change also seems to belong into #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page instead.

Status:Needs work» Needs review

Well some reading could have helped.

1) The drupal. prefix is here for a reason, it's because all this stuff lives under the Drupal object, Drupal.tableDrag, Drupal.ajax and so on. Notice how the html5 shiv doesn't have it. JS will be out of misc and in the system module very soon after this patch we can't rely on that.

jquery.farbtastic, under the $.fn.farbtastic, same thing.

2) bottom of files, fair enough, I'd rather follow up issues, it's a lot of files.

3) No, explicit dependencies, the actual script is using and the jQuery object and the Drupal object so it should depend on both even if drupal.settings depends on both as well, otherwise it'll be impossible to build AMD/commonjs ever and will make things much harder to take apart in contrib. There is nothing to clean-up here it's the whole point of the patch.

4) 'type', indeed it comes from the _js_alter() but this is needed since drupal_add_js does way too much, the other issue about js on every pages comes from drupal_add_js doing way too much and putting defaults everywhere. So well, if you have a better idea you're welcome.

5) About caching, maybe. Still works for me™ I can put that with the rest of the ajaxPageState in drupal_get_js if that suits you better. even if it's an empty array() this library definition needs to stay.

6) the js_alter hook, well, couldn't make it work another way If you have a better solution feel free to update the patch.

This patch is aimed at simplifying how we process js, the _js_alter, the wierd things in settings will be gone once we make that work #1751602: Use Assetic to package JS and CSS files but this patch needs to get in before we can start on that. So yeah, not great but works really well.

StatusFileSize
new73.88 KB
FAILED: [[SimpleTest]]: [MySQL] 40,700 pass(es), 3 fail(s), and 1 exception(s).
[ View ]

Changed the settings thing, and removed the isset from local.module, let's see what the testbot says.

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-91.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new74.54 KB
PASSED: [[SimpleTest]]: [MySQL] 40,703 pass(es).
[ View ]

Tried a few other things: can't make it work. I don't know how to do it another way, feel free to fix the patch if it doesn't suit you, there are several others for which it's good enough for now.

re-up #86.

Assigned:Unassigned» SebCorbin

This is holding up a lot of follow-up work, that will eventually lead to fixing the feedback from #89.

Assigned:SebCorbin» Unassigned
Status:Needs review» Reviewed & tested by the community

grr double post.

Assigned:SebCorbin» catch

This issue is great. I'd really, really like to review it before it goes in though (but can't do so today), so assigning to me. This shouldn't stop webchick or Dries reviewing it too.

Status:Reviewed & tested by the community» Needs review

I understand that you want to move forward. However, the parts about settings that have been outlined in #89 really do not look acceptable to me. We need to find a better solution for that. I don't think that it is proper to treat Drupal.settings as a library -- the amount of additionally required adjustments for them in this patch pretty much prove that settings do not work like a library. It's not really clear to me why those changes are part of this patch in the first place.

Can we just do the files to libraries conversion here?

On the other issues:

The drupal. prefix is here for a reason, it's because all this stuff lives under the Drupal object, Drupal.tableDrag, Drupal.ajax and so on.

That's a very custom interpretation of the server-side library names. I don't see a problem with it, but we need to make sure that this gets documented.

That said, some scripts/libraries may extend a parent object in multiple ways, so I'm relatively sure that it won't always be possible to determine a unique name à la drupal.foo or jquery.ui.bar. I wonder whether the concept will lead to inconsistencies.

No, explicit dependencies, the actual script is using and the jQuery object and the Drupal object so it should depend on both even if drupal.settings depends on both as well, otherwise it'll be impossible to build AMD/commonjs ever and will make things much harder to take apart in contrib.

I do not think that this is the case. drupal_add_library() already resolves all dependencies of dependencies, which only takes a couple lines of code. The very same can be done by a dependency resolving function for AMD/commonjs. If we wanted to, we could resolve and prepare the dependencies of all libraries within drupal_get_library() already.

However, having to manually specify all nested dependencies sounds very cumbersome and prone to errors to me. That's a typical job for computers, not for humans.

Because not all scripts needs Drupal.settings, we have to add it as a library, for now.

If the processing wasn't trying to be too smart and add default settings all over the place when it's not needed (meaning that jquery and drupal get added to all pages all the time, yes that's the reason behind #1279226: jQuery and Drupal JavaScript libraries and settings are output even when no JS is added to the page) we wouldn't need it. Then to fix that properly we'll need to change the internal API of this thing, which is going to be a huge mess to clean up, making this big patch a huge patch. Which is why the API change of js processing will be handled in #1751602: Use Assetic to package JS and CSS files issue to clean all that up and hopefully make the settings dependency obsolete.

Show me that the current patch breaks stuff and I'll change the way it's done somehow (I tried, I really tried to make it pretty, I just can't). I don't want to have an API change in this patch that will push it away that much further.

I do not think that this is the case. drupal_add_library() already resolves all dependencies of dependencies,

That's not the point. Try out AMD, you'll see the problem with what you're saying right away, read the AMD issue, look at the AMD sandbox. If you need to access "it" from your JS code, you need to declare "it" as a dependency, regardless of the rest of if it's a dependency of something else you're using.

By all means, show me a reproducible way of how this badly breaks or improve the patch to make it better while keeping the same API. Until then I really can't do much more than push for this exact patch to get in so that we can get on with the rest of the clean-up.

  1. Regarding Drupal.settings as a library. I think it is valid to regard Drupal.settings as a library, for two reasons:
    1. It is impossible to magically know which JS files need Drupal.settings. So we need some way to figure out which JS depends on it. Why not use the existing dependency system for that?
    2. It is *more* than just settings: it depends both on Drupal.js and jQuery having loaded before it.
  2. Regarding specifying "all" dependencies. As nod_ outlined, you should not see it as specifying "all things to load", but rather "all things you need access to". Instead of looking at it from a PHP POV, or a "general programmer POV", you have to look at it from a JS POV. However, this should absolutely be documented very clearly and explicitly.

I agree that settings feels like a genuine library, it depends on libraries, libraries depend on it.

+++ b/core/includes/common.incundefined
@@ -3952,23 +3894,59 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
+    // Don't add settings if there is no other JavaScript on the page, unless
+    // this is an AJAX request.
+    // @todo Clean up container call.
+    $container = drupal_container();
+    if ($container->has('request') && $container->has('content_negotiation')) {
+      $type = $container->get('content_negotiation')->getContentType($container->get('request'));

Ouch, but it has a @todo.

+++ b/core/modules/system/system.moduleundefined
@@ -1835,12 +1928,109 @@ function system_library_info() {
/**
+ * Implements hook_js_alter().
+ */
+function system_js_alter(&$javascript) {
+  // Add configuration only if there are settings added to the page. This is
+  // only necessary for JavaScript that is added through drupal_add_js(),
+  // JavaScript that is added as a library would nominate its dependency on
+  // drupal.settings.
+  if (!empty($javascript['settings'])) {
+    $javascript['settings'] += array(
+      'type' => 'setting',
+      'scope' => 'header',
+      'group' => JS_SETTING,
+      'every_page' => TRUE,
+      'weight' => 0,
+      'browsers' => array(),
+    );
+  }

Do we want to deprecate both this, and direct usage of drupal_add_js(), and then remove this again? I'm wondering if there should be a follow-up patch to make it _drupal_add_js() to indicate it's an internal function. Also wondering whether anything in core is actually using #attached['js'] now but I assume that needs to stay for external js or similar.

Yes, I would love to rename to _drupal_add_js and make it internal only ... We do use #attached['js'] and AFAIK, it remains the preferred way to add js to the page or else you break render cache. Eventually, we should deprecate drupal_add_library() and drupal_add_css() as well (unless SCOTCH changes everything).

So we asked crell about this piece of code, he told us to put the todo since we can't currently do that but it'll be possible to make it shorter soon, once some patches gets in, so… blame Crell :)

Yep, so I don't want to put API changes in this patch, so that's the only way I was able to do that. That's right, core don't use #attach js anywhere.

Yeah there are a few things API-related that will need to be taken care of to clean up the alter thing. Making a new internal function to separate and simplify what drupal_add_js is currently doing, we can actually keep that around to add implicity dependencies on jquery, drupal, and drupal.setting for every script added by drupal_add_js just so you know, we don't horribly break contrib.

I guess we can make a intermediate followup between this issue and the Assetic issue.

The @todo can only be solved when WSCCI is further along.

Ideally: #attached should be the only way to add/remove CSS/JS/libs. drupal_add_js(), if we keep it, should indeed become a private/internal function. But those are outside of the scope of this patch (as nod_ says: no API changes in this patch).

Besides that, I think the only unaddressed question here is whether it's acceptable to have Drupal.settings be specified as a library. Conceptually, it makes sense, as @catch and I have outlined above. However, @sun makes a good point: part of the concept of libs is also that they're not tied to the current page request. However, I feel that is a minor issue in comparison with the major improvements this patch brings. I think it would be acceptable to create a follow-up issue for that aspect?

So, what is still blocking this patch from going to RTBC status?

Status:Needs review» Reviewed & tested by the community

What would that follow up be? to create a new construct for page specific libraries?

Follow-up: #1762204: Introduce Assetic compatibility layer for core's internal handling of assets. The summary still needs some work but that's mostly it.

My two major concerns are:

1) The actual settings are not declarative, they are dynamic, by design. Info hooks are not supposed to contain highly dynamic data. In fact, this patch revealed that we're needlessly re-computing all library info on every single page request and missing a cache for hook_library_info() currently. In addition to that, system_js_alter() is an alter hook that injects default properties, which is architecturally wrong.

2) It is not clear how modules are supposed to add settings. It looks like they need to add the drupal.settings library first and then add the settings. This feels odd. Especially if you consider that (further) settings might be added to a page in a different (code) spot than the library.

I'd really prefer to take that settings part out of this patch and discuss it in a separate issue. It's really just that part that bugs me — all of the other js-to-library changes are looking great.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.55 KB
new74.26 KB
FAILED: [[SimpleTest]]: [MySQL] 20,387 pass(es), 497 fail(s), and 882 exception(s).
[ View ]

That's fair enough. the library def stays because we need the dependencies but it doesn't add anything else.

Let's see what the testbot says.

Thanks! RTBC if bot comes back green.

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-107.patch, failed testing.

Haha, that really scared me, it's failing because it can't write to disk somehow. I'm asking around to see what's up before retesting.

Status:Needs work» Needs review
StatusFileSize
new74.25 KB
FAILED: [[SimpleTest]]: [MySQL] 18,297 pass(es), 447 fail(s), and 403 exception(s).
[ View ]

same as #107, with correcter spelling in comment.

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-111.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new74.52 KB
PASSED: [[SimpleTest]]: [MySQL] 40,337 pass(es).
[ View ]

All right had a few issues with the tests, all green (local at least). Hope you have some space now testbot, here is the patch.

Status:Needs review» Needs work

Just thinking ahead for documentation etc. Is there still a valid use case for drupal_add_js once this patch gets in? Since all contrib modules are encouraged to use Drupal's behaviors (and thus have a dependency to drupal.js), shouldn't they define their script in hook_library_info as well?

Status:Needs work» Needs review

cross-post...

Status:Needs review» Needs work
Issue tags:-mobile, -JavaScript clean-up

The last submitted patch, core-js-explicit-dependencies-1737148-113.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+mobile, +JavaScript clean-up

On a sidenote:
Shouldn't we change drupal_add_js to do

<?php
drupal_add_library
('system', 'drupal.settings');
?>

when $options['type'] is setting so that we can just do this:
<?php
drupal_add_js
(array('dummy' => 'value'), 'setting');
?>

in stead of:
<?php
drupal_add_library
('system', 'drupal.settings');
drupal_add_js(array('dummy' => 'value'), 'setting');
?>

Just a small change dat improves DX (imho)

Status:Needs review» Needs work

The last submitted patch, core-js-explicit-dependencies-1737148-113.patch, failed testing.

1 fail on file unmanagedDelete test... Bot still acting out?

One more retest...

Status:Needs work» Needs review

(while waiting for the bot) #118 that'll be doing what drupal_add_js has been doing, implicitly adding things so I'm not really in favor of this.

True, but it seems to me that when you're adding js settings, you'll be adding js files as well anyway... But I can see your point.

Assigned:catch» Unassigned

Unassigning me since I was able to look at this, looks much better with the request stuff outside the hook_library_info() yes.

Status:Needs review» Reviewed & tested by the community

from #108: Rock & roll.

Issue tags:+Needs manual testing

This seems like the type of issue that could use a couple rounds of manual testing, correct?

Well I guess, it's been tested to death by me at least and I'm sure about a couple of other people that tested it as well.

Title:Explicitly declare all JS dependencies, don't use drupal_add_jsChange notice: Explicitly declare all JS dependencies, don't use drupal_add_js
Priority:Major» Critical
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Cool, this patch seems like a great start to reducing our mobile JS footprint. Looks like everyone's concerns from above have been addressed as well.

Committed and pushed to 8.x. Thanks!

In general, I agree with manual testing (particularly in 7.x), but while we're in code thaw, as long as basic testing has been done, I think we'll catch any JS regressions pretty fast once patches like this get committed.

This definitely needs a change notice.

There is a change notice at https://drupal.org/node/1764252, feel free to make it better or tell me how to.

I'd like to thank SebCorbin for all his work that lead to this patch, he's been working in the shadows to make core use AMD (and he succeeded! Check out the AMD sandbox).

I've never seen that many people in one of my JS-related issue, thanks to all of you :)

Now, go enjoy the speed (makes a lot more difference without toolbar activated)!

Title:Change notice: Explicitly declare all JS dependencies, don't use drupal_add_jsExplicitly declare all JS dependencies, don't use drupal_add_js
Priority:Critical» Major
Status:Active» Fixed

Awesome, thanks for the change notice! And yay for faster Drupal. :)

I went to read the change description but I don't know what AMD is. Perhaps a clearer explanation of that this means:

"This change will make possible the use of core JS in the context of AMD by contrib."

Otherwise, great work!!

AMD : Asynchronous Module Definition

More info http://github.com/amdjs/amdjs-api/wiki/AMD and #1542344: Use AMD for JS architecture

Now Drupal will also be modular in the front-end :)

Follow-up: This introduced a regression in the OpenID module. This is included in the latest patch for #1538462: Cannot log in with OpenID due to "required" attribute (another issue blocking usage of OpenID).

I'm curious if anyone had tested how this works with themes that use scripts[] = myfile.js and the fact that themes cannot implement basic hooks (so no hook_libraries). I mean I guess a theme can use themename_libraries_alter() but this is adding extra complexity for users that normally don't need to touch PHP files. Is there a follow-up for this at all?

maybe not a direct follow-up but very related #1762204: Introduce Assetic compatibility layer for core's internal handling of assets where this'll turn drupal_add_js into a legacy function adding drupal.js, jquery.once, drupalSettings and jQuery as dependency for the script. Then it's just a matter of making scripts[] use drupal_add_js to add files.

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.

Issue summary:View changes

drupal_add_js in color module

Landed to the change record by google search, and edited it for using render arrays.

Please verify this is correct:
https://drupal.org/node/1764252/revisions/view/2735445/6898303