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.
Comments
Comment #1
plachComment #2
plachComment #3
YesCT CreditAttribution: YesCT commentedupdated issue summary remaining tasks.
Comment #4
douglasmiller CreditAttribution: douglasmiller commentedI'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
Comment #6
douglasmiller CreditAttribution: douglasmiller commentedIt appears that I missed the ViewUI class. I've rolled a new patch
Comment #7
plach#6: drupal-move_removeTranslation_to_EntityInterface-1810330-4.patch queued for re-testing.
Comment #8
YesCT CreditAttribution: YesCT commented#6: drupal-move_removeTranslation_to_EntityInterface-1810330-4.patch queued for re-testing.
Comment #9
YesCT CreditAttribution: YesCT commentedremoving tag, we've got a patch thanks to c4doug
Comment #11
YesCT CreditAttribution: YesCT commentedtagging for reroll
Comment #12
douglasmiller CreditAttribution: douglasmiller commentedI've rerolled the patch and added more detail to the block comments to reflect the full namespace for overridden functions.
Comment #13
douglasmiller CreditAttribution: douglasmiller commentedstatus update
Comment #14
douglasmiller CreditAttribution: douglasmiller commentedI'm not sure why those tags were dropped...
Comment #15
YesCT CreditAttribution: YesCT commentedwhy 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.
Comment #16
douglasmiller CreditAttribution: douglasmiller commentedI've created an issue (#1917378: Improve the comments on EntityBCDecorator) for that. There are 30+ functions that all have the same comment.
Comment #17
YesCT CreditAttribution: YesCT commentedAh, that makes sense... in a way.
Comment #18
plachLooks good, thanks!
Comment #19
catchThis 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.
Comment #20
plachSorry, 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.
Comment #21
catchRight 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.
Comment #22
plachThe 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
?Comment #23
plach@catch:
Any feedback on #22? I guess this is RTBC or postponed depending on what you think it's best.
Comment #24
Gábor HojtsyComment #25
jair CreditAttribution: jair commentedComment #26
jair CreditAttribution: jair commentedI should have time in the next few days.
Comment #27
plachSince #1810370: Entity Translation API improvements added the the
EntityInterface::removeTraslation()
method, we just need to removeEntityTranslationControllerInterface::removeTranslation()
.Comment #28
plachThis should be all is needed.
Comment #29
peximo CreditAttribution: peximo commentedI have tested and reviewed the patch and seems to be ok.
Comment #29.0
peximo CreditAttribution: peximo commentedUpdated issue summary remaining tasks
Comment #29.1
YesCT CreditAttribution: YesCT commentedupdate resolution
Comment #29.2
YesCT CreditAttribution: YesCT commentedexplaining why it also removes a class
Comment #30
plach#28: ct-remove_rt-1810330-28.patch queued for re-testing.
Comment #32
plachRerolled after #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface. Should still be RTBC if green.
Comment #33
mcrittenden CreditAttribution: mcrittenden commentedComment #34
webchickCommitted and pushed to 8.x. Thanks!
Comment #35.0
(not verified) CreditAttribution: commentednoting when it was updated