Move language add logic out of language_save()

Why do we need to change the submit function?

isNew() is no longer TRUE in postSave() and the multilingual check needs to be done in preSave since we're trying to determine if the site is becoming multilingual.

So the order in which the default is set matters too

This is tagged as an IMP blocker. Is this also blocking deploying languages through config deploy? Since all the additional actions should happen on the entity being saved instead of global functions, right? Does that make this a bug, beta target, beta blocker or what? :)

@Gábor Hojtsy good point - it don't think it is a beta blocker because it does not change an API or data model - but it certainly would prevent a comprehensive test for #2224887: Language configuration overrides should have their own storage being written. That is one that tests a configuration sync that makes a site multilingual and overrides config in the same process..

1. +++ b/core/modules/language/language.module
   @@ -431,9 +431,7 @@ function language_save($language) {
   +  $language_entity->setDefault(!empty($language->default));
   

   This is confusing but due to the value vs. entity object differences. Not sure if possible to fix without further (unrelated) refactoring.

2. +++ b/core/modules/language/lib/Drupal/language/Entity/Language.php
   @@ -78,10 +81,42 @@ class Language extends ConfigEntityBase implements LanguageInterface {
   +  /**
   +   * Used to detect when the site becomes multilingual.
   +   *
   +   * @var bool
   +   */
   +  protected $multilingual;
   ...
   +    $this->preSaveMultilingual = \Drupal::languageManager()->isMultilingual();
   
   @@ -90,6 +125,54 @@ public function preSave(EntityStorageInterface $storage) {
   +    if (!$this->multilingual && !$update) {
   +      ConfigurableLanguageManager::rebuildServices();
   +    }
   

   Should also say not saved on the language entity, right?

   Also the variable names are not unified?! Does this even work? :)

3. Finally, how do we ensure those temporary values are not saved on the entity? There is no exported key list defined, so I assume now all of these would be exported, even if they are documented they are not, no? :)

@alexpott and @Gábor Hojtsy discussed this issue today and both suggested that it should actually be beta-blocking.

@alexpott explained #19/3 to me in IRC. The default behavior for saving config entities is only public property values are saved. So by defining $multilingual and $default as protected, they are not saved back. So that is fine. The variable name mismatch in #19/2 though makes the $preSaveMultilingual property public (runtime created), so it will be saved. So once that is fixed the use the right variable, it should be fine. Looks like this is very close, only needs that variable name fixed.

#23 good point - perhaps the best thing to here will be actually save the default value to the entity - the problem we have then is keeping it in sync. hmm....

Well, we can make the default language service be dynamic like the configurable language manager, when the language module is enabled it would derive this value from looking at which language is default by looking at all of them. Then we still need to ensure multiples are not defaults. Either way there is some coordination required.

Looks like needs work.

1. +++ b/core/modules/language/lib/Drupal/language/Entity/Language.php
   @@ -78,10 +81,45 @@ class Language extends ConfigEntityBase implements LanguageInterface {
   +  /**
   +   * Used to detect when the site becomes multilingual.
   +   *
   +   * This property is not saved to the language entity, but is needed for
   +   * detecting when to rebuild the services.
   +   *
   +   * @var bool
   +   */
   +  protected $preSaveMultilingual;
   ...
   +    $this->preSaveMultilingual = \Drupal::languageManager()->isMultilingual();
   

   Don't have better suggestions but this variable name and the documentation is not very clear.

2. +++ b/core/modules/language/lib/Drupal/language/Entity/Language.php
   @@ -90,6 +128,54 @@ public function preSave(EntityStorageInterface $storage) {
   +      \Drupal::service('language.default')->set($this->toLanguageObject());
   ...
   +  /**
   +   * Converts the Language entity to a Language value object.
   +   *
   +   * @return \Drupal\Core\Language\Language
   +   *   The language configuration entity expressed as a Language value object.
   +   */
   +  protected function toLanguageObject() {
   +    return new LanguageObject(array(
   +      'id' => $this->id(),
   +      'name' => $this->label(),
   +      'direction' => $this->direction,
   +      'weight' => $this->weight,
   +      'locked' => $this->locked,
   +      'default' => $this->default,
   +    ));
   +  }
   

   Since this is a beta blocker this is fine for now but we should add a @todo to remove this again after #2246665: Typehint with Drupal\Core\Language\LanguageInterface instead Drupal\Core\Language\Language and #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface.

3. +++ b/core/modules/language/lib/Drupal/language/Entity/Language.php
   @@ -90,6 +128,54 @@ public function preSave(EntityStorageInterface $storage) {
   +    $language_manager = \Drupal::languageManager();
   +    $language_manager->reset();
   +    if ($language_manager instanceof ConfigurableLanguageManagerInterface) {
   +      $language_manager->updateLockedLanguageWeights();
   +    }
   ...
   +    // If after adding this language the site will become multilingual, we need to
   +    // rebuild language services.
   +    if (!$this->preSaveMultilingual && !$update) {
   +      ConfigurableLanguageManager::rebuildServices();
   +    }
   

   Instead of hardcoding the class name here we could move the second if into the first.

   Note that its perfectly valid to call static methods non-statically. I.e. we could do

   if (!$this->preSaveMultilingual && !$update) {
     $language_manager->rebuildServices();
   }
   

   inside of the first if.

   Reading this code again, shouldn't ConfigurableLanguageManager simply override reset() to do that stuff itself?

   On the other hand this is just moving stuff around and this issue is a beta blocker, so feel free to leave to a follow-up.

1. Added @see's to how it used.
2. Added @todo's
3. Improved but it is not really possible to call static methods non statically... to quote https://stackoverflow.com/questions/15707029/calling-static-method-non-s...
   

   Yes, but you will get a warning, and God kills a kitten (don't do it).

Added code to ensure Language::$default is always correct after loading or creating and added a test. I don't think we should save the default value to the entity - maintaining this will be very messy.

+++ b/core/modules/language/lib/Drupal/language/Entity/Language.php
@@ -152,14 +176,17 @@
+   * @todo fix return type hint after https://drupal.org/node/2246665 and
+   *   https://drupal.org/node/2246679.

Actually I'm pretty sure the entire method can be removed after those, but as long as we have the todo we won't forget.

Improved but it is not really possible to call static methods non statically... to quote https://stackoverflow.com/questions/15707029/calling-static-method-non-s...

Yes, but you will get a warning, and God kills a kitten (don't do it).

This is actually not true. I tried this locally and there is no warning. PhpStorm does not complain either.

1. +++ b/core/modules/language/lib/Drupal/language/Entity/Language.php
   @@ -78,10 +80,70 @@ class Language extends ConfigEntityBase implements LanguageInterface {
   +      if (static::getDefaultLangcode() == $this->id()) {
   +        return TRUE;
   +      }
   +      else {
   +        return FALSE;
   +      }
   

   This looks like a classic example where return *thecondition* would have done the same?

2. +++ b/core/modules/language/lib/Drupal/language/Entity/Language.php
   @@ -90,15 +152,90 @@ public function preSave(EntityStorageInterface $storage) {
   +    if ($this->isDefault() && static::getDefaultLangcode() != $this->id()) {
   

   A higher level comment above would be great. This is for the case when this language was made the default but the global default is not yet up to date.

Fixed tests and nits reported in #31

Looks good to be. RTBC assuming it passes now :)

only one @todo, and it already mentions issues.
updated summary with that info.

+++ b/core/modules/language/lib/Drupal/language/Entity/Language.php
@@ -90,15 +147,92 @@ public function preSave(EntityStorageInterface $storage) {
+  /**
+   * {@inheritdoc}
+   */
+  public function get($property_name) {
+    if ($property_name == 'default') {
+      return $this->isDefault();
+    }
+    else {
+      return parent::get($property_name);
+    }
+  }

This doesn't need to block commit, but I'm curious why we need to support callers calling ->get('default') instead of requiring them to call ->isDefault(). Unless it's quick to do here, I'm fine with that being discussed and/or removed in a follow up.

Alex's quoted code also stuck out to me. It'd be great to get a code comment explaining why this method is overridden. Fine with that being a follow-up though; not worth holding a beta blocker up over this.

Committed and pushed to 8.x. Thanks!

Yay, superb, thanks all!