inconsistent checks in content_translation

Adding related issue for some more context: #2414913: Support entity translation of paragraphs

Per discussion with @plach:

- Check isSupported() of entity types, don't extend the form if they are not
- Add a new skip ui option that enables it even if there is no link template, by checking it in isSupported()
- Add a warning/description for those, stating that no UI will be provided (or not? if an entity type opts-in, then he probably *has* his own UI and doesn't need a warning, which would just confuse users...)

Good point on the warning

Thanks, we probably need to at least document this key somewhere, but I'm not sure where... ?

I think the other CT-specifc keys are documented at content_translation_entity_type_alter() or nearby.

Let's do that then.

+++ b/core/modules/content_translation/content_translation.module
@@ -92,6 +92,10 @@ function content_translation_language_types_info_alter(array &$language_types) {
+ * content_translation_ui_skip key to true.

Looks good to me, thanks, can we please enclose the key name in double quotes?

Cool, I guess we are only missing some test coverage now.

Thanks, we are almost there!

1. +++ b/core/modules/content_translation/content_translation.admin.inc
   @@ -91,9 +91,11 @@ function _content_translation_form_language_content_settings_form_alter(array &$
   -      // to be able to skip alterations.
   +      // to be able to skip alterations. Alter the title and display the message
   +      // about UI integration.
   ...
   +        $form['settings'][$entity_type_id]['#title'] = $content_translation_manager->isSupported($entity_type_id) ? $entity_type->getLabel() : t('@label (Translations can not be enabled, UI integration is not possible).', array('@label' => $entity_type->getLabel()));
   
   +++ b/core/modules/content_translation/src/Tests/ContentTranslationUISkipTest.php
   @@ -0,0 +1,44 @@
   +    $this->assertText('Test entity - Translatable check UI (Translations can not be enabled, UI integration is not possible)');
   

   Berdir suggested that the message might simply be confusing for users (see #6), I agree we should avoid it.

2. +++ b/core/modules/content_translation/tests/modules/content_translation_test/src/Entity/EntityTestTranslatable.php
   @@ -0,0 +1,32 @@
   + * Contains Drupal\content_translation_test\Entity\EntityTestTranslatable.
   ...
   +class EntityTestTranslatable extends EntityTest {
   

   Please let's make the class names match the plugin ids to avoid confusion.

I actually suggested the message. This is the opposite message of the one that I originally mentioned (which would have been on the one that *is* supported), I thought then that the non-supported entity types would be completely hidden, but now they are not, but can't be selected.

When we looked at the screenshot in #21, it looked pretty confusing to use without an explanbation

1. +++ b/core/modules/content_translation/content_translation.admin.inc
   @@ -87,13 +87,15 @@ function _content_translation_form_language_content_settings_form_alter(array &$
          if (!$entity_type_translatable) {
   +        $form['settings'][$entity_type_id]['#title'] = $content_translation_manager->isSupported($entity_type_id) ? $entity_type->getLabel() : t('@label (Translations can not be enabled, UI integration is not possible).', array('@label' => $entity_type->getLabel()));
   

   Why do we need this ternary operator? Didn't we just check that $content_translation_manager->isSupported($entity_type_id) is FALSE via the $entity_type_translatable variable?

   Also, I'd use a less technical wording for the warning: what about simply "Translation is not supported"?

2. +++ b/core/modules/content_translation/tests/modules/content_translation_test/src/Entity/EntityTestTranslatableUISkip.php
   --- /dev/null
   +++ b/inconsistent_checks_in-2449457-20-TESTS-ONLY.patch
   
   +++ b/inconsistent_checks_in-2449457-20-TESTS-ONLY.patch
   --- /dev/null
   +++ b/inconsistent_checks_in-2449457-20-interdiff.txt
   
   +++ b/inconsistent_checks_in-2449457-20-interdiff.txt
   --- /dev/null
   +++ b/inconsistent_checks_in-2449457-20.patch
   

   This patch is including also the previous patches.

Fixed the label and removed the patches and interdiffs from the previous patch.

Hm, that translation is less technical, but technically, it's also less correct :) It's still possible to translate it, but not in the UI :) Anyway, fine with me, @plach?

Afk atm, assigning to me so I don't forget about this

+++ b/core/modules/content_translation/tests/modules/content_translation_test/src/Entity/EntityTestTranslatableNoUISkip.php
@@ -0,0 +1,32 @@
+ *   data_table = "entity_test_mul_property_data",
+ *  entity_keys = {
+ *     "id" = "id",

+++ b/core/modules/content_translation/tests/modules/content_translation_test/src/Entity/EntityTestTranslatableUISkip.php
@@ -0,0 +1,33 @@
+ *   data_table = "entity_test_mul_property_data",
+ *  entity_keys = {
+ *     "id" = "id",
+ *     "uuid" = "uuid",

Wrong indentation, sorry. RTBC otherwise.

Rerolled and corrected the indentation.

Thanks!

Still applies. And blocks us in multilingual contrib domain (Paragraphs, TMGMT, and partially Diff and more).
#2473721: Check multilingual settings for paragraphs

Thanks @Anushka-mp, @sasanikolic, @Berdir, and @plach for the patch, and thanks @miro_dietiker for clarifying the significance of the issue. Depending on how severe of a blocker this is, it might be worth bumping to major.

The test coverage looks great, and while I was a bit hesitant at first about stuffing another Boolean key into the entity type annotation, it looks like there is already precedent for that in content_translation in general.

1. +++ b/core/modules/content_translation/content_translation.admin.inc
   @@ -87,13 +87,15 @@ function _content_translation_form_language_content_settings_form_alter(array &$
   +        $form['settings'][$entity_type_id]['#title'] = t('@label (Translation is not supported).', array('@label' => $entity_type->getLabel()));
   

   +1 for this UI message; it's nice and clear. (Looks like the screenshot is out of date but I got the point when looking at it and then trying to find the comma splice in the patch.) ;)
    

2. +++ b/core/modules/content_translation/content_translation.module
   @@ -92,6 +92,10 @@ function content_translation_language_types_info_alter(array &$language_types) {
   + * By default, entity types that do not have a canonical link template can not
   + * be enabled for translation. Entity types can skip this check by setting the
   + * "content_translation_ui_skip" key to true.
   + *
   

   This documentation doesn't really clarify for me what the usecase is for not having a canonical link template and wanting translation, nor what the UI has to do with it. Reading this, I don't know why whether or not my entity is translatable has anything to do with the UI. :) I also saw something on the issue about how these entity types are likely to provide their own UIs; maybe that should be explained as well?
    

3. Also, the following remark mostly out of scope since this just follows the format of the existing documentation, but I think we should improve the way we document the additional keys on content_translation_entity_type_alter(). Ideally it'd be a scannable list in key: explanation form, like we used to have on hook_entity_info(). Also, I think that there should be an @see to hook_entity_type_alter() and hook_entity_type_build() from the entity annotation definition so that I know these implementations add keys I can use as a developer. Anyone want to file a pair of followup issues for that (one for the @see on the entity annotation definition, and another for improving the formatting of content_translation_entity_type_alter())?
    
4. Speaking of hook_entity_type_build(), why is this key being defined on hook_entity_type_alter() instead of the build hook? I thought one was only supposed to use the alter hook to change existing keys, not to add new ones. But I never understood it all that well anyway.
    
5. Finally, while we're improving the docs to explain the usecase, could we get a bit of issue summary update here, including more specific steps to reproduce? I'm guessing testing this can involve using the test entity type that the patch provides.
    

In summary I think the only blocking thing is a bit of docs improvement; the patch looks great overall. Thanks!

(Adding reviewer patch credit.)

Added some description for #2 and opened a new issue for #3 (#2478535: Rewrite the documentation of content_translation_entity_type_alter()). Also, changed the issue description and explained how to reproduce with paragraphs (#5).

On 4. IMHO, _alter() is correct, because we are altering existing entity types, and someone might be adding new ones in _build() for which we want to apply this logic as well (entity creation kit, for example). But core is very inconsistent there.

We didn't understand the @see follow-up, not sure where to add a @see for what, in content_translation_entity_type_alter() to the hook documentation? but we already have the "Implements hook_entity_type_alter()" and when you're adding it to your own entity type, you add it directly in the annotation, not in the build/alter hook...

Thanks @sasanikolic for the docs and the followup!

+++ b/core/modules/content_translation/content_translation.module
@@ -94,7 +94,9 @@ function content_translation_language_types_info_alter(array &$language_types) {
+ * "content_translation_ui_skip" key to true. No UI will be provided than, such
+ * entity types are expected to implement their own UI. For example, entity

Cool, this is the bit of info that was missing for me. :) Polishing the wording a little:

By default, entity types that do not have a canonical link template cannot be enabled for translation. This can be overridden by setting the 'content_translation_ui_skip' key to true. When that key is set, the Content Translation module will not provide any UI for translating the entity type, and the entity type should implement its own UI. This is useful for (e.g.) entity types that are embedded into others for editing (which would not need a canonical link, but could still support translation).

I'll file the other followup issue and link it here shortly. :)

Oh re: #45, the docs for hook_entity_type_alter() say:

Do not use this hook to add information to entity types, unless you are just filling-in default values. Use hook_entity_type_build() instead.

I'll ask @tim.plunkett to weigh in.

Actually re: the other followup issue, there is a reference on the interface for the annotation definition, just not the class, so it's probably not worth duplicating that. Somehow though I managed to miss it.

The first check in content_translation_entity_type_alter() is if ($entity_type->isTranslatable()) {, and because of that I think it should stay an alter. We're looking to make changes to the entity type based on information that could still be gathered during _build().
If it were moved to _build(), you could end up with hook-invocation-ordering issues.

Updated the issue summary and added the suggested documentation from #46.

Hm, preview + edit conflict apparently eat my issue summary updates. Luckily I found them again in the browser history.

I also filed: #2478855: Improve documentation for hook_entity_type_build() and hook_entity_type_alter()

I just rerolled with the comment suggested by @xjm and worked on the issue summary, I think it's OK if I RTBC this again...

+1

+++ b/core/modules/content_translation/tests/modules/content_translation_test/src/Entity/EntityTestTranslatableUISkip.php
@@ -0,0 +1,33 @@
+ *   id = "entity_test_translatable_UI_skip",

So it's a bit nonstandard to have UI in uppercase in this machine name, but as it's a test entity type used only for this test I don't think it really matters. (And I struggle to make my fingers lowercase acronymns like "UI" too.) ;)

As a bugfix and a contributed project blocker with no disruption to speak of, this is a prioritized change during the beta phase per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x (with reviewer credit). Thanks!