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:

  1. Set up the multi-language site by adding the additional language.
  2. Enable the glossary view.
  3. Create some dummy articles.
  4. 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

https://www.drupal.org/node/1803758
https://www.drupal.org/node/2600642

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

D4K0 created an issue. See original summary.

D4K0’s picture

Attached patch checks for the existence of drupalSettings.path.pathPrefix and prepends it to viewPath.

D4K0’s picture

Status: Active » Needs review
D4K0’s picture

Issue tags: -i18n views +VDC

Issue tag changed to VDC.
https://www.drupal.org/issue-tags/topic

Views - Involves, uses, or integrates with views. In Drupal 8 core, use the VDC tag instead.

D4K0’s picture

Issue tags: +JavaScript

Just tagging to be more specific. Needs review.

D4K0’s picture

Issue tags: +rc target

Tagging once more.
Should be fixed before the final release, IMHO.
Considering the ajax glossary view for the extensively used feature.

droplet’s picture

Issue tags: +D8MI, +multilingual

the patch sorted the problem but I'm not sure if it's right way to handle multilingual (in Core / Views)

+++ b/core/modules/views/js/base.js
@@ -51,6 +51,10 @@
+      var viewPath = drupalSettings.path.pathPrefix + viewPath;

no var. it's passed from parameter.

dawehner’s picture

+++ b/core/modules/views/js/base.js
@@ -51,6 +51,10 @@
+    if (drupalSettings.path.pathPrefix) {

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

D4K0’s picture

You'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

Drupal.Views.getPath = function (href) {
  href = Drupal.Views.pathPortion(href);
- href = href.substring(drupalSettings.path.baseUrl.length, href.length);
+ href = href.substring(drupalSettings.path.baseUrl.length + drupalSettings.path.pathPrefix.length, href.length);

or Drupal.Views.parseViewArgs

  Drupal.Views.parseViewArgs = function (href, viewPath) {
    var returnObj = {};
    var path = Drupal.Views.getPath(href);
+ // Ensure we strip the language path prefix.
+ if ( path.indexOf(drupalSettings.path.pathPrefix) > -1 ) {
+   path  = path.substring(drupalSettings.path.pathPrefix.length, path.length);
+ }

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?

D4K0’s picture

I 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 #8

D4K0’s picture

Issue summary: View changes
D4K0’s picture

Summary updated.

dawehner’s picture

+++ b/core/modules/views/js/base.js
@@ -53,6 +53,7 @@
+    viewPath = Drupal.url(viewPath).substring(1);

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

D4K0’s picture

+++ b/core/modules/views/js/base.js
@@ -53,6 +53,7 @@
+    viewPath = Drupal.url(viewPath).substring(1);

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Good catch!

+++ b/core/modules/views/js/base.js
@@ -53,9 +53,11 @@
+    // Get viewPath url without baseUrl portion.
+    var viewHref = Drupal.url(viewPath).substring(drupalSettings.path.baseUrl.length);

If we would just have javascript testing! :)

The last submitted patch, 2: language_prefix_for_ajax-2600804-2.patch, failed testing.

The last submitted patch, 10: drupal-language_prefix_for_ajax-2600804-10.patch, failed testing.

xjm’s picture

Issue tags: -rc target

If 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!

D4K0’s picture

Thx for RTBC! Thx to dawehner&droplet. Both of them deserves credits too!

D4K0’s picture

Issue summary: View changes
Issue tags: +D8 patch release target

Summary updated.
'D8 patch release target' issue tag added.

alexpott’s picture

Have we tested this language prefix and language domain negotiation? Since in drupalSettings we split out baseUrl and the language prefix?

D4K0’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

We have not.
Status changed to 'Needs work', the summary updated.

D4K0’s picture

Issue summary: View changes
FileSize
108.66 KB

I think we have. Check out my language settings' screenshot. Is that what you mean?

dawehner’s picture

@D4K0 So are you sure this issue is tested properly?

D4K0’s picture

@dawehner Not quite yet. I'd rather go through the details once more. Things might have changed a lot in last 3month...

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue tags: -D8 patch release target +D8 major triage deferred, +Needs manual testing

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

Lendude’s picture

Issue tags: +JavaScriptTest

Well lets see if we can write a javascript test for this then

Lendude’s picture

Status: Needs work » Needs review
FileSize
4.61 KB

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

Status: Needs review » Needs work

The last submitted patch, 29: language_prefix_for_ajax-2600804-29.patch, failed testing.

Lendude’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
1.09 KB
4.27 KB

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

dawehner’s picture

Seems the move to Drupal.url stopped fixing the problem.

Mh, do you understand why and well, if not, do you think we should understand that first, before committing the fix?

Lendude’s picture

Issue tags: +DevDaysMilan

do you understand why

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

We have a test for #29 both for the language and non language test. This is sooo nice!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/views/js/base.js
@@ -53,9 +53,11 @@
   Drupal.Views.parseViewArgs = function (href, viewPath) {
     var returnObj = {};
     var path = Drupal.Views.getPath(href);
+    // Get viewPath url without baseUrl portion.
+    var viewHref = Drupal.url(viewPath).substring(drupalSettings.path.baseUrl.length);
     // Ensure we have a correct path.
-    if (viewPath && path.substring(0, viewPath.length + 1) === viewPath + '/') {
-      returnObj.view_args = decodeURIComponent(path.substring(viewPath.length + 1, path.length));
+    if (viewHref && path.substring(0, viewHref.length + 1) === viewHref + '/') {
+      returnObj.view_args = decodeURIComponent(path.substring(viewHref.length + 1, path.length));
       returnObj.view_path = path;
     }

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

dawehner’s picture

Well, 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?

alexpott’s picture

@dawehner - yep that's exactly what I was trying to suggest :)

Lendude’s picture

Status: Needs work » Needs review
FileSize
19.87 KB
17.62 KB

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

criscom’s picture

The patch in #38 fixed the problem for me. Thanks, Lendude!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I just love how @Lendude writes more and more tests

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: language_prefix_for_ajax-2600804-38.patch, failed testing.

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

Unrelated fails

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: language_prefix_for_ajax-2600804-38.patch, failed testing.

Lendude’s picture

Status: Needs work » Reviewed & tested by the community

And some more unrelated fails

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

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

diff --git a/core/modules/views/tests/src/FunctionalJavascript/GlossaryViewTest.php b/core/modules/views/tests/src/FunctionalJavascript/GlossaryViewTest.php
index 2e47b3f..42da7b6 100644
--- a/core/modules/views/tests/src/FunctionalJavascript/GlossaryViewTest.php
+++ b/core/modules/views/tests/src/FunctionalJavascript/GlossaryViewTest.php
@@ -8,7 +8,6 @@
 use Drupal\simpletest\ContentTypeCreationTrait;
 use Drupal\simpletest\NodeCreationTrait;
 use Drupal\views\Tests\ViewTestData;
-use Drupal\views\Views;
 
 /**
  * Tests the basic AJAX functionality of the Glossary View.

Fixed on commit.

  • alexpott committed 2fb6246 on 8.3.x
    Issue #2600804 by Lendude, D4K0, dawehner, droplet: AJAXified glossary...
alexpott’s picture

Status: Patch (to be ported) » Fixed

Committed 16e3391 and pushed to 8.2.x. Thanks!

  • alexpott committed 16e3391 on 8.2.x
    Issue #2600804 by Lendude, D4K0, dawehner, droplet: AJAXified glossary...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

zaporylie’s picture

I 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