"Translate" local task always visible, leads to WSOD

This makes sense, I think the other routes would be good to fix here, they should hopefully only be a couple lines. :) IMHO it would be superfluous to open even more issues for them.

I think next time it was set to review, it would go to the testbot anyway, so might as well send it now.

I'm rerolling this.

easy one. no interdiff because it's a reroll.

conflict was in ContentTranslationRouteSubscriber

          '_entity_type_id' => $entity_type_id,
        ),
        array(
<<<<<<< HEAD
          '_access_content_translation_overview' => $entity_type_id,
          '_permission' => 'translate any entity',
=======
          '_access_content_translation_overview' => $entity_type,
>>>>>>> 5 on feb 6
        ),
        array(
          'parameters' => array(

patch was just taking out a line, and a context line above it changed:

@@ -55,10 +55,8 @@ protected function alterRoutes(RouteCollection $collection, $provider) {
         ),
         array(
           '_access_content_translation_overview' => $entity_type,
-          '_permission' => 'translate any entity',
         ),
         array(
-          '_access_mode' => 'ANY',
           'parameters' => array(
             'entity' => array(
               'type' => 'entity:' . $entity_type,

So I kept the context line from head and took out the line.

yielding:

        ),
        array(
          '_access_content_translation_overview' => $entity_type_id,
        ),
        array(
          'parameters' => array(
            'entity' => array(
              'type' => 'entity:' . $entity_type_id,

was caused by #2165155: Change $entity_type to $entity_type_id and $entity_info to $entity_type/$entity_types

Looks like the contextual links code / tests still need some work :/

Ok, after few hours investigation, found some wired behaviour with contextual link

1. Contextual links do not get updated when change content translation settings on admin/config/regional/content-language. However localTask does(both localTask and Contextual links are Derivatives).
2. Contextual links get rendered only When we update the node/entity.

Going to work on this now - #DrupalDevDays

Did first some changes, because menu_router_rebuild was removed (#2107533: Remove {menu_router}) and field config entities were renamed.

The problem here is that the access callback does two different things. Checking if the entity is translatable and the access to translate the entity.

I did some refactoring to move the first check out of the access callback and the access callback is really only checking if you are allowed to translate the content.

Therefore Eric and me decided to introduce a new method at the Entity to check if it is translatable and moved the old isTranslatable to isBundleTranslatable, because thats what it was.

The "Content translation contextual links" is now working. Lets see what else this maybe breaks now ;)

Fixing the failing tests.

Was an error because of the changing of splitting the access hook into two separate functions and missed a location.
Also we do not save anything anymore in content_translation if the content is not translatable, like the description of the NodeTranslationUITest::testDisabledBundle noted, but looked before that there is one row in it.

One more problem is detected:
The "translate" link is visible on the "admin/content/comment" page, even if only one language is enabled.

Fixed the unit test, but I'm not very confident that this is a good way. Would need some input here how to do it better.
What I did was - added a mock for TypedDataManager::getPropertyInstance which returns FieldItemList, so that the langcode can be set and the defaultLanguage for the entity will be set. But as it is a Mock it always returns LANGCODE_NOT_SPECIFIED and thats why I added this to the return of LanguageManagerInterface::getLanguages, but with the english language. I know this is incorrect and should be fixed. But I'm not sure if we can somehow Mock the getPropertyInstance in a better way?
Another option would be to Mock $entity->get('langcode'), so we are skipping all of this. But that would mean, that we create this Mock somehow before we construct the $entity. Is that possible somehow?

Also renamed isBundleTranslatable to supportsTranslation, like it will be used in #2143291: Clarify handling of field translatability and I feel that this name is much better.

If the general idea of this is accepted, the other locations should be fixed too in a similar way.
These are:
* Route alter for '/add/{source}/{target}'
* Route alter for '/edit/{language}'
* Route alter for '/delete/{language}'

21: 2188675-21-content_translation-overview_access.patch queued for re-testing.

Reroll of #2188675-21: "Translate" local task always visible, leads to WSOD.

Fixed reroll for #21, replacing #26.

The code below, from ContentEntityBaseUnitTest.php:

<<<<<<< HEAD
=======
    $this->entityManager->expects($this->any())
      ->method('getFieldDefinitions')
      ->with($this->entityTypeId, $this->bundle)
      ->will($this->returnValue(array(
        'id' => array(
          'type' => 'integer_field',
        ),
        'langcode' => array(
          'type' => 'language',
        ),
      )));
>>>>>>> Apply ../patches/2188675-21-content_translation-overview_access.patch to reroll.

this patch was trying to add the langcode array element to a hunk of code that was removed in #2211949: Support keeping new revision id for migrate, so the resolution of the conflict was to keep the code from HEAD (nothing) – in other words this change was left out.

Also picked up the change in the context line (up casing of Translate) to resolve the conflict below:

<<<<<<< HEAD
    if (content_translation_translate_access($entity)) {
      $text = !empty($this->options['text']) ? $this->options['text'] : t('Translate');
=======
    if ($entity instanceof ContentEntityInterface && $entity->isTranslatable() && content_translation_translate_access($entity)) {
      $text = !empty($this->options['text']) ? $this->options['text'] : t('translate');
>>>>>>> Apply ../patches/2188675-21-content_translation-overview_access.patch to reroll.

This is how I resolved it:

    if ($entity instanceof ContentEntityInterface && $entity->isTranslatable() && content_translation_translate_access($entity)) {
      $text = !empty($this->options['text']) ? $this->options['text'] : t('Translate');

Bow to the 'bot's sensibilities.

When you're up to your posterior in re-rolling sometimes it is hard to remember that the original mission was to review the patch…

So here goes. After applying the patch from #2188675-28: "Translate" local task always visible, leads to WSOD the resulting behavior is:

I think that covers the expected combinations and they work correctly. I suppose if I were a better person I would have written the tests for these…

After reading the patch, I have a few comments/questions:

#1

+++ w/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -221,6 +221,15 @@ public function getRevisionId() {
+    // Check that the bundle is translatable, the entity has a language defined
+    // and if we have more than one language on the site.
+    return $this->supportsTranslation() && empty($this->getUntranslated()->language()->locked) && $this->languageManager()->isMultilingual();
+  }

Question: assuming the conditions are in order, I don't understand how the second test works – is it really that convoluted to find out of an entity has a language defined. I'm also wondering what it means for a entity to have a "language defined." I would think that the tests should be: 1) Is the entity translatable (i.e. the content type has the translatable box checked), 2) Is this a multilingual site, and 3) Do we have more than one language installed on the site. Is that what the test is doing? Or am I thinking about this wrong?

#2

+++ w/core/modules/content_translation/content_translation.module
@@ -245,9 +245,8 @@ function _content_translation_menu_strip_loaders($path) {
  * @param \Drupal\Core\Entity\EntityInterface $entity
  *   The entity whose translation overview should be displayed.
  */
-function content_translation_translate_access(EntityInterface $entity) {
-  return $entity instanceof ContentEntityInterface && empty($entity->getUntranslated()->language()->locked) && \Drupal::languageManager()->isMultilingual() && $entity->isTranslatable() &&
-    (user_access('create content translations') || user_access('update content translations') || user_access('delete content translations'));
+function content_translation_translate_access(ContentEntityInterface $entity) {
+  return (user_access('create content translations') || user_access('update content translations') || user_access('delete content translations'));

The @param and the actual parameters to the function need to match.

#3

+++ w/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -221,6 +221,15 @@ public function getRevisionId() {
   public function isTranslatable() {
+    // Check that the bundle is translatable, the entity has a language defined
+    // and if we have more than one language on the site.
+    return $this->supportsTranslation() && empty($this->getUntranslated()->language()->locked) && $this->languageManager()->isMultilingual();
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function supportsTranslation() {

These two functions seem like they are doing about the same thing. Is that true? If they are could they be implemented as one function or named similarly, say isEntityTranslatable() and isBundleTranslatable()?

#4

+++ w/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
@@ -229,6 +235,22 @@ public function testGetRevisionId() {
+    $this->assertFalse($this->entity->supportsTranslation());

Either: 1) Schrödinger's cat is lurking nearby, or 2) this test doesn't actually run, or 3) it works for some reason that could really use a comment…

#5

+++ w/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
@@ -240,8 +262,21 @@ public function testIsTranslatable() {
+    $this->assertFalse($this->entity->isTranslatable());

The cat again?

A couple of the problems (if YesCT is to be believed) noted in the review above, are core gates that will have to be resolved before RTBC.

Thanks for the review and great report in #31. here is some updates on them:

#31.1 - Yes, it checks for translatable and multilingual site (internally multi-lingual is true, we have more than one language. Check core/modules/language/lib/Drupal/language/ConfigurableLanguageManager::isMultilingual for more info).

#31.2 - Fixed.

#31.3 - supportTranslation() is more of configurable and renaming the method is out of this issue scope.

#31.4 - its the mock object that does the magic. As you can see few lines above assert

    $this->entityManager->expects($this->at(0))

We return true only first time when 'getBundleInfo' method call.

#31.5 - Same as #31.4.

Thanks for taking the time to explain the magic of PHP Unit and what goes on in the test for locked languages.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -221,6 +221,15 @@ public function getRevisionId() {
+  public function supportsTranslation() {

+++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
@@ -91,11 +91,19 @@ public function addTranslation($langcode, array $values = array());
+  public function supportsTranslation();

Please, let's not make this method part of the public API, as we have not reached an actual consensus on the "enabled" vs "supported" terminology yet (see #2143291: Clarify handling of field translatability). We can make it public and add it to the interface later, if that turns out to be the solution we agreed upon. Doing the opposite would imply an API change.

Also, supportsTranslation() here means the opposite of what we are referring to with "translation support" in the linked issue: isTranslationEnabled() would be more like it.

Sorry for my fault push back for @dlu review. Didn't realise that method introduced in this issue. +1 for isTranslationEnabled.

Hi Plach and vijaycs85,

I have changed supportsTranslation() as isTranslationEnabled() and made it as Protected. Updating the patch with these 2 changes.

Removed isTranslationEnabled() from TranslatableInterface.php and updating the patch

Hi,

Error in the previous patch was because of changing method isTranslationEnabled() to protected.
(PHP Fatal error: Call to protected method Drupal\Core\Entity\ContentEntityBase::isTranslationEnabled()).

PS: Did not get the above error when running tests locally.

the patch in #33 has it is public. just making the renaming.

Some changes are not clear to me in the patch:

1. +++ b/core/modules/content_translation/content_translation.module
   @@ -242,12 +242,11 @@ function _content_translation_menu_strip_loaders($path) {
   -function content_translation_translate_access(EntityInterface $entity) {
   -  return $entity instanceof ContentEntityInterface && empty($entity->getUntranslated()->language()->locked) && \Drupal::languageManager()->isMultilingual() && $entity->isTranslatable() &&
   -    (user_access('create content translations') || user_access('update content translations') || user_access('delete content translations'));
   +function content_translation_translate_access(ContentEntityInterface $entity) {
   +  return (user_access('create content translations') || user_access('update content translations') || user_access('delete content translations'));
    }
   

   Where is this access function used? I assume it is used in combination with the entity access check or otherwise the removal of the conditions would not be possible(?). Is the new idea that entity specific checks would be in the entity access checker and global things kept here?

   Looks confusing on a quick look.

2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php
   @@ -204,13 +204,11 @@ public function testDisabledBundle() {
   +    // Make sure that nothing was inserted into the {content_translation} table.
        $rows = db_query('SELECT * FROM {content_translation}')->fetchAll();
   -    $this->assertEqual(1, count($rows));
   -    $this->assertEqual($enabledNode->id(), reset($rows)->entity_id);
   +    $this->assertEqual(0, count($rows));
   

   Why would this now change? None of the other code in the file changed?

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -221,6 +221,15 @@ public function getRevisionId() {
+  public function isTranslationEnabled() {

Please, let's make this protected and remove the unit test. We can restore both if needed but there is really no reason to expose this detail in the API atm.

43: 2188675-translate-local-task-43.patch queued for re-testing.

1. Reroll
2. Changes as per #45
3.
#44.1 ContentEntityBase::isTranslatable() has got the checks already covered.
#44.2 - not sure, Seems the test started failing because of this change in #15.

Thanks for making this work again :) I guess I'm still hung up on the same two things. I just don't know enough and need to be enlightened.

1. +++ b/core/modules/content_translation/content_translation.module
   @@ -243,12 +243,13 @@ function _content_translation_menu_strip_loaders($path) {
   - * @param \Drupal\Core\Entity\EntityInterface $entity
   + * @param \Drupal\Core\Entity\ContentEntityInterface $entity
   + * @param \Drupal\Core\Entity\ContentEntityInterface $entity
   

   Param documented twice.

2. +++ b/core/modules/content_translation/content_translation.module
   @@ -243,12 +243,13 @@ function _content_translation_menu_strip_loaders($path) {
   +function content_translation_translate_access(ContentEntityInterface $entity) {
   +  $account = \Drupal::currentUser();
   +  return ($account->hasPermission('create content translations') || $account->hasPermission('update content translations') || $account->hasPermission('delete content translations'));
    }
   

   So as I said above this is much less checks than it was before. How is this used in combination with the above methods? How does it ensure all the same checks are still done?! Why did they move places now?

3. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeTranslationUITest.php
   @@ -204,13 +204,11 @@ public function testDisabledBundle() {
        $this->drupalCreateContentType(array('type' => $disabledBundle, 'name' => $disabledBundle));
    
        // Create a node for each bundle.
   -    $enabledNode = $this->drupalCreateNode(array('type' => $this->bundle));
   +    $this->drupalCreateNode(array('type' => $this->bundle));
    
   -    // Make sure that only a single row was inserted into the
   -    // {content_translation} table.
   +    // Make sure that nothing was inserted into the {content_translation} table.
        $rows = db_query('SELECT * FROM {content_translation}')->fetchAll();
   -    $this->assertEqual(1, count($rows));
   -    $this->assertEqual($enabledNode->id(), reset($rows)->entity_id);
   +    $this->assertEqual(0, count($rows));
      }
   

   Still don't understand this change. Its doing the same logic but then a different check and still passes? How is this patch making no entry appearing anymore in that table? How is that related?

I've re-rolled the patch, and then based on Gábor's review have separated the tests in the patch to run separately. We can then hopefully get a better idea of why the test change was made and whether the tests are failing properly before the patch and succeeding now because of the patch.

I have also updated the summary with some screenshots and remaining tasks.

So do I understand it right that the screenshots are pre-patch? That is not what we expect to happen WITH the patch.

Correct, @Gábor, the screenshots just demonstrate the problem, not the fix.

There are many more failures/exceptions with the tests only than I would have expected. Are these reasonable? Also, the rerolled patch is now failing because of a missing "cherry" plugin. Was this something introduced in the last two weeks since the last patch?

50: 2188675-translate-local-task-50.patch queued for re-testing.

There was some random fails. It is green now.

Can someone respond to my concerns in #49? Instead of leaving this lingering, we could get this in. We only need me understand it or someone else who understands it to positively review :) This been going on for so long. Let's get there soon?

Sorry @Gábor Hojtsy. I thought of having a deeper look for the update and missed this issue. Here is the update:

#49.2:

We had 5 checks on content_translation_translate_access.
1. $entity instanceof ContentEntityInterface
2. empty($entity->getUntranslated()->language()->locked)
3. \Drupal::languageManager()->isMultilingual()
4. $entity->isTranslatable()
5. (user_access('create content translations') || user_access('update content translations') || user_access('delete content translations'));

Here is the status of updated code on each item above:
#1 This has been removed because content_translation_translate_access got the typehint as ContentEntityInterface instead of EntityInterface. Guess, this can produce exception for access check. So we can rollback this change.
#2 and #3 aren't specific to content translation. So they are part of $entity->isTranslatable() now.
#4 called explicitly on every place where we checked content_translation_translate_access. IMHO, this can be moved back to content_translation_translate_access
#5 already there in content_translation_translate_access

#49.3:
It looks like the test used to be checking wrong behaviour. The initial comment says 'Create a bundle that does not have translation enabled.', then asserting one row in content_translation table?. The update make sure we don't create a row, if no content translation.

here is a rerolled batch (for PSR-4) and updates.

Impact of #2167167: Remove field_info_*()

Yeah, that we always call $entity->isTranslatable() along with calling access sounds problematic. Once someone just calls the access check and assume the response is trustable they will get bitten. I don't have strong opinions on the typehint, if the hook signature does not allow this, then better not do it, but if we can do this, then its fine.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -237,6 +237,18 @@ public function getRevisionId() {
+    return $this->isTranslationEnabled() && empty($this->getUntranslated()->language()->locked) && $this->languageManager()->isMultilingual();

the empty($this->getUntranslated()->language()->locked) part causing the fail. Not sure if anything changed recently around this.

Spent couple of hours on this today. let's add credit to this sprint :)

While working through the case matrix in #31, I was not able to enable translation for users, so logged a separate issue, #2300347: Regression: checkbox to enable translation for users does not save .

Back to work on this issue :)

@vijaycs85 I'm rerolling :)

Attached reroll

+++ b/core/modules/content_translation/src/Tests/ContentTranslationContextualLinksTest.php
@@ -55,11 +63,48 @@ class ContentTranslationContextualLinksTest extends WebTestBase {
+    entity_create('field_config', array(

This is now called field_storage_config, this is the reason for the exception.

Updated string field_config -> field_storage_config per #72

I'm sorry vijaycs85 because I know you are working on this, but I was very curious about this bug. :)

I think that I found the problem... Adding 'und' as langcode by default.

Hope it helps!

Cheers!!

I reviewed the states in the matrix in #31 and all the right behaviors of the translate link and resulting translate page appear to be working properly, correctly hiding and showing the link and page per the matrix. I've also separated the current tests only into their own patch in order to review the failures.

Has the concern in #62 been resolved yet? I see from the code that $entity->isTranslatable() is still called every time from within function content_translation_translate_access. Other than possibly that comment, it appears any concerns have been addressed.

Has the concern in #62 been resolved yet?

Actually it is answer for #58.4. As @Gábor Hojtsy current implementation doesn't harm and may introduce problems if we plan to merge them together.

I'm sorry vijaycs85 because I know you are working on this, but I was very curious about this bug. :)

- No problem. Assigned to me as I moved away from this issue sometime back and want to keep it in my list.

Overall, patch in #75 is ready for review.

The patch looks good to me, but it would be important to get validation from @plach that the content entity base changes are good. He had concerns about exactly this in #35.

Changes to the base content entity look fine to me, thanks.

All righto, looks good to both of us then :)

Needs a re-roll...

Back to RTBC

1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -246,6 +246,18 @@ public function getRevisionId() {
   +    return $this->isTranslationEnabled() && empty($this->getUntranslated()->language()->locked) && $this->languageManager()->isMultilingual();
   ...
   +  /**
   +   * Returns the translation support status of the bundle.
   +   *
   +   * @return bool
   +   *   TRUE if the bundle of the object has translation support enabled.
   +   */
   +  protected function isTranslationEnabled() {
   

   Why are we creating this new method? Its only use is in isTranslatable().

2. +++ b/core/lib/Drupal/Core/TypedData/TranslatableInterface.php
   @@ -92,7 +92,7 @@ public function addTranslation($langcode, array $values = array());
      /**
   -   * Returns the translation support status.
   +   * Returns the translation support status of the entity.
   

   We're dealing with typed data here of which entities are an example. Not sure the docblock change is necessary.

3. +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -223,8 +223,6 @@ protected static function buildLinks(CommentInterface $entity, EntityInterface $
   -    $container = \Drupal::getContainer();
   -
   

   Unrelated change.

4. +++ b/core/modules/content_translation/content_translation.module
   @@ -251,8 +251,9 @@ function _content_translation_menu_strip_loaders($path) {
    function content_translation_translate_access(EntityInterface $entity) {
      $account = \Drupal::currentUser();
   +
      return $entity instanceof ContentEntityInterface && empty($entity->getUntranslated()->language()->locked) && \Drupal::languageManager()->isMultilingual() && $entity->isTranslatable() &&
   -    ($account->hasPermission('create content translations') || $account->hasPermission('update content translations') || $account->hasPermission('delete content translations'));
   +  ($account->hasPermission('create content translations') || $account->hasPermission('update content translations') || $account->hasPermission('delete content translations'));
   

   Unrelated change (as far as I can tell)

5. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -7,6 +7,7 @@
   +use Drupal\Core\Entity\ContentEntityInterface;
   

   Unrelated change

6. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
   @@ -253,8 +259,21 @@ public function testIsTranslatable() {
        $this->assertTrue($this->entity->isTranslatable());
        $this->assertFalse($this->entity->isTranslatable());
   +    $this->assertFalse($this->entity->isTranslatable());
   

   This looks like madness - I think it is worth breaking this test into 3 separate test methods. $this->at() is very fragile and kind of horrible.

This results in WSOD, because the translate tab will show, it will display the list of languages, but going to one of the pages from the list will WSOD. Until you actually configure it to support translation. If the user has no access to the routes in the first place, then we avoid that WSOD.

Reproduced this WSOD as part of my Drupalaton training. Elevating to critical due to the WSOD.

Updated and shortened the issue summary.

#84.1 - Please check #58. Answers same question asked by @Gábor Hojtsy at #49.
#84.2 - Reverted.
#84.3 - Reverted.
#84.4 - Reverted.
#84.5 - Reverted.
#84.6 - fixed.

#86 was agains 8.x :(.

Missed few test fixes on merge conflict...

I think the only remaining concern was #84/1 and I see no reason not to do that. A minor change.

I only did a minor update. @vijaycs85 addressed other concerns from @alexpott.

Needs a reroll

Rerolled.

1. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -7,6 +7,7 @@
   +use Drupal\Core\Entity\ContentEntityInterface;
   

   Unnecessary change.

2. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
   @@ -128,12 +128,18 @@ protected function setUp() {
   +    $typed_data = $this->getMockBuilder('\Drupal\Core\Field\FieldItemList')
   +      ->disableOriginalConstructor()
   +      ->getMock();
   +    $this->typedDataManager->expects($this->any())
   +      ->method('getPropertyInstance')
   +      ->will($this->returnValue($typed_data));
   

   This appears to be completely unnecessary

3. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
   @@ -128,12 +128,18 @@ protected function setUp() {
   +      ->will($this->returnValue(array('en' => $language, 'und' => $language)));
   

   $language here is the english language and 'und' needs to be locked. I don't think we're testing what we think we are testing.

4. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
   @@ -253,11 +259,30 @@ public function testIsTranslatable() {
   +    $this->languageManager->expects($this->at(0))
   +      ->method('isMultilingual')
   +      ->will($this->returnValue(TRUE));
   +
        $this->assertTrue($this->entity->isTranslatable());
        $this->assertFalse($this->entity->isTranslatable());
   

   This is very convoluted

What is also weird is I can't see a test added for the appearance and non-appearance of the "Translate" local task.

Reroll + fix for #99.3

Looking at the rest of the issues.

- Fixed the fix for #99.3, now with actual language objects. The test indeed does not dabble with locked/unlocked languages. The entity is created in English, so whether the 'und' language is locked or not does not have any repercussions for this test. Once/if we test with a 'und' entity (which we don't yet), that becomes interesting. Will do that in an upcoming patch. (In other words, testIsTranslatableForUndefined() is not testing what it thinks it tests).

- #99.4 solved by separating the methods, so now not complicating with number of invocation tricks :D

- #99.5 solved by adding tests for the translate page accessibility itself.

#99.2 removed now, indeed not necessary.

#99.3 final solution now here :) The test indeed tested something else then it thought. The entity was supposed to be created English but was in fact 'und'. Once we defined 'und' as a locked language, existing stuff started to fail. So I fixed the original entity to have an 'en' langcode. Also added an additional entity with 'und'. Then we test the entities are 'en' and 'und' respectively and that their locked status is correct, and with only that variable test against a system where their bundle is translatable and multiple languages exist. Then 'en' will be translatable, 'und' will not be. testIsTranslatable() is a tiny bit too defensive now maybe, but since we did so much where we did not know the test data, I think its best to keep the test data asserts as well.

All @alexpott's concerns resolved.

Anything else?

I can not contribute to the code of this patch. But I've put the end result to a test.
* Fault can no longer be reproduced as described in the issue summary.
* "Translate" tabs only appear when Translation of the entity type is enabled AND a second language is added.
* Removing the first language makes the Translate tab disappear.
* Translation entity list pages are Ok, no WSOD.
It works good.

I found two problems, but those are not introduced by this patch. I will report these separately.

Found, but not related, problems:
#2328161: "Enable Translation" checkbox default non-predictable for new bundles
#2328167: Warning array_filter() in ContentTranslationController->overview

From code standards perspective, this looks good. Code makes sense too, and adds more testing and cleans up misnamed tests.
As @Sutharsan tested it and said that works as expected, I guess we can RTBC this one.

Found an other bug, but fortunately that gets fixed by this patch: #2328333: Notice: Undefined index: column_groups in content_translation_field_sync_widget()

Sorry, no longer applies.

Done :)

Ok great, thanks!

Committed and pushed to 8.x. Woohoo!

yay! thanks!