Static cache issues with Entity metadata wrappers

This test definitely helps identify the broken references whenever and entity is rewrapped.

However it definitely brings to question: do we fix, or is this a developer bug where they need to ensure they do not rewrap an object.

This is actually hard to fix. Or, at least elegantly.

* First the data can be null, $data = NULL.
* Second, the $type can be "entity" or a type of entity (which populates the entity info. That means the entity info can't be reliably used
* The data can be an entity ID or object.

This snippet makes the test pass.

    $entity_wrapper_cache =& drupal_static(__FUNCTION__, array());
    if (!is_object($data)) {
      $wrapper = new EntityDrupalWrapper($type, $data, $info);
    }
    else {
      if (isset($entity_wrapper_cache[$type][spl_object_hash($data)])) {
        $wrapper = $entity_wrapper_cache[$type][spl_object_hash($data)];
      }
      else {
        $wrapper = new EntityDrupalWrapper($type, $data, $info);
        $entity_wrapper_cache[$type][spl_object_hash($data)] = $wrapper;
      }
    }

    return $wrapper;

We have to check if the data is an object. If it's not, there is no chance to re-use a metadata wrapper. Or I guess there is. "If type is a valid entity type, assume this is an identifier and look up from wrapper caches."

Since $entity_info may not be defined (due to type == 'entity') we cannot use `entity keys['id']`. Or. Again, I guess we could with the assumption that "well you passed type === entity, so no benefits for you"

Here is a patch which intends to improve the `entity_metadata_wrapper` function determination of what wrapper to return.

If the type is "entity" it is much more explicit.

  if ($type == 'entity') {
    return new EntityDrupalWrapper($type, $data, $info);
  }

If a valid entity type is passed we attempt to re-use wrappers.

+++ b/entity.module
@@ -1407,16 +1407,49 @@ function entity_get_extra_fields_controller($type = NULL) {
+        // Fallback to the object hash in case we couldn't get the entity ID.
+        $cache_key = spl_object_hash($data);

I think this works. It's better to catch than not catch.

I'd also like to do some profiling on this before and after.

We should check object count and memory usage.

Discussed with jsacksick offline.

We need to increase the test coverage to map all of our assumptions and outcomes of using this static cache. For example a generic "entity" type is not statically cached. Also that the `spl_object_hash` works with unsaved entities, which could be crucial in the order refresh process of Commerce (possibly.)

This tests an unsaved node using the same pattern - modifying the title and rewrapping.

Updated to test that creating a wrapper from an entity's ID will return any previous entity object's wrapper.

Another idea to solve this without breaking BC: expose a cache clearing API on entities and use that wherever needed (e.g. rules). (Currently $wrapped_entity->clear() also clears the data property, not just the cache.)

The idea #17 is implemented in #1343196: $wrapper->reset() should unset $wrapper->cache.