Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Updated: Comment #0
Problem/Motivation
It is possible to change javascript settings via hook_js_alter(), but the settings are returned unaltered from ajax request.
For example:
function mymodule_js_alter(&$javascript) {
$javascript['settings']['data'][1]['pathPrefix'] = 'whatever/';
}
Proposed resolution
Use the altered JS settings - if they are available.
Remaining tasks
* Create proper patch.
Patch:
diff --git a/docroot/includes/ajax.inc b/docroot/includes/ajax.inc
index ab0111c..dc0491e 100644
--- a/docroot/includes/ajax.inc
+++ b/docroot/includes/ajax.inc
@@ -267,7 +267,10 @@ function ajax_render($commands = array()) {
// HTML in the page. We pass TRUE as the $skip_alter argument to prevent the
// data from being altered again, as we already altered it above. Settings are
// handled separately, afterwards.
+
+ $scripts = array();
if (isset($items['js']['settings'])) {
+ $scripts['settings'] = $items['js']['settings'];
unset($items['js']['settings']);
}
$styles = drupal_get_css($items['css'], TRUE);
@@ -289,7 +292,10 @@ function ajax_render($commands = array()) {
}
// Now add a command to merge changes and additions to Drupal.settings.
- $scripts = drupal_add_js();
+ // Only reload the settings when its not yet set.
+ if (!isset($scripts['settings'])) {
+ $scripts = drupal_add_js();
+ }
if (!empty($scripts['settings'])) {
$settings = $scripts['settings'];
array_unshift($commands, ajax_command_settings(call_user_func_array('array_merge_recursive', $settings['data']), TRUE));
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | d7_alter_js_settings_ajax-2171113-33.patch | 764 bytes | pol |
| #27 | d7_alter_js_settings_ajax-2171113-27.patch | 522 bytes | David_Rothstein |
Comments
Comment #1
fabianx commentedComment #2
wiifmHere is a patch, slightly updated to match current HEAD.
Comment #4
wiifm2: 2171113-alter-js-settings.patch queued for re-testing.
Comment #6
wiifmWas wondering why the patch was not applying. Cue penny drop.
Comment #7
wiifm2: 2171113-alter-js-settings.patch queued for re-testing.
Comment #9
fabianx commented2: 2171113-alter-js-settings.patch queued for re-testing.
Comment #10
fabianx commentedAdding Needs Backport tag and being a bug this unfortunately needs a test.
Comment #11
dawehnerNote, this will certainly somehow conflict with #1959574: Remove the deprecated Drupal 7 Ajax API ... given that we have a nice new ajax API already.
Comment #12
zazaidi commentedI concur that this issue is obsolete due to the changes made to the ajax api in #1959574 mentioned above. I tested altering JS settings by setting them via _js_alter() and then calling them back via ajax and the settings are returning altered as expected so I believe this issue can be closed.
Comment #13
zazaidi commentedComment #14
zazaidi commentedWas not able to reproduce the issue in the latest Drupal. Please reopen if necessary.
Comment #15
fabianx commentedThen back to 7.x with this bug.
Comment #16
fabianx commentedLets see if this still applies, this works fine in production since 10 months, also bumping to major.
Comment #18
fabianx commentedNeed to add the following, too to ensure it works in all cases.
Comment #19
mw4ll4c3 commentedTackling this quickly for #devdays
Comment #20
mw4ll4c3 commentedRe-rolled the patch, and included the addition suggested in #18.
Comment #21
mw4ll4c3 commentedComment #23
mw4ll4c3 commentedNot so tackled, not so quickly... investigation into test failures (and the ramifications of this change) are necessary.
Comment #24
douggreen commentedrerolled patch for D7
Comment #25
sdstyles commentedComment #27
David_Rothstein commentedThe test failures on the patch in #24 are real and are in fact causing a problem on a site where I'm using it.
The problem is that the patch collects the JavaScript settings too early (before calling drupal_get_css() and drupal_get_js(), both of which add new information to the 'ajaxPageState' setting when they are called). So those changes to ajaxPageState never make it into the final page output.
The practical problem this leads to is that Drupal can no longer track which CSS and JavaScript files were dynamically added to the page. So on an Ajax request that lazy-loads CSS or JavaScript files (e.g. an Ajax-enabled "upload" button where the code that renders already-uploaded files has some associated CSS or JavaScript), with the patch from #24 applied Drupal will keep re-adding new copies of the lazy-loaded CSS and JavaScript files to the page again and again, once for each time the "upload" button is clicked.
The duplicate files can be a particular problem with JavaScript, since depending on how the JavaScript is written multiple copies of it can interfere with each other (e.g. if the code defines a JavaScript object which is trying to maintain some kind of internal state between Ajax requests, the duplicate file will overwrite the object with a fresh copy and thereby throw out the internal state).
For a different fix that doesn't have this problem, is there any reason the attached one-line patch wouldn't work? As far as I can see it meets the goal of this issue.
Comment #28
fabianx commented#27: That should probably work fine.
I am unsure if calling drupal_alter() more times than normal could lead to any issues, but the patch is clearly simpler.
However I would suggest to move the alter() inside of the !empty() clause.
Comment #29
David_Rothstein commentedIsn't it possible (although unlikely) that a hook_js_alter() implementation adds something to $scripts['settings'] even when it was previously empty? That's why I had it outside the !empty() clause.
Comment #30
valthebaldI second @David_Rothstein that it could happen that we'd want to add something to previously empty settings in alter hook
Comment #31
stefan.r commentedGuessing we might not need tests for this?
Comment #32
David_Rothstein commentedI think tests would be great in theory, but in this case they might be more annoying than they are worth. Especially if Drupal 8 has no equivalent tests to backport (which I don't think it does) I would vote for skipping them.
Comment #33
polPatch updated.
The drupal_alter() is inside of the !empty() clause.
Updated the code style of the block.
Comment #34
polComment #35
mustanggb commentedUpdated tags:
- No tests needed
- No D8 patch needed
- Has been rerolled
Looks good to go:
- RTBC
Comment #36
polComment #37
joseph.olstadBumping to 7.61, this didn't make it into 7.60
Comment #38
joseph.olstadComment #39
joseph.olstadComment #40
polUpdating credits.
Comment #41
pol@MustangGB Why no D8 patch is needed ? Could you develop ?
Comment #42
polDeleting the wrong comment.
Comment #43
joseph.olstadComment #44
mcdruid commentedComment #45
joseph.olstadComment #46
joseph.olstadMaintain 'Drupal 7 pending commit' and RTBC based on David Rothsteins comment here: #2171113-32: Settings returned via ajax are not run through hook_js_alter()
Comment #47
mcdruid commented#29 and #30 argued that it's...
Then #33 moved the drupal_alter() inside the !empty() clause (based on #28 probably).
#33 also re-formats a long line of code which is not part of the change being made.
So I'd be inclined to go back to the patch in #27.
I'll leave it to @Fabianx to make the final call on that though.
Comment #48
joseph.olstadI agree with mcdruid on this for the same reasons. Patch 27 yes
Comment #49
mcdruid commentedComment #50
fabianx commentedRTBM for #27 - let’s get that in.
I agree with David Rothstein and my suggestion to move it into the if does not make sense.
Comment #52
mcdruid commentedThank you contributors!
Comment #53
mustanggb commentedThanks!