Changes to the Language class due to the LanguageInterface (followup)

Let's see what this breaks.

Lets Check this one

@visabhishek: that is not the goal behind this issue. All methods are to become public. If calling code fails because of this, that means it will need to be refactored to use the LanguageInterface getter and setter methods instead.

Also, whenever you post a patch that is not the first in an issue and is not a re-roll, please provide an interdiff.

@visabhishek: The patch from #8 is roughly how to solve this. It replaces many, but not all calls to the former public properties on \Drupal\Core\Language\Language with calls to the methods on \Drupal\Core\Language\LanguageInterface. Apart from converting any remaining property access, we will also need to convert type hints to \Drupal\Core\Language\Language to \Drupal\Core\Language\LanguageInterface

@Xano: Thanks for the IDEA, i will try to resolve this

This patch should at least make sure Drupal can be installed, which makes debugging easier.

@visabhishek the notes, hints for patchers and hints for reviewers in #2016679: Expand Entity Type interfaces to provide methods, protect the properties might provide more insight here. This is not strickly a child of that meta, but it is doing something similar, so we can consult that meta issue summary and even some of the child issues for understanding.

16: drupal_2226533_16.patch queued for re-testing.

Hm.. I wasn't aware of this issue :-/ It seems it in part duplicates #2246665: Typehint with Drupal\Core\Language\LanguageInterface instead Drupal\Core\Language\Language (or I suppose the other way around, but whatever...)

This issue is about completely replacing references to the class with references to the interface, where appropriate.

PHPUnit tests pass locally. Let's see what Simpletest does.

\Drupal\Core\Language\Language::sort() is a tricky piece of code that only works on arrays of languages that have the same properties as the default class.

reroll of #34. couldn't get an interdiff.

ah there was a old function name left. will fix that tomorrow.

Mix-up with another patch? Here's a proper re-roll of #34.

Just needed to reroll, waiting for the test to finish which I expect to fail actually...and after that begin to sort the tests that are failing

Still some stuff that needs to be changed. There seems to be a lack of get/set for the locked property which is needed.

another patch that won't go through but hopefully there is only one issue left that has to do with the LanguageInterface and that is the lack of get/set functions for property locked.

All other failings are not due to this change...

49: 2226533_49.patch queued for re-testing.

oh sorry apparently missed head by some inches so rerolling this patch

reroll of 47 and applying the changes I made for patch 49

reroll of #53, couldn't get an interdiff at the moment. Think something is seriously wrong with this since a lot of errors is coming from tests that has nothing to do with languageinterface to do

ok well this is kinda irritating and impossible when whatever new patch committed to core invalidate this patch.

is there a way to strip down this patch into smaller once?

I think you should assign this issue to yourself aslong as you are working on it

Reroll.

Looking to override #61 with fixes

@martin107 if you can get into irc and join #drupal-contribute that would be awesome.

#61 failed with

Fatal error: Cannot access protected property Drupal\Core\Language\Language::$id in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/user/user.install on line 223

and the interdiff for 61 and 62 is in user.install:

> -  $langcode = \Drupal::languageManager()->getDefaultLanguage()->id;
> +  $langcode = \Drupal::languageManager()->getDefaultLanguage()->getId();

seems reasonable. :)

----
now #62
has
Drupal\comment\Tests\CommentTranslationUITest 127 passes 52 fails 11 exceptions
Drupal\config_translation\Tests\ConfigTranslationUiThemeTest 15 passes 3 fails 1 exceptions
Drupal\config_translation\Tests\ConfigTranslationViewListUiT 30 passes 3 fails 1 exceptions
Drupal\config_translation\Tests\ConfigTranslationListUiTest 114 passes 32 fails 6 exceptions
Drupal\config_translation\Tests\ConfigTranslationOverviewTes 55 passes 12 fails 3 exceptions
Drupal\content_translation\Tests\ContentTestTranslationUITes 95 passes 55 fails 12 exceptions
Drupal\content_translation\Tests\ContentTranslationWorkflows 147 passes 12 fails 4 exceptions
Drupal\content_translation\Tests\ContentTranslationSettingsT 112 passes 2 fails 1 exceptions
Drupal\custom_block\Tests\CustomBlockTranslationUITest 105 passes 50 fails 11 exceptions

and then...

Drupal\system\Tests\Entity\EntityUUIDTest 88 passes

Fatal error: Call to a member function getString() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationUiTest.php on line 597
FATAL Drupal\config_translation\Tests\ConfigTranslationUiTest: test runner returned a non-zero error code (255).

and

Drupal\file\Tests\RemoteFileSaveUploadTest 254 passes

Fatal error: Cannot access protected property Drupal\Core\Language\Language::$locked in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Form/LanguageSelectElementTest.php on line 64
FATAL Drupal\system\Tests\Form\LanguageSelectElementTest: test runner returned a non-zero error code (255).
Drupal\system\Tests\Form\FormObjectTest 43 passes

...
and more...

we should probably use id(), not getId(), until #2246695: replace all core usages of id() with getid() is resolved.

@yesct if you keep on looking for the fatal erros in #62 which seems to pretty the same as I have there are some of them questionable if they really are language based.

about the id and getid. in the issue you referenced to if you read my comment again you'll see that I point out that in quite a lot of issues for creating get/set functions people have used id() + getid() and some cases only getid().

I've had a careful look at the difference between #56 and #62 nothing jumps out to explain my failed rebase ....

Looking at CommentTranslationUITest there are two exceptions :-

Exception Notice     ContentTranslatio  208 Drupal\content_translation\Tests\Co
    Undefined index:
    itDrupal\content_translation\Tests\ContentTranslationUITest->doTestAuthoringInfo()
    Drupal\content_translation\Tests\ContentTranslationUITest->testTranslationUI()
    Drupal\simpletest\TestBase->run()
    simpletest_script_run_one_test('1',
    'Drupal\comment\Tests\CommentTranslationUITest')

and

Exception Notice     ContentTranslatio  209 Drupal\content_translation\Tests\Co
    Undefined index:
    frDrupal\content_translation\Tests\ContentTranslationUITest->doTestAuthoringInfo()
    Drupal\content_translation\Tests\ContentTranslationUITest->testTranslationUI()
    Drupal\simpletest\TestBase->run()
    simpletest_script_run_one_test('1',
    'Drupal\comment\Tests\CommentTranslationUITest')

both cases relating to bad offsets "it" and "fr" in an piece of code like :-

$entity->translation[$langcode]['uid']

$langcode is the bad offset

This was saved by ( that is a language_save() ) in ContentTranslationTestBase::setupLanguages()

So something in the save/read is not longer working .. hmm

It late on Friday night for me ... it might be slow to unpick my mistakes...and this might be just one of many errors.
I will look at this again on Sunday ... sorry I can't look at it on Saturday

oh, I thought this was adding getId().
It's not! :) Sorry. So using getId() seems fine in this issue.

The reroll lasted less than 24 hours! This is a reroll of #62

I think I have made better choices this time...in the area of ContentTranslaionManagerAccessCheck::access() .. Here is what it would look like after patch

      switch ($operation) {
        case 'create':
          $source = language_load($source) ?: $entity->language();
          $target = language_load($target) ?: \Drupal::languageManager()->getCurrentLanguage(Language::TYPE_CONTENT);
          return ($source->getId() != $target->getId()
            && isset($languages[$source->getId()])
            && isset($languages[$target->getId()])
            && !isset($translations[$target->getId()])
            && $controller->getTranslationAccess($entity, $operation))
            ? static::ALLOW : static::DENY;

        case 'update':
        case 'delete':
          $language = language_load($language) ?: \Drupal::languageManager()->getCurrentLanguage(Language::TYPE_CONTENT);
          return isset($languages[$language->getId()])
            && $language->getId() != $entity->getUntranslated()->language()->getId()
            && isset($translations[$language->getId()])
            && $controller->getTranslationAccess($entity, $operation)
            ? static::ALLOW : static::DENY;
      }

Please shout if anything seems obviously wrong.

Let's see what testbot thinks...

Also from review in #66 - Fatal error: Cannot access protected property Drupal\Core\Language\Language::$locked ..
That look like an isolated problem which could be cherry picked but I am leaving this as a straight reroll just to avoid confusion

hi thanks for working on this @martin107, 24h is actually quite long time in this issue.

ehm was going to point out the small problems but I did a new patch continueing the work I had done simply cause I wanted to know what was going on. added that patch to a test issue to see what happends and it turned out to have the similar problems that patch in 71 have.
After a chatt with @yesct she suggested to diff my patch and #71 + ul the patch.

This patch will fail as #71 on same fatal errors, getvalue and getstring. and as earlier a lot of language tests are failing.

No major fix - Correcting access to protected properties. Reduces the exception count a bit.

More issues fixed in the same vein as #75

except in answer to reported error "Undefined LanguageInterface"

-  $language_content = \Drupal::languageManager()->getCurrentLanguage(LanguageInterface::TYPE_CONTENT);
+  $language_content = \Drupal::languageManager()->getCurrentLanguage(\Drupal\Core\Language\LanguageInterface::TYPE_CONTENT);

Others might think this is really saying the "use" statement was missing from the top of the file!

this is taking out the only usage (besides the test) of sortByWeightAndTitleKey()
why?
patch #30 was the one to introduce the change
in core/lib/Drupal/Core/Language/Language.php

-  public static function sort(&$languages) {
-    uasort($languages, 'Drupal\Component\Utility\SortArray::sortByWeightAndTitleKey');
+  public static function sort(LanguageInterface $a, LanguageInterface $b) {
+    if ($a->getWeight() == $b->getWeight()) {
+      return strnatcasecmp($a->getName(), $b->getName());
+    }
+    else {
+      return $a->getWeight() < $b->getWeight() ? -1 : 1;
+    }

adding some sort related issues.

also, if the typehint issue gets in first, this should be easier to review. adding it as related.

+++ b/core/modules/config_translation/lib/Drupal/config_translation/Access/ConfigTranslationOverviewAccess.php
@@ -69,7 +69,7 @@ public function access(Route $route, Request $request, AccountInterface $account
-      !$this->sourceLanguage->locked;
+      !$this->sourceLanguage->isocked();

+++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.php
@@ -455,7 +455,7 @@ public function searchFormAlter(array &$form, array &$form_state) {
-      $language_options[$langcode] = $language->locked ? t('- @name -', array('@name' => $language->name)) : $language->name;
+      $language_options[$langcode] = $language->islocked() ? t('- @name -', array('@name' => $language->getName())) : $language->getName();

I think these should both be isLocked()

been discussing this with @yesct and I think that the best approach to the Language::sort is to let it be as it is on head instead of the changes this patch is doing. simply because there are issues like #2239399: Languages should be sorted by label instead of title dealing with this.

The tests are written for that solution and if we change it and doesn't change the tests they are going to fail.

Breaking this interdiff into stages for review

1) As testing get further .. exposed more issues with accessing protected properties..interdiff-75-getters.txt

2) Fixed #81 ( hanging my head -- sorry for the distraction ) ...interdiff-75.locked.txt

3) Credit to filijonka for this code tidy ... interdiff-75-tidy.txt

1) Fixed more access to protected methods ... via getters.

2) No more white space.

I will sit on this .. until it goes green

Corrected a few more $language->getName() and $language->isLocked()

Ahem should have used search/replace for this a bit sooner.

tried the reverting this back to using the language::sort that is on head and that created a lot of errors.
basicly all test just gave that sort(null) instead of sort(array of languages).

So let's ignore that for now and see where we end up when it's green. perhaps these issues dealing with this function is sorted out by then.

this is yet another patch for the changing that we have begun.

ooh needs review...silly moi

The error above was caused by a check in language.module saying if(!empty($language->locked)) which I changed to correct function but since locked is a boolesk variable it should be sufficient to just do if(!$language->isLocked())

fixed that and did some more changes..

hmm had added a whitespace by accident and hopefully that was the reason for prev. failure.

I noticed somewhere that we had a line saying $language->direction ? ... : ...; and that was changed to getDirection.
but direction isn't a boolesk variable and afaik never has.

So my guess is that we're getting close to a good result but there are probalby some logical errors involved too.

yes this is a logcial error that turns up. working on it..

I have a fix for one of the test errors....from #87 ( Drupal\node\Tests\NodeTokenReplaceTest )

this->languageInterface->id becomes this->languageInterface->getId() in two places see below..

I will leave it here so we can come back to it.

But it highlights some non-core standard code...

$this->languageInterface->getId() !!!! ($languageInterface needs a name change )

There has been some discussion about how to reduce the size of the patch ... I will file a minor issue just for the name change.
rather than making our lives harder ( PS #2270339: $languageInterface is a misleading variable name in TokenReplaceUnitTestBase )

--- a/core/modules/node/lib/Drupal/node/Tests/NodeTokenReplaceTest.php
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTokenReplaceTest.php
@@ -89,7 +89,7 @@ function testNodeTokenReplacement() {
     $this->assertFalse(in_array(0, array_map('strlen', $tests)), 'No empty tokens generated.');
 
     foreach ($tests as $input => $expected) {
-      $output = $this->tokenService->replace($input, array('node' => $node), array('langcode' => $this->languageInterface->id));
+      $output = $this->tokenService->replace($input, array('node' => $node), array('langcode' => $this->languageInterface->getId()));
       $this->assertEqual($output, $expected, format_string('Sanitized node token %token replaced.', array('%token' => $input)));
     }
 
@@ -101,7 +101,7 @@ function testNodeTokenReplacement() {
     $tests['[node:author:name]'] = $account->getUsername();
 
     foreach ($tests as $input => $expected) {
-      $output = $this->tokenService->replace($input, array('node' => $node), array('langcode' => $this->languageInterface->id, 'sanitize' => FALSE));
+      $output = $this->tokenService->replace($input, array('node' => $node), array('langcode' => $this->languageInterface->getId(), 'sanitize' => FALSE));
       $this->assertEqual($output, $expected, format_string('Unsanitized node token %token replaced.', array('%token' => $input)));
     }

reverted some of the previous changes made by me and did some more changes.

some of the current errors seems to be due to the tricky part that we have two different kind of language interfaces. one in module and one in core.

1) Just wanted to comment for the record that #98 slips in the fixes from #97 thank you.( I was just about to do that )
NodeTokenReplaceTest is now green..

2) Its been suggest on IRC that I keep this issue updated with the concepts being developed as common currency in #2270339: $languageInterface is a misleading variable name in TokenReplaceUnitTestBase

$languageInterface is being replaced by $interfaceLanguage

As a way differentiate between the two

A) $interfaceLanguage the interface settings being used for the test and created :-
$this->languageInterface = \Drupal::languageManager()->getCurrentLanguage();

B) LanguageInterface:( @see \Drupal\Core\Language\LanguageInterface AND \Drupal\language\LanguageInterface )

As always naming things is hard, and the more eyes on this the better... so please direct comments to the other issue. I am aiming to put the other issue to bed soon after the 22nd of May

1.
#101 looks right, cause that language is a LanguageEntity which is a language/LanguageInterface (configurable)
which #2246679: Make Language module's LanguageInterface (to be ConfigurableLanguageInterface) extend Core's LanguageInterface will help clarify.
but we should probably revert it all the way back to what it was.

doing that.

2.
also,
fixing the typo
gtetId
from #89

Would have thought that would have fixed more than one fail.
Maybe the gtegId is in a section of code that isn't used, or isn't tested?

@yesct the reason for at least 140 of these fail and probably more is that in the language.module there are using both of the languageinterfaces. So sometimes it uses Drupal\Core\Language and sometimes it uses Drupal\Language\Language(?) Not sure I got the last one correct but basicly it means that sometimes it is e.g ->getId() and othertimes its ->id.

That is what I'm doing at the moment to figure out these errors.

Patch no longer applies ( this is just a straight reroll - lots of merging, auto merging and 2 trivial conflicts to resolve )

So that patch lasted about 4 hours -- Who knew Europe would wake up and go on a commit spree...

just reverted back some of the changes made in language module. went it through and think this would make it much better. i hope

So I hope to shine a spotlight on the ->getId() versus ->id debate - As it too is kicking me in the bum.

Looking at the recent conversion of book_language_entity_delete ( see book.module )

the function argument when I followed it back to its source function .. takes me to \Drupal\language\Entity\Language
where the magic getter function facilitates access to the id :-

  public function get($property_name) {}

So that should be reverted along with a few others I can see...

This is opposed to code of the form

\Drupal::languageManager()->getCurrentLanguage(LanguageInterface::TYPE_URL)->getId()

where the getId() can be traced to \Drupal\Core\Language\LanguageInterface. An separate Interface.

PS sorry cross post filijonka has beaten me to the fix

as soone as this test is done there will be a new patch which solves a big amount of errors I think.

made also sure it's still on head

reroll

As advised by filijonka this reroll is based on #118

Lots of merging, auto-merging and conflicts in rebase ...

Mostly this is a happy reroll because the side issue raised in #103 has landed in core.

The $languageInterface is being replaced by $interfaceLanguage issue.

5 conflicts in this reroll led to the selection of 4 tests for local testing as always testbot will give a more complete answer ...

php core/scripts/run-tests.sh  --url http://dev.drupal.co.uk --verbose --file ./core/tests/Drupal/Tests/Core/Entity/EntityManagerTest.php
php core/scripts/run-tests.sh  --url http://dev.drupal.co.uk --verbose --file ./core/modules/system/lib/Drupal/system/Tests/System/TokenReplaceUnitTest.php
php core/scripts/run-tests.sh  --url http://dev.drupal.co.uk --verbose --file ./core/modules/node/lib/Drupal/node/Tests/NodeTokenReplaceTest.php
php core/scripts/run-tests.sh  --url http://dev.drupal.co.uk --verbose --file ./core/modules/config/lib/Drupal/config/Tests/ConfigEntityTest.php

So the target is is back to the 48 fails of #118...

PS

I have been asked by the community to ensure that the distinction between ->id and ->getId is preserved, and is a concern myself...
The answer is it an ongoing thing that will come out right in the wash....

There are drives in separate issues show clear differences between

\Drupal\Core\Language\LanguageInterface and \Drupal\language\Entity\Language

When these initiatives are complete and have caused more 'Happy Rerolls' here then I will take the time to visual inspect this monster of a patch and post further comments

PPS see the fail below.... test bot reports out of memory errors will force retest again soon....

122: 2226533_122.patch queued for re-testing.

I am at the limit of what I can do tonight... so I am just pushing the obvious fix.

I am "unassigning" but not changing the amount I work on this .. I am still committed to lurking on this until its is green and stable.

BUT because YesCT and filijonka are working on this just as hard and I don't want to permanently signal that I am always working at this moment
I will try some more in about 12 hours.

Drupal\locale\Tests\LocaleContentTest now passes.

As a side note I have changed a local variable $language_interface to become $interface_language ( it holds a language object not an interface )
I know it should be $interfaceLangauge but then it would be out of keeping with the other un-CamelCased local variables in the file!

triggering testbot

ok so now back to 40 fails, different between this and #87 are that errors like fatal errors on getString and getValue are gone. Which was what I hoped for but also hoped that we would be a little be less than 40.

Cherry picking something obvious resulted in

./core/modules/system/lib/Drupal/system/Tests/Entity/EntityTranslationTest.php going green

Interface constants should be ONLY pulled from the interface directly not through an intermediate object

So changes are an iterations of the form

-    $field = $entity->getTranslation(Language::LANGCODE_DEFAULT)->get($this->field_name);
+    $field = $entity->getTranslation(LanguageInterface::LANGCODE_DEFAULT)->get($this->field_name);

PS I used git diff -M to create the patch .. but I am a bit suspicious about the file size ( 50kb versus 500kb )

nice catch! haven't seen those! great work @martin107

this is strange but doesn't really matter but those changes that are made in latest patch is already done in #118. I was looking at that local branch wondering why my grep didn't return these and they were already changed to LanguageInterface..

oh well still a good catch

@martin107 You need to do a diff without the M flag and mostly likely you need to reroll it to.

129: 2226533_129.patch queued for re-testing.

ok asking for a retest on #129 simply because when I applied that patch the changes that are in #133 interdiff are already there + when doing the tests locally they don't give the same results as in #129. So wanna see the result one more time

@filijonka so I understand from IRC that some of the tests pass locally for you

Could you post your PHP version ...

I am storing a link to the relevant test results here as the current links maybe overwritten by future retesting
https://qa.drupal.org/pifr/test/792483

testbot was php5.4

let's see what happends on testbot. I'm running on php 5.4.x as bot is.

the test parameter I changed is simply because locally the verbose message showed that there were more than 1 user on the system and since I used both cleaning the caches with drush and removed temporary tests we now it can exist more than 1 user. I'm just making sure to take a high number here and locally it got green then.

edit: adding that I don't think that is the solution I would just like to confirm if that is the case.

Since I don't know how this works on drupal but my thought is that if we really want to make sure that this test should pass we should choose a high number. In the back of my head it's simply like this: Can we now that all previous tests has cleaned up after itself so the setup for this test is the one adding a user?

hopefully this is down to one error.....

of course another reroll, auto-merged everything so no conflicts to handle
oh forgott to pull..coming new reroll soon

ok something must be very wrong with patch 145 so redid this based on patch 143

I've not seen this line @147 before in language.module but can't really find when it was introduced but I don't think it wasn't recently.

$language->is_new = $language_entity->isNew();

is_new is not a property of language as far as i can see but we should get fatals in that case..well let's see what the bot says

Sorry! I didn't have time to take this further in the sprint. An other issue came back to haunt me.

Please ignore. #147 is the active patch.

Straight reroll of #147.

Reroll form #147 because my recent patches had more errors..

This is a PSR-4 reroll!

Did you do it using the steps at:
https://groups.drupal.org/node/424758
?

@YesCT yes I did... although it looks like I messed something up .. locally I had test passing related to all conflicts I had to resolve!!!
Ii looks like HEAD moved on between submission and testbot testing.

Given :-

1) This issue has a history of frequent re rolling .. I notice several comments appealing for strategies on how to break this patch into parts..
I still have no useful insight into this question :-(

2) The commit spree juggernaut that is Austin is about to start.

I think this issue should be put aside for at least a week.

I starting to reroll this now

Now that #2246665: Typehint with Drupal\Core\Language\LanguageInterface instead Drupal\Core\Language\Language has been committed, lots of the changes that were made in both patches need to be unwound.
This patch is 40% smaller and should cause less conflicts in the future and so be easier to reroll.

I rerolled from #153 the last patch to at one time pass all tests.

This is not a happy reroll... When rebasing there were 120 conflicts to resolve! ( individual changes not files )

I am certain there will be some errors ... where there were non-trivial decisions to be made I tested the function until it passed

I am not expecting this to pass first time..

ALSO

Note in many files CoreLanguageInterface in this patch has become BaseLanguageInterface to reflect the current state of HEAD

some knots and kinks have been brushed out...

1) I fixed up these files in light of the PSR-4 changes.

core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.php -- removed
core/modules/node/lib/Drupal/node/Controller/NodeController.php -- change ported to correct file and duplicate deleted.
core/modules/node/lib/Drupal/node/NodeStorage.php -- both changes already in core, duplicate deleted.

I made a patch at this point to see how much more of a reduction this would make ( the patch was 55% of the #153 )

2) I backout some ... ahem reroll confusion from the following files.
TranslationLinkTest.php
LanguageServiceProvider.php

3) Duplicate use statement reported in test failure was removed.(#160)

Running all the unit test did not expose any problems BUT my expectation is there are more problems lurking...

My apologies For the record here is the interdiff I omitted.

Testing of the last patch ran only a little further and exposed another duplicate use statement issue similar to #162.3

I have listed all files changes in this patch 'git diff 8.x --name-only'
and through a little sed hackery I have run php -l on all relevant files...

So I don't think there are anymore linting issues like this for the test bot to find.

Does anyone know of a more convenient way of running the syntax checking part of testbot on my local machine ?

I was kind of expecting that running unit tests would identify these problems for me but hey-ho

So this time testbot should run deeper and expose more complex flaws..