updated as of comment #18

Problem/Motivation

The language.negotiation config values have helper methods to get and set their values that are used in both runtime and editing configuration. This means that overrides can bleed into stored configuration.

language_negotiation_url_prefixes()
language_negotiation_url_prefixes_update()
language_negotiation_url_prefixes_save()
language_negotiation_url_domains()
language_negotiation_url_domains_save()

Proposed resolution

Per comment #4, the helper methods listed above should not be used to set config. Instead, config for language negotiation should be set directly using editable config factories. Also, per #7.2, the following two functions should be removed, as they are not necessary:

language_negotiation_url_domains_save()
language_negotiation_url_prefixes_save()

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary Instructions
Add automated tests Instructions

User interface changes

None.

API changes

The following 2 global functions are removed:

language_negotiation_url_domains_save()
language_negotiation_url_prefixes_save()

Use ConfigFactory::getEditable() instead.

Comments

Gábor Hojtsy’s picture

Issue tags: +D8MI, +language-config, +sprint
andypost’s picture

Issue summary: View changes

language_negotiation_url_prefixes()
language_negotiation_url_prefixes_update()
language_negotiation_url_prefixes_save()
language_negotiation_url_domains()
language_negotiation_url_domains_save()

andypost’s picture

Issue summary: View changes

Suppose all this API functions doc-blocks should be updated to point about "careful usage because of overrides"
Also there's 2 @todo in code for that issue

Gábor Hojtsy’s picture

At least they should not be used in setting config. The functions to set may go away. The forms (should) ensure to load editable config and save back to it.

andypost’s picture

Status: Active » Needs review
FileSize
2.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,067 pass(es). View

Suppose we should remove all *save() and language_negotiation_url_prefixes_update()

Also it looks that this config should have no overrides because it's language independent (no reason to translate a list of domains or url prefixes) so site-wide

Patch adds comments and initial usage clean-up while I skimmed code

andypost’s picture

Usage:

language_negotiation_url_prefixes_update()
1) new language saved \Drupal\language\Entity\ConfigurableLanguage::postSave()
2) site default language changed \Drupal\language\EventSubscriber\ConfigSubscriber::onConfigSave

language_negotiation_url_prefixes_save()
language_configurable_language_delete and from update above

language_negotiation_url_domains_save() when configurable language added or deleted

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/language/language.module
    @@ -332,7 +332,10 @@ function language_language_types_info() {
     function language_negotiation_url_prefixes() {
    -  return \Drupal::config('language.negotiation')->get('url.prefixes');
    +  // Should always return data without overrides.
    +  return \Drupal::configFactory()
    +    ->getEditable('language.negotiation')
    +    ->get('url.prefixes');
     }
     
     /**
    @@ -368,7 +371,10 @@ function language_negotiation_url_prefixes_save(array $prefixes) {
    
    @@ -368,7 +371,10 @@ function language_negotiation_url_prefixes_save(array $prefixes) {
      * Reads language domains.
      */
     function language_negotiation_url_domains() {
    -  return \Drupal::config('language.negotiation')->get('url.domains');
    +  // Should always return data without overrides.
    +  return \Drupal::configFactory()
    +    ->getEditable('language.negotiation')
    +    ->get('url.domains');
     }
    

    There may still be other overrides based on domain or organic group or whatever other condition, overrides are not necessarily language specific only. The getters should still return possibly overridden data.

  2. +++ b/core/modules/language/src/Form/NegotiationUrlForm.php
    @@ -209,12 +209,11 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +      // Save new domain and prefix values.
    +      ->set('url.prefixes', $form_state->getValue('prefix'))
    +      ->set('url.domains', $form_state->getValue('domain'))
           ->save();
     
    -    // Save new domain and prefix values.
    -    language_negotiation_url_prefixes_save($form_state->getValue('prefix'));
    -    language_negotiation_url_domains_save($form_state->getValue('domain'));
    

    These look great. IMHO we should remove the save functions entirely to avoid overridden data saved.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
4.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,583 pass(es). View

Reverted getters.
Removed the save functions, thinking to remove language_negotiation_url_prefixes_update()

The rest of usage is

core/modules/language/src/Entity/ConfigurableLanguage.php:136:    language_negotiation_url_prefixes_update();
core/modules/language/src/EventSubscriber/ConfigSubscriber.php:63:        language_negotiation_url_prefixes_update();

Also looks that language_configurable_language_insert() & language_configurable_language_delete() could be moved to \Drupal\language\Entity\ConfigurableLanguage::postSave and postDelete() methods

Gábor Hojtsy’s picture

Status: Needs review » Needs work

The proposed changes all look great now. AFAIS language_negotiation_url_prefixes_update() may still very well leak overrides in. Although you fixed how it saves now, it still gets its source data from language_negotiation_url_prefixes(). It should either be removed or at least get its data from the editable configuration.

No concerns with the patch other than that.

andypost’s picture

Status: Needs work » Needs review
FileSize
898 bytes
5.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,453 pass(es). View

Here's re-roll and clean-up - no reason to set default language when the same, but maybe it's for separate issue too?

@Gabor no, it uses editable now but maybe we really should get prefixes after override?

+++ b/core/modules/language/language.module
@@ -324,7 +324,8 @@ function language_negotiation_url_prefixes() {
-  $prefixes = language_negotiation_url_prefixes();
+  $config = \Drupal::configFactory()->getEditable('language.negotiation');
+  $prefixes = $config->get('url.prefixes');

is fine?

Gábor Hojtsy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/language/language.module
    @@ -324,7 +324,8 @@ function language_negotiation_url_prefixes() {
     function language_negotiation_url_prefixes_update() {
    -  $prefixes = language_negotiation_url_prefixes();
    +  $config = \Drupal::configFactory()->getEditable('language.negotiation');
    +  $prefixes = $config->get('url.prefixes');
    

    Right this looks good, sorry for not figuring it out from the diff earlier.

  2. +++ b/core/modules/language/src/EventSubscriber/ConfigSubscriber.php
    @@ -57,7 +57,7 @@ public function onConfigSave(ConfigCrudEvent $event) {
    -      if ($language) {
    +      if ($language && $language->getId() != $saved_config->getOriginal('default_langcode')) {
    

    This change is not even remotely related to config overrides.

So #8 looks like a good patch but the changes in #10 are too much for this scope AFAIS. Wanna post it again? :)

andypost’s picture

Status: Needs work » Needs review
FileSize
4.94 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,486 pass(es). View

Yep, so here's a re-roll of #8
I see no way to add tests for that
Also filed #2462729: Move ConfigurableLanguage hook implementations in language module to the entity

Gábor Hojtsy’s picture

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

As for tests, I think the test can set $GLOBAL['config'] value for language.negotiation to something custom and call the language_negotiation_url_prefixes_update() function and assert that the override did not end up in config.

As for testing the forms, you can save a language override (it does not matter that you override things that are otherwise not translatable) and call the form to observe the values are not used. That would also prove that NegotiationUrlForm is not yet immune to overrides, language_negotiation_url_prefixes() and language_negotiation_url_domains() are still used in the buildform (just found while looking at the code to suggest tests).

Marking as needs work for fixing the use of language_negotiation_url_prefixes() and language_negotiation_url_domains() in NegotiationUrlForm and for tests.

andypost’s picture

FileSize
825 bytes
5.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,918 pass(es). View

Fixed usage in form, will try tests

Gábor Hojtsy’s picture

Status: Needs work » Needs review

Amazing, thanks! Sending to testbot.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Back to needs work for lack of new tests then.

YesCT’s picture

YesCT’s picture

Issue summary: View changes

added templates to issue summary.

robertdbailey’s picture

Issue summary: View changes

Saw this one sitting idle for a while, so I updated issue summary (proposed resolution) according to what appears to be happening in the patch.

robertdbailey’s picture

robertdbailey’s picture

For the tests, I could use some help in documenting some steps to reproduce:

For setting and then checking the global config value for language negotiation, I'm trying things like this:

$GLOBALS['config']['language.negotiation'] = array('url.prefixes' => array('en' => 'whatever'));
language_negotiation_url_prefixes_update();
dpm(\Drupal::config('language.negotiation')->get('url.prefixes'));

I think I need a different data type to initialize the language-negotiation global config variable. I haven't been able to see the way to make runtime changes also modify stored configuration.

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

@robertdbailey: you can write a test that puts that to settings.php on install and then invokes the URL of NegotiationUrlForm to verify that those overrides don't show up there anymore. Without the patch if there is some kind of override for language settings, that will show up in the form (and get saved back when you hit save on the form). There is no UI to reproduce the bug because language.negotiation config values are not translatable and the only UI to edit config overrides in core is for languages. What you can also do in the test is to add a foreign language, set an override in that language of language.negotiation and then access the language URL settings page in that language, and check the overrides are not applied.

robertdbailey’s picture

Status: Needs work » Needs review
FileSize
2.11 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,593 pass(es), 2 fail(s), and 0 exception(s). View
7.59 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,588 pass(es). View

created tests for checking English language.negotiation overrides and then Spanish language.negotiation overrides. Please review.

The last submitted patch, 23: 2403229-lng-leak-23-test_only-must_fail.patch, failed testing.

vijaycs85’s picture

Issue tags: -Needs tests

Tests in #23 looks good (though it's missing second part of @Gábor Hojtsy suggestion in #22) and the main fix has been in review since #14. I think this is good to go.

robertdbailey’s picture

FileSize
1.8 KB
2.46 KB
7.94 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,650 pass(es). View

Thanks for reviewing, @vijaycs85! I've updated the tests to include the second part of #22. Please review.

robertdbailey’s picture

FileSize
1.8 KB
2.46 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,608 pass(es), 2 fail(s), and 0 exception(s). View
7.94 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,624 pass(es). View

Patches from #25 did not seem to be queued properly for testing. Retrying as #26.

The last submitted patch, 27: 2403229-lng-leak-26-test_only-must_fail.patch, failed testing.

alexpott’s picture

Status: Needs review » Needs work

This change looks excellent. I think the removal of the language_negotiation_url_*_save() methods is acceptable - forms and other APIs should be interacting directly with the configuration object here.

+++ b/core/modules/language/src/Form/NegotiationUrlForm.php
@@ -111,8 +111,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-    $prefixes = language_negotiation_url_prefixes();
-    $domains = language_negotiation_url_domains();

I think we should be deprecating both of these methods and scheduling them for removal in Drupal 9, since people really should just load the configuration from config.

Gábor Hojtsy’s picture

Agreed with @alexpott.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
2.46 KB
8.61 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,075 pass(es). View

Deprecating both language_negotiation_url_prefixes() and language_negotiation_url_domains().
I'm wondering if we should do the same with language_negotiation_url_prefixes_update()

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

There is no use of language_negotiation_url_prefixes_update() in core, only one mention in comments. Not sure we can deprecate it though unless we have some other place where its implemented. The ConfigSubscriber::onConfigSave() (where a copy of its code resides) in language is not a reusable place. I don't think moving that around would be in scope for this issue anyway. Let's get this in then.

penyaskito’s picture

@Gábor: There is one use of language_negotiation_url_prefixes_update() inConfigurableLanguage::postSave. But yeah, we can leave that for a follow-up anyway.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

It's great to have this fixed. Nice work everyone. This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 52e5d45 and pushed to 8.0.x. Thanks!

I don't think a CR is necessary for the two functions that should be removed.

  • alexpott committed 52e5d45 on 8.0.x
    Issue #2403229 by robertdbailey, andypost, penyaskito, Gábor Hojtsy:...
andypost’s picture

There is one use of language_negotiation_url_prefixes_update() inConfigurableLanguage::postSave. But yeah, we can leave that for a follow-up

This follow-up still needed

Gábor Hojtsy’s picture

Issue tags: -sprint

Amazing, thanks all!

penyaskito’s picture

Status: Fixed » Closed (fixed)

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