There has been several reports of "Fatal error: Cannot access empty property in field.attach.inc", in various line numbers in various functions.

Those happen when Field API receives an $entity without a 'bundle' property (i.e a $node with an empty $node->type). Field API is in no way bullet-proofed for such cases (don't babysit broken code), and those fail in several cryptic ways, depending on the call sequence.

The causes can be anything (for #1028230: Fatal error: Cannot access empty property in field.attach.inc it's _node_index_node() trying to manipulate a node that failed to load, for #1015580: Media entities : Cannot access empty property in field.attach.inc line 677 it's Media module failing to initialize files that existed before the module was enabled, but there seems to be a second, unrelated case), but the issues get assigned to Field API, and the error messages provide very little indication on the actual source of the problem.

Attached patch makes sure we fail "cleanly" on those cases, while providing some info to track the bug :
- raise an exception when entity_extract_ids() hits an $entity without bundle
- the exception message contains the entity type, and a partial callstack

I'm aware it's unusual to provide explicit callstack info in error messages. The bugs mentioned above, however, would have been extremely difficult to pinpoint without it.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yched’s picture

Side note : At some point, CCK D6 had a fair number of wrongly targeted error reports, because it was fed with $node objects without a type, and raised warnings. Until we stopped the flood by adding a check to silently bypass such invalid data.

This is less an option in D7 because of the 'entity' abstraction, and because the number of places to check would make this ugly. If it's broken, we fail.

catch’s picture

Subscribing.

yched’s picture

Version: 7.x-dev » 8.x-dev

Bump + moving to d8.

We'd really need to get this (or something like this) in. Threads like #1028230: Fatal error: Cannot access empty property in field.attach.inc are impossibly hard to sort out with people chiming 'me too' while the various underlying actual bugs are completely unrelated.

sun’s picture

Status: Needs review » Reviewed & tested by the community

I think we need to extend Drupal's exception handler function to provide that backtrace instead. Exceptions should be able to specify how many callers to show (or potentially some other funky algorithms).

However, this proposal doesn't invalidate this patch. This code merely needs to be updated when a new facility exists.

Lars Toomre’s picture

In general, I support this patch. It will reduce the number of Drupal WTF occurrances.

However, I must ask why this particular case deserves special debugging information display? Core functions in many places could benefit from is_* checking and then writing a helpful message to *wherever* (watchdog?). Why is this case special?

I have often seen in the issue comments rather stern and authoratize statements that the core practice is to assume that the parameter should be set when calling that particular piece of code. The inelegant failing should then be interpeted as a major problem by the developer. Those comments frankly often have seemed rather arrogant and off-putting., but whatever.

What is different in this case? To conform with 'policy', why shouldn't the core function entity_extract_ids() assume that the bundle property is set and to paraphrase those comments above 'fail inelegantly' if it is not set? Why does this particular function (an important one no doubt) deserve debugging output? If it does, why should not core function 'xyz' deserve similar treatment?

We need to be consistent and I might suggest less arrogant in some issue queue comments. Also, as I started with, I support this patch and hope that similar such debugging patches get applied to reduce the Drupal WTF occurances.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Count me as someone who doesn't like patches like this, you can call me inconsistent and arrogant if you like to shut down discussion.

This is adding over 20 lines of code to a short function, to account for downstream bugs in contrib modules. Nearly all the core bug reports related or similar to this have been due to media module (or media + media_gallery), which is very unstable at the moment, yet also popular. media_gallery has no tests last time I looked, haven't checked media yet - but it's the sort of thing that should be caught by those, not bloat in field API.

It is annoying when contrib module bugs get posted against the core queue due to PHP errors getting thrown by core functions, but that is a side-effect of a very interdependent system like Drupal (and partly due to us using structured arrays all over the place). If we did type and consistency checking like this in all core functions it could end up double the size.

Also I don't see why a module couldn't implement hook_entity_info_alter(), check for invalid/missing bundles, and log to watchdog/flash big warnings on the screen that way, doing the check on runtime is overkill. edit: for example this could be a useful devel add-on.

bojanz’s picture

If we did type and consistency checking like this in all core functions it could end up double the size.

Then lets double the size.
"Fatal error: Cannot access empty property in field.attach.inc" is the exact opposite of helpful.
I don't need fancy backtraces and additional info, but at least have the system tell me "hey man, what happened to the bundle key?"

This is one of my pet peeves in Drupal (hence maybe me being a bit harsh), the fact that from forms to menus and entities, it gives the most cryptic message possible for the most stupid errors, usually requiring me to open the appropriate core file and see what exactly is going on.
Just catching all errors and outputing "Fatal: I'm a teapot." would be just as helpful as the current approach.
It's horrible DX. Assuming each developer is perfect and never makes errors is not feasible.

catch’s picture

Issue tags: +Needs backport to D6

In this case though, why can't we add a similar check to devel? I agree it's not on to have to ask end-users to apply patches to get debugging information, but they can install devel module.

I posted several patches to make the various exceptions thrown by field CRUD functions more helpful (in these cases there were exceptions with a nice message, but I still had to go in and debug() to see what was actually going wrong). I don't like the pattern, but I liked the messages when I got them ;)

But similarly devel module could implement hook_field_create_field() and the rest, and chuck the same errors.

If moshe says "no you are not putting all the crap in devel" then fair enough, but someone please explain why that's such a bad idea otherwise. Not only would it avoid adding all the extra code to core, it would also mean we could add loads of it all over the place - could be sub-module of devel to keep things separated.

bojanz’s picture

But why require the devel module for normal DX? This is not just important for developers, would also make it easier for users to report issues, since the error could actually give a hint on what's happening.

The patch in #0 could be cut back to:

+    if (is_null($bundle)) {
+      throw new Exception(t('Encountered an entity without a bundle property (entity type: @entity_type).', array('@entity_type' => $entity_type)));
+    }

And that alone would be enough to bring DX into a new age (if we do the same in a bunch of other places as well. We already did it for EntityFieldQuery).

Sure, backtraces could be left to the devel module, though we'd need to think of a way to make it possible then.

yched’s picture

Issue tags: -Needs backport to D6

Two things here :

- catch the errors in one single spot (entiy_extract_ids()) with a message reflecting the actual problem, rather than at a random place down the line with completely cryptic symptoms.
--> exception in entiy_extract_ids(), as a minimal 1st step.

- being able to identify where the problems come from and where the issue should be redirected to, instead of piling up in the Field API maintainers queue, leaving it to them to do the triage and resulting in an unmanageable list of "me too"s
--> there's no other way than callstack info.

@catch : don't think that this is just about poorly coded contrib modules. Several occurrences of #1028230: Fatal error: Cannot access empty property in field.attach.inc is caused by comment_node_update_index() during cron (or the new "poorman's cron" at the end of a random page request), because comment_load_multiple() returns NULL on some cids for some reason. *Each* report and 'me too' post over there currently requires I individually ask each reporter to apply a debug patch, keep a mental track of 'yet unresolved' reports, etc. I *won't* be playing this game much longer.

Now, I reckon that this is not an existing pattern in core, that some other areas (menu.inc...) also collect errors coming originating up the callstack.

The difference between this and "babysitting broken code" is that the added code and CPU only takes place whithin a if (failure condition) { check, as opposed to safety isset() checks littered all over the code flow or whatever.
It only adds 20 lines because there's currently no generic mechanism for this. As sun points out in #4, that's only a matter of refactoring, and IMO can't be held against the overall approach.

yched’s picture

Issue tags: +Needs backport to D6

crosspost, I didn't intend to remove the tag (although I don't understand why this woudl need a d6 backport ?)

catch’s picture

This is not just important for developers, would also make it easier for users to report issues, since the error could actually give a hint on what's happening.

Right, but anyone can install devel module, not just developers.

@yched:

The difference between this and "babysitting broken code" is that the added code and CPU only takes place whithin a if (failure condition) { check, as opposed to safety isset() checks littered all over the code flow or whatever.

mea culpa, I missed that this was primarily breaking the ternary into an if else, and the code only gets executed in the fallback.

So I will stop being a pain here, except to ask 1. Can we have an issue opened for adding the backtrace enhancement somewhere central? 2. Could a @todo be added to the code pointing to that issue?

sun’s picture

joachim’s picture

> Right, but anyone can install devel module, not just developers.

True. And most developers should be using Devel if they've any sense :)

But if this can all be done with alter hooks, how about adding a Debug module to core?

That way we have none of the performance loss, the extra babysitting code is kept out of the way, and it's simple to tell a bug reporter to enable it.

bjaspan’s picture

I am in the camp that believes programs should be written conservatively, which means validating arguments and failing explicitly. On my side of the argument is probably every OS kernel ever written that will return "invalid argument" for a NULL pointer instead of crashing the whole computer. What's good enough for the kernel is good enough for Drupal. :-) Obviously, therefore, I think "don't babysit broken code" is a misguided statement. Especially in a language like PHP which has practically no validity checking of any kind, paranoid code is good thing.

I'm not a huge fan of this patch as written only because it is so obvious that the error handling logic is misplaced in the Entity API function. As others have said, that logic should be elsewhere, and then Entity API should just use it.

Would it really be any harder to create a DrupalException class that includes stack information elsewhere and then use in this patch? If so, for some reason, then as an intermediate step, we could create a separate EntityException class which contains the backtrace logic rather than building it directly into entity_extract_ids().

yched’s picture

Status: Needs work » Needs review
FileSize
1.26 KB

OK, since #1158322: Add backtrace to all errors is open for debate (thks @sun), and although I kind of fear a fair share of bikeshedding over there, rerolled with only the "raise exception" part.

bojanz’s picture

+    $bundle = $entity->{$info['entity keys']['bundle']};
+    // Explicitly fail for malformed entites missing the bundle property.
+    if (is_null($bundle)) {

Wouldn't it be better to do an !isset on $entity->{$info['entity keys']['bundle']}, instead of throwing a notice when the bundle key is completely missing (and not just empty)?

yched’s picture

Good call. Extra care about $node->type === '', though, I think I encountered one such case in #1028230: Fatal error: Cannot access empty property in field.attach.inc.

bojanz’s picture

+    if (!isset($entity->{$info['entity keys']['bundle']}) || $entity->{$info['entity keys']['bundle']} === '') {

Can't this basically be shortened to an empty() call? Or does core prefer to do it this way?

I don't care much about this type of nitpicking, so it has my +1 on RTBC in any case.

yched’s picture

empty() would also catch $entity->{$bundle_key} === '0' or (int) 0 as erroneous, while it should theoretically be valid (numeric values for bundles names, why not, I guess)

bojanz’s picture

Status: Needs review » Reviewed & tested by the community

Okay, makes sense.
Marking this as RTBC.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/includes/common.inc
@@ -7361,12 +7361,25 @@ function entity_info_cache_clear() {
+    // Explicitly fail for malformed entites missing the bundle property.

Typo in "entites".

+++ b/includes/common.inc
@@ -7361,12 +7361,25 @@ function entity_info_cache_clear() {
+      throw new Exception(t('Encountered an entity of type @entity_type with no bundle property.', array('@entity_type' => $entity_type)));

Exceptions and errors should be less verbose/wordy.

"Missing bundle property on entity type @entity_type."

+++ b/includes/common.inc
@@ -7361,12 +7361,25 @@ function entity_info_cache_clear() {
+
+

Duplicate newline.

Powered by Dreditor.

rfay’s picture

Status: Needs work » Reviewed & tested by the community

subscribe

sun’s picture

Status: Reviewed & tested by the community » Needs work
yched’s picture

Status: Needs work » Needs review
FileSize
1.42 KB

Updated for sun's remarks in #22.

"Encountered an entity of type @entity_type with no bundle property" vs "Missing bundle property on entity type @entity_type" :
The former was more in line with the other Field API exception messages, but, a shorter version suits me fine. Adjusted the proposed message a bit though : the error is about an entity of a given type, not about the entity type.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

Lars Toomre’s picture

Silly question... Because this patch defines a new translation string, are we prevented from adding this patch to D7 where it is most needed at present?

moshe weitzman’s picture

@Lars - we try not to change strings and perhaps try not to add them but if it naturally happens during a bug fix, it happens. Not a big deal.

chx’s picture

As one of the proponents of the "no babysitting broken code" what can I say -- bring it on! Everything is better than "Fatal: I'm a teapot".

Dries’s picture

Priority: Major » Normal

I'm lowering the priority to 'normal'. I don't believe this is 'major'.

sun’s picture

Priority: Normal » Major

One may or may not believe in god or something, but changing an issue priority based on personal belief, without any other sign of an actual reason, is not very respectful.

moshe weitzman’s picture

Priority: Major » Normal

It is customary not to override a priority set by the maintainer. This is not a democracy.

rfay’s picture

Priority: Normal » Major

I maintain that this and several other "appropriate communication to the user about failures" issues are clearly major. Really, they've been major for years, but we're just getting to them.

rfay’s picture

Priority: Major » Normal

x-post. I was just expressing my support for the importance of this. Setting back to moshe's setting so others can fight about it.

Crell’s picture

I am very +1 on throwing explicit exceptions rather than teapot fails. (I sense a new meme...) Note that all Exceptions in PHP include a backtrace by default, so that can be decoded wherever the exception is caught. We don't need to do anything else there. (Devel can do what it wants.) Note that creating an exception object is actually rather expensive for exactly that reason: Most of its creation time is generating the backtrace internally.

However, I have one problem with the attached patch. Simply throwing Exception everywhere doesn't allow for actually robust error handling, since you cannot catch separate exceptions separately. The preferred approach is to create a more specific Exception class that makes sense in that case, extending from one of the SPL exception classes: http://www.php.net/manual/en/spl.exceptions.php

That way you can catch different exception types sanely and handle them appropriately.

yched’s picture

@Crell :
Specific exception class : sure, but I do fear the bikeshedding potential here.
EntityException ? (most of core "specific" exceptions are currently something like SomeNamespaceException)
InvalidEntityException ?

Extending one of the SPL exception classes : definitely a good idea, but can we please discuss that in a more general issue and not have this poor thread assigned the mission of building an agreement for the whole of core D8 :-) ?

What to do with the callstack is the topic of #1158322: Add backtrace to all errors, but I'll simply state once more that the goal here is to stop completely unrelated issues piling up in one or several Field API issues (be it titled "fatal error in (some function) in field_attach.inc" or, only slightly better, "Exception : entity without a bundle property"), without any way to sort out the reports other than asking each reporter individually to take a couple debugging steps and provide feedback.

Lars Toomre’s picture

@ysched: Looking at the number of issues that you have had (and are likely to have in the future) in Field API issue queue, this patch as currently is needs to go in. There is no need to ask users to apply a patch to get critical information about what upstream function is causing the errors that show up in the Field API.

This patch is what @catch describes as a d7 patch that will help greatly to reduce both user WTF experience and support requirements when dealing with technically inexperienced users dealing with a new Drupal sub-system API.
As such, this at minimum should go into D7.

I suppose it also should go into D8 as a placeholder to keep development current with production code. However, we need somehow to mark this issue as a "needs to be solved better" in D8 with a link perhaps to #1158322: Add backtrace to all errors. Perhaps we also want institute some mechanism to rollback this patch after 'X' time if the development version has not been implemented?

yched’s picture

To be extra clear : this patch alone is only a minimal measure, which clarifies currently obscure error messages (+ 1 for the 'teapot' meme). Without #1158322: Add backtrace to all errors, we'll still need to ask individual reporters to apply a debug patch before we can sort out the bug.

fago’s picture

Generally, +1 for the patch and fighting malformed entities.

However, I have one problem with the attached patch. Simply throwing Exception everywhere doesn't allow for actually robust error handling, since you cannot catch separate exceptions separately. The preferred approach is to create a more specific Exception class that makes sense in that case, extending from one of the SPL exception classes: http://www.php.net/manual/en/spl.exceptions.php

Agreed. What about adding an "EntityMalformedException", or just "EntityException". At least, it should be in the "entity" namespace.

chx’s picture

Status: Reviewed & tested by the community » Needs work
Michelle’s picture

I was totally baffled by getting this error on my entity that doesn't even have any fields and would never have found this issue if not for chx. (Thanks, chx!) To help anyone else that may be in the same boat as me, I'm adding in field.multilingual.inc which is the file with the error for me so that it comes up (hopefully!) when searching on the error message.

More search engine food: cannot access empty property in field.multilingual.inc line 275

Michelle

fago’s picture

Status: Needs work » Needs review
FileSize
1.74 KB

Re-rolled patch with the suggested EntityMalformedException.

fuscata’s picture

subscribing

yched’s picture

Bump.
Considering comments #35 and #36, I'd say this should be ready now. RTBC anyone ?

chx’s picture

Status: Needs review » Reviewed & tested by the community

Yeah. It's ready. I was thinking whether the class being in entity.inc will cause havoc but no -- by the time common.inc is loaded the registry is in place.

chx’s picture

#10 alone should be enough to mark this major -- anyone seriously developing D7 will run into this, repeatedly and it's untrivial to figure out what happens. But, here we are half a year after D7 is released and despite every agreeing this sits here RTBC for two weeks and it's not even major and we wonder why the D7 uptake is so small.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +String freeze

While on its face, this patch violates both string freeze and "we don't babysit broken code," OTOH it seems this is tripping up enough people that it's worth it.

Committed and pushed to 8.x and 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

Status: Closed (fixed) » Needs review
FileSize
724 bytes

I think most of what #42 does makes sense, but I don't understand why empty string must trigger an exception. Here's a reversion of just that part.

The use-case for empty string being useful as a bundle value indicating no bundle is for File Entity module (both http://drupal.org/project/file_entity, and an earlier version of it bundled with Media 1.x). That module is still in flux, so I'm not sure what we'll settle on, see #1227084: Provide built-in support for pre-defined file types and #1260050: Provide administrative UI for adding/removing file types, but at least in the Media 1.x branch currently, we have this idea that in some cases we might not be able to determine the file type, and in this case, we want the file entity to not receive any fields. Perhaps at a later time, with another module enabled, the file type can be determined, and then that file can get the appropriate fields for its type.

Is this a good enough reason to allow empty string? Otherwise, in File Entity, we'll have to implement some magic string like 'UNKNOWN', and write special code to disallow fields to be added to this magic bundle.

effulgentsia’s picture

For those interested, linking the issues for looking into how this is affecting:
Media 1.x: #1266620: Missing bundle property on entity of type file error
Media 2.x: #1268378: Empty file type causes exceptions in Drupal 7.8

xjm’s picture

Re: #49 -- It seems to me that it would be better to implement a file_generic or filetype_unknown bundle or something for those cases. An empty string is not informative, and could easily show up because of bad code in addition to your case where it is picked deliberately.

yched’s picture

Not too fond of supporting bundle == '' either.
- Easier to hit false positives on isset() / empty() checks somewhere down the road.
- For entity types with no bundles (i.e. 1 single bundle, e.g. 'user' entity type), we explicitly make sure that the bundle gets set to the entity type string, so that it doesn't stay empty.
- Either way, the feature you mention of 'this bundle can hold no fields' doesn't exist in Field API, and would have to be enforced from the outside, whatever the bundle name. There could be a feature of 'bundle name consisting of an empty string cannot hold fields', but that would sound like implicit magic rather than an explicit feature.

chx’s picture

@effulgentsia, I have written you a patch that adds type on-the-fly. There's no problems with adding another if !$type $type='unknown' and at the same time do nothing else. It'll be actually easier to work with those entities because it's more evident what the type is instead of trying to figure out what the media maintainers wanted with an empty bundle.

Damien Tournoud’s picture

As yched was pointing out, bundle-less entities have a single default bundle that the same string as their entity type. It would be best if File Entity respected this convention.

webchick’s picture

Yeah... I'd have to agree. While I'm really sorry for breaking APIs suddenly by inadvertently making this check more strict than it was in 7.7 and below, it sounds like Media module is relying on a weird bit of undocumented magic here for its purposes. I think it would be better from both a future maintainability and DX perspective for the module to be explicit about the behaviour that it's intending in this case (so code to directly turn off field-ability if it hits this case), and to follow conventions already established by other entities.

effulgentsia’s picture

Status: Needs review » Fixed

Ok, moving to fixed as it was in #47, but not "closed (fixed)", to keep this visible for a couple weeks.

Re #54: the use-case for Media is different than bundleless entity *types* like users. Here, we want to make the file entity type have bundles (e.g., image, audio, video), but for *some* file entities to intentionally not be any of these. For Drupal 7.7 and below, setting $file->type to empty string worked perfectly for this. However, if field system maintainers consider this to be a fluke, rather than an intentional feature, as per #52, then so be it. Media can adapt to work within the new, stricter, more maintainable, rules.

Some time soon, however, I think we'll need to become stricter about maintaining backwards compatibility in 7.x point releases: even undocumented and unintentional features are still features when the source code is public and there's no documentation saying *not* to make a particular assumption. But I also realize that sometimes it's really hard to know when something is merely a bug fix, and when it's a BC break, so no worries, I still love you all.

webchick’s picture

Thanks, effulgentsia. That's definitely fair criticism. And I really do apologize for the unintended side-effects of this patch. I'll try really hard to be more on the lookout and conscientious of this kind of potential breakage in the future. :\

webchick’s picture

I also updated the bottom of http://drupal.org/drupal-7.8 with a note about this being a known issue.

wellsys_world’s picture

Status: Fixed » Needs work

Hi,

I came across this problem because we were getting a WSOD with a Views page. I upgraded to 7.8, found this thread and applied the patch in #49.

Now I am getting the following message on /user and can't log in as an Admin:

EntityMalformedException: Missing bundle property on entity of type node. in entity_extract_ids() (line 7390 of /var/www/vhosts/drupal/includes/common.inc).

There's nothing useful in the PHP error logs and nothing to help me track down if this is now core or another module causing this.

Help! I need my site back

Thanks,
Wellsy

catch’s picture

run this.

SELECT nid FROM node WHERE type = '';

If you get results, you have invalid nodes in your system - those are likely to be throwing errors on node/$nid even without this patch applied.

yched’s picture

Status: Needs work » Fixed

As @catch said, the actual error predates this patch, which was only about providing a single and clear point of failure and an explicit error message when such an error condition is met. So let's leave this thread to 'fixed', please open a separate issue for the actual bug.

As mentioned above, actually pointing the faulty code requires printing the callstack stored in the exception that gets raised - #1158322: Add backtrace to all errors

wellsys_world’s picture

@catch - thanks for the tip. Unfortunately I get no results through phpmyadmin.

I know my way around LAMP but am new to Drupal. The problem is only on one test site at the moment so I'll watch the threads and keep reading

Thanks

DamienMcKenna’s picture

Another scenario: I ran into this error today with a view that was set to use a specific entity build mode but which wasn't actually available due to Strongarm not executing early enough (#1273424: Cannot export 'entity_view_modes' variable with Strongarm); once I worked out another way of storing the build modes (so they actually worked) the error disappeared.

Status: Fixed » Closed (fixed)

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

ezeedub’s picture

Here's #49 re-rolled for 7.10.

I got here via the media queue. Just looking at the log... Was #42 intended to be committed instead of #49?

effulgentsia’s picture

@ezeedub: see conversation from #51 to #57. #49 was rejected. However, as per #56, Media was fixed to comply. Please add a link here to the Media issue you're coming from, and we can continue the conversation there.

ezeedub’s picture

Thanks effulgentsia. I have to confess, I kinda suspected I was doing something wrong by posting this patch. I had read the messages you mentioned. The problem was... I didn't understand them! :/ Groking entities is a work in progress...

I actually got here from your comment #10 here: #1266620: Missing bundle property on entity of type file error which has since been marked as a dupe of #1268378: Empty file type causes exceptions in Drupal 7.8.

I actually think I'm out of the woods again, but I'll post what happened in that 2nd thread for completeness...

HongPong’s picture

This may be a tangent but if you have malformed files entities - see #1446440: Unable to view me media management pages (EntityMalformedException: Missing bundle property...) for my one-off SQL fix for column type in the files_managed table.