We can now remove the entity_load_unchanged() call from FieldTranslationSynchronizer::synchronizeFields().

This could use some serious DX review. Quoting @yched from #1807692-82: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget:

Interesting.
In the previous patch, this was translation_entity_sync() function calling entity_load_uncached().
So now that the code moved to a class, we avoid calling functional stuff like entity_load_uncached() within a method and go for the OO equivalent ?

Does that mean that entity_load() too is considered bad practice in OO code, and that any class willing to load an entity needs to get an injected EntityManager and do $this->entityManager->getStorageController($entity_type)->load(array($id)); instead ? Ew :-/
Definitely not a debate for this thread, but IMO that would deserve a spin off discussion to establish the practice.

Files: 
CommentFileSizeAuthor
#12 et-di-1935762-4.patch3.91 KBplach
PASSED: [[SimpleTest]]: [MySQL] 52,479 pass(es).
[ View ]
#7 et-di-1935762-4.patch3.91 KBplach
PASSED: [[SimpleTest]]: [MySQL] 52,506 pass(es).
[ View ]
#6 et-di-1935762-3.interdiff.do_not_test.patch1.17 KBplach
#6 et-di-1935762-3.patch3.72 KBplach
PASSED: [[SimpleTest]]: [MySQL] 52,563 pass(es).
[ View ]
#1 et-di-1935762-1.patch3.65 KBplach
PASSED: [[SimpleTest]]: [MySQL] 52,529 pass(es).
[ View ]

Comments

StatusFileSize
new3.65 KB
PASSED: [[SimpleTest]]: [MySQL] 52,529 pass(es).
[ View ]

Minor, but since we hardcode the class name of the synchronizer in the test, we might as well hardcode the entity manager, no?

Otherwise looks good.

Well, the field sychronizer class is hardcoded because it's a white-box test. We already have the black-box web test, in fact the point of the former is testing that specific implementation of the synchronizer interface. Retrieving the entity manager from the container seemed to be the easiest way to get an instance, otherwise we should bother with providing namespaces...

That argument makes sense. I guess in the end it's a matter of taste... Because of that I was about to mark this RTBC, but then I noticed:

1. The constructor is missing the usual "Constructs a Foo object." PHPDoc.

2.

     $entity_type = $entity->entityType();
     // @todo Use the entity storage controller directly to avoid accessing the
     //   global scope.
-    $entity_unchanged = isset($entity->original) ? $entity->original : entity_load_unchanged($entity_type, $entity->id());
+    $entity_unchanged = isset($entity->original) ? $entity->original : $this->entityManager->getStorageController($entity->entityType())->loadUnchanged($entity->id());

This is nitpick-y, but since this will have to be re-rolled for 1. anyway: you should either remove the $entity_type local variable or re-use it in the new line, as it was used before.

Regarding the DX question in the OP, this is definitely the WayToGo(tm) currently. If this is considered too verbose we could easily let the entity manager retrieve the storage controller itself and then do $this->entityManager->load($entity_type, array($ids)) (and similarly for ->loadUnchanged()). That should be a separate issue, though I guess.

StatusFileSize
new3.72 KB
PASSED: [[SimpleTest]]: [MySQL] 52,563 pass(es).
[ View ]
new1.17 KB

+++ b/core/modules/translation_entity/lib/Drupal/translation_entity/FieldTranslationSynchronizer.php
@@ -31,7 +43,7 @@ public function synchronizeFields(EntityInterface $entity, $sync_langcode, $orig
     // @todo Use the entity storage controller directly to avoid accessing the
     //   global scope.
-    $entity_unchanged = isset($entity->original) ? $entity->original : entity_load_unchanged($entity_type, $entity->id());
+    $entity_unchanged = isset($entity->original) ? $entity->original : $this->entityManager->getStorageController($entity->entityType())->loadUnchanged($entity->id());

Removed @todo.

StatusFileSize
new3.91 KB
PASSED: [[SimpleTest]]: [MySQL] 52,506 pass(es).
[ View ]

Crosspost :)

Added the missing PHP doc.

Regarding the DX question in the OP, this is definitely the WayToGo(tm) currently. If this is considered too verbose we could easily let the entity manager retrieve the storage controller itself and then do $this->entityManager->load($entity_type, array($ids)) (and similarly for ->loadUnchanged()). That should be a separate issue, though I guess.

I think having the discussion here is helpful because we have a concrete example to put our eyes on, we can certainly implement it elsewhere. That said, I have no real solution to offer atm, let's see what entity people think of yours :)

At least visually, #7 passes my smell test. (I leave it to others to figure out how something can visually pass a smell test.)

Sure thing. Sorry if it sounded like I was trying to prevent discussion. What I was trying to point out is that the approach you took is the currently preferred way. There is most definitely no harm in discussion alternatives here. Code-wise this is RTBC now, though. (Except that it should be "Constructs" and not "Construct" but that's neither here nor there...). So leaving at "needs review" for the DX discussion?! (Although in that case we should probably change the title to attract some more people.)

yeah, I think that works as well. We could debate whether we want to have individual controllers injected or the whole manager, but injecting the whole manager seems fine to me.

StatusFileSize
new3.91 KB
PASSED: [[SimpleTest]]: [MySQL] 52,479 pass(es).
[ View ]

Ok, given the last comments, I think we can get this in.

PS: /me fails at PHP docs :(

Status:Needs review» Reviewed & tested by the community

(Reuploaded #7 adding an "s" to "Construct")

RTBC +1.

Status:Reviewed & tested by the community» Fixed

Committed to 8.x. Thanks.

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

Issue summary:View changes

Updated issue summary.