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

Comments

slashrsm created an issue. See original summary.

slashrsm’s picture

slashrsm commented
Related issues: +#2593379: Analyze/improve/fix handling of bubbleable metadata in entity_embed
thenchev’s picture

thenchev commented
Assigned: Unassigned » thenchev
Status: Active » Needs review
StatusFileSize
new 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 commented
Status: Needs work » Needs review
StatusFileSize
new clean_up-2760407-5.patch44.36 KB
new interdiff-2760407-5.txt1.76 KB

This should fix tests.

slashrsm’s picture

slashrsm 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 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2760407-7.txt9.6 KB
new clean_up-2760407-7.patch45.35 KB

Everything from #6 should be covered.

slashrsm’s picture

slashrsm commented
StatusFileSize
new 2760407_8.patch54.32 KB
new interdiff.txt9.24 KB

Minor update.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
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 commented
StatusFileSize
new 2760407_10.patch56.52 KB
new interdiff.txt15.04 KB

Makes sense.

samuel.mortenson’s picture

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

Looks good to me.

slashrsm’s picture

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