Problem/Motivation

Working on #2403229-8: language.negotiation configuration can have overrides bleed in I found that language_configurable_language_insert() & language_configurable_language_delete() could be moved to \Drupal\language\Entity\ConfigurableLanguage::postSave and postDelete() methods

Proposed resolution

Move them to entity because no reason to split the logic within the same module

Remaining tasks

Commit.

User interface changes

None.

API changes

No, just removing 2 hook implementations.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because the code works as-is, but the code organization makes it hard to follow.
Issue priority Normal because the code works the same way either way.
Disruption No disruption. Hook implementations are not part of the API, they should not be invoked as standalone functions.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Issue tags: +language-base

Looks like these were not converted when postSave() and postDelete() methods were introduced.

rpayanm’s picture

Status: Active » Needs review
FileSize
2.91 KB

Please review.

Status: Needs review » Needs work

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

sidharthap’s picture

Status: Needs work » Needs review
FileSize
2.9 KB

Here is a updated patch. Not sure if it is the right way.

rpayanm’s picture

FileSize
1.42 KB
2.85 KB
+++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
@@ -178,6 +185,15 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
+    unset($prefixes[$this->id()]);
...
+    unset($domains[$this->id()]);

$this is not accessible in static context.

Let me try.

Status: Needs review » Needs work

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

Status: Needs work » Needs review

rpayanm queued 5: 2462729-5.patch for re-testing.

Status: Needs review » Needs work

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

The last submitted patch, 4: 2462729-4.patch, failed testing.

andypost’s picture

Probably this affected by parent issue but anyway

  1. +++ b/core/modules/language/language.module
    @@ -399,35 +399,6 @@ function language_modules_uninstalled($modules) {
    -function language_configurable_language_insert(ConfigurableLanguageInterface $language) {
    
    +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
    @@ -144,6 +144,15 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    +    if ($this->isLocked()) {
    

    Insert only!
    if ($this->isLocked() || $update)

  2. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
    @@ -178,6 +187,16 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +    // Remove language from language prefix list.
    +    $prefixes = language_negotiation_url_prefixes();
    +    unset($prefixes[$language_manager->getCurrentLanguage()->getId()]);
    +    language_negotiation_url_prefixes_save($prefixes);
    +
    +    // Remove language from language domain list.
    +    $domains = language_negotiation_url_domains();
    +    unset($domains[$language_manager->getCurrentLanguage()->getId()]);
    +    language_negotiation_url_domains_save($domains);
    

    Use editable config, so save will happen once

segi’s picture

Here is the updated patch, based on @andypost feedbacks.

rpayanm’s picture

Status: Needs work » Needs review

@segi you must change the Status to "Need review" for testbot test it.

mon_franco’s picture

Assigned: Unassigned » mon_franco
Issue tags: +#drupaldevdays

I have reviewed the last patch, everything seems correct. The only recommendation, after to talk with @tstoeckler about this issue, is to use the object instead of these two functions: language_negotiation_url_domains() and language_negotiation_url_domains_save

mon_franco’s picture

Assigned: mon_franco » Unassigned
Xano’s picture

Status: Needs review » Needs work
yannickoo’s picture

Issue tags: -#drupaldevdays +drupaldevdays
axe312’s picture

- just wanted to remove the hashtag from the issue tag ... yannickoo was faster -

segi’s picture

Updated the patch, I removed call of language_negotiation_url_domains() and language_negotiation_url_domains_save() functions.

segi’s picture

Status: Needs work » Needs review
mon_franco’s picture

Assigned: Unassigned » mon_franco

Hi,
Issue reviewed, everything seems correct :)

mon_franco’s picture

Assigned: mon_franco » Unassigned
andypost’s picture

Status: Needs review » Needs work
+++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
@@ -144,6 +144,17 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
+    \Drupal::configFactory()->getEditable('language.negotiation')
+    ->set('url.domains', $domains)
+    ->save();

@@ -178,6 +189,18 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
+    \Drupal::configFactory()->getEditable('language.negotiation')
+    ->set('url.prefixes', $prefixes)
+    ->set('url.domains', $domains)
+    ->save();

wrong indent

andypost’s picture

segi’s picture

FileSize
3.06 KB
1.17 KB

I fixed the text indent, but I used this indent because I saw it in the language_negotiation_url_domains_save() and language_negotiation_url_prefixes_save() functions, so the indent wrong there as well.

segi’s picture

Status: Needs work » Needs review
penyaskito’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice +sprint

Looks good to me, and still applies.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. This issue is a task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary. However making this about fixing config overrides bleeding in also makes this address a bug (partially).
  2. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
    @@ -144,6 +144,17 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    +    if ($this->isLocked() || $update) {
    +      return;
    +    }
    

    Lets not do an early return let's wrap the write to config in an if (!$this->locked() && !$update) {.

  3. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
    @@ -144,6 +144,17 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    +    $domains = \Drupal::config('language.negotiation')->get('url.domains');
    +    $domains[$this->id()] = '';
    +    \Drupal::configFactory()->getEditable('language.negotiation')
    +      ->set('url.domains', $domains)
    +      ->save();
    
    @@ -178,6 +189,18 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
    +    // Remove language from language prefix list.
    +    $prefixes = \Drupal::config('language.negotiation')->get('url.prefixes');
    +    unset($prefixes[$language_manager->getCurrentLanguage()->getId()]);
    +
    +    // Remove language from language domain list.
    +    $domains = \Drupal::config('language.negotiation')->get('url.domains');
    +    unset($domains[$language_manager->getCurrentLanguage()->getId()]);
    +    \Drupal::configFactory()->getEditable('language.negotiation')
    +      ->set('url.prefixes', $prefixes)
    +      ->set('url.domains', $domains)
    +      ->save();
    

    Let's just do one config getEditable() instead of the get()s in each of the methods here which will also fix some of the config override bleeding into active config issues.

    The other issue can add the test and address the other problems with the procedural functions.

tim.plunkett’s picture

Title: Move ConfigurableLanguage hook implementations in language module to the the entity » Move ConfigurableLanguage hook implementations in language module to the entity
+++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
@@ -144,6 +144,17 @@ public function postSave(EntityStorageInterface $storage, $update = TRUE) {
+    $domains = \Drupal::config('language.negotiation')->get('url.domains');
...
+    \Drupal::configFactory()->getEditable('language.negotiation')

@@ -178,6 +189,18 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
+    $prefixes = \Drupal::config('language.negotiation')->get('url.prefixes');
...
+    $domains = \Drupal::config('language.negotiation')->get('url.domains');
...
+    \Drupal::configFactory()->getEditable('language.negotiation')

Why not just get this config object once (in each method) and manipulate it directly?

segi’s picture

I rerolled the patch. My plan is I will continue with the fixes based on feedbacks.

segi’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
@@ -178,6 +189,18 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
+    // Remove language from language prefix list.
+    $prefixes = \Drupal::config('language.negotiation')->get('url.prefixes');
+    unset($prefixes[$language_manager->getCurrentLanguage()->getId()]);
...
+    // Remove language from language domain list.
+    $domains = \Drupal::config('language.negotiation')->get('url.domains');
+    unset($domains[$language_manager->getCurrentLanguage()->getId()]);
+    \Drupal::configFactory()->getEditable('language.negotiation')
+      ->set('url.prefixes', $prefixes)
+      ->set('url.domains', $domains)
+      ->save();

Why are we using getCurrentLanguage() here? We are supposed to react to deleting of possibly multiple entities, not just one AFAIS.

segi’s picture

I tried to fix the patch base on feedbacks, but the requests for config management weren't clean for me, so I did a solution. I hope I step closer to final solution.

segi’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

#2403229: language.negotiation configuration can have overrides bleed in already added the test coverage and resolved the bleeding. This one just cleans up the code now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: 2462729-32.patch, failed testing.

Gábor Hojtsy’s picture

Trackertest GET http://ec2-52-88-182-40.us-west-2.compute.amazonaws.com/checkout/user/login returned 0 (0 bytes) does not seem related at all.

Gábor Hojtsy queued 32: 2462729-32.patch for re-testing.

Gábor Hojtsy’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 32: 2462729-32.patch, failed testing.

penyaskito queued 32: 2462729-32.patch for re-testing.

penyaskito’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Postponed
Issue tags: +D8 patch release target

Yes this change makes sense and has no risk but it does not meet our beta guidelines for Drupal 8 inclusion at this time. In my opinion this can go in after Drupal 8 release. I don't think we need to wait to 8.1.

Tagging with a new tag "D8 patch release target" since I don't think we have a tag for this.

andypost’s picture

Issue tags: +minor version target

suppose the right tag

Gábor Hojtsy’s picture

Issue tags: -sprint

Removing from sprint then.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Status: Postponed » Needs work

yeah.. 8.0 beta is over now :)

andypost’s picture

Version: 8.4.x-dev » 8.5.x-dev
Status: Needs work » Needs review
FileSize
615 bytes
3.9 KB

reroll + fix unused use

andypost’s picture

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

segi’s picture

I reviewed the last patch it looks good and the test went well.

segi’s picture

Status: Needs review » Reviewed & tested by the community
tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
@@ -173,6 +181,18 @@ public static function postDelete(EntityStorageInterface $storage, array $entiti
+    $prefixes = $config->get('url.prefixes');
+    $domains = $config->get('url.domains');
+    unset($prefixes[$entity->id]);
+    unset($domains[$entity->id]);
...
+    // Save the modified settings.
+    $config->set('url.prefixes', $prefixes)
+      ->set('url.domains', $domains)

This can be written even more succinctly by using $config->clear('url.prefixes.' $entity->id()) (and the same for the domains).

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
3.43 KB
888 bytes

Updated patch as per comment #55 & also added interdiff.

Status: Needs review » Needs work

The last submitted patch, 56: 2462729-56.patch, failed testing. View results

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
3.43 KB
825 bytes

maybe this patch will solve the PHPLint issue.

Status: Needs review » Needs work

The last submitted patch, 58: 2462729-58.patch, failed testing. View results

yogeshmpawar’s picture

Status: Needs work » Needs review
FileSize
3.33 KB
921 bytes

my bad few things i missed.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Nice. We could save a few more charactes with chained calls, even skip the $config variable completely but not sure if that really is an improvements, lets see what @alexpott thinks.

alexpott’s picture

Adding credit. The lack of chaining is okay. Makes no difference really.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 877e4a8 and pushed to 8.6.x. Thanks!

  • alexpott committed 877e4a8 on 8.6.x
    Issue #2462729 by segi, Yogesh Pawar, andypost, rpayanm, Gábor Hojtsy,...

Status: Fixed » Closed (fixed)

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