| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | other |
| Category: | task |
| Priority: | normal |
| Assigned: | Wim Leers |
| Status: | active |
| Issue tags: | JavaScript, Needs tests |
Issue Summary
Problem/Motivation
Expected: drupal_add_js('something.js', array('scope' => 'footer')) (or the library definition equivalent) is listed in drupalSettings.ajaxPageState.js.
Result: only the JS files that are in the 'header' scope are listed in drupalSettings.ajaxPageState.js.
Proposed resolution
diff --git a/core/includes/common.inc b/core/includes/common.inc
index 10b09d6..70c0b81 100644
--- a/core/includes/common.inc
+++ b/core/includes/common.inc
@@ -3831,7 +3831,7 @@ function drupal_get_js($scope = 'header', $javascript = NULL, $skip_alter = FALS
// Provide the page with information about the individual JavaScript files
// used, information not otherwise available when aggregation is enabled.
- $setting['ajaxPageState']['js'] = array_fill_keys(array_keys($items), 1);
+ $setting['ajaxPageState']['js'] = array_fill_keys(array_keys($javascript), 1);
unset($setting['ajaxPageState']['js']['settings']);
// Provide the page with information about the individual CSS files used,This is analogous to how the CSS page state is stored. $items only contains the items for the current scope; and since settings on most pages are added when $scope == 'header', that explains why only JS files that are in the 'header' scope get added to the page state.
Remaining tasks
Get proper test coverage. In my initial testing it turned out to be impossible to reproduce the same problem in the tests, likely because WebTestBase does not actually execute JavaScript, it merely simulates the crucial aspects.
User interface changes
None.
API changes
None.
Comments
#1
#2
Now that #1824500: In-place editing for Fields has landed, this is super easy to reproduce.
drupalSettings.ajaxPageState.js.edit.jsis listed there (which has'scope' => 'footer') . It's not. This is the bug.#3
Works well as a stop-gap measure. It's the whole API that needs to be worked on to avoid this kind of uglyness, see #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox].
In the meanwhile, this works.
#4
Hm. Unfortunately, doesn't sound like it's possible to work #2 into a test since it's JS-only. :(
Committed and pushed to 8.x. Thanks!
#5
It is in fact possible to add tests for this, similarly to how I'm doing tests over at #1875632: JS settings merging behavior: preserve integer keys (allow for array literals in drupalSettings): using
json_decode().However, all of this will change in the issue that nod_ pointed to in #3 so it's not clear if it's worth the effort. I personally believe it's worth it, even if only because that'll ensure tests in #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox] will test this edge case too. Thoughts?
#6
Yeah, let's do it. That issue can always remove the test if it turns out to not be relevant.
Switching some metadata around.
#7
I'll likely get this done tomorrow.