Problem/Motivation

When configuring language path prefixes, there may be only one empty path prefix, that of the site default language. The intention of this is to protect users from setting an empty prefix for something that would not end up being selected as a fallback in the negotiation, basically making that language inaccessible by path. However, we made the fallback language configurable, so that this would be the site default language is not necessarily true. So you may now end up with a configuration where you cannot change the fallback language and remove the prefix before going live unless you also modify the site default language, which is silly. That was the reason we separated the default and the fallback languages to begin with.

Proposed resolution

Either only validate that at most one path prefix may be empty or validate that that is for the fallback language configured in the negotiation (not the site default).

Remaining tasks

Agree on approach. Implement.

User interface changes

Limitation on which language may get an empty prefix may change depending on configuration.

API changes

Likely none.

CommentFileSizeAuthor
#15 2411343-empty_language_prefix-15.patch4.99 KBpcambra
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,839 pass(es). View
#15 interdiff-2411343-7-15.txt3.54 KBpcambra
#7 2411343-empty_language_prefix-7.patch3.91 KBpcambra
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,130 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

andypost’s picture

Language as entity could have prefix constraint

Gábor Hojtsy’s picture

@andypost: can you elaborate on that? I don't understand.

andypost’s picture

I mean language is entity with prefix field that can have constraints applied
So when language entity is validated a (LanguageDefaultConstraint) constrain may check that default language is changed so prefix should be not empty or any other logic.

Gábor Hojtsy’s picture

Yeah IF the fallback language is still tied to the default language.

balagan’s picture

Is it a duplicate of this? https://www.drupal.org/node/338055

Gábor Hojtsy’s picture

No, its not.

pcambra’s picture

Status: Active » Needs review
FileSize
3.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,130 pass(es). View

Let's see if I'm in the right track

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Issue tags: +sprint

Looks good. Some remarks.

  1. The UI also has a hint for the default language in the setting label. That is not yet modified here. That needs to change to "Selected fallback language" or something along those lines. I think we have a problem of using the right terminology because the selected language may be forced and not necessarily a fallback, but for the intention of the validation of this form I think that is correct to assume it would be the fallback.
  2. The form has this help text which needs to be adjusted: For the default language, this value may be left blank
  3. . I think "Selected fallback language" is good wording here too.

  4. +++ b/core/modules/language/src/Tests/LanguageConfigurationTest.php
    @@ -83,13 +83,16 @@ function testLanguageConfiguration() {
    +    $this->assertText(t('The prefix may only be left blank for the negotiation selected language.'), 'Only negotiation language prefix can be changed to empty string.');
    

    The feedback message is not very clear. I think we can leave that, because the text looked for is descriptive enough.

  5. +++ b/core/modules/language/src/Tests/LanguageSwitchingTest.php
    @@ -161,7 +161,7 @@ protected function doTestLanguageBlockAnonymous($block_label) {
    -   * Test languge switcher links for domain based negotiation
    +   * Test language switcher links for domain based negotiation
    

    If you are fixing the grammar here, then we need a dot at the end too :)

balagan’s picture

+++ b/core/modules/language/src/Form/NegotiationUrlForm.php
@@ -142,14 +143,17 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
+          $form_state->setErrorByName("prefix][$langcode", $this->t('The prefix may only be left blank for the negotiation selected language.'));

The wording is the same here that I mentioned to Gábor on IRC, which does not make sense for me (negotiation selected language). I know that now it maches the page title, but I really don't like it.

Gábor Hojtsy’s picture

Yeah for this screen we may use "selected fallback language" or "selected negotiation fallback language" but TECHNICALLY it may be a negotiation provider invoked before the URL negotiation, in which case, its not the fallback. We assume its the fallback for the design of how we expect the URL negotiation to be used, but it may not be 100% true. That makes it hard to label it @balagan.

balagan’s picture

I could do with "language selected by negotiation", or "negotiated language", my problem is the word order.

Gábor Hojtsy’s picture

@balagan: no, those are not correct. The text refers to the language the site admin selected in the negotiation method named "Selected language". If you go to your detection and selection methods page, that has the "Selected language" option as the last one. This help text refers to the setting that the admin set there. If we want to be super specific then "The language configured for the "Selected language" negotiation method" is what this is about.

balagan’s picture

I still feel using "negotiation selected language" is wrong. Can we use "'selected language' negotiation", and on the admin/config/regional/language/detection/selected page I would change the title from "Configure selection language negotiation" to "Configure selected language negotiation". I guess I will have to open another issue for that.

Gábor Hojtsy’s picture

@balagan: your proposed names may or may not be good depending on the sentence you put them. Please do open an issue for the page title on that method, that needs fixing definitely.

pcambra’s picture

Status: Needs work » Needs review
FileSize
3.54 KB
4.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,839 pass(es). View

Let's see if this wording works better.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed a0add30 and pushed to 8.0.x. Thanks!

  • alexpott committed a0add30 on 8.0.x
    Issue #2411343 by pcambra: Only allowing to have empty language path...
Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, this is a great improvement! Thanks @pcambra!

Status: Fixed » Closed (fixed)

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