Problem/Motivation

Path prefix (eg. /en) is removed from incoming urls even when Part of the URL that determines language is set to Domain. To reproduce:

  1. Add a language (eg. German) so there are at least two.
  2. Visit /admin/config/regional/language/detection/url and set path prefixes. Then change Part of the URL that determines language to Domain and set the domains (eg. default (current) domain for English and some other domain for German).
  3. Add a node with alias looking like it's prefixed with langcode (eg. /en/test).
  4. Node page returns 404 because /en is removed from path by LanguageNegotiationUrl::processInbound.

Proposed resolution

Remove path prefix from the url only when Part of the URL that determines language is set to Path prefix.

Remaining tasks

None

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#14 interdiff.txt1.05 KBblazey
#14 2855977-14.patch4.38 KBblazey
#11 2855977-11.patch4.16 KBblazey
#5 interdiff.txt590 bytesblazey
#5 2855977-5.patch4.17 KBblazey
#2 2855977-2.patch3.6 KBblazey
#2 2855977-2-test-only.patch2.28 KBblazey

Comments

blazey created an issue. See original summary.

blazey’s picture

blazey commented
Assigned: blazey » Unassigned
Status: Active » Needs review
StatusFileSize
new 2855977-2-test-only.patch2.28 KB
new 2855977-2.patch3.6 KB

Attaching browser test and the fix.

The last submitted patch, 2: 2855977-2-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: 2855977-2.patch, failed testing.

blazey’s picture

blazey commented
StatusFileSize
new 2855977-5.patch4.17 KB
new interdiff.txt590 bytes

PathProcessorTest was not setting all required config items.

blazey’s picture

blazey commented
Status: Needs work » Needs review
leksat’s picture

leksat commented

@blazey,

+++ b/core/tests/Drupal/Tests/Core/PathProcessor/PathProcessorTest.php
@@ -120,6 +120,7 @@ function testProcessInbound() {
         'language.negotiation' => array(
           'url' => array(
             'prefixes' => array('fr' => 'fr'),
+            'source' => LanguageNegotiationUrl::CONFIG_PATH_PREFIX,

What this change does? Does it make PathProcessorTest fail? Should it go to the test-only patch then?

blazey’s picture

blazey commented

@leksat, this line actually makes PathProcessorTest pass. It didn't set all the required config items.

leksat’s picture

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

Ah, right. Thanks!

The code looks good and there is a test. RTBC!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2855977-5.patch, failed testing.

blazey’s picture

blazey commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2855977-11.patch4.16 KB

Rerolling.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

As far as I can see from D7's locale_language_from_url() this patch is correct. In Drupal 7 the path was only stripped if LOCALE_LANGUAGE_NEGOTIATION_URL_PREFIX and not LOCALE_LANGUAGE_NEGOTIATION_URL_DOMAIN. So we must have broken this when we converted language negotiation to the new routing system. Going to ask a language subsystem maintainer for a +1.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I asked @GaborHojtsy in IRC and he agreed that this is a bug and that the fix looks correct. Nice find.

+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationUrl.php
@@ -103,12 +103,14 @@ public function processInbound($path, Request $request) {
     $parts = explode('/', trim($path, '/'));
     $prefix = array_shift($parts);

There's no need to do the explode or array_shift if the source is not LanguageNegotiationUrl::CONFIG_PATH_PREFIX. A small optimisation for sure but as we're adding the if here this seems in scope.

blazey’s picture

blazey commented
StatusFileSize
new 2855977-14.patch4.38 KB
new interdiff.txt1.05 KB

Thanks for review. Attaching updated patch.

blazey’s picture

blazey commented
Status: Needs work » Needs review
leksat’s picture

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

The last change is tiny. Back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed b26016d to 8.4.x and 4bd7b24 to 8.3.x. Thanks!

  • alexpott committed b26016d on 8.4.x 
    Issue #2855977 by blazey: LanguageNegotiationUrl::processInbound removes...

  • alexpott committed 4bd7b24 on 8.3.x 
    Issue #2855977 by blazey: LanguageNegotiationUrl::processInbound removes...

Status: Fixed » Closed (fixed)

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