Protect Drupal\Core\Language\Language::id, and use getId()

I'm not sure I agree about postponing this issue - it certainly does not need to go in before the beta but it actually easy to reroll since the correct thing to do with be always to accept the changes from HEAD and redo the fixes - which is exactly what did not occur earlier so other changes got partially reverted in the patches.

I'm ok with rerolling this frequently.

I agree we should resolve the conflicts usually by keeping what is in head, and then doing what the patch is intending... usually changing ->id to ->getId() ... etc.

Fixed test fails.

Left #10.1 in since it related to this patch as we're using this method on the array elements returned by this patch. Removed #10.2 since yep it is unrelated change.

Rerolled.

Reroll.

Trying to fix all those ->id references.

Nope and the id properties are no longer protected... perhaps the patches should be combined.

Sorry for the screw up guys, it is my first patch work in a long time.

This should be the complete patch.

Fixing more instances of ->id.

1. +++ b/core/lib/Drupal/Core/Page/DefaultHtmlFragmentRenderer.php
   @@ -100,7 +100,7 @@ public function preparePage(HtmlPage $page, &$page_array) {
   -    $html_attributes['lang'] = $language_interface->id;
   +    $html_attributes['lang'] = $language_interface->getId();
   

   It is odd that we have ->id() on entities and ->getId()here, quite confusing actually.
   This bit should be directly at least described in the IS ...

2. +++ b/core/modules/book/book.module
   @@ -478,7 +478,7 @@ function template_preprocess_book_export_html(&$variables) {
      $attributes['dir'] = $language_interface->direction ? 'rtl' : 'ltr';
   

   This will have a small conflict with #2070737: Change values of LanguageInterface::DIRECTION_(LTR/RTL) to ('ltr'/'rtl'). Just to keep in mind, maybe.

3. +++ b/core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
   @@ -114,11 +114,15 @@ protected function setUp() {
   +    $language = $this->getMock('Drupal\Core\Language\LanguageInterface');
   +    $language->expects($this->any())
   +      ->method('getId')
   +      ->will($this->returnValue('en'));
   ...
   -      ->will($this->returnValue((object) array('id' => 'en')));
   +      ->will($this->returnValue($language));
   

   Much better if you ask me. Just as a general tip (not a review) you can use ->willReturnValue($language)); This safes a little bit of typing which is really nice.

4. +++ b/core/tests/Drupal/Tests/Core/PathProcessor/PathProcessorTest.php
   @@ -44,10 +45,8 @@ protected function setUp() {
   -    foreach (array('en' => 'English', 'fr' => 'French') as $langcode => $language_name) {
   -      $language = new \stdClass();
   -      $language->id = $langcode;
   -      $language->name = $language_name;
   +    foreach (array('en', 'fr') as $langcode) {
   +      $language = new Language(array('id' => $langcode));
          $languages[$langcode] = $language;
   

   What happened with the name? Should we just try to move this over?

@dawehner Since I'm not the original author of the patch I can't comment much on the issues you pointed out.

IMHO

1. Yes, I'm also confused about it. And one implementation of getId() is return $this->id() and the other one is return $this->id.

2. I just rerolled and got no conflict on that part. But got a conflict about the change from ContentEntityInterface to FieldableEntityInterface, that I solved in favor of FieldableEntityInterface.

Uploading just a reroll.

Yeah, so what should we do here. D8MI people?

renaming getId() to id() is a separate issue. and the opposite of .. #2246695: replace all core usages of id() with getid()

@tstoeckler suggested adding getId() to EntityBase and EntityInterface and marking id() deprecated. Anyway.. not the problem of this issue.

I rerolled the one from 11 to check the one from 24, which also needed a reroll.
The results were different. So I'm doing it again. (right now.)

patch coming in a bit.

one of the issues conflicting with this was #2341927: Language module annotation improvements [ plus minor followup ]
I'm not sure if we want it to be getId() ... guess can see if there are any fails.
$this->langcodes[$i] = $language->id();

anyway.

attached is the reroll of 11, and of 24.
and an interdiff of those.

for example,
- $this->assertTrue($this->entity->language()->getID() == 'en');
+ $this->assertTrue($this->entity->language()->getId() == 'en');

I think we should go with the reroll of 11. ... guess we should see what the testbot says.

I missed one.
This should pass.

oh, of course it needs a reroll. ... for ... #2123867: Simplify/cleanup language handling in EntityFormController
looks like those hunks just went away.

I will reroll this now.

automatic 3-way merge. no conflicts.

ok. *deep breath*

making id protected on ConfigurableLanguage also. Let's see how this does.

cool green again.

I read through all the lines of this patch.

Most are straight forward ->id to ->getId()

some are changes in the test that were setting id, and so have been reworked around doing that.

while reading I found one ->name to ->getName() that snuck in. I took that out.
And since starting all this a long time ago, I have noticed #2305593: [policy] Set a standard for @var inline variable type declarations so have fixed the change here to be better for more IDEs.

I think this is ready.

For a reviewer, try using the git --color-words in your diff to make reviewing easier.

I think we could actually get this done. putting on the sprint board for d8mi.

I am rerolling this now.

conflicts were from
a context line removed in #2350915: Don't require bundle option to be passed in when creating a MenuLinkContent entity
a context line changed in #2347465: Convert all instances of #type link/links to convert to use routes
context lines changed in #2070737: Change values of LanguageInterface::DIRECTION_(LTR/RTL) to ('ltr'/'rtl')
no tricky things.

oops. I missed a conflict.
fixed.

  $node = $variables['node'];
  if ($node->language()->getId() != LanguageInterface::LANGCODE_NOT_SPECIFIED) {

$node did not know what kind of thing it is, so I told it with a @var.

+++ b/core/tests/Drupal/Tests/Core/PathProcessor/PathProcessorTest.php
@@ -44,10 +45,8 @@ protected function setUp() {
-      $language->name = $language_name;

Looks like we missed the name while converting. Except that, +1 to RTBC.

The changes spread across all over the core. Would be great to get it in in before any other issues to avoid merge conflicts and reroll pains.

well, name is being done in #2341341: Change public 'name' property access on languages to getName() and add back setName()

ah, in this hunk in PathProcessorTest

     // Set up some languages to be used by the language-based path processor.
     $languages = array();
-    foreach (array('en' => 'English', 'fr' => 'French') as $langcode => $language_name) {
-      $language = new \stdClass();
-      $language->id = $langcode;
-      $language->name = $language_name;
+    foreach (array('en', 'fr') as $langcode) {
+      $language = new Language(array('id' => $langcode));
       $languages[$langcode] = $language;

Yeah, when we converted this test instead of making a new object and then setting the id and name, to: passing the needed info in the constructor.. that name was not needed at all. So that is why the line is taken out and a corresponding line not added back. [edit] So I think this is an ok change here.

Alright. let's get this in.

1. +++ b/core/modules/user/src/Tests/UserInstallTest.php
   @@ -47,8 +47,8 @@ public function testUserInstall() {
   -    $this->assertEqual($anon->langcode, \Drupal::languageManager()->getDefaultLanguage()->id);
   -    $this->assertEqual($admin->langcode, \Drupal::languageManager()->getDefaultLanguage()->id);
   +    $this->assertEqual($anon->langcode, \Drupal::languageManager()->getDefaultLanguage()->getId(), 'Anon user language is the default.');
   +    $this->assertEqual($admin->langcode, \Drupal::languageManager()->getDefaultLanguage()->getId(), 'Admin user language is the default.');
   

   The test assertion message here is less useful to a developer than the default - which would tell you what the two langcodes are.

2. +++ b/core/tests/Drupal/Tests/Core/Utility/TokenTest.php
   @@ -70,8 +70,10 @@ public function testGetInfo() {
   -    $language = $this->getMock('\Drupal\Core\Language\Language');
   -    $language->id = $this->randomMachineName();
   +    $values = array('id' => $this->randomMachineName());
   +    $language = $this->getMockBuilder('\Drupal\Core\Language\Language')
   +      ->setConstructorArgs(array($values))
   +      ->getMock();
   

   This is not a real mock :) Since we don't actually mock any functions might as well just create a language object. If we want to use mocks then we should mock the interface and the getId() method. However this is out-of-scope for this change and should be dealt with in a new issue.

Neither of the two above issues should hold up this change - let's get this done.

Committed/pushed to 8.0.x, thanks!

wow! thanks so much everyone!

--

opened issues for things noticed in #45

#2355543: TokenTest Language mock is not accurate

#2355545: UserInstallTest testUserInstall() custom assert message not more helpful than the default.

Thanks!