Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comment | File | Size | Author |
---|---|---|---|
#63 | interdiff-1691394-59-63.txt | 504 bytes | hgoto |
#63 | drupal-1691394-63-d7-test_only_should_fail.patch | 5.52 KB | hgoto |
#63 | drupal-1691394-63-d7-combined.patch | 7.15 KB | hgoto |
#44 | core-ajax-currentPath-update-1691394-44.patch | 4.47 KB | nod_ |
#42 | core-ajax-currentPath-update-1691394-42.patch | 3 KB | nod_ |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedSounds good to me.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedSounds good to me too.
Comment #3
pwolanin CreditAttribution: pwolanin commentedHere'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.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe also need to make sure that AJAX requests are not overriding that.
Comment #6
pwolanin CreditAttribution: pwolanin commentedTest fails look unrelated to the patch at first glance - any idea?
Comment #7
nod_related. the tests hardcode the array index things are supposed to be. the new info offseted things.
Comment #8
nod_related. the tests hardcode the array index things are supposed to be. the new info offseted things.
Comment #9
pwolanin CreditAttribution: pwolanin commentedWow, that seems rather too fragile. ok, so does need work.
Comment #10
pwolanin CreditAttribution: pwolanin commentedmakes the test more robust
Comment #11
nod_looks good to me too
Comment #12
tim.plunkettThis 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()
Comment #13
pwolanin CreditAttribution: pwolanin commentedThe 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.
Comment #14
tim.plunkettComment #15
tim.plunkettAdded a test and removed the calls to t().
Comment #16
pwolanin CreditAttribution: pwolanin commentedGood start, but should we have a test that verifies it's in the page source?
Comment #17
pwolanin CreditAttribution: pwolanin commentedAdded more tests
Comment #18
pwolanin CreditAttribution: pwolanin commentedremove more t() calls.
Comment #19
pwolanin CreditAttribution: pwolanin commentedoops, wrong patch
Comment #20
pwolanin CreditAttribution: pwolanin commentedminor tweak to the test path.
Comment #21
tim.plunkettThat is some might ugly escaping in the regex, but I don't think it can be helped.
Looks good overall!
Comment #22
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks.
Comment #23
pwolanin CreditAttribution: pwolanin commentedComment #24
tim.plunkettHere's a stab at a backport.
Comment #25
jbrown CreditAttribution: jbrown commented#24: drupal-1691394-24-combined.patch queued for re-testing.
Comment #26
jbrown CreditAttribution: jbrown commentedThis is fine!
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedDid anyone ever follow up on Damien's comment in #4?
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.
Comment #28
Wim Leers#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.
Comment #29
sdboyer CreditAttribution: sdboyer commentedstrongly 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.
Comment #30
catch@Wim could you open a new issue for this, 'Add current path to drupal JS settings' isn't very descriptive.
Comment #31
Wim Leers#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?
Comment #32
catchHere's an attempt at a re-title.
Comment #33
nod_Putting the patch from #2002786: drupalSettings gets updated with wrong path during ajax requests that solves the issue.
Comment #34
Wim LeersThen I'll also copy my patch review from there:
Comment #35
nod_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.
Comment #36
Wim LeersConsidering 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.
Comment #37
nod_Not a test wizard. One fail, the other doesn't. Feel free to improve on it, that's all I can do here :)
Comment #38
Wim LeersLet'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!
Comment #39
nod_Comment #40
nod_add comment in the form.
Comment #41
Wim LeersI 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."
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."
Comment #42
nod_added comments, changed the wording a bit.
Comment #43
dawehnerAs said on IRC yesterday: A reference to drupal_get_js would be nice, as there these parameters are defined.
Should be public function ...
Comment #44
nod_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.
Comment #45
SpleshkaPossibly we have to provide variable for this? Some contribs may require to avoid changing their data on ajax for Drupal.settings too.
Comment #46
nod_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.
Comment #47
dawehner@nod_
In general please try to make the life of the reviewers/commiters easy and change as less as possible ....
Comment #48
jessebeach CreditAttribution: jessebeach commentedDoes it make sense to keep the data under a different key in the response rather than simply dumping it?
Comment #49
webchickLooks like this is still in discussion.
Comment #50
nod_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.
Comment #51
jessebeach CreditAttribution: jessebeach commentedre: #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.
Comment #52
webchick#44: core-ajax-currentPath-update-1691394-44.patch queued for re-testing.
Comment #53
catchHmm 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!
Comment #54
Wim Leers#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.
Comment #55
jbrown CreditAttribution: jbrown commentedI had to make this commit to Persona to fix the problem:
http://drupalcode.org/project/persona.git/commitdiff/1b83761
Comment #56
brunocampos CreditAttribution: brunocampos commentedUpdated 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 ?
Comment #57
cilefen CreditAttribution: cilefen commentedThe 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.
Comment #58
hgoto CreditAttribution: hgoto as a volunteer commentedI'll tackle this.
Comment #59
hgoto CreditAttribution: hgoto as a volunteer commentedI 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.
currentPath
inDrupal.settings
.Drupal.settings
when an Ajax response occurs. Concretely there are 3 variables to keep:basePath
,pathPrefix
andcurrentPath
. (scriptPath
doesn't exist in D7.)In my understanding, this patch covers all of the above. I'd like someone to review this.
(fixed some typo and poor description)
Comment #60
hgoto CreditAttribution: hgoto as a volunteer commentedComment #63
hgoto CreditAttribution: hgoto as a volunteer commentedThere was a test which failed.
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.
Comment #65
hgoto CreditAttribution: hgoto as a volunteer commentedThe second patch in comment #63 failed the test but this is an expected result. I change the status to NR again.
Comment #68
stefan.r CreditAttribution: stefan.r commentedComment #72
stephencamilo CreditAttribution: stephencamilo as a volunteer commentedComment #73
hestenetReset issue status.