Problem/Motivation

Annotation of the Shortcut Entity Type contains a declaration of the translation handler. However, this is not necessary, because this handler will be added automatically by content_translation module. According to the content_translation_entity_type_alter() function comment:

If the entity paths match the default pattern above and there is no need for an entity-specific translation handler, Content Translation will provide built-in support for the entity.

Proposed resolution

Remove translation handler.

Remaining tasks

Review.

User interface changes

Nope.

API changes

Nope.

Data model changes

Nope.

Release notes snippet

Nope.

CommentFileSizeAuthor
#14 core-remove-default-translation-handler-3072929-14-D8.patch12.77 KBid.aleks
#10 core-remove-default-translation-handler-3072929-10-D8.patch13.77 KBid.aleks
#5 shortcut-remove-translation-handler-3072929-5-D8.patch1.35 KBiyyappan.govind
#2 shortcut-remove-translation-handler-3072929-2-D8.patch607 bytesid.aleks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

id.aleks created an issue. See original summary.

id.aleks’s picture

id.aleks CreditAttribution: id.aleks at Smile commented
Assigned: id.aleks » Unassigned
Status: Active » Needs review
FileSize
shortcut-remove-translation-handler-3072929-2-D8.patch607 bytes

Please test and review

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Sounds reasonable to me.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: Remove declaration of the translation handler for Shortcut Entity » Remove declaration of the default translation handler from entity type annotations
Component: shortcut.module » entity system
Priority: Normal » Minor
Status: Reviewed & tested by the community » Needs work
Issue tags: +Entity Field API, +Needs issue summary update

I see a dozen or so more of these in test entity types. Let's also remove those?

iyyappan.govind’s picture

iyyappan.govind
he/him
Primary language Tamil
Location Chennai, India
CreditAttribution: iyyappan.govind at UniMity Solutions Pvt Limited commented
FileSize
shortcut-remove-translation-handler-3072929-5-D8.patch1.35 KB

Hi Wim Leers
I have removed the content translation handlers in test entity types also. Please review it. Thanks

iyyappan.govind’s picture

iyyappan.govind
he/him
Primary language Tamil
Location Chennai, India
CreditAttribution: iyyappan.govind at UniMity Solutions Pvt Limited commented

Set to needs review

iyyappan.govind’s picture

iyyappan.govind
he/him
Primary language Tamil
Location Chennai, India
CreditAttribution: iyyappan.govind at UniMity Solutions Pvt Limited commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: shortcut-remove-translation-handler-3072929-5-D8.patch, failed testing. View results

id.aleks’s picture

id.aleks CreditAttribution: id.aleks as a volunteer and at Smile commented

@iyyappan.govind
I think that removing content_translation module from the modules to enable list is wrong. Actually, this module is adding a translation handler to Shortcut Entity, which we are trying to remove. So in fact, your logic in the patch is wrong. Sorry for the straightforwardness.
@Wim Leers
I see that you changed the issue component to the entity system. Just to clarify: you want to remove all translation handlers that match the default paths pattern for all core entity types? If so, it makes sense for me and I will do this shortly.

id.aleks’s picture

id.aleks CreditAttribution: id.aleks as a volunteer and at Smile commented
Related issues: +#3072728: Remove declaration of the translation handler for Media Entity
FileSize
core-remove-default-translation-handler-3072929-10-D8.patch13.77 KB

@Wim Leers

I have removed all declarations of the default translation handler. Affected CT`s are:

  1. media
  2. shortcut
  3. entity_test
  4. entity_test_admin_routes
  5. entity_test_base_field_display
  6. entity_test_cache
  7. entity_test_field_methods
  8. entity_test_mul
  9. entity_test_mul_changed
  10. entity_test_mul_default_value
  11. entity_test_mul_langcode_key
  12. entity_test_mulrev
  13. entity_test_mulrev_changed
  14. entity_test_mulrevpub
  15. entity_test_rev
  16. entity_test_string_id

We had related issue Remove declaration of the translation handler for Media Entity. I decided to combine them and do all the works within this issue. That`s why you can see changes in the test for Media module.

Please test and review.

id.aleks’s picture

id.aleks CreditAttribution: id.aleks as a volunteer and at Smile commented
Status: Needs work » Needs review
id.aleks’s picture

id.aleks CreditAttribution: id.aleks as a volunteer and at Smile commented
amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Entity Field API, -Needs issue summary update

The patch looks good to me, just a couple of points to fix:

  1. +++ b/core/modules/media/tests/src/Kernel/MediaTranslationTest.php
@@ -55,6 +55,10 @@ class MediaTranslationTest extends MediaKernelTestBase {
+    $this->assertEquals($translation_handler_class, 'Drupal\content_translation\ContentTranslationHandler', 'Translation handler is set to use the content_translation handler.');

    The expected class needs to be the first argument passed to assertEquals(), and it should use the ContentTranslationHandler::class notation :) 

  2. +++ b/core/modules/media/tests/src/Kernel/MediaTranslationTest.php
@@ -100,4 +104,14 @@ class MediaTranslationTest extends MediaKernelTestBase {
+  public function testTranslationHendler() {

    This test method can be removed, it only repeats the same assertion from above.

id.aleks’s picture

id.aleks CreditAttribution: id.aleks as a volunteer and at Smile commented
Status: Needs work » Needs review
FileSize
core-remove-default-translation-handler-3072929-14-D8.patch12.77 KB

@amateescu

1. I agree with you. It's fixed.
2. Fixed. It's my fault.

Please review. Thanks.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Looks great, thanks for the quick update!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed f0e9115 and pushed to 8.8.x. Thanks!

function content_translation_entity_type_alter(array &$entity_types) {
  // Provide defaults for translation info.
  /** @var $entity_types \Drupal\Core\Entity\EntityTypeInterface[] */
  foreach ($entity_types as $entity_type) {
    if ($entity_type->isTranslatable()) {
      if (!$entity_type->hasHandlerClass('translation')) {
        $entity_type->setHandlerClass('translation', 'Drupal\content_translation\ContentTranslationHandler');
      }

AS the issue summary states setting the default is not necessary. Less boilerplate for the win!

  • alexpott committed f0e9115 on 8.8.x 
    Issue #3072929 by id.aleks, iyyappan.govind, amateescu, Wim Leers:...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.