Currently, the "Missing data values" exception is a black box that is needlessly frustrating to debug.

EntityMetadataWrapperException: Missing data values. in EntityMetadataWrapper->value() (line 83 of /var/www/public/sites/all/modules/contrib/entity/includes/entity.wrapper.inc).

Instead, let's increase the verbosity of the error so developers have context about what went wrong.

EntityMetadataWrapperException: Missing data values on type "node" property "nid" with ID "false" from FUNCTION in /var/www/src/sites/all/modules/custom/MODULE/MODULE.module line 20 in EntityMetadataWrapper->value() (line 84 of /var/www/public/sites/all/modules/contrib/entity/includes/entity.wrapper.inc).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

FluxSauce’s picture

Status: Active » Needs review
FileSize
1.09 KB
FluxSauce’s picture

Sorry, got a little too far up the callstack, this will be more actionable.

joelpittet’s picture

Big +1 to this, thanks for writing this up. One question since it wasn't mentioned nor there before the patch.

+++ b/includes/entity.wrapper.inc
@@ -80,7 +80,15 @@ abstract class EntityMetadataWrapper {
+      throw new EntityMetadataWrapperException(t('Missing data values on type "@parent_type" property "@name" with ID "@parent_id" from @function in @file line @line', array(
...
+      )));

Should this be translated?

FluxSauce’s picture

Status: Needs review » Needs work

Good call! Looking at #817160: Don't translate exceptions and it looks like no, I shouldn't be translating it. Hard habit to break! I'll do something similar to https://api.drupal.org/api/drupal/includes%21errors.inc/function/_drupal...

d0t101101’s picture

+1
This patch helped me to break down our cryptic EntityAPI error messages to something meaningful. Couldn't have sorted a nasty bug without it so quickly! Thanks, and hope to see this applied to the next release.
.

joelpittet’s picture

re #3 We shouldn't translate exception messages. I just found this out today we have docs:)
https://www.drupal.org/node/608166

FluxSauce’s picture

Honestly forgot about this and it fell off my radar. Friday is my contrib day, I'll re-roll!

FluxSauce’s picture

Assigned: Unassigned » FluxSauce
Status: Needs work » Needs review
FileSize
1.1 KB

Re-rolled with sprintf.

Status: Needs review » Needs work

The last submitted patch, 8: entity-data_value_exception_verbose-2482795-8.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: entity-data_value_exception_verbose-2482795-8.patch, failed testing.

Status: Needs work » Needs review
joelpittet’s picture

Maybe -dev is broken. This shouldn't cause failures.

Status: Needs review » Needs work

The last submitted patch, 8: entity-data_value_exception_verbose-2482795-8.patch, failed testing.

FluxSauce’s picture

Status: Needs work » Needs review

Waited a week, let's see if the test system will pass.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Let's go with RTBC on that:) Random testbot failure.

FluxSauce’s picture

Random testbot failure.

¯\_(ツ)_/¯

Thanks!

leewillis77’s picture

Hi;

I just tested the patch from #8 which gives me output like this:

EntityMetadataWrapperException: Missing data values on type "field_contact_firstname" property "profile2" with ID "false" from value.

That doesn't seem right - surely the field name / entity type should be the other way around, e.g.

EntityMetadataWrapperException: Missing data values on type "profile2" property "field_contact_firstname" with ID "false" from value.

If so - revised patch attached :)

sokru’s picture

+1 for commiting #19 or #8

fago’s picture

Status: Reviewed & tested by the community » Needs work

+1 on giving a better error message, but I do not think it's our job to add a debug backtrace. For that other error handlers like the one from devel can be used. Besides that, in another recent addition there was a method debugIdentifierLocation() added to the wrappers - it probably makes sense to re-use this one here also to add info about the wrapper location to the exception.

SocialNicheGuru’s picture

@fago, I thought I could access it like this $this->debugIdentifierLocation() but I got this error:
PHP Fatal error: Call to undefined method EntityDrupalWrapper::debugIdentifierLocation() in modules/all/entity/includes/entity.wrapper.inc on line 374

$this is defined and is an object. How do I access debugIdentifierLocation?

FrittenKeeZ’s picture

The method EntityMetadataWrapper::debugIdentifierLocation() does not exist in the latest release of the Entity module.

Falco010’s picture

+1 confirming patch #19 is working, thank you for saving so much headache!!!

emarchak’s picture

@fago, given that EntityMetadataWrapper::debugIdentifierLocation() does not exist, what would be the next steps to get this patch in? Removing the backtrace?

Spokje’s picture

Reworked patch #19 so that if the devel-module is enabled, the "verbose" information in the exception will be shown.
If not, the original Exception will be thrown.

Spokje’s picture

Assigned: FluxSauce » Unassigned
natanmoraes’s picture

Status: Needs review » Needs work

Found a few issues on codesniffer. Please fix these and any other that may appear with phpcs.

  1. +++ b/includes/entity.wrapper.inc
    @@ -80,8 +80,24 @@ abstract class EntityMetadataWrapper {
    +	  if (module_exists('devel')) {
    

    Line indented incorrectly; expected 6 spaces, found 3

  2. +++ b/includes/entity.wrapper.inc
    @@ -80,8 +80,24 @@ abstract class EntityMetadataWrapper {
    +        ¶
    

    Whitespace found at end of line

  3. +++ b/includes/entity.wrapper.inc
    @@ -80,8 +80,24 @@ abstract class EntityMetadataWrapper {
    +        ¶
    

    Whitespace found at end of line

  4. +++ b/includes/entity.wrapper.inc
    @@ -80,8 +80,24 @@ abstract class EntityMetadataWrapper {
    +        ¶
    

    Whitespace found at end of line

  5. +++ b/includes/entity.wrapper.inc
    @@ -80,8 +80,24 @@ abstract class EntityMetadataWrapper {
    +      }
    

    Closing brace indented incorrectly; expected 3 spaces, found 6

  6. +++ b/includes/entity.wrapper.inc
    @@ -80,8 +80,24 @@ abstract class EntityMetadataWrapper {
    +	  else {
    

    Spaces must be used to indent lines; tabs are not allowed

  7. +++ b/includes/entity.wrapper.inc
    @@ -80,8 +80,24 @@ abstract class EntityMetadataWrapper {
    +	}
    

    Closing brace indented incorrectly; expected 4 spaces, found 1

Other than that, the patch looks good and did the trick to help debug an error

vitalius2009’s picture

I suggest including the name and the parents's type and id in any case, regardless whether the devel module is enabled or not. We don't enable this module on the production sites in most cases, but we need to know the reason for this weird error.

vitalius2009’s picture

Status: Needs work » Needs review
scotwith1t’s picture

Status: Needs review » Reviewed & tested by the community

This was SUPER helpful for me today. Had a site where the client deleted entities which were in an entity queue and the page using the queue was failing with the super-unhelpful "Missing data values" message. After trying to no avail to figure out what was happening, this patch helped me immediately figure out what the issue was, resaved the entity queue so it could stop referencing the deleted node and voilà! Thanks!

Normunds Lauva’s picture

FINALLY! Hours/days in total wasted of precious life doing desparate debugging! Thanks a lot for this!

denix’s picture

Shall we expect a new release? Thanks for this!

apaderno’s picture

Issue tags: -Debugging +Debug
TR’s picture

Status: Reviewed & tested by the community » Needs work

From #21:

+1 on giving a better error message, but I do not think it's our job to add a debug backtrace. For that other error handlers like the one from devel can be used. Besides that, in another recent addition there was a method debugIdentifierLocation() added to the wrappers - it probably makes sense to re-use this one here also to add info about the wrapper location to the exception.

Yes, it would be super useful to have this information, but @fago's review in #21 has never been addressed, which seems to be why this issue never progressed.

The patch in #29 should be changed to use debugIdentifierLocation() as @fago said. The patch should also be changed to use ddebug_backtrace() from the devel module rather than rolling our own solution. I would be fine with checking to see if the ddebug_backtrace() function exists (rather than checking for the devel module) - if it does then use ddebug_backtrace(), else use the backtrace code that's already in the patch. That addresses the concern raised in #29 about getting some of this more detailed information on sites that aren't running devel.