Do not throw exceptions when wrapping an entity with a stale reference

Hm, having FALSE as a valid value for an entity doesn't really sound right to me. If the info says there is an entity property which is an entity, I'd not say FALSE should be a valid value.

Anyway, I see the three options to handle this case:
1) Throw an exception, as it is now. Fine for BC, and as said makes sense to me. If there is entity reference with a stale id, e.g. you would still be able to get the id and the type of the wrapper. Then if you want to make use of the entity and it is not there, I'd expect an exception - not?
2) Return NULL, what as to the docs means the property is not set at all (and is valid). Sounds right, but it is not really practicable as it would mean we would have to manually filter list-of-referenced-entities to not contain invalid entity ids in order to be consistent - ouch, much unnecessary entity-loading....
3) Return FALSE, as suggested. This would mean we would have to additionally check for FALSE in some cases of the code, as well as any module using the wrapper would have to follow that change - e.g. rules.

In the end, option 1) and 3) are possible ways to go. As 3) would be an API change now and I think 1) makes sense, I'd tend to stay with 1). Any good reasons / use-cases why 3) should be preferred?

Maybe, a little helper would help also? E.g. we have already validate, so we could add something like

public function hasValidValue() {
  try {
    $value = $this->value();
    return isset($value) && $this->validate($value);
  }
  catch (EntityMetadataWrapperException $e) {
    return FALSE;
  }
}

I agree that we need to solve this, it keeps popping up in Rules itself too.

Re-visiting options 1) to 3), I don't think they are mutually exclusive. 1) would apply anyway if the entity-wrapper is considered empty, i.e. have value NULL.

Also, recently a patch went in that made the wrappers interpret FALSE entity values returned from getter callbacks as "not set", i.e. the wrapper returns NULL. That makes option 3) more unattractive as assigning another meaning to FALSE when returned from the getter callback or returned from the wrapper is certainly confusing.

So what about a changed option 2)
*) Don't throw an exception when accessing $entityWrapper->value() but return NULL if it cannot be loaded. Thus, that way the property would appear as "is not set".
*) Keep throwing exceptions when accessing $entity->property->value() as now if the entity is not loadable.
*) Don't filter list of entity-ids for being valid, just as a single entity wrappers containing an id. Document that there is no warranty the ids are valid for getIdentifier() and value() when returning ids.

Thoughts?

>I'll see if Damien has any other thoughts toward a solution in here.
Yep, more input on this issue would be very valuable!

Good question.

Currently, it would fetch the wrappern, and then load each entity on its own. Thus, we would have to return the NULL value for the entity.

We could add a $wrapper->commerce_line_items->value() call before looping over the wrapper, what would trigger the multiple-loading. Thus we could filter invalid values at this step? Afterwards, the single-load would benefit from the static cache.

I'm not sure about the filtering though, as it would be a bit inconsistent. Before the load is triggered you'd get more values than afterwards. For that reason I tend to think it would better stay with the empty wrapper.

We should probably introduce a helper to ease checking it though, e.g.
$wrapper->isEmpty()
? Or is that to easily confused with PHP's empty() ? $wrapper->isset() or $wrapper->hasValue()?

No, there would be a wrapper but the $wrapper->value() would return NULL - generally this is the case for wrappers of a property that is not set.
Accessing $wrapper->value() is fine, but returns NULL. However once you'd do $wrapper->property->value() you'd get an exception ("unable to fetch data as the parent data structure is not set" or so).

Coming from a duplicate issue #1253140: Don't throw exception if can't load entity -- subscribe

*) Don't throw an exception when accessing $entityWrapper->value() but return NULL if it cannot be loaded. Thus, that way the property would appear as "is not set".
*) Keep throwing exceptions when accessing $entity->property->value() as now if the entity is not loadable.
*) Don't filter list of entity-ids for being valid, just as a single entity wrappers containing an id. Document that there is no warranty the ids are valid for getIdentifier() and value() when returning ids.

I think that's the way to go. Attached patch implements it. Please test.

+++ b/includes/entity.wrapper.inc
@@ -947,10 +946,19 @@ class EntityListWrapper extends EntityMetadataWrapper implements IteratorAggrega
+   * still accessible, however the entity object value would be NULL. That way,
+   * there may be NULL values in lists of entity objects due to stale entity
+   * references.

Instead of getting NULLs, maybe we should array_filter() first?

That would make it rather inconsistent, because a foreach over the list would get you a wrapper for each item. The same way just getting the identifiers does contain stale references by design (it's not loaded yet).
But directly using value() on the list would remain different results, what's inconsistent and might trigger odd bugs if code relies upon the same things to be there.

Consider

foreach ($list_wrapper as $wrapper) {
 $v[] = $wrapper->value();
}

and

foreach ($list_wrapper->value() as $entity) {
 $v[] = $entity;
}

Pretty much the same - so should be the results.

> That would make it rather inconsistent, because a foreach over the list would get you a wrapper for each item.

Why would array_filter() result in getting the wrapper -- It's just to remove the NULLs form the list, or maybe I missed what you meant in #13 ;)

It would result in the two code-snippets from #13 getting different results in $v. Once with NULL values, once without.

Subscribe.

Committed #11.

if I could still offer a different solution.
make sure that the we can load the entity as per
http://api.drupal.org/api/drupal/includes--common.inc/function/entity_lo...