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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

effulgentsia’s picture

Issue tags: +Novice

Sounds good to me.

Damien Tournoud’s picture

Sounds good to me too.

pwolanin’s picture

Status: Active » Needs review
FileSize
511 bytes

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.

Damien Tournoud’s picture

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.

pwolanin’s picture

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

nod_’s picture

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

nod_’s picture

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

pwolanin’s picture

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

pwolanin’s picture

Status: Needs work » Needs review
FileSize
1.55 KB

makes the test more robust

nod_’s picture

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

looks good to me too

tim.plunkett’s picture

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()

pwolanin’s picture

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.

tim.plunkett’s picture

Issue tags: +Needs tests
tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.86 KB
1.36 KB

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

pwolanin’s picture

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

pwolanin’s picture

Added more tests

pwolanin’s picture

remove more t() calls.

pwolanin’s picture

oops, wrong patch

pwolanin’s picture

minor tweak to the test path.

tim.plunkett’s picture

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!

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

pwolanin’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)
tim.plunkett’s picture

Status: Patch (to be ported) » Needs review
FileSize
3.84 KB
3.36 KB

Here's a stab at a backport.

jbrown’s picture

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

jbrown’s picture

Status: Needs review » Reviewed & tested by the community

This is fine!

David_Rothstein’s picture

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.

Wim Leers’s picture

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.

sdboyer’s picture

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.

catch’s picture

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

Wim Leers’s picture

#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?

catch’s picture

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

Here's an attempt at a re-title.

nod_’s picture

Status: Active » Needs review
FileSize
659 bytes

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.

Wim Leers’s picture

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.

nod_’s picture

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.

Wim Leers’s picture

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.

nod_’s picture

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

Wim Leers’s picture

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!

nod_’s picture

Status: Needs work » Needs review
FileSize
2.43 KB
nod_’s picture

add comment in the form.

Wim Leers’s picture

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."

nod_’s picture

Status: Needs work » Needs review
FileSize
3 KB

added comments, changed the wording a bit.

dawehner’s picture

@@ -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 ...

nod_’s picture

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.

Spleshka’s picture

+++ 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.

nod_’s picture

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.

dawehner’s picture

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 ....

jessebeach’s picture

+      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?

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Looks like this is still in discussion.

nod_’s picture

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.

jessebeach’s picture

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.

webchick’s picture

catch’s picture

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!

Wim Leers’s picture

#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.

jbrown’s picture

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

brunocampos’s picture

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 ?

cilefen’s picture

Issue tags: -Novice

The only Drupal 7 patch so far was #24. It will be challenging to backport because there were actually two Drupal 8 commits: #22 and #53, so there were changes since the #24 patch, and the #24 may be incomplete according to its comment, and two years have passed.

hgoto’s picture

Assigned: Unassigned » hgoto

I'll tackle this.

hgoto’s picture

I create an updated patch for D7.

As cilefen told, there were discussion after #24 and I agree that we need to backport 2 commits #22 and #53.

In a patch for D7, we should do the following things.

  1. Introduce a value currentPath in Drupal.settings.
  2. Prevent overwriting the path-related values in Drupal.settings when an Ajax response occurs. Concretely there are 3 variables to keep: basePath, pathPrefix and currentPath. (scriptPath doesn't exist in D7.)
  3. Add tests to confirm the above 2 changes work properly.

In my understanding, this patch covers all of the above. I'd like someone to review this.

(fixed some typo and poor description)

hgoto’s picture

Assigned: hgoto » Unassigned
Status: Patch (to be ported) » Needs review

The last submitted patch, 59: drupal-1691394-59-d7-combined.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 59: drupal-1691394-59-d7-test_only_should_fail.patch, failed testing.

hgoto’s picture

There was a test which failed.

5	1	AJAX.AJAXFrameworkTestCase
✓		- setUp
✗	
testAJAXRender
fail: [Other] Line 87 of modules/simpletest/tests/ajax.test:
ajax_render() loads settings added with drupal_add_js().

This is caused because the patch #59 prevents overwriting the path-related values in Drupal.settings when an Ajax response occurs.

I fixed the point and try again.

Status: Needs review » Needs work

The last submitted patch, 63: drupal-1691394-63-d7-test_only_should_fail.patch, failed testing.

hgoto’s picture

Status: Needs work » Needs review

The second patch in comment #63 failed the test but this is an expected result. I change the status to NR again.

  • Dries committed 7587186 on 8.3.x
    - Patch #1691394 by pwolanin, tim.plunkett: added current Drupal path to...
  • catch committed 7ed9614 on 8.3.x
    Issue #1691394 by pwolanin, nod_, tim.plunkett: Fixed Drupal settings...

  • Dries committed 7587186 on 8.3.x
    - Patch #1691394 by pwolanin, tim.plunkett: added current Drupal path to...
  • catch committed 7ed9614 on 8.3.x
    Issue #1691394 by pwolanin, nod_, tim.plunkett: Fixed Drupal settings...
stefan.r’s picture

Assigned: Unassigned » Fabianx
Issue tags: -Needs backport to D7

  • Dries committed 7587186 on 8.4.x
    - Patch #1691394 by pwolanin, tim.plunkett: added current Drupal path to...
  • catch committed 7ed9614 on 8.4.x
    Issue #1691394 by pwolanin, nod_, tim.plunkett: Fixed Drupal settings...

  • Dries committed 7587186 on 8.4.x
    - Patch #1691394 by pwolanin, tim.plunkett: added current Drupal path to...
  • catch committed 7ed9614 on 8.4.x
    Issue #1691394 by pwolanin, nod_, tim.plunkett: Fixed Drupal settings...

  • Dries committed 7587186 on 9.1.x
    - Patch #1691394 by pwolanin, tim.plunkett: added current Drupal path to...
  • catch committed 7ed9614 on 9.1.x
    Issue #1691394 by pwolanin, nod_, tim.plunkett: Fixed Drupal settings...
stephencamilo’s picture

Status: Needs review » Closed (won't fix)
hestenet’s picture

Status: Closed (won't fix) » Needs review

Reset issue status.