Problem/Motivation

When working on #2593379: Analyze/improve/fix handling of bubbleable metadata in entity_embed we realised that there are many functions in the trait that are pure wrappers around entity operations. It would be better if we'd rely on core APIs and simplify our code.

Proposed resolution

- Remove loadEntity(), loadMultipleEntities(), canRenderEntity(), canRenderEntityType(), buildEntity()
- Refactor code that uses them to call core APIs directly instead
- Remove functions that are providing dependencies (entity manager, module handler, plugin manager), inject where appropriate

At this point only functions that deal with building of the render array are left. Rename trait to better reflect this fact and mark it @internal.

CommentFileSizeAuthor
#10 interdiff.txt15.04 KBslashrsm
#10 2760407_10.patch56.52 KBslashrsm
#8 interdiff.txt9.24 KBslashrsm
#8 2760407_8.patch54.32 KBslashrsm
#7 clean_up-2760407-7.patch45.35 KBthenchev
#7 interdiff-2760407-7.txt9.6 KBthenchev
#5 interdiff-2760407-5.txt1.76 KBthenchev
#5 clean_up-2760407-5.patch44.36 KBthenchev
#3 clean_up-2760407-3.patch42.6 KBthenchev
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

slashrsm created an issue. See original summary.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH for Acquia commented
Related issues: +#2593379: Analyze/improve/fix handling of bubbleable metadata in entity_embed
thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH for Acquia commented
Assigned: Unassigned » thenchev
Status: Active » Needs review
FileSize
clean_up-2760407-3.patch42.6 KB

Here is the initial patch. On localhost I have some test fails but they look weird, might be because of my installation. Uploading for now to see what the test bot says.
p.s. didn't rename the trait yet. Still thinking about that.

Status: Needs review » Needs work

The last submitted patch, 3: clean_up-2760407-3.patch, failed testing.

thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH for Acquia commented
Status: Needs work » Needs review
FileSize
clean_up-2760407-5.patch44.36 KB
interdiff-2760407-5.txt1.76 KB

This should fix tests.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH for Acquia commented
Status: Needs review » Needs work 
  1. +++ b/src/EntityEmbedDisplay/FieldFormatterEntityEmbedDisplayBase.php
@@ -17,6 +18,13 @@ abstract class FieldFormatterEntityEmbedDisplayBase extends EntityEmbedDisplayBa
   /**
+   * The entity type manager service.
+   *
+   * @var \Drupal\Core\Entity\EntityTypeManagerInterface
+   */
+  protected $entityTypeManager;

    This member is already defined on the parent class. 

  2. +++ b/src/EntityEmbedDisplay/FieldFormatterEntityEmbedDisplayBase.php
@@ -47,19 +55,21 @@ abstract class FieldFormatterEntityEmbedDisplayBase extends EntityEmbedDisplayBa
-    $this->setEntityManager($entity_manager);
+    $this->entityTypeManager = $entity_type_manager;

    Already set in parent constructor. 

  3. +++ b/src/EntityHelperTrait.php
@@ -4,150 +4,15 @@ namespace Drupal\entity_embed;
 trait EntityHelperTrait {

    Maybe just rename it to EntityEmbedHelperTrait. 

  4. +++ b/src/EntityHelperTrait.php
@@ -188,7 +53,7 @@ trait EntityHelperTrait {
+    \Drupal::moduleHandler()->alter(array("{$context['data-entity-type']}_embed_context", 'entity_embed_context'), $context, $entity);

@@ -222,7 +87,7 @@ trait EntityHelperTrait {
+    \Drupal::moduleHandler()->alter(array("{$context['data-entity-type']}_embed", 'entity_embed'), $build, $entity, $context);

@@ -245,7 +110,7 @@ trait EntityHelperTrait {
+    $display = \Drupal::service('plugin.manager.entity_embed.display')->createInstance($plugin_id, $plugin_configuration);

    Can we keep using $this-> form? Only two classes use it and it is marked as internal. 

  5. +++ b/src/Form/EntityEmbedDialog.php
@@ -705,10 +736,12 @@ class EntityEmbedDialog extends FormBase {
+    $entity = current($entity);

    Let's make sure that this is set to NULL of the entity does not exist.

  6. There is a usage of the trait that is not needed any more in EntityEmbedDialog.
thenchev’s picture

thenchev CreditAttribution: thenchev at MD Systems GmbH for Acquia commented
Status: Needs work » Needs review
FileSize
interdiff-2760407-7.txt9.6 KB
clean_up-2760407-7.patch45.35 KB

Everything from #6 should be covered.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH for Acquia commented
FileSize
2760407_8.patch54.32 KB
interdiff.txt9.24 KB

Minor update.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented

My only suggestion would be to create a rendering service instead of a trait - the two dependencies required to use the trait aren't used anywhere else and this way you would only have to include one service, which could potentially reduce code.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH for Acquia commented
FileSize
2760407_10.patch56.52 KB
interdiff.txt15.04 KB

Makes sense.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH for Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks!

  • slashrsm committed cc17745 on 8.x-1.x authored by Denchev 
    Issue #2760407 by Denchev, slashrsm, samuel.mortenson: Clean up...

Status: Fixed » Closed (fixed)

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