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.

CommentFileSizeAuthor
#12 et-di-1935762-4.patch3.91 KBplach
#7 et-di-1935762-4.patch3.91 KBplach
#6 et-di-1935762-3.interdiff.do_not_test.patch1.17 KBplach
#6 et-di-1935762-3.patch3.72 KBplach
#1 et-di-1935762-1.patch3.65 KBplach
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
et-di-1935762-1.patch3.65 KB
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

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.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

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...

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

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.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

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.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
et-di-1935762-3.patch3.72 KB
et-di-1935762-3.interdiff.do_not_test.patch1.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.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
et-di-1935762-4.patch3.91 KB

Crosspost :)

Added the missing PHP doc.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

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 :)

Crell’s picture

Crell CreditAttribution: Crell commented

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

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

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.)

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

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.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
FileSize
et-di-1935762-4.patch3.91 KB

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

PS: /me fails at PHP docs :(

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

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

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

RTBC +1.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Updated issue summary.

klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
CreditAttribution: klonos commented
Issue summary: View changes
Parent issue: » #1919322: entity_load_unchanged() should be part of the storage controller
Related issues: +#1807692: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget
klonos’s picture

klonos
he/him
Primary language English
Location 90% Melbourne, Australia - 10% Larissa, Greece
CreditAttribution: klonos commented
Parent issue: #1919322: entity_load_unchanged() should be part of the storage controller » #1807692: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget
Related issues: -#1807692: Introduce a column synchronization capability and use it to translate alt and titles through the image field widget +#1919322: entity_load_unchanged() should be part of the storage controller