"Enable Translation" checkbox default non-predictable for new bundles

I don't remember the issue number on top of my head, but we discussed it. Basically we have 2 places to enable content translation. 1) in content type 2) content translation admin page. Each has different validation rules ( say admin page doesn't allow to enable translation for a content type if there is no translatable field, where as content type does). One of @gaber suggestion was to remove this option in content type edit and provide link to admin page.

EDIT: this is the issue I was talking about: #2306087: Entity configuration form allows to enable translation without any translatable field

Initial patch...

IMHo it should not be enabled by default. Not clear from the issue summary is if this happens with new content types? The bundle is not known at the time, so you should theoretically get FALSE. Theoretically. However looks like https://api.drupal.org/api/drupal/core%21modules%21content_translation%2... defaults to the first defined bundle for the entity type is none was provided, so the default state of the checkbox for new content types will be "random" depending on whether the first bundle returned was or was not configured multilingual.

The patch makes sense. I'd add a comment above, something like "For new content types, we don't know the bundle name yet, default to no translatability". Also needs tests.

Also the same problem probably happens with new vocabularies, new custom block types, etc.

Was wondering, if it is OK to do something like this:

--- a/core/modules/content_translation/content_translation.admin.inc
+++ b/core/modules/content_translation/content_translation.admin.inc
-            '#default_value' => content_translation_enabled($entity_type_id, $bundle),
+            '#default_value' => content_translation_enabled($entity_type_id, $bundle, FALSE),
           );
 
           foreach ($fields as $field_name => $definition) {
diff --git a/core/modules/content_translation/content_translation.module b/core/modules/content_translation/content_translation.module
index b3ea75d..9a3c57a 100644
--- a/core/modules/content_translation/content_translation.module
+++ b/core/modules/content_translation/content_translation.module
-function content_translation_enabled($entity_type, $bundle = NULL) {
+function content_translation_enabled($entity_type, $bundle = NULL, $check_all_bundles = TRUE) {
   $enabled = FALSE;
 
   if (\Drupal::service('content_translation.manager')->isSupported($entity_type)) {
-    $bundles = !empty($bundle) ? array($bundle) : array_keys(entity_get_bundles($entity_type));
+    $bundles = !empty($bundle) ? array($bundle) : ($check_all_bundles) ? array_keys(entity_get_bundles($entity_type)) : array();
     foreach ($bundles as $bundle) {
       if (content_translation_get_config($entity_type, $bundle, 'enabled')) {
         $enabled = TRUE;
@@ -701,7 +704,7 @@ function content_translation_language_configuration_element_process(array $eleme
     $element['content_translation'] = array(
       '#type' => 'checkbox',
       '#title' => t('Enable translation'),
-      '#default_value' => $context['bundle'] ? content_translation_enabled($context['entity_type'], $context['bundle']) : FALSE,
+      '#default_value' => content_translation_enabled($context['entity_type'], $context['bundle'], FALSE),
       '#element_validate' => array('content_translation_language_configuration_element_validate'),
       '#prefix' => '<label>' . t('Translation') . '</label>',
     );

but it might be API change, so going to update other places as per #4 and testcase.

Here is the updated patch with test for content type page. Manually tested other bundle pages(comment, custom block type, shortcut set, taxonomy) and the fix in #4 fixed all bundles as the fix is in language element.

Not sure, if we need to add test cases for other pages as the fix in central. If it is necessary, feel free to add test. otherwise, I will check later this week.

The code and test looks great. Only cosmetic stuff to clean things up for commit :)

1. +++ b/core/modules/content_translation/content_translation.module
   @@ -701,7 +701,7 @@ function content_translation_language_configuration_element_process(array $eleme
   -      '#default_value' => content_translation_enabled($context['entity_type'], $context['bundle']),
   +      '#default_value' => $context['bundle'] ? content_translation_enabled($context['entity_type'], $context['bundle']) : FALSE,
   

   I'd still add a one line comment as explained above in my previous comments.

2. +++ b/core/modules/content_translation/src/Tests/ContentTranslationEntityBundleUITest.php
   @@ -0,0 +1,46 @@
   + * Definition of Drupal\entity\Tests\ContentTranslationEntityBundleUITest.
   

   Contains \Drupal...

3. +++ b/core/modules/content_translation/src/Tests/ContentTranslationEntityBundleUITest.php
   @@ -0,0 +1,46 @@
   + * Tests the content translation on entity bundle UI.
   

   More specific comment would be great. Eg. "Tests bundle translatability defaults on content types".

4. +++ b/core/modules/content_translation/src/Tests/ContentTranslationEntityBundleUITest.php
   @@ -0,0 +1,46 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   

   setUp is not required to have an inheritdoc I believe, also should have space after the modules property

Thanks @Gábor Hojtsy. fixed all in #10.

1. +++ b/core/modules/content_translation/content_translation.module
   @@ -701,7 +701,9 @@ function content_translation_language_configuration_element_process(array $eleme
   +      // For new content types, we don't know the bundle name yet,
   +      // default to no translatability.
   

   "For adding a new bundle, we don't ..." instead of content type. This applies to all entity bundles.

   Also you could have used more space on the line, "default" could have fit on the first line AFAIS.

2. +++ b/core/modules/content_translation/src/Tests/ContentTranslationEntityBundleUITest.php
   @@ -0,0 +1,43 @@
   + * Contains Drupal\entity\Tests\ContentTranslationEntityBundleUITest.
   

   \Drupal

updated...

Looks good to me :)

Looking at content_translation_enabled() I think we can make the code a bit less brittle by not using empty - see attached patch.

+++ b/core/modules/content_translation/content_translation.module
@@ -300,11 +300,11 @@ function content_translation_set_config($entity_type, $bundle, $setting, $value)
+function content_translation_enabled($entity_type, $bundle = FALSE) {

Doesn't work for the 9 calls which assumes that no arg2 will check all bundles. and it is an API change. I was trying to do like
function content_translation_enabled($entity_type, $bundle = NULL, $check_all_bundle = TRUE) { or
function content_translation_enabled($entity_type, $bundle = 'all') {
but again, it is a change that we need to make all places where we have this call.

Yeah this first bundle picking was designed for no-bundle AKA single bundle entity types like user to make it easy to get their translation setting. A possibility is to make it intelligent using the entity type manager and figure out from there if the entity can have bundles and if no bundle was provided, always return FALSE from this function. If we really want to make it more intelligent. Not sure the magic factor is worth it though?

@alexpott: seems like fails in ContentTranslationSettingsTest where the article content type is saved and then made translatable is failing. Not sure what exactly are you doing here, is an empty string passed on here? How does this fix it?

While I suggested in #2 to remove this element from the content type and instead only have 1 place, the content language settings page also uses this element, so it would not help in not needing to fix this bug.

We (schrammos and k4v) reviewed the two patches. We think #14 is fine, it fixes the issue and does not change the api.

Reworking content_translation_enabled() could be an new issue, we should check the semantics are right here.

The current documentation of content_translation_enabled() is not clear ("Determines whether the given entity type is translatable"). It should be something like "Determines if a specific bundle is translatable. If no bundle is given the function returns if at least one of the entity types bundles is translatable." But we are not sure if this is what the author intended.

If you want #14 to be the "latest patch", needs to hide the diversion...

Committed and pushed to 8.x. Thanks!

Yay, superb.