Problem/Motivation

The EntityMetadataWrapper is really helpful but it is a relatively painful to call entity_metadata_wrapper each time we need it.
Plus, developpers should not have to instanciate it multiple times nor pass it to all their functions as arguments to avoid performances issues.

Proposed resolution

I propose to include a getter in the Entity base class which will build the wrapper on-demand then serve it to the developper.
The EntityMetadataWrapper instance would be easily accessible without needing to be instanciated many times.

Remaining tasks

Create a getter in the Entity class.

API changes

New Entity::wrapper public method, returning an EntityMetadataWrapper class.

Current usage :

// In each function needing it it creates a new EntityMetadataWrapper class.
$wrapper = entity_metadata_wrapper($entity->entityType(), $entity);

With this new method :

// Uses always the same EntityMetadataWrapper class because it is stored in the Entity item.
$wrapper = $entity->wrapper();
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DuaelFr’s picture

Assigned: DuaelFr » Unassigned
Status: Active » Needs review
FileSize
743 bytes

Remaining tasks

Create a getter in the Entity class.

This patch is part of the #1day1patch initiative.

DuaelFr’s picture

Improved comment on the wrapper() method.

garphy’s picture

Status: Needs review » Reviewed & tested by the community

Works for me, and quite useful. Small & clean patch.

fago’s picture

Status: Reviewed & tested by the community » Needs work

I like the idea, problem is just that the wrapper could be changed to hold another entity. Let's better create a new wrapper for each method call.

DuaelFr’s picture

I am really sorry fago but I don't understand what you mean.

The wrapper is stored in the Entity so I don't understand how and why it could hold another Entity. Each Entity could have its own wrapper without problem.

I am afraid that developpers would misuse this method if we create a new wrapper on each call.
It is so easy to do something like this :

echo $entity->wrapper()->field_foo->value() . $entity->wrapper()->field_bar->value();

With the above code it would cause no performance loss but if we create a new wrapper each time how to inform developpers that they have to do womething like the following ?

$wrapper = $entity->wrapper();
echo $wrapper->field_foo->value() . $wrapper->field_bar->value();
garphy’s picture

I think what fago means is this :

$wrapper = $entity->wrapper();
$wrapper->set($another_entity);

and then, $entity->wrapper() holds the entity_metadata_wrapper of $another_entity.

Please correct is I'm mistaken

fago’s picture

#6 gets it

DuaelFr’s picture

Status: Needs work » Needs review
FileSize
415 bytes
958 bytes

I found the time to improve this.
I hope it won't be too bad regarding to performances.

joachim’s picture

> + elseif ($this->wrapper->value() !== $this) {

I doubt that comparing two objects is particularly good for performance.
However, at this stage we know that the current entity object and the wrapped entity object are of the same entity type (because AFAIK you can only do $wrapper->set() with a entity of the type the wrapper is already set to). So we could just compare IDs by calling the ID getter function on the $wrapper and on $this.

DuaelFr’s picture

Take a closer look at the official documentation. "==" would not be a good idea because it would compare each property of the two objects recursively but "===" (and "!==") compare the instances of the object so it is just a quick reference check.

The comparison on IDs is as unsafe as not comparing at all because you can always clone an object then alter it. You would get two objects sharing the same ID but not the same values.

I really think that we cannot do better that this condition.

joachim’s picture

Ah right, got it. (http://php.net/manual/en/language.oop5.object-comparison.php for reference.)

Though I would be inclined to say that if you clone an entity, change properties of the clone, and then put the cloned entity into the wrapper that you got from the original... then your code is totally messed up and insane and badly needs a rewrite ;)

DuaelFr’s picture

I think the same for the use case exposed in #6 :)

GoZ’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Seems good to me.

Tested on 7.x-1.x-dev

fago’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/includes/entity.inc
@@ -112,6 +113,22 @@ class Entity {
+      $this->wrapper->set($this);

This seems dangerous, I'D rather create a new wrapper. I modified the code to do so, improved docs a bit and committed it.

Thanks!

  • fago committed e80d6a5 on 7.x-1.x authored by DuaelFr
    Issue #2039601 by DuaelFr, fago: Added Ease EntityMetadataWrapper usage...
DuaelFr’s picture

\o/

+    if (empty($this->wrapper)) {
+      $this->wrapper = entity_metadata_wrapper($this->entityType, $this);
+    }
+    elseif ($this->wrapper->value() !== $this) {
+      // Wrapper has been modified outside, so let's better create a new one.
+      $this->wrapper = entity_metadata_wrapper($this->entityType, $this);
+    }

You could have done an "or" clause to avoid code duplication but it's still cool.
Thank you!

(If you have a bit more time I suggest to review #2066373: Add an isEmpty() and hasProperty() methods to improve DX that can be pretty useful too)

Status: Fixed » Closed (fixed)

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

heddn’s picture

Can we get https://www.drupal.org/documentation/entity-metadata-wrappers updated to reference this usage with some examples?

Upchuk’s picture

Updated the docs to include this information:

https://www.drupal.org/documentation/entity-metadata-wrappers