I have an image field using the file field sources module so that I can easily link it to the IMCE interface.

When I select File Browser and then the Browse link for my image field, what I should get is a new window opened that has the IMCE file browser page.

What I instead get is a new window with the following:

Additional uncaught exception thrown while handling exception.
Original

EntityMalformedException: Missing bundle property on entity of type node. in entity_extract_ids() (line 7389 of /var/www/html/includes/common.inc).
Additional

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

This does not happen when I set the field permissions to Public instead of custom for the image field in question.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fangel’s picture

I'm assuming this issue is related to 7.x-1.0, because iirc 6.0 doesn't have the notation of Entities, and hence wouldn't trigger a EntityMalformedException..

However, the reason for this when using custom settings, and when hook_field_permissions is called without an object (I believe this is the case when creating new entities, and a few other times), the empty object is passed to entity_extract_ids, which since 7.8 fails..

The solution is to do something like what _field_permissions_field_view_access already does - return a default value when the object isn't set..

-M

fangel’s picture

FWIW, I've addressed the problem by swapping the order of the check for creation permissions, and the call to entity_extract_ids.

Haven't seen any adverse side-effects - so try it out.

fangel’s picture

Okay, adjusted the patch so it's no longer necessary to run it with -p6 :)

thekevinday’s picture

Version: 6.x-1.0 » 7.x-1.0-beta1
Status: Active » Reviewed & tested by the community

oh wow, your right it is d7.
I screwed up and set the wrong version number here.

The provided patch solves the problem.

David_Rothstein’s picture

Title: IMCE+File Field Sources breaks when using custom permissions » IMCE+File Field Sources (and other?) breaks when using custom permissions: "EntityMalformedException: Missing bundle property"
Status: Reviewed & tested by the community » Needs work

Marked #1326626: "EntityMalformedException: Missing bundle property on entity" error when editing a node as duplicate.

Yeah, although it's not too common, I guess some modules do call field_access('edit'...) without an entity, and since it's legal to do that, the code in Field Permissions has to support it.

However, seems like the patch will break things in the case of an entity without an ID...?

David_Rothstein’s picture

Title: IMCE+File Field Sources (and other?) breaks when using custom permissions: "EntityMalformedException: Missing bundle property" » "Create" permissions don't work correctly sometimes, and "EntityMalformedException: Missing bundle property" errors can appear
Component: User interface » Code
Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
4.21 KB

However, seems like the patch will break things in the case of an entity without an ID...?

Turns out that in some cases the patch breaks things this way and in some cases they were already broken :/

The above patch will break "create" permissions on e.g. nodes (and causes test failures because of that), but for user account entities (and maybe others) it seems they were already broken. #876550: The "create" permission does not work for fields that are attached to any entity other than nodes was supposed to fix that, but there was a change from empty() to !isset() in the followup commit there that broke it again.

Since that issue caused the EntityMalformedException error also and it's in the same part of the code, let's fix both at the same time. They are both critical bugs.

The attached patch should work; please feel free to test this against IMCE+File Field Sources and anything else. I have also written a test for the "create" permissions on user account entities.

fangel’s picture

Status: Needs review » Reviewed & tested by the community

It would appear to work - and it looks sane too :)

fago’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/field_permissions.module
@@ -171,14 +171,25 @@ function _field_permissions_field_view_access($field_name, $obj_type, $object, $
 function _field_permissions_field_edit_access($field_name, $obj_type, $object, $account) {

This looks like a rather old d7 left over - we have entities now thus its $entity_type, $entity.

+++ b/field_permissions.module
@@ -171,14 +171,25 @@ function _field_permissions_field_view_access($field_name, $obj_type, $object, $
-  list($id, $vid, $bundle) = @entity_extract_ids($obj_type, $object);

If there is an entity object, passing it on to enttiy_extract_ids() must be save. Else, the bug lies in the code who passes a malformed entity.

+++ b/field_permissions.module
@@ -171,14 +171,25 @@ function _field_permissions_field_view_access($field_name, $obj_type, $object, $
+  // If this is a new object, check if the user has access to edit the field on
+  // object creation.

While it's not documented for hook_field_access() how the no entity case should be interpreted, I'd assume having to check for field-access for *all* entities.

If it would be about creation, I'd expect an edit operation to be checked on a not yet saved entity object.

+++ b/field_permissions.module
@@ -171,14 +171,25 @@ function _field_permissions_field_view_access($field_name, $obj_type, $object, $
+    // If the object doesn't exist, we assume it's new.
+    $is_new = TRUE;
+  }

Instead, I'd suggest checking for the existence of $entity->is_new or - assume the entity is new if the id-key is not set or empty.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
13.47 KB

Thank you for the review. Here is a revised patch.

Responses to your comments:

function _field_permissions_field_edit_access($field_name, $obj_type, $object, $account) {

This looks like a rather old d7 left over - we have entities now thus its $entity_type, $entity.

That's true, although that's a preexisting issue not really related to this patch (the module already had the pattern of using 'object' rather than 'entity' everywhere). However, I wound up making enough code changes in this patch anyway that it did seem worth it to just fix that here too, so the attached patch now switches to using 'entity' everywhere.

-  list($id, $vid, $bundle) = @entity_extract_ids($obj_type, $object);

If there is an entity object, passing it on to enttiy_extract_ids() must be save. Else, the bug lies in the code who passes a malformed entity.

No, the @ was actually working around a core bug, #1301522: field_ui_default_value_widget() does not pass along the entity type when it creates the default value form, as described in the code comments. However, the PHP warnings associated with that bug don't get triggered as a result of this patch anymore, since we are no longer passing NULL entities to this function (which itself was a more serious bug and indeed the fault of this module). Therefore, I've removed the @ and the associated code comment.

+  // If this is a new object, check if the user has access to edit the field on
+  // object creation.

While it's not documented for hook_field_access() how the no entity case should be interpreted, I'd assume having to check for field-access for *all* entities.

If it would be about creation, I'd expect an edit operation to be checked on a not yet saved entity object.

Good point, the issue here is indeed not specific to the "create" case. I've looked into it a bit more and decided that in the case where no entity is provided, we actually want to check if there is any entity on the site for which access should be granted, and only deny access if there isn't (not sure if that's what you were suggesting by "check for field-access for all entities" or not).

This general rule also encompasses (and improves) what the module's code was previously doing for field_access('view') in the case of Views, so I've rewritten the code to combine both and added extensive comments to explain the reasoning behind it.

+    // If the object doesn't exist, we assume it's new.
+    $is_new = TRUE;
+  }

Instead, I'd suggest checking for the existence of $entity->is_new or - assume the entity is new if the id-key is not set or empty.

Good idea about checking for $entity->is_new (if it's present); I've added that. The empty() ID check was already in the previous patch, and I haven't removed it :)

fago’s picture

Good point, the issue here is indeed not specific to the "create" case. I've looked into it a bit more and decided that in the case where no entity is provided, we actually want to check if there is any entity on the site for which access should be granted, and only deny access if there isn't (not sure if that's what you were suggesting by "check for field-access for all entities" or not).

No, that is not what I meant. I've interpreted it as "check whether the user has access to edit/view *any* entity." :/

+ * For example, Views calls field_access('view') without providing the entity,
+ * in order to determine if the field can be included in the query itself. So
+ * we only want to return FALSE if we know that there are no entities for which
+ * access will be granted. Later on, Views will invoke field_access('view')
+ * again, indirectly, when rendering the fields using field_view_field(), and
+ * at that point the entity will be passed along so we can do our normal checks
+ * on it.

That makes much sense.

Well, having two different interpretations definitely sucks and could potentially lead to security issues. We should work on clarifying that.

As said, I've followed the "check whether the user has access to edit/view *any* entity." approach and implemented and documented it the same way for entity_access() and entity property access callbacks of the entity api module.
My use-case for that comes with Rules: Someone may only configure a rule using a property or entity if he has access to do so for all possible entities, as one cannot know the entity beforehand. It probably makes sense to restrict available tokens the same way...

Thus, probably both are valid use-cases. Not sure what would be a good solution here. :/

Not having that field_access() check for all entities what force the entity api property access for fields to return FALSE in the general case and result in people not being able to set/read fields in Rules (without the bypass access checks permission). Ouch. :/

David_Rothstein’s picture

Hm, I see your point. Yes, it sounds like there are two perfectly valid use cases, and it would be hard to distinguish which is intended (unless Drupal core mandated that callers pass along some parameter indicating which one they cared about). This is ugly.

Is Rules using this idea now for field access, or just entity access? You wrote this:

Not having that field_access() check for all entities what force the entity api property access for fields to return FALSE in the general case and result in people not being able to set/read fields in Rules (without the bypass access checks permission). Ouch. :/

But I couldn't find where the Entity module returned FALSE in the general case (maybe I was looking in the wrong place).

I do see how a lot of things in Rules require 'bypass rules access'... but maybe that's not so bad in the grand scheme of things. Given how powerful Rules is, it's reasonable to say that people must have a general, powerful permission in order to use a lot of it. But I also see why it would be useful to have more granular access control.

zilverdistel’s picture

I tried the patch in #9 and it worked (in my case anyway). Thanks!

Aeternum’s picture

Patch in #9 worked for me too

RobLoach’s picture

Version: 7.x-1.0-beta1 » 7.x-1.x-dev
FileSize
13.47 KB

Hmmm, how do you get the testing bot to hit this?

itserich’s picture

I am not good at applying patches but want to bump this issue up.

igalarza’s picture

Patch on #14 worked fine for me. Thanks!

RobLoach’s picture

FileSize
13.47 KB

Testing bot?

RonBen’s picture

Hi I am new to drupal... I was facing the same problem...I applied the patch on #14. After applying the patch I am getting following error :
Warning: include_once(C:\wamp\www\drupal-7.8\sites\all\modules\field_permissions\field_permissions.module) [function.include-once]: failed to open stream: Permission denied in drupal_load() (line 1100 of C:\wamp\www\drupal-7.8\includes\bootstrap.inc).
Warning: include_once() [function.include]: Failed opening 'C:\wamp\www\drupal-7.8/sites/all/modules/field_permissions/field_permissions.module' for inclusion (include_path='.;C:\php\pear') in drupal_load() (line 1100 of C:\wamp\www\drupal-7.8\includes\bootstrap.inc).

Any solution for this problem ? I have other permission modules like Node access user reference , content access installed. I hope that has nothing to do with this. Thanks in advance.

RonBen’s picture

can somebody please help me with this. Is there some minor fix required ? Need to fix this asap.

stewsnooze’s picture

I've just applied this to a site which suffered from EntityMalformedException as soon as I created custom field permissions. It fixed it fine.

RobLoach’s picture

komlenic’s picture

Thanks all - I just love when a critical issue that I discovered today, had a fix committed yesterday :)

pianomansam’s picture

Confirmed newest dev version with this patch fixes the issue. When get we get this into a stable (or beta at least) release?

RobLoach’s picture

As you wish ;-) ... http://drupal.org/node/1416662

Status: Fixed » Closed (fixed)

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

pianomansam’s picture

Seems to be working well. Thanks!

flightrisk’s picture

My symptom was also the bundle error. Took several hours to trace it to this module. I only got the error when I had created a "custom" permission set for a field, in my case a Taxonomy field. When I tried to go to the content type and edit that field I got the error. So fixed now for me as well with the Jan 25 release