Should we expect that in drupalSettings.path.pathPrefix we will have the language code, in case we have a multilingual installation with URL language detection enabled, and we are on a page with a language prefix URL?

If so, apparently we have currently an empty value (if I did not make mistakes :))

CommentFileSizeAuthor
#20 interdiff_14-20.txt1.1 KBmondrake
#20 drupal-2146035-20.patch4.12 KBmondrake
#16 drupal-2146035-14.patch4.05 KBolli
#14 interdiff-10.txt3.51 KBolli
#14 drupal-2146035-14.patch4.18 KBolli
#13 drupal-2146035-13.patch3.72 KBolli
#10 drupal-2146035-10.patch3 KBmondrake
#10 drupal-2146035-10-test-only.patch738 bytesmondrake
#4 interdiff.txt471 bytesolli
#4 drupal-2146035-4.patch2.28 KBolli
#2 drupal-2146035.patch1.82 KBolli
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Good question, I don't know :o

olli’s picture

olli CreditAttribution: olli commented
Category: Support request » Bug report
Status: Active » Needs review
Issue tags: +D8MI
FileSize
drupal-2146035.patch1.82 KB
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Issue summary: View changes
Status: Needs review » Needs work

Confirm that #2 solves the issue, the pathPrefix is now filled in in the context described by the OP.

However, I think it would make sense to adjust the Drupal.js url() function accordingly, so that UrlGenerator::generateFromPath() on the server side and Drupal.url() on the client side provide same behaviour:

@@ -323,11 +323,11 @@ if (window.jQuery) {
 
   /**
    * Returns the URL to a Drupal page.
    */
   Drupal.url = function (path) {
-    return drupalSettings.path.basePath + drupalSettings.path.scriptPath + path;
+    return drupalSettings.path.basePath + drupalSettings.path.scriptPath + drupalSettings.path.pathPrefix + path;
   };
 
   /**
    * Format a string containing a count of items.
    *
olli’s picture

olli CreditAttribution: olli commented
Issue tags: +Needs tests, +Needs manual testing
FileSize
drupal-2146035-4.patch2.28 KB
interdiff.txt471 bytes

This needs testing.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review

Let's see testbot

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
+++ b/core/lib/Drupal/Core/Routing/UrlGenerator.php
@@ -307,9 +307,8 @@ public function generateFromPath($path = NULL, $options = array()) {

I wonder whether we also have to adapt the generateFromRoute function.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented

Please also consider any effect on Toolbar JS code that this change might produce. See #2218313: Toolbar subtree is broken if you use a different language..

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Issue tags: +JavaScript

As for manual testing, I tested patch in #4 with the contrib Pagerer module I am developing/maintaining.

Pagerer uses Drupal.url() to build dynamically pager links on the client-side. In fact, the OP is due to the fact that working on its conversion to D8, I came across the issue. It is visible on the page available at path pagerer/example, with the Pagerer example module installed. Pager links created on the server-side are correct, but those created dynamically (in slider/scrollpane/mini pager styles) do not bear the language prefix, hence paging to pages with default site language. The patch in #4 fixes the problem.

Leaving to NR for any further testing.

olli’s picture

olli CreditAttribution: olli commented

Re #6: How?

Re #7: With this code, toolbar links have the correct prefix:
1. Install standard + language
2. Add Dutch language
3. Goto /nl, expand "Reports", click "Status report"
You should be at /nl/admin/reports/status

Without this patch the js loads from /toolbar/subtrees/hash/nl, with this code it loads /nl/toolbar/subtrees/hash/nl.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Title: drupalSettings.pathPrefix does not bear language identifier » drupalSettings.path.pathPrefix does not bear language identifier
Issue tags: -Needs tests, -Needs manual testing
FileSize
drupal-2146035-10-test-only.patch738 bytes
drupal-2146035-10.patch3 KB

As #4 with a test to check that drupalSettings.path.pathPrefix is filled in with the language code. Test only should fail, the other pass. Interdiff == test only. I doubt we can do testing on the js side as of now, but manual testing in #8 and #9 covers enough?

The last submitted patch, 10: drupal-2146035-10-test-only.patch, failed testing.

jessebeach’s picture

jessebeach CreditAttribution: jessebeach commented 
- $path = str_replace('%2F', '/', rawurlencode($prefix . $path));

We had run the $prefix through rawurlencode, but the raw prefix is passed to configuration now. Did this just open an attack vector?

olli’s picture

olli CreditAttribution: olli commented
FileSize
drupal-2146035-13.patch3.72 KB

This adapts generateFromRoute instead.

olli’s picture

olli CreditAttribution: olli commented
FileSize
drupal-2146035-14.patch4.18 KB
interdiff-10.txt3.51 KB

... with the change in drupal.js and an interdiff. (ignore #13)

olli’s picture

olli CreditAttribution: olli commented

14: drupal-2146035-14.patch queued for re-testing.

olli’s picture

olli CreditAttribution: olli commented
Component: javascript » language.module
FileSize
drupal-2146035-14.patch4.05 KB

Reroll for psr-4.

olli queued 16: drupal-2146035-14.patch for re-testing.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs review » Reviewed & tested by the community

This fixes OP, the issue reported in #8, has automated tests for the backend, and was manually tested on the frontend in #8 and #9. I tested manually again now and it still applies and works as expected. RTBC

chx’s picture

chx CreditAttribution: chx commented

I only have one question -- do we really need a complete test for this? Isn't it possible to just add that assert ... somewhere?

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Reviewed & tested by the community » Needs review
FileSize
drupal-2146035-20.patch4.12 KB
interdiff_14-20.txt1.1 KB

Re #19, moved that test code to the setUp() method. I tried to move it to checkUrl() but that does not seem to bring up drupalSettings. The other methods seem to have very specific scope so I have not touched. If we don't like this we still have #16.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

Lovely. Compared to #16 we are running an extra drupalGet at the cost of saving a Drupal install (there are two test methods). That's a big win as far as I am concerned. If the esteemed core committers disagree, well, #16 is good to go too.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Title: drupalSettings.path.pathPrefix does not bear language identifier » drupalSettings.path.pathPrefix does not contain the language identifier
Status: Reviewed & tested by the community » Fixed

Looks good - nice find.

Committed 22ca7bc and pushed to 8.x. Thanks!

  • alexpott committed 22ca7bc on 8.x 
    Issue #2146035 by olli, mondrake: Fixed drupalSettings.path.pathPrefix...

Status: Fixed » Closed (fixed)

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