Updated: Comment #0

Problem/Motivation

#1862202: Objectify the language system moved most of the language negotiation code to classes. The language entity CRUD API, which is rarely used, was held back to limit the size of the patch.

Proposed resolution

  • Move language entity CRUD API to a dedicated LanguageStorageManager service.
  • Inject it where appropriate.

Remaining tasks

  1. Decide whether keeping BC procedural wrappers.
  2. Write a patch
  3. Fix failing tests
  4. Reviews

User interface changes

None

API changes

TBD

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

Status: Postponed » Active

The parent issue has been committed.

larowlan’s picture

Assigned: Unassigned » larowlan

/me takes a look

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Active » Needs review
FileSize
11.72 KB

assuming you mean language_{save,delete}
left behind old wrappers and didn't convert anything, lets see what the bot says.

Status: Needs review » Needs work

The last submitted patch, 3: language-crud-2166923.1.patch, failed testing.

andypost’s picture

  1. +++ b/core/modules/language/language.services.yml
    @@ -7,3 +7,6 @@ services:
    +  language.storage:
    

    Suppose language.crud better name for that. There's path.crud already in core services

  2. +++ b/core/modules/language/lib/Drupal/language/LanguageStorageInterface.php
    @@ -0,0 +1,38 @@
    +  public function delete($langcode);
    ...
    +  public function save(Language $language);
    

    Looks like not CRUD ;)

larowlan’s picture

Status: Needs work » Needs review
FileSize
752 bytes
11.71 KB

misused StorageController::create()

kim.pepper’s picture

Suppose language.crud better name for that. There's path.crud already in core services

We're using Storage and Repository as well in a few places too. I think CRUD is the odd one out.

larowlan’s picture

Green, so just a matter of names from here.

jibran’s picture

Some minor issues. Do we need unit test here? so that we can retire some simpletests.

  1. +++ b/core/modules/language/lib/Drupal/language/LanguageStorage.php
    @@ -0,0 +1,166 @@
    +  /**
    +   * Saves a language.
    +   *
    +   * @param \Drupal\Core\Language\Language $language
    +   *   The language to save.
    +   *
    +   * @return \Drupal\Core\Language\Language
    +   *   The saved language.
    +   */
    ...
    +  /**
    +   * Deletes a language.
    +   *
    +   * @param string $langcode
    +   *   The language code to delete.
    +   *
    +   * @return bool
    +   *   TRUE if language is successfully deleted. Otherwise FALSE.
    +   */
    

    we can use inheritdoc here.

  2. +++ b/core/modules/language/lib/Drupal/language/LanguageStorageInterface.php
    @@ -0,0 +1,38 @@
    + * Contains \Drupal\language\LanguageStorageInterface.
    + */
    +namespace Drupal\language;
    +
    +use Drupal\Core\Language\Language;
    +
    +
    +/**
    

    White spaces not correct.

kim.pepper’s picture

Green, so just a matter of names from here.

Just a matter of names? Surely there is a 100 comment bike-shedding opportunity here. ;-p

larowlan’s picture

I actually wrote 'Let the bikeshedding begin' and then took it out.

martin107’s picture

6: language-crud-2166923.2.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 6: language-crud-2166923.2.patch, failed testing.

martin107’s picture

Issue tags: +Needs reroll
kim.pepper’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
11.22 KB

Re-rolled and fixed the issues in #9.

plach’s picture

This is looking good, thanks! Just one remark:

+++ b/core/modules/language/lib/Drupal/language/LanguageStorage.php
@@ -0,0 +1,154 @@
+      ConfigurableLanguageManager::rebuildServices();
...
+        ConfigurableLanguageManager::rebuildServices();

We have no use statement for these. Also I think it would make sense to add an instanceof check.

Also, could we try and see whether removing BC procedural wrappers would increase the size of the patch unreasonably?

Status: Needs review » Needs work

The last submitted patch, 15: 2166923-language-crud-15.patch, failed testing.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
11.81 KB
3.85 KB

We have no use statement for these. Also I think it would make sense to add an instanceof check.

ConfigurableLanguageManager is in the same namespace, so no need for a use statement. Not sure what you mean about instance of? Do you mean on $this->languageManager? If so, looking at the service.yml, it's not an instance of ConfigurableLanguageManager.

I've fixed up the call to variable_set() and converted some \Drupal::service() calls to injected services. Looks like this patch was quite old.

kim.pepper’s picture

I looked at removing BC wrappers, and there is a tonne of calls to them, including in the installer. I'd suggest we make that a follow up.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

ok then RTBC.

plach’s picture

Status: Reviewed & tested by the community » Needs review

ConfigurableLanguageManager is in the same namespace, so no need for a use statement.

D'oh! Silly me :)

+++ b/core/modules/language/lib/Drupal/language/LanguageStorage.php
@@ -0,0 +1,171 @@
+    if (!$multilingual && $language->is_new) {
+      ConfigurableLanguageManager::rebuildServices();
...
+      if (!$this->languageManager->isMultilingual()) {
+        ConfigurableLanguageManager::rebuildServices();
Not sure what you mean about instance of? Do you mean on $this->languageManager? If so, looking at the service.yml, it's not an instance of ConfigurableLanguageManager.

Service implementations can be swapped so the actual language manager instance stored in $this->languageManager may or may not be ConfigurableLanguageManager, that is the one the Language module and that replaces the core language manager. In that case we would be calling the rebuildServices() method on an object that does not define that method. So, yes, I really think we should add the following check to to those ifs:

$this->languageManager instanceof ConfigurableLanguageManager
kim.pepper’s picture

I understand how dependency injection works, but sorry, that still doesn't make sense.

Do we want to call ConfigurableLanguageManager::rebuildServices() *only* if the injected LanguageManager happens to be an instance of ConfigurableLanguageManagerInterface? I thought we'd want to do that all the time?

plach’s picture

I thought we'd want to do that all the time?

We want to call it only if it's actually there: we cannot assume every implementation of the language manager will need to rebuild services. The proper fix should probably be making ::rebuildServices() non-static, moving it onto the ConfigurableLanguageManagerInterface, then checking whether the language manager in an instance of CLMI.

kim.pepper’s picture

Thanks for the explanation.

I've applied fixes as per your recommendations in #23.

plach’s picture

Title: Move language entity CRUD API to a dedicated service » Move language entity (CR)UD API to a dedicated service
Status: Needs review » Reviewed & tested by the community

Awesome, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/language/lib/Drupal/language/LanguageStorage.php
@@ -0,0 +1,171 @@
+  /**
+   * The language_entity storage controller.
+   *
+   * @var \Drupal\Core\Entity\EntityStorageControllerInterface
+   */
+  protected $storageController;

This has been renamed in #2188613: Rename EntityStorageController to EntityStorage

Xano’s picture

Status: Needs work » Needs review
FileSize
13.99 KB
1.86 KB
kfritsche’s picture

Status: Needs review » Reviewed & tested by the community

Changes seems reasonable and fine.
Setting back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/language/language.module
@@ -436,96 +436,32 @@ function language_get_default_langcode($entity_type, $bundle) {
+ * @deprecated Use \Drupal::service('language.storage')->save()
...
+ * @deprecated Use \Drupal::service('language.storage')->delete()

This is not the correct format.

I also wonder if more of this should be moving into the language entity?

Also this patch is incorrectly rerolled given #2223249: Remove hook_language_insert/update/delete/presave in favour of entity hooks

alexpott’s picture

+++ b/core/modules/language/lib/Drupal/language/LanguageStorage.php
@@ -0,0 +1,171 @@
+    // Let other modules modify $language before saved.
+    $this->moduleHandler->invokeAll('language_presave', array($language));
...
+    if ($language->is_new) {
+      $this->moduleHandler->invokeAll('language_insert', array($language));
+      watchdog('language', 'The %language (%langcode) language has been created.', $t_args);
+    }
+    else {
+      $this->moduleHandler->invokeAll('language_update', array($language));
+      watchdog('language', 'The %language (%langcode) language has been updated.', $t_args);
+    }
...
+      $this->moduleHandler->invokeAll('language_delete', array($language));

All this has been removed

+++ b/core/modules/language/lib/Drupal/language/LanguageStorage.php
@@ -0,0 +1,171 @@
+class LanguageStorage implements LanguageStorageInterface {
...
+  protected $languageStorage;
...
+    $this->languageStorage = $entity_manager->getStorage('language_entity');

LanguageStorage::languageStorage??? Super confusing. And is why I wonder if more of this should be moving into the language entity?

kfritsche’s picture

Status: Needs work » Needs review
FileSize
13.6 KB
4.1 KB

Fixed deprecated messages and the wrong re-roll mentioned by alexpott.

Also renamed the languageStorage to entityStorage. I hope this is more clear than languageStorage.

kfritsche’s picture

FileSize
8.59 KB
15.44 KB

Patch is just some minor things which alexpott also told me, which seems incorrect.
* In other entities (e.g node) watchdog messages for CRUD is happening in form controllers, moved that there
* If we are in the language module, we are sure that we have the ConfigLanguageManager, so removed the if statements there too

There is still discussion going on here and I just wanted to summarize the latest status I heard of and that we at the end don't forget about this smaller stuff.

* LanguageStorage doesn't make sense as it is no storage at all. At maximum is a language to language entity mapper
* One idea was to move the methods from current LanguageStore to the ConfigurableLanguageManager - plachs concern is that we then adding a circular dependency
* Other idea is that the LanguageStore is extending the ConfigEntityStorage and change the methods accordingly and the Language entity is using this storage then

plach’s picture

Actually we can't be sure we are dealing with a ConfigurableLanguageManager instance: another module could swap-in a different implementation for the language manager service (see #23).

kfritsche’s picture

Fixed #33.

Also after talking with plach we decided to extend the ConfigEntityStorage for the LanguageStore. So we don't have a special service for that anymore. Everything related to that is now in the language entity storage, which seems more reasonable.

Hope this is now better? Otherwise we also would be fine to do it in another way, if this still doesn't seem good.

alexpott’s picture

+++ b/core/modules/language/lib/Drupal/language/LanguageStorage.php
@@ -0,0 +1,151 @@
+    // Save the record and inform others about the change.
+    $multilingual = $this->languageManager->isMultilingual();
+    $language_entity->save();
+
+    if (!empty($language->default)) {
+      // Update the config. Saving the configuration fires and event that causes
+      // the container to be rebuilt.
+      $this->configFactory
+        ->get('system.site')
+        ->set('langcode', $language->id)
+        ->save();
+      $this->defaultLanguage->set($language);
+    }
+
+    $this->languageManager->reset();
+    if ($this->languageManager instanceof ConfigurableLanguageManagerInterface) {
+      $this->languageManager->updateLockedLanguageWeights();
+    }
+
+    // Update URL Prefixes for all languages after the new default language is
+    // propagated and the language_list() cache is flushed.
+    language_negotiation_url_prefixes_update();
+
+    // If after adding this language the site will become multilingual, we need
+    // to rebuild language services.
+    if (!$multilingual && $language->is_new && $this->languageManager instanceof ConfigurableLanguageManagerInterface) {
+      $this->languageManager->rebuildServices();
+    }

This should move into the postSave method so all LanguageEntity saves do this.

+++ b/core/modules/language/lib/Drupal/language/LanguageStorage.php
@@ -0,0 +1,151 @@
+      $this->languageManager->reset();
+      if ($this->languageManager instanceof ConfigurableLanguageManagerInterface) {
+        $this->languageManager->updateLockedLanguageWeights();
+      }
+
+      // If after deleting this language the site will become monolingual, we
+      // need to rebuild language services.
+      if (!$this->languageManager->isMultilingual() && $this->languageManager instanceof ConfigurableLanguageManagerInterface) {
+        $this->languageManager->rebuildServices();
+      }

This needs to move to postDelete

kfritsche’s picture

FileSize
4.34 KB
16.7 KB

Looks much better now, thanks.

I moved the noted stuff to the language entity postSave and postDelete methods.

+++ b/core/modules/language/lib/Drupal/language/LanguageStorage.php
@@ -0,0 +1,151 @@
+    if (!empty($language->default)) {
+      // Update the config. Saving the configuration fires and event that causes
+      // the container to be rebuilt.
+      $this->configFactory
+        ->get('system.site')
+        ->set('langcode', $language->id)
+        ->save();
+      $this->defaultLanguage->set($language);
+    }

This is still in the LanguageStorage, as we don't have this field in the language entity. So its still something special when you save the Language instead of the language entity, but I believe this is okay. As the main idea is, that cache get also cleared when language entities are imported or similar and these can't have the default field.

alexpott’s picture

+++ b/core/modules/language/lib/Drupal/language/Entity/Language.php
@@ -90,6 +91,28 @@ public function preSave(EntityStorageInterface $storage) {
+    // If after adding this language the site will become multilingual, we need
+    // to rebuild language services.
+    if (!$language_manager->isMultilingual() && $this->isNew() && $language_manager instanceof ConfigurableLanguageManagerInterface) {
+      $language_manager->rebuildServices();
+    }

This is not quite that right. Firstly we don't need to do this for an update. Plus we only need to do this if the language_manager has become multilingual by adding more than one language.

Status: Needs review » Needs work

The last submitted patch, 36: drupal_2166923_36.patch, failed testing.

kfritsche’s picture

FileSize
2.06 KB
17.05 KB

Some tests still fail, this has to be further investigated, but I have to leave for now.

But fixed the most tests, because of a missing use...

Regarding #37:
Reworked this a little bit, like it was before. It is now only get fired if a new language gets added and if the site has now 2 languages.

kfritsche’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 39: drupal_2166923_39.patch, failed testing.

martin107’s picture

Issue tags: +Needs reroll
kfritsche’s picture

Is this still needed?

Most of the stuff is done in the ConfigurableLanguage, see #2336177: ConfigurableLanguage cannot be created/removed directly, integrate language_save() and language_delete().

Otherwise this can be closed.

sidharrell’s picture

Is this still active? Does it still need a reroll?

alexpott’s picture