There have been several issues raised against this module, such as #1298864: Error while viewing nodes with field collections when eva module enabled which cover a few problems with extracting entities from their build content in _eva_extract_entity_from_build().
While that function will usually work, it is only guessing at where the full entity is within the build content, so I propose adding a call to a function that modules providing custom entity types can implement, which would tell EVA where to take the entity from. See the attached patch, I trust it is simple enough to understand. Modules can return FALSE in order to stop EVA continuing operating on any completely incompatible entity types, to avoid EntityMalformedExceptions. If the entity extraction fails anyway, the patch will also stop EVA continuing.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | eva-1772204-11.patch | 1.37 KB | tim.plunkett |
Comments
Comment #1
mkadin commentedThis seems fine to me...but 2 Q's:
1) Is it more drupaly to use module_invoke_all or something like that for module providing the entity?
And more importantly
2) If a module developer is going to go through the trouble of passing this data through a special function call, isn't it just as easy (if not easier) to put the entity in a place where EVA expects it?
Also, I think if the entity can't be found, there SHOULD be an error message. Otherwise developers will not know what to fix!
Comment #2
james.williams1) Yes that's fair to say, although I think only the module providing the entity should get to define this, as what happens when multiple modules try to set the location? module_invoke(), perhaps followed by a drupal_alter() might be the best solution.
2) This means that modules have to define their behaviour according to how EVA wants them to, which I don't think is fair. For an example of a module which otherwise clashes with EVA, Reply module defines its own 'reply' entity type, and the '#entity' property on Reply entities is not the actual reply entity, rather it is the entity that is being replied to (such as a node, user, etc). So EVA attaches wrongly, and can cause EntityMalformedExceptions because it expects this to be done in a certain way. Allowing modules to tell EVA how it should act is better (more helpful, less imposing) than EVA enforcing all modules to act in a certain way in my opinion.
If the entity can't be found, then that's not necessarily because it's not there -- it could just be that it's incompatible with EVA, in which case EVA should not be allowed to cause fatal errors, when it ought to just degrade gracefully. It should probably log the event in watchdog though to help developers as I agree developers should be helped. But it's a big error for end users to see that could be easily avoided.
Comment #3
mkadin commentedOk I'm starting to see your point...A module could already be using the #entity key in the build array for something else. I'm thinking the easiest solution might be to have an array of key value pairs where the key is the entity_type and the value is the key of the build array where the entity can be found. Then we just Drupal that array and allow other modules to jump in and do whatever they need. I'll throw together a patch for you to check out.
As for the error message, I think perhaps the best approach is to avoid the entity malformed exception, but as you said, provide an error message of our own that says that the entity could not be found. I do think this should appear as a normal error message and not just as a watchdog() so that people installing a custom entity module or EVA won't be lost trying to figure out why it won't work. In a production environment, these errors shouldn't be visible to end-users anyway.
Comment #4
mkadin commentedHow about this?
Comment #5
james.williamsThat looks like a much better solution, yes.
A few things though just to keep improving this:
1)
$viewsineva_entity_view_alter()should not be looped over if the entity could not be extracted. Otherwise that's just an unnecessary loop.2) I don't believe error messages are hidden from end-users. PHP error messages would normally be hidden, but ones created from
drupal_set_message()are different. Perhaps a different way of showing the message is needed, or a permission should just be checked first?3) While
_eva_extract_entity_from_build()now finds the entity itself in a customisable way, it still assumes that$build['#entity_type']is valid/correct - but this cannot be guaranteed for the same reasons as the entity itself. Rather than using$build['#entity_type']you could just pass the entity type as a second parameter to_eva_extract_entity_from_build()since its caller (in this case,eva_entity_view_alter()which receives it as a second parameter) should know this.I hope that makes sense! Sorry I don't have the time to actually write the modifications for a patch.
Comment #6
mkadin commentedThanks for the feedback
1) I've moved some stuff around so that the views loop doesn't iterate unless we have a valid entity to attach to.
2) You're right that this error message will show up on the site, but I still disagree with you on this error message issue. With the current's code structure, you won't get the error message on an entity unless the site builder specifically tried to attach a view to that entity and the entity couldn't be extracted from the build array. The site builder ought to recognize the message when they attach a view.
3) Good point. Added the $type parameter.
I'm feeling good about this.
Comment #7
james.williamsI'm happy to agree to disagree on point (2), that's the kind of thing that just depends on personal preference/philosophy. Thanks for the constructive discussion!
I haven't actually tested the patch though and probably won't get chance to, but I think feeling good is justified :0)
Comment #8
earwax commentedI have applied mkadin's patch from comment #6 to my EVA module 7.x-1.2 module and so far everything appears to be running fine. I think it fixed my other issue posted here: http://drupal.org/node/1545244#comment-6926780
I assume that it's okay to apply to the EVA module 7.x-1.2 and not the 7.x-1.x-dev version? Or should I update to the 7.x-1.x-dev? let me know what is the correct way to test this so it can make it into the next update.
Comment #9
redndahead commentedHere is a re-roll against dev. I also switched the drupal_set_message to a watchdog call. I agree this shouldn't be forcefully shown on the page. Watchdog should be enough as I had this error occur on pages where eva was never used. It was due to another modules error, but it still shouldn't show up here anyway.
Comment #10
tim.plunkettI've rerolled this to include the smallest change needed to fix the bug.
No watchdog call, no undocumented alter hook. Those are features that could be added in another issue.
Comment #11
tim.plunkettI'm working a bigger change to eva_entity_view_alter() and will post another issue soon, but to help reduce conflicts with other issues, let's only make essential changes here.
Comment #12
tim.plunkettCommitted this: http://drupalcode.org/project/eva.git/commitdiff/fd195ac