Problem/Motivation
AJAX filtering of the glossary view does not work on multi-language sites.
Parsing the path for the view arguments fails when the language pathPrefix set.
Bug ported from D7 @see https://www.drupal.org/node/1803758
Reproduction steps:
- Set up the multi-language site by adding the additional language.
- Enable the glossary view.
- Create some dummy articles.
- Filter the glossary view by the title's first character link.
Priority:
Considered major priority as it breaks the multi-language site Views functionality.
Proposed resolution
Add pathPrefix to the view path by the help of Drupal.url
Remaining tasks
none
Related Issues
https://www.drupal.org/node/1803758
https://www.drupal.org/node/2600642
Comment | File | Size | Author |
---|---|---|---|
#38 | language_prefix_for_ajax-2600804-38.patch | 17.62 KB | Lendude |
#38 | interdiff-2600804-29-38.txt | 19.87 KB | Lendude |
#31 | language_prefix_for_ajax-2600804-31.patch | 4.27 KB | Lendude |
#31 | interdiff-2600804-29-31.txt | 1.09 KB | Lendude |
#29 | language_prefix_for_ajax-2600804-29.patch | 4.61 KB | Lendude |
Comments
Comment #2
D4K0 CreditAttribution: D4K0 as a volunteer commentedAttached patch checks for the existence of drupalSettings.path.pathPrefix and prepends it to viewPath.
Comment #3
D4K0 CreditAttribution: D4K0 as a volunteer commentedComment #4
D4K0 CreditAttribution: D4K0 as a volunteer commentedIssue tag changed to VDC.
https://www.drupal.org/issue-tags/topic
Comment #5
D4K0 CreditAttribution: D4K0 as a volunteer commentedJust tagging to be more specific. Needs review.
Comment #6
D4K0 CreditAttribution: D4K0 as a volunteer commentedTagging once more.
Should be fixed before the final release, IMHO.
Considering the ajax glossary view for the extensively used feature.
Comment #7
droplet CreditAttribution: droplet commentedthe patch sorted the problem but I'm not sure if it's right way to handle multilingual (in Core / Views)
no var. it's passed from parameter.
Comment #8
dawehnerI'm curios whether we actually need the additional condition, isn't pathPrefix always set? I'm also curious whether we could use
Drupal.url
instead ...Comment #9
D4K0 CreditAttribution: D4K0 as a volunteer commentedYou're both right! Thx. I've made some more research on this topic.
Drupal.Views.parseViewArgs
function fails in parsing glossary links args as it expects the path without pathPrefix.It gets the path from
Drupal.Views.getPath
. The thing is it returns the path with pathPrefix(strips baseUrl only).That's what make the mess and breaks args parsing.
Stripping pathPrefix in
Drupal.Views.getPath
or
Drupal.Views.parseViewArgs
sorts things out keeping the args parsing code untouched.
AJAX glossary view filtering, sorting and paging then work as designed.
I wonder the place to fix it.
Drupal.Views.getPath
says @return {string} An internal path.Does pathPrefix counts as the regular part of the internal path?
Comment #10
D4K0 CreditAttribution: D4K0 as a volunteer commentedI think pathPrefix counts as the regular part of the internal path as I can see hrefs of generated links include prefixPath.
So here come the new patch using
Drupal.url
mentioned by dawehner at #8Comment #11
D4K0 CreditAttribution: D4K0 as a volunteer commentedComment #12
D4K0 CreditAttribution: D4K0 as a volunteer commentedSummary updated.
Comment #13
dawehnerIt would be great to document why we need to substring (1) ... I think. In an ideal world I would also suggest to give that new path a different
variable name and change the usages below.
Comment #14
D4K0 CreditAttribution: D4K0 as a volunteer commentedsubstring(1) seems not be correct as we need to strip the whole baseUrl.
So here come's the patch that fix it, including recommendations mentioned by dawehner #13
Comment #15
dawehnerGood catch!
If we would just have javascript testing! :)
Comment #18
xjmIf only we had JS testing indeed. :) That said, this looks like it is probably good to go into a patch release.
Aside: note that "rc target" should only be added by committers in general (but now we are out of the RC phase so it does not matter in any case).
Thanks for the patch!
Comment #19
D4K0 CreditAttribution: D4K0 as a volunteer commentedThx for RTBC! Thx to dawehner&droplet. Both of them deserves credits too!
Comment #20
D4K0 CreditAttribution: D4K0 as a volunteer commentedSummary updated.
'D8 patch release target' issue tag added.
Comment #21
alexpottHave we tested this language prefix and language domain negotiation? Since in drupalSettings we split out baseUrl and the language prefix?
Comment #22
D4K0 CreditAttribution: D4K0 as a volunteer commentedWe have not.
Status changed to 'Needs work', the summary updated.
Comment #23
D4K0 CreditAttribution: D4K0 as a volunteer commentedI think we have. Check out my language settings' screenshot. Is that what you mean?
Comment #24
dawehner@D4K0 So are you sure this issue is tested properly?
Comment #25
D4K0 CreditAttribution: D4K0 as a volunteer commented@dawehner Not quite yet. I'd rather go through the details once more. Things might have changed a lot in last 3month...
Comment #27
xjm(There's normally no "patch release" target tag; we just indicate that by filing it against the production branch.)
Does this happen to any AJAX view on a multilingual site, or only the glossary view? Deferring triage of this on that question.
Also, from #21 on it looks like there are a few specific things that should be tested manually. (FWIW I don't quite understand what #21 is asking either so maybe @alexpott can clarify.) It would be good to document the exact testing steps for the different cases (in addition to the ones in the summary) and what the results were.
As an aside, we do now have JavaScriptTestBase available to add tests for JS functionality and regressions, though there aren't a whole lot of examples yet.
Comment #28
LendudeWell lets see if we can write a javascript test for this then
Comment #29
LendudeThe patch needed a reroll and it needed javascript tests, so here we go.....
The reroll of the patch (if I got that right), doesn't solve the problem in the automated test nor does it solve it in my manual tests. So we need to look for a fix still.
Comment #31
LendudeThis is basically a rollback to an earlier version of this fix with some cleanup. Seems the move to Drupal.url stopped fixing the problem. Didn't bother looking into why (lazy, I know).
Passes locally and passes manual testing now.
Comment #32
dawehnerMh, do you understand why and well, if not, do you think we should understand that first, before committing the fix?
Comment #33
LendudeWell, I now had a little time to do a little digging, and the fix in #29 not working didn't make sense at all. This has to do with the fact that it's working fine.....
Passes tests locally, passes test manually, and the fails on d.o in #29 are completely unrelated....requeued the test for #29
So the fix not working properly was mostly due to my brain not working.
Ignore #31, review needed on #29.
Comment #34
dawehnerWe have a test for #29 both for the language and non language test. This is sooo nice!
Comment #35
alexpottSo this is used for way more than just the glossary view. The problem is that maybe we decide to remove the glossary view and suddenly test coverage is lost. Tricky - not sure what to do here. I think a simple mitigation is moving the test to views and using a test view that is a copy of the glossary view. Since this a views bug - not a node bug.
Comment #36
dawehnerWell, conceptually the glossary is a quite complex view and would be worth for itself.
What about copying the existing glossary into the test module and rename it and continue using it as it is?
Comment #37
alexpott@dawehner - yep that's exactly what I was trying to suggest :)
Comment #38
LendudeThere was already a view named test_glossary, so I just expanded that to include the whole glossary view. The only test I could find that was using the old test_glossary passed locally with the new one.
Since this is now a views test that just uses nodes, I moved this to the views namespace.
Comment #39
criscomThe patch in #38 fixed the problem for me. Thanks, Lendude!
Comment #40
dawehnerI just love how @Lendude writes more and more tests
Comment #42
LendudeUnrelated fails
Comment #44
LendudeAnd some more unrelated fails
Comment #46
alexpottCommitted 2fb6246 and pushed to 8.3.x. Thanks! Setting to patch to be ported for eventual cherry-pick to 8.2.x after 8.2.0 is released.
Fixed on commit.
Comment #48
alexpottCommitted 16e3391 and pushed to 8.2.x. Thanks!
Comment #51
zaporylieI know this is pretty old issue but I've create a follow-up: #2914677: Unused member variable in GlossaryViewTest - this is related to one of the coding standard issues: #2909366: Fix 'Drupal.Commenting.VariableComment.EmptyVar' coding standard