ConfigurableLanguage cannot be created/removed directly, integrate language_save() and language_delete()

Reviewing #3. This results in mostly much nicer code, yay :) Especially like the more standard entity form for language. As for where we log, I don't think we have a standard. I don't think we answered questions like "Do we want to log all the changes separately one by one when a config import happens?". We may want to ask that question sometime. Historically we have not been very consistent with our logging unfortunately.

1. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
   @@ -222,9 +228,29 @@ public static function preDelete(EntityStorageInterface $storage, array $entitie
   +//    @todo Prevents language uninstall :) since locked language entities are
   +//    deleted on uninstall.
   +//      if ($entity->locked) {
   +//        throw new \Exception('Can not delete a locked language ' . $entity->id());
   +//      }
   

   Right, we can remove it here. Do we have tests / core to ensure locked languages cannot be deleted through the UI though? Yes they don't appear on the list/form but they still have a delete URL that should throw a 403 or 404.

2. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
   @@ -319,4 +345,31 @@ public function setNegotiationMethodId($method_id) {
   +      return \Drupal::entityManager()->getStorage('configurable_language')->create(array(
   ...
   +      return \Drupal::entityManager()->getStorage('configurable_language')->create(array(
   

   Why not use static create on the class here? Like everywhere else.

3. +++ b/core/modules/language/src/Form/LanguageDeleteForm.php
   @@ -107,13 +107,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    \Drupal::logger('language')->notice('The %language (%langcode) language has been removed.', $t_args);
   

   Forms have an injected logger, no? Your other changes to forms use $this->logger

Awesome clean-up!
Suppose logging needs separate issue to unify (prefer forms because this is a place where visitors interacts by changing data).
Locking of config seems still tricky, the node-types is another example.
Maybe better to provide an actual locking of config with dependencies somehow

+++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
@@ -228,11 +228,6 @@
-//    @todo Prevents language uninstall :) since locked language entities are
-//    deleted on uninstall.

is there other way to prevent delete or module uninstall?

Yeah I tested admin/config/regional/language/delete/und manually too and it throws a 403 perfectly. I certainly don't have any more concerns with #7 and I reviewed the change notice draft as well on IRC and made suggestions for improvements. Looks good to me.

Also reclassifying and retitling as a bug, since the after-effects code in language_save and language_delete now non-standard wrapper functions makes it impossible to use ConfigurableLanguage to create and delete them without needing to take care of the after effects separately. Once we integrate them into the config entity, the after effects happen properly on eg. config import or any other use of the API as well.

I saw this was just a re-roll... so read through it hoping to just re-RTBC it. but... since I read through it:

0.
one thing to note is that so many 'name' to 'label' changes are because the object is changing from Language to ConfigurableLanguage and ConfigurableLanguage uses the 'label' property, not 'name'. ok.

1. +++ b/core/modules/language/language.module
   @@ -679,10 +604,9 @@ function language_form_system_regional_settings_alter(&$form, FormStateInterface
   +  $default_language->set('default', TRUE);
   

   this is in here a few times. shouldn't we use a setDefault() ?
   Probably a separate issue.

2. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
   @@ -225,6 +231,21 @@ public static function preDelete(EntityStorageInterface $storage, array $entitie
   +  public static function postDelete(EntityStorageInterface $storage, array $entities) {
   

   needs a doc block.
   nit. fixed.

3. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
   @@ -225,6 +231,21 @@ public static function preDelete(EntityStorageInterface $storage, array $entitie
   +
   +
   

   extra newline.
   nit.
   fixed.

4. +++ b/core/modules/language/src/Entity/ConfigurableLanguage.php
   @@ -319,4 +340,31 @@ public function setNegotiationMethodId($method_id) {
   +   * @param string $langcode
   +   *
   

   add the doc line.
   fixed.
   also added the @return for this new method.

5. +++ b/core/modules/language/src/Form/LanguageAddForm.php
   @@ -148,4 +139,25 @@ public function validatePredefined($form, FormStateInterface $form_state) {
   +
   +
   

   extra space.
   nit.
   fixed.

6. +++ b/core/modules/language/src/Tests/Condition/LanguageConditionTest.php
   @@ -44,17 +44,8 @@ protected function setUp() {
   -    $language = new Language(array(
   -      'id' => 'it',
   -      'name' => 'Italian',
   -      'direction' => Language::DIRECTION_LTR,
   -    ));
   -    language_save($language);
   +    ConfigurableLanguage::createFromLangcode('it')->save();
   

   just noting that in a bunch of places, this kind of simplification was done. Probably the test did not rely on the name or the direction. So now they are not customized. (create defaults to re-using the langcode as the label if label was not specified).

7. +++ b/core/modules/language/src/Tests/LanguageConfigurationElementTest.php
   @@ -85,11 +84,10 @@ public function testDefaultLangcode() {
   -    $old_default->default = FALSE;
   -    language_save($old_default);
   -    $new_default = \Drupal::languageManager()->getLanguage('cc');
   -    $new_default->default = TRUE;
   -    language_save($new_default);
   +
   +    $new_default = ConfigurableLanguage::load('cc');
   +    $new_default->set('default', TRUE);
   +    $new_default->save();
   

   I was wondering about this set default. why dont we have to unset the old default?

8. +++ b/core/modules/language/src/Tests/LanguageDependencyInjectionTest.php
   @@ -66,8 +65,9 @@ function testDependencyInjectedNewDefaultLanguage() {
   -    language_save($default_language);
   ...
   +    $default_language->set('default', TRUE);
   +    $default_language->save();
   

   was this broken before? why did you need to set the default to true?

9. +++ b/core/modules/language/src/Tests/LanguageFallbackTest.php
   @@ -25,11 +25,9 @@ protected function setUp() {
   +      $language = ConfigurableLanguage::createFromLangcode($langcode);
   +      $language->set('weight', $i--);
   +      $language->save();
   

   are we going to eventually convert this to pass in the weight to the create method? or have a setWeight()?

10. +++ b/core/modules/language/src/Tests/LanguageUILanguageNegotiationTest.php
    @@ -88,15 +89,10 @@ function testUILanguageNegotiation() {
         $default_language = \Drupal::languageManager()->getDefaultLanguage();
    
    @@ -108,7 +104,9 @@ function testUILanguageNegotiation() {
    -    language_save($default_language);
    +    $default_language = ConfigurableLanguage::load($default_language->getId());
    +    $default_language->set('default', TRUE);
    +    $default_language->save();
    

    I'm not sure about this. why all this setting up of the default language to replace the language_save()? $default_language is set up above. maybe it doesn't need to be now?

    Or... hm.

11. +++ b/core/modules/language/src/Tests/Views/LanguageTestBase.php
    @@ -26,11 +26,8 @@ protected function setUp() {
    -    // Create English and another language beside English.
    -    $language = new Language(array('id' => 'en'));
    -    language_save($language);
    -    $language = new Language(array('id' => 'xx-lolspeak', 'name' => 'Lolspeak'));
    -    language_save($language);
    +    // Create another language beside English.
    +    ConfigurableLanguage::create(array('id' => 'xx-lolspeak', 'label' => 'Lolspeak'))->save();
    

    ok. this is just taking out code creating english... which is silly, if the tests are a standard install, they will already have english.

12. +++ b/core/modules/locale/src/Tests/LocaleLocaleLookupTest.php
    @@ -31,15 +32,14 @@ public function setUp() {
    -    $new_language_default = new Language(array(
    +    ConfigurableLanguage::create(array(
           'id' => 'fr',
           'name' => 'French',
           'direction' => LANGUAGE::DIRECTION_LTR,
           'weight' => 0,
           'method_id' => 'language-default',
           'default' => TRUE,
    -    ));
    

    this does not match the same pattern as the other replacements. It is not using the set(weight). and it is not taking out the 'name' and using label, etc. hm. Maybe because this is followed by a save() and not by a drupalPostForm().

13. +++ b/core/modules/locale/src/Tests/LocaleStringTest.php
    @@ -40,8 +40,7 @@ protected function setUp() {
    -      $language = new Language(array('id' => $langcode));
    -      $languages[$langcode] = language_save($language);
    +      ConfigurableLanguage::createFromLangcode($langcode)->save();
    

    ok. $languages was unused anyway.

14. +++ b/core/modules/menu_ui/src/Tests/MenuLanguageTest.php
    @@ -38,11 +38,10 @@ protected function setUp() {
    +      ConfigurableLanguage::create(array(
             'id' => $language_code,
    -        'name' => $this->randomMachineName(),
    -      ));
    ...
    +        'label' => $this->randomMachineName(),
    +      ))->save();
    

    hm. but this create is changing 'name' to 'label'...

15. +++ b/core/modules/system/src/Tests/Entity/EntityTranslationFormTest.php
    @@ -36,12 +36,12 @@ protected function setUp() {
    -      $this->langcodes[$i] = $language->id;
    ...
    +      $this->langcodes[$i] = $language->id();
    

    this is good. but I'm not totally sure why it's necessary to change ->id to ->id() in this patch. but ok.

16. +++ b/core/modules/system/src/Tests/Theme/TwigTransTest.php
    @@ -69,9 +70,9 @@ protected function setUp() {
    -    $language->default = TRUE;
    ...
    +    $language->set('default', TRUE);
    

    why? are we setting default to protected here? no. I dont understand this change.

17. +++ b/core/modules/taxonomy/src/Tests/TermLanguageTest.php
    @@ -96,12 +95,9 @@ function testDefaultTermLanguage() {
    -    $old_default = \Drupal::languageManager()->getDefaultLanguage();
    -    $old_default->default = FALSE;
    

    why are we ok taking out unsetting the old default to false again?

18. +++ b/core/modules/views/src/Tests/Wizard/WizardPluginBaseUnitTest.php
    @@ -56,12 +56,11 @@ public function testCreateView() {
    -      'name' => 'Italian',
    +      'label' => 'Italian',
    

    we are not taking out the setting of name here? we took it out in other places where the label/name did not matter.

@YesCT: re point 0, yes the ConfigurableLanguage has a label instead of name so the API is a bit different. But @alexpott also fixed the form to reflect the ConfigurableLanguage properties, which is why $edit keys needed to change as appropriate also.

The minor cleanups in #15 are great, the further cleanups in #16 as well. I also agree with @alexpott's reasoning which is why there was so little in the end to correct from #7. So AFAIS back to RTBC.

@alexpott Thanks a lot for the response.
Looks good to me also.

hm berdir pointed out that this draft change record
https://www.drupal.org/node/2336669
got tweeted
https://twitter.com/drupal8changes/status/510226011301371904

I'm not sure where to report that.
#2292469: [meta] Improve change records doesn't mention the twitter bot.

[EDIT]
@tvn gave me the issue via irc: #2337375: Feed showing draft change records

re #17

+++ b/core/modules/language/src/Form/LanguageFormBase.php
@@ -72,11 +73,11 @@ public function commonForm(array &$form) {
-    $form['name'] = array(
+    $form['label'] = array(
       '#type' => 'textfield',
       '#title' => $this->t('Language name in English'),
       '#maxlength' => 64,
-      '#default_value' => $language->label,
+      '#default_value' => $language->label(),
       '#required' => TRUE,

ah, I see here in the LanguageFormBase. :)

YesCT re #19 #2337375: Feed showing draft change records

#2337585: add a Entity::deleteMultiple() to replace procedural entity_delete_multiple() for a question that came up in irc regarding this.

[edit: oops. posted to wrong issue.]

Committed/pushed to 8.0.x, thanks!

Yay, thanks a lot! Published the change record.