For ajax callbacks that want to inform Drupal about something related to the current page, there is not guaranteed way to find the current page path.

We already add the base path and other info to the JS settings, we should add the Drupal path of the current page too.

Note by default this and more info is exposed in the CSS classes in the HTML, but reconstructing the path from that is going to be fragile so it would be better to have a guaranteed place to find it.

Files: 
CommentFileSizeAuthor
#44 core-ajax-currentPath-update-1691394-44.patch4.47 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 57,899 pass(es).
[ View ]
#42 core-ajax-currentPath-update-1691394-42.patch3 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 57,973 pass(es).
[ View ]
#40 core-ajax-currentPath-update-1691394-40.patch2.57 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 57,656 pass(es).
[ View ]
#39 core-ajax-currentPath-update-1691394-39.patch2.43 KBnod_
PASSED: [[SimpleTest]]: [MySQL] 58,005 pass(es).
[ View ]
#37 core-ajax-currentPath-update-1691394-37-fail.patch1.73 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 57,924 pass(es), 1 fail(s), and 4 exception(s).
[ View ]
#37 core-ajax-currentPath-update-1691394-37.patch2.37 KBnod_
FAILED: [[SimpleTest]]: [MySQL] 38,485 pass(es), 799 fail(s), and 420 exception(s).
[ View ]
#33 core-ajax-currentPath-update.patch659 bytesnod_
PASSED: [[SimpleTest]]: [MySQL] 58,145 pass(es).
[ View ]
#24 drupal-1691394-24-tests.patch3.36 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 39,354 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
#24 drupal-1691394-24-combined.patch3.84 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 40,094 pass(es).
[ View ]
#20 drupal-1691394-20-combined.patch4.67 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 37,110 pass(es).
[ View ]
#19 drupal-1691394-18-combined.patch4.66 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 37,108 pass(es).
[ View ]
#18 drupal-1691394-18-tests.patch4.16 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 37,106 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
#18 drupal-1691394-18-tests.patch4.16 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 37,103 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
#17 drupal-1691394-17-tests.patch3.43 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 32,414 pass(es), 5 fail(s), and 1 exception(s).
[ View ]
#17 drupal-1691394-17-combined.patch3.93 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 37,109 pass(es).
[ View ]
#15 drupal-1691394-15-tests.patch1.36 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 37,289 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#15 drupal-1691394-15-combined.patch1.86 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 37,290 pass(es).
[ View ]
#10 1691394-10.patch1.55 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 37,288 pass(es).
[ View ]
#3 1691394-3.patch511 bytespwolanin
FAILED: [[SimpleTest]]: [MySQL] 37,287 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Comments

Issue tags:+Novice

Sounds good to me.

Sounds good to me too.

Status:Active» Needs review
StatusFileSize
new511 bytes
FAILED: [[SimpleTest]]: [MySQL] 37,287 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Here's a starting patch.

Need maybe a test like the one to check for basePath in core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.php

That test sould see that an aliased and non-aliased path puts the correct Drupal path into the settings.

We also need to make sure that AJAX requests are not overriding that.

Status:Needs review» Needs work

The last submitted patch, 1691394-3.patch, failed testing.

Test fails look unrelated to the patch at first glance - any idea?

related. the tests hardcode the array index things are supposed to be. the new info offseted things.

related. the tests hardcode the array index things are supposed to be. the new info offseted things.

Wow, that seems rather too fragile. ok, so does need work.

Status:Needs work» Needs review
StatusFileSize
new1.55 KB
PASSED: [[SimpleTest]]: [MySQL] 37,288 pass(es).
[ View ]

makes the test more robust

Component:ajax system» javascript
Status:Needs review» Reviewed & tested by the community

looks good to me too

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/JavaScriptTest.phpundefined
@@ -75,8 +75,9 @@ class JavaScriptTest extends WebTestBase {
-    $this->assertEqual(280342800, $javascript['settings']['data'][3]['dries'], t('JavaScript setting is set correctly.'));
-    $this->assertEqual('rocks', $javascript['settings']['data'][3]['drupal'], t('The other JavaScript setting is set correctly.'));
+    $last_settings = end($javascript['settings']['data']);
+    $this->assertEqual(280342800, $last_settings['dries'], t('JavaScript setting is set correctly.'));
+    $this->assertEqual('rocks', $last_settings['drupal'], t('The other JavaScript setting is set correctly.'));

This is a nice clean-up, but seems unrelated to the issue, as it doesn't add a check for the current path. Also, while adding a check for that, keep in mind that assertion messages shouldn't be wrapped in t()

The clean-up is needed to avoid the test failures from the 1st patch, so it's related.

ok, so follow-up (maybe suitable for a novice)

1) remove t()

2) add a test case.

Issue tags:+Needs tests

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new1.86 KB
PASSED: [[SimpleTest]]: [MySQL] 37,290 pass(es).
[ View ]
new1.36 KB
FAILED: [[SimpleTest]]: [MySQL] 37,289 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Added a test and removed the calls to t().

Good start, but should we have a test that verifies it's in the page source?

StatusFileSize
new3.93 KB
PASSED: [[SimpleTest]]: [MySQL] 37,109 pass(es).
[ View ]
new3.43 KB
FAILED: [[SimpleTest]]: [MySQL] 32,414 pass(es), 5 fail(s), and 1 exception(s).
[ View ]

Added more tests

StatusFileSize
new4.16 KB
FAILED: [[SimpleTest]]: [MySQL] 37,103 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
new4.16 KB
FAILED: [[SimpleTest]]: [MySQL] 37,106 pass(es), 4 fail(s), and 1 exception(s).
[ View ]

remove more t() calls.

StatusFileSize
new4.66 KB
PASSED: [[SimpleTest]]: [MySQL] 37,108 pass(es).
[ View ]

oops, wrong patch

StatusFileSize
new4.67 KB
PASSED: [[SimpleTest]]: [MySQL] 37,110 pass(es).
[ View ]

minor tweak to the test path.

Status:Needs review» Reviewed & tested by the community

That is some might ugly escaping in the regex, but I don't think it can be helped.

Looks good overall!

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

Version:8.x-dev» 7.x-dev
Status:Fixed» Patch (to be ported)

Status:Patch (to be ported)» Needs review
StatusFileSize
new3.84 KB
PASSED: [[SimpleTest]]: [MySQL] 40,094 pass(es).
[ View ]
new3.36 KB
FAILED: [[SimpleTest]]: [MySQL] 39,354 pass(es), 4 fail(s), and 1 exception(s).
[ View ]

Here's a stab at a backport.

#24: drupal-1691394-24-combined.patch queued for re-testing.

Status:Needs review» Reviewed & tested by the community

This is fine!

Status:Reviewed & tested by the community» Needs review

Did anyone ever follow up on Damien's comment in #4?

We also need to make sure that AJAX requests are not overriding that.

I just tried it now and it seemed to be an issue. After an Ajax request takes place, code that runs inside Drupal.behaviors is getting "system/ajax" as the value of Drupal.settings.currentPath, which seems like it would be unexpected.

This might be an issue in the Drupal 8 code too, not sure.

Version:7.x-dev» 8.x-dev
Category:feature» bug
Priority:Normal» Critical
Status:Needs review» Active

#27: Yes, it is a problem in Drupal 8 too. Combined with the lazy loading of contextual links and in-place editing stuff in Drupal 8, it means that on almost all pages, drupalSettings.currentPath is no longer correct post-page load (because one or more AJAX requests have happened). We opened an issue for this at #2002786: drupalSettings gets updated with wrong path during ajax requests.

It is not inherently caused by this issue/patch, but by the too simplistic assumptions made by the server-side part of the AJAX framework. Or, put differently, this patch invalidated those assumptions. So in a sense, this issue *is* responsible.

We clearly need to resolve it, and there's no obvious solution right now. nod_ tried something in #2002786: drupalSettings gets updated with wrong path during ajax requests, but it's extremely weird/hacky. IMHO the root cause is that we are treating JS settings as another type of JS asset, whereas it really needs special treatment: it should be its own asset type.

Elevating this issue to a critical bug: we cannot ship Drupal 8 like this, it's a broken API.

IMHO the root cause is that we are treating JS settings as another type of JS asset, whereas it really needs special treatment: it should be its own asset type.

strongly seconded. this has become exceedingly clear to me in working on the Assetic-based refactoring of asset handling. JS settings are their own beast, and ought to be treated as such.

@Wim could you open a new issue for this, 'Add current path to drupal JS settings' isn't very descriptive.

#30: I agree that it isn't very descriptive; the problem is that this issue added "the current Drupal path" to Drupal's JS settings, and did so in a manner that it is broken by design: see #27 and #28. Hence David Rothstein didn't commit the backport of this to Drupal 7's git repo, and it is in fact a problem in the (committed) Drupal 8 implementation too. I think it makes sense to solve it in this issue — and probably to retitle this issue?

Title:Add current Drupal path to drupal JS settingsDrupal settings gets broken by AJAX requests

Here's an attempt at a re-title.

Status:Active» Needs review
StatusFileSize
new659 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,145 pass(es).
[ View ]

Putting the patch from #2002786: drupalSettings gets updated with wrong path during ajax requests that solves the issue.

When using the ajax framework, drupalSettings is updated (to update ajaxPageState mainly) but currently interferes with some values like currentPath.

The patch removes a few variables from the settings that are sent back to the browser to avoid overriding them. If we get #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links somewhere we'll only need to remove the path key from $settings and that'll work.

This is a valid (didn't say pretty) patch because ajax requests won't change a page URL. If you're thinking of PJAX, well there will be a custom ajax command for that anyway and that custom command can mess with drupalSettings.currentPath all it want. It's just that by default, path settings won't be updated.

Then I'll also copy my patch review from there:

Hrm… this indeed solves the problem for now. But at the same time, it is indeed also a mind-bogglingly bizarre/weird/hacky solution to the problem.

I'm fine with this being committed as an intermediate solution, but only if a clear plan is laid out to solve this cleanly afterwards.

Looking at it right now I don't think it's that bad a solution. And I don't really see how we can make it 'clean' either.

Status:Needs review» Needs work

Considering that #1941288: Regression: ajaxPageState not being updated with AjaxResponse, assets (js/css) being added twice is going to commit a similar hack, I guess we can do this at least for now, to fix a critical.

We should have test coverage for this. It does enter tricky territory for SimpleTest, but I *think* it should be possible to write a test for it. So NW just for that.

Status:Needs work» Needs review
StatusFileSize
new2.37 KB
FAILED: [[SimpleTest]]: [MySQL] 38,485 pass(es), 799 fail(s), and 420 exception(s).
[ View ]
new1.73 KB
FAILED: [[SimpleTest]]: [MySQL] 57,924 pass(es), 1 fail(s), and 4 exception(s).
[ View ]

Not a test wizard. One fail, the other doesn't. Feel free to improve on it, that's all I can do here :)

Status:Needs review» Needs work

+++ b/core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.module
@@ -534,6 +534,7 @@ function ajax_forms_test_validation_number_form_callback($form, $form_state) {
+  drupal_add_js(array('test' => 'currentPathUpdate'), 'setting');

Let's #attach it, and not add more drupal_add_js() calls? :)

Test may not be super elegant, but none of the WebTest-based JS tests are. So that's fine!

Status:Needs work» Needs review
StatusFileSize
new2.43 KB
PASSED: [[SimpleTest]]: [MySQL] 58,005 pass(es).
[ View ]

StatusFileSize
new2.57 KB
PASSED: [[SimpleTest]]: [MySQL] 57,656 pass(es).
[ View ]

add comment in the form.

Status:Needs review» Needs work

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -133,6 +133,9 @@ protected function ajaxRender(Request $request) {
+      foreach (array('basePath', 'currentPath', 'scriptPath', 'pathPrefix') as $item) {

I think we should have a comment for this too. Something like: "This is an Ajax response, hence the page that is requesting it must already have the original page's drupalSettings. We exclude the basic, path-specific settings in drupalSettings. If we don't do this, then the original page's path-specific settings will be overridden, making the page's JavaScript think it lives at an Ajax response path."

+++ b/core/modules/system/tests/modules/ajax_forms_test/ajax_forms_test.module
@@ -534,6 +534,12 @@ function ajax_forms_test_validation_number_form_callback($form, $form_state) {
+  // Adds JS settings to check that upon loading the form with AJAX
+  // drupalSettings.currentPath value is not updated with the AJAX path.

Hrm I don't think it's very clear. What about this: "We attach a JavaScript setting, so that one of the generated AJAX commands will be the settings command. We can then check the settings command to ensure that currentPath is not being output again in the Ajax response."

Status:Needs work» Needs review
StatusFileSize
new3 KB
PASSED: [[SimpleTest]]: [MySQL] 57,973 pass(es).
[ View ]

added comments, changed the wording a bit.

@@ -133,6 +133,14 @@ protected function ajaxRender(Request $request) {
+      // override the page's values.
+      foreach (array('basePath', 'currentPath', 'scriptPath', 'pathPrefix') as $item) {

As said on IRC yesterday: A reference to drupal_get_js would be nice, as there these parameters are defined.

@@ -185,6 +185,18 @@ function testLazyLoad() {
+  function testCurrentPathChange() {

Should be public function ...

StatusFileSize
new4.47 KB
PASSED: [[SimpleTest]]: [MySQL] 57,899 pass(es).
[ View ]

added @see drupal_add_js

I added public to all the methods in the class otherwise it'd look stupid to have only one properly declared.

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -133,6 +133,15 @@ protected function ajaxRender(Request $request) {
+      foreach (array('basePath', 'currentPath', 'scriptPath', 'pathPrefix') as $item) {
+        unset($settings[$item]);

Possibly we have to provide variable for this? Some contribs may require to avoid changing their data on ajax for Drupal.settings too.

We have to hardcode that because it's hardcoded in drupal_add_js too. Module developers should do that another way, they can check the request object like what was done before, and not update their js settings if they don't want them overridden.

Status:Needs review» Reviewed & tested by the community

@nod_
In general please try to make the life of the reviewers/commiters easy and change as less as possible ....

+      foreach (array('basePath', 'currentPath', 'scriptPath', 'pathPrefix') as $item) {
+        unset($settings[$item]);
+      }
       $this->addCommand(new SettingsCommand($settings, TRUE), TRUE);

Does it make sense to keep the data under a different key in the response rather than simply dumping it?

Status:Reviewed & tested by the community» Needs review

Looks like this is still in discussion.

those values are taken from the current request, if you're making an ajax request, you need to know the values to even do the request in the first place, you'd be getting the same values.

So no I don't feel we should add them somewhere. The only use case I can think of is pjax, and that was pointed out in the other issue and in #33.

Status:Needs review» Reviewed & tested by the community

re: #50, nod_ and I discussed whether or not to keep the request path data in the response object. He's convinced me that the information is available on the JS side and can be retained and passed around there without needing to retrieve it from the response, which really, just adds weight to the response.

I'm good with this patch.

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

Hmm this isn't my favourite patch but I don't see an obvious alternative, especially given this is broken in Drupal 7 as well. Committed/pushed to 8.x, thanks!

#53: indeed! :/ (I said the same in #34 essentially.)

Until we have a nice JS settings system (and that won't be until D9), this is what we'll have to settle for.

I had to make this commit to Persona to fix the problem:
http://drupalcode.org/project/persona.git/commitdiff/1b83761

Issue summary:View changes

Updated my make file to update to Drupal 7.26 but the previous patch i had (#24) doesn't apply anymore. All the new ones seem to be to Drupal 8. Any help regarding Drupal 7 ?