Change public 'name' property access on languages to getName() and add back setName()

Attach reroll

I'm looking at this now too. :)

This needs to change name on Language.

I thought maybe this issue might also need to set label property on ConfigurableLanguage to protected... but. Maybe we should do this one as two different issues. ...
Or maybe we should ... but the question is, what method would we use? the label() or getName()?

I am about to reroll this.

Better title, tags.

(just a small note to remind myself)

using a var (and/or a type hint)
for

      foreach ($this->container->get('language_manager')->getLanguages($flags) as $langcode => $language) {
        $options[$langcode] = $language->isLocked() ? t('- @name -', array('@name' => $language->name)) : $language->getName();
      }

in LanguageSelectElementTest
would help

---
just a reroll.

next let's see what the bot says,
and also make the properties protected.

accidentally uploaded same one twice. I canceled one of them.

oh, looks like I missed one. wonder why it didn't fail. maybe we are missing test coverage for it is a locked language.

at first I did

+++ b/core/modules/system/src/Tests/Form/LanguageSelectElementTest.php
@@ -55,8 +55,10 @@ function testLanguageSelectElementOptions() {
     foreach ($ids as $id => $flags) {
       $this->assertField($id, format_string('The @id field was found on the page.', array('@id' => $id)));
       $options = array();
-      foreach ($this->container->get('language_manager')->getLanguages($flags) as $langcode => $language) {
-        $options[$langcode] = $language->isLocked() ? t('- @name -', array('@name' => $language->name)) : $language->getName();
+      /* @var $languages \Drupal\Core\Language\LanguageInterface[] */
+      $languages = $this->container->get('language_manager')->getLanguages($flags);
+      foreach ($languages as $langcode => $language) {
+        $options[$langcode] = $language->isLocked() ? t('- @name -', array('@name' => $language->getName())) : $language->getName();
       }
       $this->_testLanguageSelectElementOptions($id, $options);
     }

but get languages should know it is returning languages... so maybe a bit different...

yeah, I think this is better:

-      foreach ($this->container->get('language_manager')->getLanguages($flags) as $langcode => $language) {
-        $options[$langcode] = $language->isLocked() ? t('- @name -', array('@name' => $language->name)) : $language->getName();
+      /* @var $language_manager \Drupal\Core\Language\LanguageManagerInterface */
+      $language_manager = $this->container->get('language_manager');
+      foreach ($language_manager->getLanguages($flags) as $langcode => $language) {
+        $options[$langcode] = $language->isLocked() ? t('- @name -', array('@name' => $language->getName())) : $language->getName();

will do the protected next.

and making name protected on Language.

need to think about what to do with ConfigureableLanguage.

1.
#2341313: Convert all t() calls to $this->t() in views. added a ->name direct call. found it and fixed it to getName().

2.
LanguageManager tries to rename a language object... hm.

    // Add the site's default language if flagged as allowed value.
    if ($flags & LanguageInterface::STATE_SITE_DEFAULT) {
      $default = isset($default) ? $default : $this->getDefaultLanguage();
      // Rename the default language. But we do not want to do this globally,
      // if we're acting on a global object, so clone the object first.
      $default = clone $default;
      $default->name = $this->t("Site's default language (@lang_name)", array('@lang_name' => $default->getName()));
      $filtered_languages[LanguageInterface::LANGCODE_SITE_DEFAULT] = $default;
    }

should probably construct a new one with the new name?

3.
ContentTranslationSettingsTest

    $field_override = BaseFieldOverride::loadByName('entity_test_mul', 'entity_test_mul', 'name');

@penyaskito pointed out this might need to provide an extra method with a callback and not the property name.

4.
I'm not sure how to track down things like: Call to a member function on a non-object

Sample:
"Fatal error: Call to a member function getOption() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views_ui/src/Tests/DisplayCRUDTest.php on line 141
FATAL Drupal\views_ui\Tests\DisplayCRUDTest: test runner returned a non-zero error code (255)."

maybe run the test locally and look at the trace?

I thought maybe I could go back to a passing patch in #2226533: Changes to the Language class due to the LanguageInterface (followup) and see how this differed... but that one didn't make $name protected. :/

and good news ;) needs a reroll cause #2304403: Convert language:weight into a protected property

just the reroll.

Thanks. now the comment makes more sense. :)

+++ b/core/includes/update.inc
@@ -746,7 +746,9 @@ function update_language_list($flags = LanguageInterface::STATE_CONFIGURABLE) {
   if ($flags & LanguageInterface::STATE_SITE_DEFAULT) {
     $default = isset($default) ? $default : \Drupal::languageManager()->getDefaultLanguage();
     // Rename the default language.
-    $default->name = t("Site's default language (@lang_name)", array('@lang_name' => $default->getName()));
+    $default = new Language( array(
+     'name' =>  t("Site's default language (@lang_name)", array('@lang_name' => $default->getName()))
+    ));

maybe we dont need the $default right above that change either.
[edit]
this might not be so trivial as to just set the name on the new language... maybe we need to set other things it would have gotten from the getDefaultLanguage() like id (and maybe weight or direction).

this might be worth it's own little issue to fix this one test. I think we did a bunch of children out of the "Changes" issue to separate out tests that needed to be corrected before, so it wouldn't be too unusual to do that for this issue also.

+++ b/core/includes/update.inc
@@ -746,7 +746,9 @@ function update_language_list($flags = LanguageInterface::STATE_CONFIGURABLE) {
     $default = isset($default) ? $default : \Drupal::languageManager()->getDefaultLanguage();
     // Rename the default language.
-    $default->name = t("Site's default language (@lang_name)", array('@lang_name' => $default->name));
+    $default = new Language( array(
+     'name' =>  t("Site's default language (@lang_name)", array('@lang_name' => $default->getName()))
+    ));

This looks wrong. We are getting default language and simply overriding with name? don't we have setName()?

I agree with @vijaycs85 that code block looks wrong - the default language is not necessarily the default values from new Language() - I think we need a setter for this use case of altering the default language label.

+++ b/core/tests/Drupal/Tests/Core/PathProcessor/PathProcessorTest.php
@@ -9,6 +9,7 @@
 use Drupal\Core\Language\Language;
...
+use Drupal\Core\Language\Language;

yeah, auto-merge doesn't know namespace :) added twice.

what happened to the change to PathProcessorTest from #30 ?

(small nit, let's stick with array() syntax. #2135291: [Policy, no patch] PHP 5.4 short array syntax coding standards for Drupal 8 is not decided yet.)

only 2 fails and they were ..

[13-Oct-2014 11:39:28 UTC] Uncaught PHP Exception InvalidArgumentException: "The URI "base://>j*>&:}A" is invalid. You must use a valid URI scheme. Use base:// for a path, e.g., to a Drupal file that needs the base path. Do not use this for internal paths controlled by Drupal." at /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Url.php line 207	Fatal error	Unknown	0	Unknown	
[13-Oct-2014 11:39:28 UTC] Uncaught PHP Exception InvalidArgumentException: "The URI "base://>j*>&:}A" is invalid. You must use a valid URI scheme. Use base:// for a path, e.g., to a Drupal file that needs the base path. Do not use this for internal paths controlled by Drupal." at /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Url.php line 207

is a random fail
#2353347: Random failure in DisplayPathTest
retesting.

ok. that is green, but I think we want the setter and to use it in update.inc

adds the setName... to both Language and ConfigurableLanguage
thought about making a separate issue, but I think it might be ok as part of making name protected, so it's here.

not sure about the unit test, if it is the correct way to test a setter.

also I didn't add a unit test to ConfigurableLanguageUnitTest. We might need to do that. Not sure.

[edit]
I also wonder if we should put a comment on setName, and try and explain that most of the time, a language should be created with it's name passed in the constructor, and warn about when we *should* use setName()

+++ b/core/tests/Drupal/Tests/Core/Language/LanguageUnitTest.php
@@ -19,12 +19,15 @@ class LanguageUnitTest extends UnitTestCase {
+    $new_name = $this->randomMachineName();
+    $this->assertSame($language, $language->setName($new_name));

this doesn't seem correct to me. Cause if the setName didn't do anything at all, it could still pass.

well, here is a different assert.
I'll wait for feedback.

Just one more update for setName(). Everything else looks good.

oops, syntax error. here is the correct one.

ah, good.

I'm not sure about removing that clone... but ok. (there is a comment about why in #23 where it was taken out)

In this change, I'm just making the change in update.inc more like that one in LanguageManager

I think this needs a final review and might be rtbc now. oh, wait, it needs feedback (thinking) on the unit tests on setName().

Some nitpicks...

1. +++ b/core/includes/update.inc
   +++ b/core/includes/update.inc
   @@ -744,9 +744,10 @@ function update_language_list($flags = LanguageInterface::STATE_CONFIGURABLE) {
   

   We are missing the LanguageInterface import. Probably not in scope, however.

2. +++ b/core/includes/update.inc
   @@ -744,9 +744,10 @@ function update_language_list($flags = LanguageInterface::STATE_CONFIGURABLE) {
   -    $default->name = t("Site's default language (@lang_name)", array('@lang_name' => $default->name));
   +    $name = t("Site's default language (@lang_name)", array('@lang_name' => $default->getName()));
   +    $default->setName($name);
   

   We don't need the variable, its value is not reused.

3. +++ b/core/lib/Drupal/Core/Language/LanguageManager.php
   @@ -146,11 +146,10 @@ public function getLanguages($flags = LanguageInterface::STATE_CONFIGURABLE) {
   +      $name = $this->t("Site's default language (@lang_name)", array('@lang_name' => $default->getName()));
   +      $default->setName($name);
   

   Same here, we don't need $name.

51.1 yeah #2226533-305: Changes to the Language class due to the LanguageInterface (followup) has some clean up that will fix that. So not doing it here.

I'll take out the $name temp var now.

RTBC then.

Committed/pushed to 8.0.x, thanks!

Thanks!

wow! super thanks to everyone. :)