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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

blazey created an issue. See original summary.

blazey’s picture

Assigned: blazey » Unassigned
Status: Active » Needs review
FileSize
2.28 KB
3.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

FileSize
4.17 KB
590 bytes

PathProcessorTest was not setting all required config items.

blazey’s picture

Status: Needs work » Needs review
Leksat’s picture

@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

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

Leksat’s picture

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

Status: Needs work » Reviewed & tested by the community
FileSize
4.16 KB

Rerolling.

alexpott’s picture

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

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

FileSize
4.38 KB
1.05 KB

Thanks for review. Attaching updated patch.

blazey’s picture

Status: Needs work » Needs review
Leksat’s picture

Status: Needs review » Reviewed & tested by the community

The last change is tiny. Back to RTBC.

alexpott’s picture

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.