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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

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
plach’s picture

Status: Postponed » Active
YesCT’s picture

Issue tags: +medium, +needs initial patch

updated issue summary remaining tasks.

douglasmiller’s picture

Status: Active » Needs review
FileSize
6.72 KB

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.

douglasmiller’s picture

Status: Needs work » Needs review
FileSize
7.36 KB

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

plach’s picture

YesCT’s picture

YesCT’s picture

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.

YesCT’s picture

Issue tags: +Needs reroll

tagging for reroll

douglasmiller’s picture

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

douglasmiller’s picture

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

status update

douglasmiller’s picture

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

YesCT’s picture

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.

douglasmiller’s picture

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.

YesCT’s picture

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

plach’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

catch’s picture

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.

plach’s picture

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.

catch’s picture

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.

plach’s picture

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?

plach’s picture

@catch:

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

Gábor Hojtsy’s picture

Component: translation_entity.module » content_translation.module
jair’s picture

Issue tags: +Needs reroll
jair’s picture

Assigned: Unassigned » jair

I should have time in the next few days.

plach’s picture

Title: Move EntityTranslationControllerInterface::removeTranslation() into EntityInterface » Remove 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().

plach’s picture

Status: Needs work » Needs review
FileSize
12.03 KB

This should be all is needed.

peximo’s picture

Status: Needs review » Reviewed & tested by the community

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

peximo’s picture

Issue summary: View changes

Updated issue summary remaining tasks

YesCT’s picture

Issue summary: View changes

update resolution

YesCT’s picture

Issue summary: View changes

explaining why it also removes a class

plach’s picture

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.

plach’s picture

Assigned: jair » Unassigned
Status: Needs work » Reviewed & tested by the community
FileSize
2.6 KB
mcrittenden’s picture

Issue tags: -Needs reroll
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

noting when it was updated