Updated: Comment #29

This is a follow-up to #1188388: Entity translation UI in core

Problem/Motivation

The removeTranslation() should go into the EntityInterface as well, since all the data-structure handling methods concerning entity translation are there, the translation controller is focused only on the UI.

Proposed resolution

Originally: Move removeTranslation() into the EntityInterface
Now: Since #1810370: Entity Translation API improvements added the the EntityInterface::removeTraslation() method, we just need to remove EntityTranslationControllerInterface::removeTranslation().

Since removeTranslation was the last thing in ContentTranslationControllerNG, this also removes that class.

Remaining tasks

none.

User interface changes

Most likely, *no* new or changed features/functionality in the user interface, modules added or removed, changes to URL paths, changes to user interface text.

API changes

This will probably be a small internal change and no major API changes/additions that would affect module, install profile, and theme developers. We could include an example of before/after code.

Background

Originally in #1188388-19: Entity translation UI in core
plach

The removeTranslation() should probably go into the EntityInterface as well, since all the data-structure handling methods concerning entity translation are there, the translation controller is focused only on the UI.

And responded in support in #1188388-81: Entity translation UI in core
Gabor

Sounds good.
Files: 
CommentFileSizeAuthor
#32 ct-remove_rt-1810330-32.patch2.6 KBplach
PASSED: [[SimpleTest]]: [MySQL] 58,318 pass(es).
[ View ]
#28 ct-remove_rt-1810330-28.patch12.03 KBplach
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ct-remove_rt-1810330-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 drupal-move_removeTranslation_to_EntityInterface-1810330-12.patch7.47 KBc4doug
PASSED: [[SimpleTest]]: [MySQL] 50,410 pass(es).
[ View ]
#6 drupal-move_removeTranslation_to_EntityInterface-1810330-4.patch7.36 KBc4doug
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-move_removeTranslation_to_EntityInterface-1810330-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#4 drupal-move_removeTranslation_to_EntityInterface-1810330-3.patch6.72 KBc4doug
FAILED: [[SimpleTest]]: [MySQL] 49,964 pass(es), 530 fail(s), and 252 exception(s).
[ View ]

Comments

Title:move removeTranslation() into EntityInterface [Follow-up to Entity Translation UI in core]Move EntityTranslationControllerInterface::removeTranslation() into EntityInterface
Component:language system» translation_entity.module
Issue tags:+API clean-up

Status:Postponed» Active

Issue tags:+medium, +needs initial patch

updated issue summary remaining tasks.

Status:Active» Needs review
StatusFileSize
new6.72 KB
FAILED: [[SimpleTest]]: [MySQL] 49,964 pass(es), 530 fail(s), and 252 exception(s).
[ View ]

I've moved the removeTranslation function from the EntityTranslationControllerInterface to the EntityInterface. I also updated any child interfaces and classes as well as a call to removeTranslation in translation_entity.pages.inc

Status:Needs review» Needs work

The last submitted patch, drupal-move_removeTranslation_to_EntityInterface-1810330-3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-move_removeTranslation_to_EntityInterface-1810330-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

It appears that I missed the ViewUI class. I've rolled a new patch

Issue tags:-needs initial patch

removing tag, we've got a patch thanks to c4doug

Status:Needs review» Needs work

The last submitted patch, drupal-move_removeTranslation_to_EntityInterface-1810330-4.patch, failed testing.

Issue tags:+Needs reroll

tagging for reroll

Issue tags:-Needs reroll
StatusFileSize
new7.47 KB
PASSED: [[SimpleTest]]: [MySQL] 50,410 pass(es).
[ View ]

I've rerolled the patch and added more detail to the block comments to reflect the full namespace for overridden functions.

Status:Needs work» Needs review
Issue tags:-translatable fields, -language-content, -medium

status update

I'm not sure why those tags were dropped...

Status:Needs review» Needs work

+++ b/core/lib/Drupal/Core/Entity/EntityBCDecorator.phpundefined
@@ -316,6 +316,13 @@ public function delete() {
+  /**
+   * Forwards the call to the decorated entity.
+   */
+  public function removeTranslation($langcode) {
+    return $this->decorated->removeTranslation($langcode);
+  }
   /**
    * Forwards the call to the decorated entity.

why does this look like a duplicate?

I should open up the patched file, but makes me think that maybe the 1 line descriptions need to be improved/made more specific to make the different more obvious.

Status:Needs work» Needs review

I've created an issue (#1917378: Improve the comments on EntityBCDecorator) for that. There are 30+ functions that all have the same comment.

Ah, that makes sense... in a way.

Status:Needs review» Reviewed & tested by the community

Looks good, thanks!

Status:Reviewed & tested by the community» Needs work

   /**
+   * Implements \Drupal\Core\Entity\EntityInterface::removeTranslation().
+   */
+  public function removeTranslation($langcode) {
+    $translations = $this->getTranslationLanguages();
+    // @todo Handle properties.
+    // Remove field translations.
+    foreach (field_info_instances($this->entityType(), $this->bundle()) as $instance) {
+      $field_name = $instance['field_name'];
+      $field = field_info_field($field_name);
+      if ($field['translatable']) {
+        $this->{$field_name}[$langcode] = array();
+      }
+    }
+  }

This should be a Field API function, probably field_attach_*() something which we then call from the entity class - at least until all field_attach_*() stuff moves to entity hook implementations.

Status:Needs work» Needs review

Sorry, I disagree: we are dealing fields here just because our current (broken) implementation relies on field values to determine whether an entity translation exists. What we want is a method dealing with a whole entity translation, thus EntityInterface is the correct place IMO.

Determining reliably whether an entity translation exists will be properly addressed in #1810370: Entity Translation API improvements.

Right but only the field API knows if a field is translatable. I think the method should live on the interface, but the logic itself should be handled by field API - the entity system shouldn't know anything about field module at all.

The entity system knows about the Entity Field API and can leverage it to inspect field values and retrieve field definitions to know if they are translatable, if it needs to. That said, I think the final implementation won't deal with field values at all. Do you want us to postpone this on #1810370: Entity Translation API improvements so we don't have to move the current code in Entity?

@catch:

Any feedback on #22? I guess this is RTBC or postponed depending on what you think it's best.

Component:translation_entity.module» content_translation.module

Issue tags:+Needs reroll

Assigned:Unassigned» jair

I should have time in the next few days.

Title:Move EntityTranslationControllerInterface::removeTranslation() into EntityInterfaceRemove EntityTranslationControllerInterface::removeTranslation()
Status:Needs review» Needs work

Since #1810370: Entity Translation API improvements added the the EntityInterface::removeTraslation() method, we just need to remove EntityTranslationControllerInterface::removeTranslation().

Status:Needs work» Needs review
StatusFileSize
new12.03 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch ct-remove_rt-1810330-28.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This should be all is needed.

Status:Needs review» Reviewed & tested by the community

I have tested and reviewed the patch and seems to be ok.

Issue summary:View changes

Updated issue summary remaining tasks

Issue summary:View changes

update resolution

Issue summary:View changes

explaining why it also removes a class

Status:Reviewed & tested by the community» Needs work
Issue tags:+translatable fields, +API clean-up, +D8MI, +language-content, +Needs reroll, +medium

The last submitted patch, ct-remove_rt-1810330-28.patch, failed testing.

Assigned:jair» Unassigned
Status:Needs work» Reviewed & tested by the community
StatusFileSize
new2.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,318 pass(es).
[ View ]

Issue tags:-Needs reroll

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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

Issue summary:View changes

noting when it was updated