Problems

entity_access() is not consistent with node_access()
node_access('create', $node_type, $account);

works correctly and respects the create $node_type content permission. One could therefore assume that

entity_access('create', 'node', $node_type, $account);

works, as well. However, the following notice ist thrown:

Trying to get property of non-object in entity_metadata_no_hook_node_access() (line 619 of modules/callbacks.inc)
entity_access() has an incomplete API
The fact that such a notice is thrown is inline with the documentation of entity_access(), which states for the (third) $entity argument:
Optionally an entity to check access for. If no entity is given, it will be determined whether access is allowed for all entities of the given type.

When requesting access for creating a node, there is obviously no $node object to pass to entity_access(). So, following the documentation, nothing should be passed at all. But we want to pass the node type (or in Entity API terms: bundle) information in order for entity_access() to respect the node type specific access permissions.

entity_access('create', 'node', NULL, $account);

is senseless when considering any form of node type-specific access control.

entity_access() does not work with un-saved (stub) entities
Trying to gain node type-specific access control while respecting the documentation for entity_access(), one could try the following:
$node = entity_create('node', array('type' => $node_type));
entity_access('create', 'node', $node, $account);

This, however, throws the following notice:

Undefined property: stdClass::$nid in entity_metadata_no_hook_node_access() (line 619 of modules/callbacks.inc).

In a very strange way this is consistent with node_access(). Calling

$node = entity_create('node', array('type' => $node_type));
node_access('create', $node, $account);

also throws a notice.

Putting everything together, there is no way to call entity_access() while respecting bundle-specific (read: node type-specific) create permissions!

Solution

Several approaches were discussed in this issue. The agreed solution is that the following pattern is the correct way to check for 'create' access:

For create operations, the pattern is to create an entity and then
check if the user has create access.

@code
$node = entity_create('node', array('type' => 'page'));
$access = entity_access('create', 'node', $node, $account);
@endcode

It remains an error to pass a non-entity argument to entity_access().

Debugging

This issue has been resolved in Entity API, but other modules may still trigger the exception by calling entity_access() incorrectly, so diagnostic logging is available in an additional patch to help with tracking down the sources of any such invalid calls.

If you are reading this issue because you are experiencing this error, and you have applied the patch in #308, then check your watchdog logs.

See comment #249 for details, and #275 for a real example use-case.

Original report by Adam S

/**
 * Access callback for the node entity.
 *
 * This function does not implement hook_node_access(), thus it may not be
 * called entity_metadata_node_access().
 */
function entity_metadata_no_hook_node_access($op, $node = NULL, $account = NULL) {
  if (isset($node)) {
    // If a non-default revision is given, incorporate revision access.
    $default_revision = node_load($node->nid);
    if ($node->vid != $default_revision->vid) {
      return _node_revision_access($node, $op);
    }
    else {
      return node_access($op, $node, $account);
    }
  }
  // Is access to all nodes allowed?
  if (!user_access('access content', $account)) {
    return FALSE;
  }
  if (user_access('bypass node access', $account) || (!isset($account) && $op == 'view' && node_access_view_all_nodes())) {
    return TRUE;
  }
  return FALSE;
}

When calling node_access the second parameter can be either a node type string or the node object. If the node access is being check on node create then the $node variable can be a string.

In the function above, if the $op is create then there can't be a revision and there isn't a need to be checking for a revision. Perhaps, there should be a condition that the $op isn't create.

CommentFileSizeAuthor
#318 entity-callback.patch597 bytesifernando
#308 entity-node_access-1780646-308.diagnostic.patch2.23 KBjweowu
#273 entity-node_access-1780646-191-273.interdiff.do-not-test.patch1.95 KBjweowu
#273 entity-node_access-1780646-273.diagnostic.patch13.81 KBjweowu
#256 entity-node_access-1780646-191-256.interdiff.do-not-test.patch1.59 KBjweowu
#256 entity-node_access-1780646-256.diagnostic.patch13.45 KBjweowu
#249 entity-node_access-1780646-191-249.interdiff.do-not-test.patch963 bytesjweowu
#249 entity-node_access-1780646-249.diagnostic.patch12.9 KBjweowu
#248 4.jpg39.69 KBmsh2050
#248 5.jpg87.35 KBmsh2050
#241 3.jpg95.47 KBmsh2050
#237 1.jpg73.68 KBmsh2050
#237 2.png13.96 KBmsh2050
#202 1780646-202-entity-node-access.patch13.95 KBwodenx
#202 1780646-202-entity-node-access.interdiff.txt2.03 KBwodenx
#210 Screen Shot 2013-08-20 at 3.29.45 PM.png27.96 KBacrosman
#191 entity-entity_access-1780646-191.patch12.41 KBMile23
#189 entity-entity_access-1780646-189.patch12.42 KBMile23
#179 entity_node_access-1780646-179.patch587 byteslauriii
#171 entity-entity_access-1780646-171.patch14.75 KBdropfen
#168 entity-entity_access-1780646-168.patch14.41 KBdropfen
#166 entity-entity_access-1780646-166.patch14.74 KBsergiu.savva
#148 entity-entity_access-1780646-148.patch13.99 KBMile23
#139 entity_node_access-1780646.patch603 bytesa.milkovsky
#136 entity-entity_access-1780646-136.patch12.4 KBJvE
#107 entity-entity_access-1780646-107.patch13.99 KBMile23
#101 entity-entity_node_access-1780646-100.patch5.47 KBhenk
#99 entity-entity_node_access-1780646-99.patch5.49 KBMile23
#97 entity-entity_node_access-1780646-97.patch7.24 KBsergiu.savva
#90 entity-create_test-1780646-90.patch4.4 KBMile23
#88 entity-create_test-1780646-88.patch3.87 KBMile23
#84 entity-create_test-1780646-84.patch1.47 KBMile23
#77 1780646_shame.patch636 bytesMile23
#75 1780646_me3.patch768 bytesMile23
#72 1780646_me2.patch1.16 KBMile23
#71 1780646_me.patch1.16 KBMile23
#70 1780646.patch3.14 KBMile23
#57 entity-node_access-1780646-57.patch3.7 KBsergiu.savva
#39 entity-node-access-callback-1780646-39.patch1.66 KBsergiu.savva
#30 entity-node-access-callback-1780646-30.patch1.68 KBsergiu.savva
#26 1780646-26-entity_node_access.patch1.45 KBItangalo
#25 1780646-25-entity_node_access.patch675 bytesItangalo
#16 entity-node-access-1780646-16.patch925 bytessergiu.savva
#8 entity-node-access-1780646-8.patch1.05 KBweri
#1 1780646-entity-api-node-access.patch954 bytesAdam S
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Adam S’s picture

I've included a patch.

Adam S’s picture

Priority: Normal » Major
Status: Active » Needs review

Can I please have this patch review? I might be the only one with this problem but it's still a bug.

Silicon.Valet’s picture

Status: Needs review » Reviewed & tested by the community

I don't see any obvious problems and this clears a notice for me.

Jorrit’s picture

Works fine, thanks!

MXT’s picture

I can confirm that the patch resolve the issue.

Can this be committed please?

Thank you very much!

wiifm’s picture

+1, was having the same issue as described in #1780626: Entity Reference incorrectly calls entity_access() - this patch fixed it.

Jorrit’s picture

The patch is not correct, because when isset($node), $op != 'create' and $node->vid == $default_revision->vid, node_access($op, $node, $account); is not invoked.

I have rewritten the function to:

  if (isset($node)) {
    // If a non-default revision is given, incorporate revision access.
    if ($op != 'create') {
      $default_revision = node_load($node->nid);
      if ($node->vid != $default_revision->vid) {
        return _node_revision_access($node, $op);
      }
    }
    return node_access($op, $node, $account);
  }
  // Is access to all nodes allowed?
  if (!user_access('access content', $account)) {
    return FALSE;
  }
  if (user_access('bypass node access', $account) || (!isset($account) && $op == 'view' && node_access_view_all_nodes())) {
    return TRUE;
  }
  return FALSE;
weri’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.05 KB

Comment #7 is correct. I generated a new patch for that.

rwilson0429’s picture

When I try to use the patch in #8 on Windows7 using cygwin, I get the error below. I am doing something wrong or can this patch only be applied to unix or lynux server?

$ patch -p1 --dry-run < entity-node-access-1780646-8.patch
(Stripping trailing CRs from patch.)
patching file modules/callbacks.inc
patch unexpectedly ends in middle of line
Hunk #1 succeeded at 607 with fuzz 1.

rogical’s picture

Status: Needs review » Reviewed & tested by the community

patch at #8 is fine.

git apply -v entity-node-access-1780646-8.patch
Checking patch modules/callbacks.inc...
Hunk #1 succeeded at 612 (offset 5 lines).
Applied patch modules/callbacks.inc cleanly.

MXT’s picture

The issue is still present in the latest release (7.x-1.0 - 25 Dec) but patch in #8 still resolves it.

Can this be definitively committed please?

Thank you very much

head’s picture

bomarmonk’s picture

This fixed my issue, but the patch is not committed to head yet (the development version is dated December 26th).

Richard Moger’s picture

#8 worked for me. thx.

sergiu.savva’s picture

when Drupal call the next code?

line 715:

$default_revision = node_load($node->nid);
if ($node->vid != $default_revision->vid) {
  return _node_revision_access($node, $op);
}
sergiu.savva’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
925 bytes

#7 and #8 worked for me.

I propose an alternative, to check if node exist.

  if (isset($node)) {
    // If node exist.
    if (isset($node->nid)) {
      // If a non-default revision is given, incorporate revision access.
      $default_revision = node_load($node->nid);
      if ($node->vid != $default_revision->vid) {
        return _node_revision_access($node, $op);
      }
    }
    else {
      return node_access($op, $node, $account);
    }
  }
sandu.camerzan’s picture

bkmatwa’s picture

Thank you 'sergiu.savva' your proposed alternative works perfectly.

Itangalo’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #16 works like a charm. Thanks!

wrodenbusch’s picture

sergiu.savva's patch works for me. Thanks!

ptitwolfy’s picture

Not sure this is related to Entity API or References Dialog, but Patch in #16 unfortunately remove the Edit function of an entity reference field. Working with patch in #8

stevieb’s picture

patch in #8 works here

timbrandin’s picture

#16 works!

eiriksm’s picture

#8 works for me

Itangalo’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
675 bytes

I found that the patch actually screws up the standard node access check – it misses an important 'else' case.

The attached patch keeps the relevant structure for the if statements, thus allowing node_access to be run properly.
As a bonus, the patch is even smaller.

(This bug doesn't show if you use an account that bypasses node access control, so I missed it at first.)

Itangalo’s picture

Ok, so there is another case to take care of. When excluding the non-object $node parameter, the 'create' operation isn't taken care of properly.

This patch solves that issue. It could probably be written in a prettier way.

gmclelland’s picture

Patch in #26 solves the problem for me.

jpstrikesback’s picture

Testing #26 nothing has blown up yet :)

magicmirror’s picture

#26 worked for me so far as well, thanks

sergiu.savva’s picture

Hi Itangalo,

node_access() function already includes validation for $op variable

file: modules/node/node.module
line: 2882

if (!$node || !in_array($op, array('view', 'update', 'delete', 'create'), TRUE)) {
    // If there was no node to check against, or the $op was not one of the
    // supported ones, we return access denied.
    return FALSE;
  }

We don't need to check $node variable type, because this function is here
file: modules/node/node.module
line: 2895

  // $node may be either an object or a node type. Since node types cannot be
  // an integer, use either nid or type as the static cache id.

  $cid = is_object($node) ? $node->nid : $node;
  // Is access to all nodes allowed?
  if (!user_access('access content', $account)) {
    return FALSE;
  }
  if (user_access('bypass node access', $account) || (!isset($account) && $op == 'view' && node_access_view_all_nodes())) {
    return TRUE;
  }

All these access checks are in the function node_access().
Why not use directly node_access() ?

Status: Needs review » Needs work

The last submitted patch, entity-node-access-callback-1780646-30.patch, failed testing.

sergiu.savva’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, entity-node-access-callback-1780646-30.patch, failed testing.

Itangalo’s picture

@sergiu.savva: I don't have the details in my head, but I have a strong recollection of the patch failing to grant access when I used it together with Field collection.

When the $op == 'create', the old patch gave access FALSE. I'll have to check it again.

Rajab Natshah’s picture

Hi,
Is this going to be in the next release.
Are there any effect after having this patch?

Thanks. This notice only comes when we have an Inline Entity Form.

I do not if fago Agree to get this or ditching it.

knorbi’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, entity-node-access-callback-1780646-30.patch, failed testing.

betawavetom’s picture

#26 worked like a charm. Thank you!

sergiu.savva’s picture

I'm confused.
In file entity.test line 927 when node is NULL simpletest check view permission for user and return true.

Why user must have access for view node, that doesn't exist?

    $account = $this->drupalCreateUser(array('bypass node access'));
    $this->assertTrue(entity_access('view', 'node', NULL, $account), 'Access without data checked.');

For Inline Entity Form it work ok.

Please test new patch.

sergiu.savva’s picture

Status: Needs work » Needs review
JvE’s picture

philipz’s picture

#39 applied cleanly and worked for me. Thank you!

drupov’s picture

Yeah, patch from #39 worked for me too!

Itangalo’s picture

@sergiu.savva: I have not applied patch in #39, but from reading the code it seems that it does not take care of the case when $node is a string.

The line if (isset($node->nid)) { will generate a PHP warning if $node is a string. (Right? Or does the isset() function take care of this?)

Not a major thing, but looking at the title of this issue I think it should be taken care of.

sergiu.savva’s picture

Hi Itangalo,

Error is excluded.

try this one:

$node = 'node';
if (isset($node->nid)) {
echo 'node exist';
} else {
echo 'node is new'; 
}

result well be :
node is new

explanation of code:

// if node is set (not NULL)
if (isset($node)) {

    // Check if node exist 
    if (isset($node->nid)) {
...
    }

// Otherwise call function node_access()
// variable $node can be object or string
    return node_access($op, $node, $account);
}
liza’s picture

confirming same situation: #8 is the only patch that worked for me.

Itangalo’s picture

@sergiu.savva: You're absolutely right. Thanks for making me see this.

sergiu.savva++

willhowlett’s picture

#8 worked for me

kaizerking’s picture

ok thanks i have applied the patch,
but when i want to use inline entity form for field collection.I am getting this error.
Recoverable fatal error: Argument 2 passed to field_collection_item_access() must be an instance of FieldCollectionItemEntity, string given, called in H:\wamp\www\drop_job_dev\profiles\drop_jobs\modules\contrib\entity\entity.module on line 553 and defined in field_collection_item_access() (line 765 of H:\wamp\www\drop_job_dev\profiles\drop_jobs\modules\contrib\field_collection\field_collection.module).

cedewey’s picture

#39 worked for me.

sergiu.savva’s picture

Hi kaizerking,

Can you to describe workflow ?
You have field collection field and inline entity form, how they are interfere?

kaizerking’s picture

Create a node type organization
create field collection field "location"-set it to hidden
create another field entity reference and widget> inline entity form refer-> to field collection field "location"cardinality unlimited. now try creating node organization

jyee’s picture

#39 works for me as well.

Assuming that #49 is an issue with Field Collection and not Entity, this should be marked as RTBC.

kaizerking’s picture

Yes probably it should be marked RTBC

froboy’s picture

Status: Needs review » Reviewed & tested by the community

#39 worked for me too. As it seems this has been tested now, I'm going to mark it as so.

fonant’s picture

Patch #39 worked here.

sergiu.savva’s picture

one more patch )

review test cases for entity node access, and result is a new patch

Yaazkal’s picture

#57 works great.

afmdsouza’s picture

Patch #57 works for me too.

spencerthayer’s picture

patch -p1 < entity-node_access-1780646-57.patch
can't find file to patch at input line 16
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|From dd5a5ae626919ed3c1fdd0b1f1d5f982557b120c Mon Sep 17 00:00:00 2001
|From: Sergiu Savva <sergiusavva@gmail.com>
|Date: Sun, 3 Mar 2013 21:17:07 +0200
|Subject: [PATCH] Issue #1924974 by sergiu.savva: Changed test case for
| entity_metadata_no_hook_node_access().
|
|---
| entity.test           |    9 +++++----
| modules/callbacks.inc |   26 +++++++++++---------------
| 2 files changed, 16 insertions(+), 19 deletions(-)
|
|diff --git a/entity.test b/entity.test
|index 746713e..8ecd709 100644
|--- a/entity.test
|+++ b/entity.test
--------------------------

What am I doing wrong? Never had this happen with a patch before.

fago’s picture

Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

What exactly is the problem here, is $op == 'create' not working correctly? Calling the function which parameters does not work right in which way? Please be sure to provide a good problem description as part of the issue summary.

Howsoever, it does not look right that the the patch requires several test assertions to change.

Anselm’s picture

#57 Solved the problem!

thanks!

Anselm’s picture

Dave Reid’s picture

Status: Postponed (maintainer needs more info) » Needs review

It's pretty clear from the original description, that when you are looking for access to create a new node, that the call to entity_access('create', 'my-node-type') fails because entity_metadata_no_hook_node_access() only checks if $node is set, but not if it's an object. I'm getting this error constantly on my site using the inline_entity_form module.

videographics’s picture

I can confirm #64. I'm constantly seeing the same thing on the forms that use inline_entity_form module.

ladybug_3777’s picture

The very simple patch (comment #1) on this page: http://drupal.org/node/1879714 is working for me. But looking through the comments on this page makes me wonder if the simple answer is incorrect. Can someone tell me what sort of errors I may have if I apply the other patch?

jyee’s picture

You likely wouldn't receive errors per se. That is, the patch from #1879714: Trying to get property of non-object in entity_metadata_no_hook_node_access() is designed to resolve php error messages and it does address that issue. What it fails to address is when the $node arg is a string, not an object. So the "error" you would experience would likely be incorrect access or denial of access to a node (or node revision).

jyee’s picture

Status: Needs review » Reviewed & tested by the community

The patch from #39 was rtbc and #57 added tests. The tests look good to me, so i'm going to move back to rtbc.

@fago, are you concerned that the test are incorrect or is there a problem with the overall solution? Does @Dave Reid's answer in #64 provide the information you need?

Rajab Natshah’s picture

Thanks sergiu.savva your patch at #57 works well.

Mile23’s picture

FileSize
3.14 KB

Redo of #57 with no whitespace or formatting errors.

Full authorship credit to sergiu.savva please. :-)

Mile23’s picture

FileSize
1.16 KB

Turns out the only valid reason for $node to be a string is if $op is 'create'.

I was seeing a lot of this error: Notice: Trying to get property of non-object in entity_metadata_no_hook_node_access() (line 619 of [path]/sites/all/modules/contrib/entity/modules/callbacks.inc).

This was while trying to use Inline Entity Form to add nodes from a field. sergiu.savva's patch didn't help in that use case, so here's my patch that seems to help.

Mile23’s picture

FileSize
1.16 KB

Hah.

Wrongy-wrong patch.

Let's try again.

Mile23’s picture

Status: Reviewed & tested by the community » Needs review

Status: Needs review » Needs work

The last submitted patch, 1780646_me2.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
768 bytes

3rd time a charm?

::hangs head::

Status: Needs review » Needs work

The last submitted patch, 1780646_me3.patch, failed testing.

Mile23’s picture

Mile23’s picture

Status: Needs work » Needs review
burgs’s picture

Status: Needs review » Reviewed & tested by the community

#77 Seems to have fixed the problem. I haven't delved too much into the code to see whether it makes perfect sense, but it does seem like a lot simpler a solution to the above patches.

sergiu.savva’s picture

HI Mile23

Patch entity-node_access-1780646-57.patch work fine for me with Inline Entity Form.

1) Patch name is incorrect, more info you can fiend here http://drupal.org/node/1054616 (Patch naming conventions)
2) Look the first patch 1780646-entity-api-node-access.patch is same code as yours.

operations’s picture

I wonder why there are many patches above! it is a bit confusing. However, patch #77 (mile23) solved my problem which is: Notice warnings appearing when using Inline Entity Form module.

Notice: Trying to get property of non-object in entity_metadata_no_hook_node_access() (line ...... /entity/modules/callbacks.inc).

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs review

I actually don't think #77 is the right approach. We really should let node_access() handle most of the default cases, and only if we're working with the non-default revision, do the custom logic.

Dave Reid’s picture

Also we're missing tests to confirm this is fixed and it won't regress in the future.

Mile23’s picture

OK, here's a patch to the tests that will demonstrate the problem (and thus will fail auto-testing).

The test also addresses #1237014: Notices in entity_metadata_user_access() when loading an anonymous node, which is the use-case I'm actually facing.

Status: Needs review » Needs work

The last submitted patch, entity-create_test-1780646-84.patch, failed testing.

kjbowrin’s picture

Priority: Major » Critical

Why wasn't this in 1.1? This needs a fix.

Mile23’s picture

Actually, looking back on it there's a typo in the user entity data (should be 'bundle'=>'user' instead of 'user'=>'user'), but it still illustrates the problem with node.

Mile23’s picture

Title: entity_metadata_no_hook_node_access() should respect that the $node parameter can be a string or an object » entity_metadata_no_hook_node_access() should respect that the $node parameter isn't always an object
Status: Needs work » Needs review
FileSize
3.87 KB

So, actually.

I've been completely wrong, and sorry to waste everyone's time.

The $node parameter is *never* a string, because entity_metadata_no_hook_node_access() is a callback for entity_access().

entity_access() says that the $entity (that eventually becomes our $node) is either an entity object or 'nothing' (presumably they mean NULL), and if it's NULL then check access for all bundles of this entity type. Which kinda makes no sense to my feeble mind, but there you go.

Basically unless you provide a prototype node object to entity_metadata_no_hook_node_access(), it can never determine user access for the 'create' op.

My fix punts on this scenario by returning FALSE.

Dave Reid’s picture

I don't see how the patch in #88 would work with a new, mocked node object considering that it requires that $node->nid is set. The maintainers of inline_entity_form say the bug lives here: #1904186: Incorrect use of entity_access(). Personally I'd like to get fago to weigh in here after more concerns have been raised. It feels wrong that this access check doesn't support all the same things that node_access() does.

Mile23’s picture

You're right. It shouldn't check for $node->nid, if $op is 'create'. Mock me. :-) New patch with this change.

"It feels wrong that this access check doesn't support all the same things that node_access() does."

It's just that this API is designed as if bundles don't exist. node_access() of course has that object-or-string property to get around the same problem.

jojonaloha’s picture

I've read through the comments and I agree with #88 in that the currently documented use of entity_access() is that $node is either an object or NULL. That would mean that Dave Reid is right that the issue actually lies with modules like inline_entity_form calling entity_access() with incorrect parameters (as entity_access() is not documented to accept strings for the $entity parameter).

However, I think that due to entity_access() and node_access() being very similar in the parameters they accept, it makes sense for entity_metadata_no_hook_node_access() to allow for the $node parameter to be a node type before it is passed to node_access(). That is why I think #7/8 is the simplest solution, and is tested and working for me.

As for the patches in #88/90, is it actually the case that modules that call entity_access() will create mock-objects when $op == 'create'? It doesn't seem to follow the pattern that modules I've seen have been using (see below), but if it is expected, then I think the patch in #7/8 should be adjusted so either a node type as a string or a mock node object can be passed.

This snippet is from references_dialog:

entity_access('create', $entity_type, $entity_type == 'node' ? $bundle : NULL)
JvE’s picture

I cannot find the API definition of the 'access callback' in the entity info array anywhere. Lots of examples and remarks to use it, but no definition.
This makes it hard to blame any implementation of it by other modules like inline_entity_form for bugs.

Can we change the entity_access() $entity definition from:

$entity: Optionally an entity to check access for. If no entity is given, it will be determined whether access is allowed for all entities of the given type.

to:

$entity: Either an entity object to check access for, the name of the bundle to check access for or NULL to determine whether access is allowed for all entities of the given type.

?

This makes it more in line with drupal's node_access and allows developers more finegrained access control. (even though I dislike multi-type arguments)

It does mean fixing entity_metadata_no_hook_node_access() and any other module implementing an entity access callback without considering "create".

Thoughts?

Mile23’s picture

I'm not arguing for one approach or another here. It's just a design decision that needs to be made. The tests I wrote will still apply (and could no doubt be improved), either way.

entity_access() is called from at least 25 places, and here they are: http://drupalcontrib.org/api/drupal/contributions%21entity%21entity.modu...

At least three of them deal with nodes, in particular rules_node_integration_access(): http://drupalcontrib.org/api/drupal/contributions%21rules%21modules%21no...

It says: return entity_access('view', 'node'); which is kind of meaningless given the granularity of node bundle permissions.

So there's some work to do regardless of which way this goes.

tstoeckler’s picture

Issue summary: View changes

Created a real issue summary

tstoeckler’s picture

Title: entity_metadata_no_hook_node_access() should respect that the $node parameter isn't always an object » entity_access() does not allow for bundle-specific access control for the 'create' operation

I just ran into this issue and found this issue very confusing, due to the amount of patches and the different things being discussed here. I don't think there is anyone to blame here, but I think there are multiple issues that are related, but distinct. It probably makes sense to talk about all of them here, but in order to achieve a certain point of clarity I wrote a proper issue summary and identified 3 problems with entity_access(), that have been discussed here in some fashion. I am now re-titling the issue for the broadened scope.

Please consider that I just stepped into this issue, so please do improve the issue summary if you can and think it needs improvement. And if you find my title change inappropriate feel free to revert.

I hope, though, that with the issue summary @fago will be able to make sense of this issue and give some feedback on a wanted approach.

Mile23’s picture

Thanks, tstoeckler.

sergiu.savva’s picture

sergiu.savva’s picture

We should let node_access() handle most of the default cases and first check revision access then check node access.

fago’s picture

@tstoeckler: Thanks for the updated summary!

entity_access('create', 'my-node-type')

That's clearly unsupported usage - check the entity_access() docs. The current way of doing that kind of check is to create a new object and run the check, e.g.

$node = entity_create('node', array('type' => 'page'));
$access = entity_access('create', 'node', $node);

This is what we do in d8 as well right now.

There is no point in changing the callback to support something entity_access() does not support. However, it looks like entity_metadata_no_hook_node_access() assumes $node->nid is set, what looks like a bug that needs to be fixed.

entity_access() is not consistent with node_access()

Yes. That can be discussed, but surely is not a critical bug.

entity_access() has an incomplete API

No, see above example.

entity_access() does not work with un-saved (stub) entities

Entity API (module) does not support a concept of stub-entities, but yes - it has to work with un-saved entities (see example above). If not, that's a bug.

Mile23’s picture

Thanks for the clarification, @fago.

Try this on for size.

jamix’s picture

#99 worked over here, thanks.

henk’s picture

I have the same problem when adding new node (in multiple widget) with Inline Entity Form, after change:

if ('create' == $op) {
  if (isset($node->type)) {         
         return node_access($op, $node->type, $account);
         }
}

to

if ('create' == $op) {
  if (isset($node)) {         
         return node_access($op, $node, $account);
         }
}

patch start work.

Status: Needs review » Needs work

The last submitted patch, entity-entity_node_access-1780646-100.patch, failed testing.

wismoyo’s picture

Status: Needs work » Needs review

#8: entity-node-access-1780646-8.patch queued for re-testing.

wismoyo’s picture

Status: Needs review » Needs work

The last submitted patch, entity-entity_node_access-1780646-100.patch, failed testing.

sergiu.savva’s picture

Status: Needs work » Needs review

Hi Mile23,

I reviewed your patch (entity-entity_node_access-1780646-99.patch) I have some suggestions:

1) If the node is null I can't see it, there is no sense to check user access.

if ('view' == $op) {
    if (user_access('access content', $account))
if (isset($node)) {
     ......
  }
  if ('view' == $op) {
    if (user_access('access content', $account)) {
      return TRUE;
    }
    if (!isset($account) && node_access_view_all_nodes()) {
      return TRUE;
    }
  }
  if (user_access('bypass node access', $account)) {
    return TRUE;
  }
  return FALSE;

2) For what to use the user_access if this makes node_access ?
path: modules/node/node.module
line: 3002

node.module

$cid = is_object($node) ? $node->nid : $node;

  // If we've already checked access for this node, user and op, return from
  // cache.
  if (isset($rights[$account->uid][$cid][$op])) {
    return $rights[$account->uid][$cid][$op];
  }

  if (user_access('bypass node access', $account)) {
    $rights[$account->uid][$cid][$op] = TRUE;
    return TRUE;
  }
  if (!user_access('access content', $account)) {
    $rights[$account->uid][$cid][$op] = FALSE;
    return FALSE;
  }

Can you tell me what problems you have with this patch http://drupal.org/comment/reply/1780646#comment-7392860 ?
If you have some suggestions, please tell me.

Mile23’s picture

serigu: Thanks for the comments. Yah, I went crazy with user_access(). Bit of a mistake.

I wrote some tests, adapted from NodeAccessTestCase and NodeRevisionPermissionsTestCase (node.module's access tests). TDD makes us happy.

Node_access() requires that $node objects have a nid. So access is denied if the object doesn't have one, except in the case of 'create' op.

Thus: In order to check whether a user has create access for a node type, you have to generate an entity and then check if the user can create it, like this:

$node = entity_create('node', array('type' => 'page'));
$access = entity_access('create', 'node', $node);

In all other cases, the node has to have a nid, or access is denied.

The original code asks node_access_view_all_nodes() for an opinion. This doesn't make much sense, since node_access_view_all_nodes() is part of a DBTNG tag-based access system that node doesn't concern itself with, either in node_access() or in tests. So I took node_access_view_all_nodes() out, not knowing what special cases it addresses; I couldn't find any reference to it in the issue queue.

I ran a bunch of other contrib tests to see if this patch mucks with anything that isn't broken already, and the results were positive. Other than the fact that, for instance, entityreference doesn't have any tests to run.

operations’s picture

Hi Mile23,

I have applied the patch #77 as mentioned in my earlier comment and it worked for my case. However, what does the latest patch resolves? should I apply it?

Many thanks in advance.

Mile23’s picture

re: #108:

1) Clarifies the API in docblocks.

2) Fixes entity_metadata_no_hook_node_access() to not throw errors if you try to call it as if it were node_access(), denying access in those cases.

3) Provides a big pile of tests to make sure this stuff all works. The tests are adapted from the node_access() and _node_revision_access() tests, so they test the same behavior.

Offlein’s picture

I just patched with #107 and, although my warnings were removed, the Inline Entity Form module is not working properly now. I am no longer able to "Add New Node" within a node that has an Inline Entity Form field. The button simply isn't there. No PHP errors that I saw. The "Add existing node" is present.

I apologize if I missed a step that would've fixed this issue. The thread's very long.

sergiu.savva’s picture

Hi Offlein,

Try patch #97, it must work.

Mile23’s picture

Sinan Erdem’s picture

Patch #97 works for me.

bojanz’s picture

I've committed #1990064: Invoke entity_access() properly for create operations to IEF, now Entity API just needs to fix its node callback.

Offlein’s picture

Mile23, you're right -- that was exactly what was necessary. Thank you!

C-Logemann’s picture

Status: Needs review » Reviewed & tested by the community

Applied patch of #107 together with actual release of IEF and my errors are gone. I think everything is fine.

philipz’s picture

I did the same as @C_Logemann and errors are gone too.

mbarcelo’s picture

I did the same as @C_Logemann and @philipz errors are gone too.

Cauliflower’s picture

I also can confirm the errors are gone.

dropfen’s picture

Status: Reviewed & tested by the community » Needs work

Sorry, but the Issue is not solved.
After I applied the patch #107 , the reference dialog widget had no "Add - Button" more.

Offlein’s picture

@dropfen did you see #110 and #112?

dropfen’s picture

I saw them, but I'm a little confused which patch is the one which will be commited, so i hoped you guys can tell me that.
Are these patches just momentan hackish solutions, and what will be going to the next release.

For first I'll try the #97, thanx

#97 works, thank you

Mile23’s picture

Status: Needs work » Reviewed & tested by the community

OK, sorry about all the patches along the way, but #107 is the one I'd like to see committed, since it's the one that implements the API as described by fago in #98. Also, bojanz committed #1990064: Invoke entity_access() properly for create operations to IEF in hopes that this issue would be resolved.

So for Inline Entity Form you'd either use the dev version or apply the patch: http://drupal.org/node/1990064#comment-7417524

For bugs in other modules, mention a ticket so we can work on them.

As far as being hackish, the patch at #107 is 14k, which is big for a patch. The reason it's big is because it's mostly regression tests that I wrote or adapted from node's tests to make sure the permissions work the same way. Its the opposite of hackish, if I do say so myself. :-)

JvE’s picture

@Mile23: For Inline Entity Form there is no need for dev or patch, release 7.x-1.2 contains the proper API usage.

dropfen’s picture

@Mile23, thanks for the reaction ;)
And of course thanks for patches, to all contributers.

dropfen

azinck’s picture

#107 works well for me. +1 RTBC

mohamadaliakbari’s picture

#107 works well for me, too.

dww’s picture

#107 solved my problems, too. I don't have time energy for a thorough review (or running) of the automated tests included with the patch, but hurray for supplying tests with the bug fix!

MXT’s picture

I can confirm that #107 resolves my issues.

Thank you

JvE’s picture

-    $this->assertTrue($wrapper->access('edit', $account), 'Access to node granted.');
+    $this->assertFalse($wrapper->access('edit', $account), 'Access to node granted.');
-    $this->assertTrue($wrapper->status->access('edit', $account2), 'Access for admin property allowed for the admin.');
+    $this->assertFalse($wrapper->status->access('edit', $account2), 'Access for admin property allowed for the admin.');

I do not understand why the conditions are inverted but the descriptions are not.

+  if (!is_object($node)) {
+  if (isset($node->type)) {
+  if (!isset($node->nid)) {

Isn't all this checking only hiding bugs from other modules?

There is no way to tell now if acces is being denied because it should or because some module is not adhering to the API.

If you state

 * @param $node
 *   A node to check access for. Must be a node object. Must have nid,
 *   except in the case of 'create' operations.

then all calling code is responsible for adhering to those demands and no checking should be done.


So functionally the code becomes:

>
function entity_metadata_no_hook_node_access($op, $node = NULL, $account = NULL) {
  if ($op == 'create') {
    return node_access($op, $node->type, $account);
  }
  // Handle the revision special case.
  $default_revision = node_load($node->nid);
  if ($node->vid !== $default_revision->vid) {
    return _node_revision_access($node, $op, $account);
  }
  return node_access($op, $node, $account);
}

which still passes all your tests.
And when another module passes in something wrong for $node then a notice or error will occur, hopefully leading to the responsible module being fixed.
Or am I missing something?

JvE’s picture

Status: Reviewed & tested by the community » Needs work
sergiu.savva’s picture

Status: Needs work » Needs review
Nikro’s picture

Status: Needs review » Reviewed & tested by the community

This worked for me.
Here #1990064: Invoke entity_access() properly for create operations they got rid of the inner IEF handling of the argument and thus they delegated it here.

This patch takes forever to be committed. Who ever is following this, c'mon guys, there are 11k+ IEF users, most of them probably already applied one of these patches. Recently IEF updated to 1.2 so they got rid of a partial handling of this, which generated notices again.

Mile23’s picture

Quoting JvE: "And when another module passes in something wrong for $node then a notice or error will occur, hopefully leading to the responsible module being fixed."

Prior to the clarification by fago in #98, it wasn't exactly clear how the API worked. People seem to think that Entity API (contrib) behaves like Node API, when it comes to asking permission to create a node. They pass in a string for node type, like you're supposed to do in Node API, and then something undefined happens.

If #107 is committed, developers will wonder why their users can't create nodes, and perhaps RTFM. :-)

The responsible way to do what you're talking about would be to throw an exception instead of denying permission, maybe InvalidArgumentException. This would count as a useful explosion. :-)

Make a patch if you want.

JvE’s picture

Status: Reviewed & tested by the community » Needs work

@Nikro: That patch may solve *your* problem or even the most common problems. But it also introduces some problems, which should probably be taken care of before anything is committed.

I do not like that the patch simply switches asserts around in entity.test to make the tests pass. Those tests are there for a reason, if they fail after a patch then the solution is not to simply change the tests.

entity_access('view', 'node', NULL, $account);
This should return TRUE for $accounts which may view any node.

 $wrapper = entity_metadata_wrapper('node', NULL, array('bundle' => 'page'));
  $wrapper->access('edit', $account);

This should return TRUE for $accounts which may edit any node.

After applying the patch from #107 these situations fail.

JvE’s picture

Priority: Critical » Major
Status: Needs work » Needs review
FileSize
12.4 KB

Attached patch contains a fix for the bug in Entity API as well the additional tests.

sergiu.savva’s picture

#136 works well for me, for inline entity form use dev version [7.x-1.2+2-dev]

vasike’s picture

Status: Needs review » Reviewed & tested by the community

indeed the #136 seems to work -> RTBC.

a.milkovsky’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
603 bytes

Here is my fix of

Notice: Trying to get property of non-object in entity_metadata_no_hook_node_access() (line 619 of [path]/sites/all/modules/contrib/entity/modules/callbacks.inc).

JeroenT’s picture

After applying patch #136 I get following errors:

Notice: Undefined index: module in FieldInfo->prepareInstanceWidget() (line 575 of /***/modules/field/field.info.class.inc).
Notice: Undefined index: module in FieldInfo->prepareInstanceDisplay() (line 610 of /***/modules/field/field.info.class.inc).
Notice: Undefined index: entityreference in field_ui_existing_field_options() (line 1545 of /***/modules/field_ui/field_ui.admin.inc).

JeroenT’s picture

Sorry I accidentally deleted the module entityreference. Patch #136 works for me!

a.milkovsky’s picture

Status: Needs review » Reviewed & tested by the community

so, RTBC

JvE’s picture

To avoid confusion: #136 is RTBC.
#139 does not solve anything, it just hides the error.

Mile23’s picture

Sure wish we could get some movement on this. I like #107, personally. :-)

JvE’s picture

Haha.
#107 breaks existing tests and removes functionality so that's a no-go.

Mile23’s picture

Pretty sure it does neither, JvE.

Which tests does it break?

JvE’s picture

From entity_access():

* @param $entity
* Optionally an entity to check access for. If no entity is given, it will be
* determined whether access is allowed for all entities of the given type.

Patch #57 (which #107 is based on) removes this functionality to call without $entity being set. It will always return FALSE.

Worse, it even deliberately modifies the existing tests for that case to hide the fact that it breaks that functionality:

--- a/entity.test
+++ b/entity.test
@@ -924,7 +924,7 @@ class EntityMetadataTestCase extends EntityWebTestCase {
   function testAccess() {
     // Test without data.
     $account = $this->drupalCreateUser(array('bypass node access'));
-    $this->assertTrue(entity_access('view', 'node', NULL, $account), 'Access without data checked.');
+    $this->assertFalse(entity_access('view', 'node', NULL, $account), 'Access without data checked.');
 
     // Test with actual data.
     $values[LANGUAGE_NONE][0] = array('value' => '<b>2009-09-05</b>');
@@ -935,9 +935,9 @@ class EntityMetadataTestCase extends EntityWebTestCase {
     // Test per property access without data.
     $account2 = $this->drupalCreateUser(array('bypass node access', 'administer nodes'));
     $wrapper = entity_metadata_wrapper('node', NULL, array('bundle' => 'page'));
-    $this->assertTrue($wrapper->access('edit', $account), 'Access to node granted.');
+    $this->assertFalse($wrapper->access('edit', $account), 'Access to node granted.');
     $this->assertFalse($wrapper->status->access('edit', $account), 'Access for admin property denied.');
-    $this->assertTrue($wrapper->status->access('edit', $account2), 'Access for admin property allowed for the admin.');
+    $this->assertFalse($wrapper->status->access('edit', $account2), 'Access for admin property allowed for the admin.');

     // Test per property access with data.
     $wrapper = entity_metadata_wrapper('node', $node, array('bundle' => 'page'));
Mile23’s picture

@JvE: Regarding your first point, the crux of the issue is that there is exactly no way to determine access for all nodes without a node type.

So the question was: What's the calling pattern for entity_access() for create operations?

Fago (the project maintainer) answered this in #98. Patch #107 changes the documentation you cite in order to reflect the calling pattern.

As to your second point, you're correct about the tests, in that the descriptions should have been updated. However, they do reflect a policy of denying access if there is no node entity available to glean a type from.

Here's a rework of #107 with better test descriptions.

Mile23’s picture

Status: Reviewed & tested by the community » Needs review

Changing status. Again. :-)

JvE’s picture

Status: Needs review » Reviewed & tested by the community

This is ridiculous.
What is wrong with the patch from #136?
It solves the issue and does not need to change the existing tests in order to pass them.

The problem with #107 is not the lack of proper descriptions. It is that it degrades the functionality.

There are specific tests to ensure the functionality of using NULL to check access to all nodes.

Take the original test case:

// Test that a user with 'bypass node access' is able to view all nodes.
$account = $this->drupalCreateUser(array('bypass node access'));
$this->assertTrue(entity_access('view', 'node', NULL, $account), 'Access without data checked.');

You propose to change this into:

// Test that a user with 'bypass node access' cannot view all nodes.
$account = $this->drupalCreateUser(array('bypass node access'));
$this->assertFalse(entity_access('view', 'node', NULL, $account), 'Access without data denied.');

And why on earth do you want to deny node edit access to users with both 'administer nodes' and 'bypass node access'?

I think you misinterpreted fago's comment about unsupported usage. That was about passing a node-type string, not about not passing a $node object.

Mile23’s picture

Status: Reviewed & tested by the community » Needs review

Resetting the status because clearly there is contention about this, and more than a few patches.

@JvC: "This is ridiculous."

Please remain civil.

@JvC: "And why on earth do you want to deny node edit access to users with both 'administer nodes' and 'bypass node access' [in that test]?"

Because, as I said before, there is no way to determine what kind of node it is, if the node is NULL. That's what that one line is testing. It's making sure that if the prototype node is NULL, no one gets create access, regardless of their permissions.

I don't like the way Entity API (contrib) works; if I were designing it, it would work differently. But Fago says in #98 that the way it SHOULD work is that we have to create a prototype node before we can determine if the user has create access to such a node. If no prototype node is passed in, there is no way to know whether a user has create access to that node type. Therefore, no access is allowed if we don't have a prototype node.

This is what #148 does.

In #136, you had it throw an exception if the prototype node is supplied incorrectly, which is a design decision that would be nice to get a call from Fago about. That is: Should it throw an exception or simply return FALSE?

However, in #136 this exception pattern is implemented by checking if $node is set, not if it's an object. This means that passing in a type string will not trigger an exception, which is exactly where you'd want to throw one.

@JvC: "I think you misinterpreted fago's comment about unsupported usage."

#98 seems pretty clear to me. Also, it would be great if Fago would weigh in on this.

JvE’s picture

Mile23: I don't like the way Entity API (contrib) works; if I were designing it, it would work differently.

Please open a new issue if you want to change the design of an API.
Let's fix the bug with $op == 'create' in this one.


Mile23: But Fago says in #98 that the way it SHOULD work is that we have to create a prototype node before we can determine if the user has create access to such a node. If no prototype node is passed in, there is no way to know whether a user has create access to that node type. Therefore, no access is allowed if we don't have a prototype node.

I agree for the special case of the "create" operation. I do not agree with your interpretation that this logic should be applied to all operations.


Mile23: It's making sure that if the prototype node is NULL, no one gets create access, regardless of their permissions.

The way you've rewritten the test it's making sure that a request for access to edit all nodes is denied to users with both 'administer nodes' and 'bypass node access' permissions.
And also that a request for access to view all nodes is denied to users with 'bypass node access' permissions.
Not only do I not agree with that, I also think it has no place in this issue.


Mile23: This means that passing in a type string will not trigger an exception, which is exactly where you'd want to throw one.

If $node is a string then $node is set and $node->type is not set and if $op is 'create' an exception will be thrown.

Mile23’s picture

Either we're required to make a prototype node or we're not. Fago seems to have said that we do, so that takes care of the are-strings-allowed? question.

So: For 'create' we need a prototype node.

For 'view' we need a node that's being viewed.

For 'update' we need a node that's being edited.

For 'delete' with need a node that's being deleted.

Therefore: In all cases we need a node entity in order to determine access.

I agree for the special case of the "create" operation. I do not agree with your interpretation that this logic should be applied to all operations.

See above. Also, check the tests I added; they're the permissions tests from the core node module, with a bit of refactoring for the different calling patterns. Your argument is with them.

And also that a request for access to view all nodes is denied to users with 'bypass node access' permissions.

....if the node is nonexistant. You can't view NULL. As we discussed before, it might make more sense to throw an exception in this circumstance, but that's for someone like Fago to say.

You see, the *whole point* of entity_metadata_no_hook_node_access() is to take the calling pattern of Entity API and shim it up against the calling pattern of node_access(), doing as little access logic as possible. 'bypass node access' is a permission defined and used by the node module. Therefore, it shouldn't be the responsibility of entity_metadata_no_hook_node_access() to come up with 'bypass node access' logic. node_access() will do all that for us, *if* the user called our function correctly.

JvE’s picture

Either we're required to make a prototype node or we're not.

We're not. We can pass in nothing/NULL to get access checked for all nodes.

Only if you want to limit a create check to nodes of a certain type are you required to use the prototype construct.

Using Entity API to check if a user can view all nodes?
entity_access('view', 'node')
Using Entity API to check if a user can edit all nodes?
entity_access('update', 'node')
Using Entity API to check if a user can delete all nodes?
entity_access('delete', 'node')
Using Entity API to check if a user can create nodes of any type?
entity_access('create', 'node')

Only if you need to check access to a specific instance or type of node do you need to pass in a node or prototype node (with type).

The bug to fix in this issue is that the function assumes that a node always has a nid and so errors out on the create operation.

Other bugs to fix might be that the above examples do not always accurately determine the correct access. This does not mean that access should simply always be denied as you suggest.
To remove the ability to check access for all entities of type "node" from Entity API I suggest you open a new issue.

malberts’s picture

So far the patch in #136 works for me.

Offlein’s picture

I'm really enjoying seeing two smart people hash it out raw-dog style in the Drupal issue queue, but I'm also wondering if either of you have PM'd Fago to get his input since it seems like there's ambiguity over his methodology here. One of you guys should message him if you haven't already!

Mile23’s picture

We can pass in nothing/NULL to get access checked for all nodes.

Again: We're shimming up to node_access(), which doesn't allow this. There is no way to determine node module's opinion on access to all nodes of all types.

But, as Offlein says, if you want to ping Fago about it, please go ahead.

Dave Reid’s picture

The documentation for entity_access() explicitly states:

Determines whether the given user has access to an entity.

Having this function used for 'can I view/edit/create/delete this type of entity' seems very wrong and abuse of something that it wasn't meant to do, which is why the changes in the tests are correcting that assumption.

JvE’s picture

The documentation for entity_access() explicitly states:

* @param $entity
* Optionally an entity to check access for. If no entity is given, it will be
* determined whether access is allowed for all entities of the given type.

Maybe this was meant to be or not meant to be, I have no opinion on that.

All I'm saying is that I disagree with changing/removing this functionality for nodes in a bugfix patch.

The bug is that entity_metadata_no_hook_node_access() does not take into account that for create operations the node's nid is not set.
The proposed patch fixed that and introduced a change changed the way Entity API works and is tested. I agreed on the bugfix but disagreed on extraneous API changes being part of the patch.
I even reworked the patch to fix the bug while leaving the API alone.
And then I get flak for it? Treated as if I do not understand the way things work? That I'm interpreting the documentation and intentions of the developers wrong? Sod that.
I'm not interested in having a discussion about whether Entity API should treat entities more like nodes or nodes more like entities. I'm interested in getting a clean fix for a bug committed to the module.

jpstrikesback’s picture

now now, no sodding flak, that is entirely unnecessary and possibly painful ;)

@fago, do you have an opinion?

fago’s picture

Title: entity_access() does not allow for bundle-specific access control for the 'create' operation » entity_access() fails to check node type specific create access

As pointed out in #98

entity_access('create', 'my-node-type')

is un-supported usage of entity_access() and entity_access() docs should be pretty clear on param $entity

 * @param $entity
 *   Optionally an entity to check access for. If no entity is given, it will be
 *   determined whether access is allowed for all entities of the given type.

So it's not a string. It's either an entity or NULL. If no entity is given, it will be determined whether access is allowed for all entities of the given type.

I'm not interested in having a discussion about whether Entity API should treat entities more like nodes or nodes more like entities. I'm interested in getting a clean fix for a bug committed to the module.

Agreed - I'm not interested in API changes at this point. API additions - maybe. So maybe, there are better options to model the entity create access, but that's another discussion. For that please open a new issue or chime in the respective d8 discussion over at #1979094: Separate create access operation entity access controllers to avoid costly EntityNG instantiation.

Thus, re-phrasing the issue title to make that more clear.

malberts’s picture

What would be the preferred patch or patch direction then? #136 or #148? Or something completely different?

Mile23’s picture

@fago: Thanks for the input.

However, it's still unclear just what the API actually is, in order to determine whether we're changing it or not.

The error this thread is trying to fix results from ambiguous documentation for entity_access(), and an incomplete mapping between the expectations of Entity API and Node API.

Therefore we need an API design call before a final patch can be made and hopefully committed, to help #1990064: Invoke entity_access() properly for create operations and others.

So here's the question: If you call entity_metadata_no_hook_node_access([any operation], NULL, $account), what should happen? Should the function A) Invent a new access behavior on behalf of Node API in order to estimate access for all node types? Or B) Return FALSE since there is no way to ask Node API about it?

If A) you want patch #136.

If B) you want patch #148.

Thanks.

JvE’s picture

@Mile23: as stated by fago and me; please open a new issue for API changes.

I wish you were more careful with the facts in order not to confuse people.
First a mistake about breaking exisitng tests, then a mistake about my patch not working and now a mistake about it inventing new access behaviour.
I really do understand that you want the behaviour of Entity API to change. This is just not the way to achieve that.

The problem people are having is that with the existing code it is not possible to check create access for node entities through the Entity API.
Patch #136 solves that without changing the behaviour. Is it perfect? Probably not, but it gets the job done without breaking or changing anything else.
Your string of patches (based on sergiu.savva's) fixes the problem as well, but it also changes existing behaviour and its accompanying tests, thus introducing new problems.

Mile23’s picture

Well then let's get on with the committing, or the whatever has to happen before the committing. Please. :-)

sergiu.savva’s picture

I've been changed a little bit JvE patch, entity-entity_access-1780646-136.patch.

We should let node_access() to handle most of the default cases and first check revision access then check node access.

Status: Needs review » Needs work

The last submitted patch, entity-entity_access-1780646-166.patch, failed testing.

dropfen’s picture

clean some coding standards and add return on line 338

dropfen’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, entity-entity_access-1780646-168.patch, failed testing.

dropfen’s picture

Status: Needs work » Needs review
FileSize
14.75 KB

new try

Status: Needs review » Needs work

The last submitted patch, entity-entity_access-1780646-171.patch, failed testing.

JvE’s picture

Status: Needs work » Reviewed & tested by the community

Patches #166, #168 and #171 do not solve the problem, they only break more.

sergiu.savva: We should let node_access() to handle most of the default cases and first check revision access then check node access.

Yes, patch #136 does that. But not for the "create" option, which should be handled differently because the Node module treats it differently. You patch does not take this into account.

#136 back to RTBC

dallas_tech’s picture

Applied #136 using patch in bash. After going to add a node (which has an entity reference to a node), I got the error below:

"EntityMetadataWrapperException: Permission to create a node was requested but no node type was given. in entity_metadata_no_hook_node_access() (line 635...drupal/profiles/commerce_kickstart/modules/contrib/entity/modules/callbacks.inc)"

I'm going to try #139. Honestly all I really need is to cover up the error, since everything (at least in appearance) was working just fine. If I can do that, I don't mind waiting for a fix in a scheduled update.

dallas_tech’s picture

#139 did not work for me. It did not generate errors, but it disabled the inline entity altogether from a non-administrator role (set with the proper permissions). I guess I'll have to shop for a patch on this page until I find one that works.

dallas_tech’s picture

Tried # 136 again on a restored-from-backup site and made sure I had all my ducks in a row. Still same error. Happens on more than one content type with entity reference. The error shows up and the form does not, which makes the problem worse, since as I mentioned before, I wasn't having problems with functionality. Happens for admin account as well as user account. More interestingly, when I click on the "edit" link on the entity reference from manage fields, I get the same error.

"EntityMetadataWrapperException: Permission to create a node was requested but no node type was given. in entity_metadata_no_hook_node_access() (line 635...drupal/profiles/commerce_kickstart/modules/contrib/entity/modules/callbacks.inc)"

The version installed are:

  • Drupal 7.22
  • Entity API, Entity Reference, Inline Entity form 7.x-1.0

I have applied all updates and patches.

malberts’s picture

@dallas_tech, try with the -dev versions of ER and IEF to see if they make a difference.

Mile23’s picture

@dallas_tech: It could be any of the modules in the commerce kickstart profile that use (contrib) Entity API. Maintainers for ER and IEF are aware of this problem and have committed patches to dev, IIRC. It just remains that we need a patch here so everyone can move forward.

Also, this illustrates why throwing an exception in entity_metadata_no_hook_node_access() isn't the greatest idea: The problem lies elsewhere, but the error ends up being reported here.

lauriii’s picture

Same patch with a.milkovsky comment #139, but made it just a bit cleaner.

This should fix the error messages while using enity reference on nodes.

Notice: Undefined property: stdClass::$nid in entity_metadata_no_hook_node_access() (line 619 of sites/all/modules/entity/modules/callbacks.inc).
Notice: Undefined property: stdClass::$vid in entity_metadata_no_hook_node_access() (line 620 of sites/all/modules/entity/modules/callbacks.inc).

dropfen’s picture

@lauriii
https://drupal.org/node/1780646#comment-7564907

@dallas_tech
I'm using the #97 with entityreference_dialog_widget, and it works.

edutrul’s picture

I tried patch #97 and works like a charm! thanks!

Sinan Erdem’s picture

I applied the patch on #97 to version 7.x-1.1 and the errors were gone. So far, all seems to work ok. Thanks.

fago’s picture

Status: Reviewed & tested by the community » Needs work

#136 looks already pretty good, let's proceed with this one. Here some small remarks:

+++ b/entity.test
@@ -1037,6 +1037,249 @@ class EntityMetadataTestCase extends EntityWebTestCase {
+
+class EntityMetadataNodeCreateAccessTestCase extends EntityWebTestCase {

Misses docs. Anyway, we can we just merge this and others into the first test case and just add it as another method? One node-access test case in entity api should be enough.

+++ b/entity.test
@@ -1037,6 +1037,249 @@ class EntityMetadataTestCase extends EntityWebTestCase {
+      //$revision = clone $node;

Should be removed.

+++ b/modules/callbacks.inc
@@ -612,23 +612,50 @@ function entity_metadata_field_file_validate_item($items, $context) {
+        throw new EntityMetadataWrapperException('Permission to create a node was requested but no node type was given.');

Let's throw an EntityMalformedException as entity_extract_ids() does.

Also, this illustrates why throwing an exception in entity_metadata_no_hook_node_access() isn't the greatest idea: The problem lies elsewhere, but the error ends up being reported here.

True, but we have just that option and ignoring the wrong API usage. If there are not any major problems with that exception, I think we should better be verbose about something being wrong instead of failing silently.

Mile23’s picture

Thanks for the input, @fago.

@JvE: Rock on. :-)

joseph.muennich’s picture

Did this code make it into the 2013-Jul-29 release of 7.x-1.x-dev? I am not seeing it in this new release.

C-Logemann’s picture

@joseph.muennich: Usually a maintainer reports in an issue that (s)he has committed a patch and changes the status to "fixed".
You can help with testing or coding if you are able to do this.

joseph.muennich’s picture

Thanks for the comment @C_Logemann . I understand the process, but thought the issue resolved after reading the maintainers comment of "let's proceed with this one" followed by a dev release. I thought it possible the patch was included but the status not updated.

joachim’s picture

There's an issue over at Services Entity to support this (#1865102: entity bundle is not passed to entity_access() for 'create' op) but I'm a bit confused about which is the latest patch here.

Also, if the entity access callback now expects & passes along a created entity, is this an API change which affects modules that use Entity API?

Mile23’s picture

Status: Needs work » Needs review
FileSize
12.42 KB

+class EntityMetadataNodeCreateAccessTestCase extends EntityWebTestCase {
Misses docs. Anyway, we can we just merge this and others into the first test case and just add it as another method? One node-access test case in entity api should be enough.

I left it separate so it could be discussed in the issue on its own terms. Then later I left it separate because EntityMetadataNodeRevisionAccessTestCase is basically NodeRevisionPermissionsTestCase with some Entity API stuff thrown in, and it would be easier to maintain in and of itself.

Now, however, folding the create access test into EntityMetadataNodeAccessTestCase results in the create test failing. If I remove the db_delete('role_permission') line from EntityMetadataNodeAccessTestCase::setUp(), the create test passes, but the other tests fail. So basically I think the two tests should remain separate because they create the same permissions, with their own needs.

Given that, here's a patch, based on #136, with the changes suggested in #183.

Status: Needs review » Needs work

The last submitted patch, entity-entity_access-1780646-189.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
12.41 KB

Woops, forgot to uncomment.

yannickoo’s picture

Cool, #191 fixed all these errors which I had with Inline Entity Form module :)

Mile23’s picture

How many needed for RTBC? :-)

yannickoo’s picture

In my case it was just the Inline Entity Form module, let's wait for another user which has a more complex setup ;) But tests passed fine and the non-test related code looks also fine to me.

nicholas.alipaz’s picture

As yannickoo said, wfm, fixes all issues in Inline Entity Form module.

acrosman’s picture

I applied the patch in #191 because I was seeing the notices described in #179. There was only 1 node type consistently triggering those notices on the site I'm working with. Logged in as user 1, I now get the error below when attempting to edit or create nodes of that type:

EntityMalformedException: Permission to create a node was requested but no node type was given. in entity_metadata_no_hook_node_access() (line 639 of /var/www/sites/default/modules/entity/modules/callbacks.inc).

The node type in question contains a entity reference (field collection), that in turn contains an entity reference to another node type. I'm not sure if this suggests the module is now working properly and exposing another issue, or if this is an error in the patched version.

yogaf’s picture

As yannickoo said, wfm, fixes all issues in Inline Entity Form module.

Rob_Feature’s picture

Status: Needs review » Needs work

I'm seeing the same as in #196 with the patch from #191. Breaks everything when creating a node with an entity reference field. Since this didn't work I tried the patch in #136 and also got the same error...

Mile23’s picture

So we got it fixed in Inline Entity Form, now it's time to work on Entity Reference. :-)

Hmm. This issue seems to be related: #1780626: Entity Reference incorrectly calls entity_access()

Frippuz’s picture

I tried #191 on a site I'm currently working on.

While trying to add a new node I get the error message: "EntityMalformedException: Permission to create a node was requested but no node type was given. in entity_metadata_no_hook_node_access() (line 639 of .../contrib/entity/modules/callbacks.inc)." thus dissallowing me to add the node of this type.

Perhaps this has to to with entityreference wich I use on the node types I get the error on.

Mile23:Recognize this?

shawn_smiley’s picture

The patch in #191 worked for me with both releases 1.1 and 1.2.

wodenx’s picture

I am unable to reproduce the errors described in #196, 198 and 200 with a clean install of 7.23 and only ctools, entity and entityreference enabled.

@Frippuz, @Rob_Feature and @acrosman can you provide any more info? what other modules do you have enabled. what are the field, instance and widget settings for the entityreference fields on the nodes that create the issues?

Attached patch adds a diagnostic hack supporting the legacy pattern of providing bundle rather than object for create ops. Does this fix the problems you're experiencing.

This version of the patch also addresses another issue. Property level 'edit' access checks invoke entity_access('update', ...) on the parent entity. This throws notices for new nodes (and probably other entities) because they don't have identifiers yet. It seems the correct behavior is to invoke entity_access('create', ...) if the parent entity id doesn't exist.

*** EDIT: Please try the patch in #191 first as that is the preferred solution. Only use this patch if you require support for the legacy pattern of providing the bundle as a string rather than as a property of a supplied entity.

jrbeeman’s picture

Status: Needs work » Needs review

I'm not familiar enough with the issue history to mark as RTBC, but I can confirm that the patch in #202 resolves the notices for me on a site with a content type that utilizes Inline Entity Form.

yannickoo’s picture

@#203 it also works with the patch from #191 but other users said that it does not work with a (e.g.) content type which has a Entity Reference field.

acrosman’s picture

I applied the patch in #202 against a fresh copy of the dev from 8/14. The error I saw with the #191 patch is gone, but I'm having trouble with the entity reference form. As noted before (and in more detail below) the node contains a field collection which bridges back to a different node type. When I try to create a parent node I am able to create the new field collection entity, and the child nodes of that collection, however when I save the parent node the references to the field collection's child get lost. This is complicated by a hook_form_alter on the reference forms; I'll experiment with removing that form alter as soon as I have some time (hopefully later today, but that might be happen) to see if that's to blame.

Here's a bit more information about the node type in question:

There are actually two entity references being made for this content type. One is an OG reference using the default settings (so field type entity reference and widget is OG Reference), as you might imagine that's on several node types and has never caused trouble. The second is a field collection (field type Field Collection, widget Embedded). The reason I focused on that one before is it's the more complex case since the field collection contains an entity reference (field type Entity Reference, widget Autocomplete, bundle type is a node).

I can pull the full list of modules if that's really helpful. Modules that directly interact with this node type include (this should be pretty complete):

  • ECK
  • Entity API
  • Corresponding Entity References
  • References dialog
  • Entity Reference
  • Field Collection
  • OG
  • Entity Tokens
  • EVA
  • DS
  • Panels
  • Views Content Panes
  • Pathauto
  • Date API
  • A custom module (form alter as noted above)
metametamind’s picture

I applied patch #202 and still (intermittently, which is odd) receive this error:


Patch #202 works correctly. Above bug report was due to user error.

Frippuz’s picture

My list of modules in use is pretty long. I will try to locate the faulty one.

MXT’s picture

I can confirm that Patch in #202 works well for me: all notice error have disappeared and no other issues encountered using content types with entity reference fields.

(I'm using entity 7.x-1.2)

Thank you

acrosman’s picture

The issue I'm seeing with the patch in #202 (reported in #205) is related to the custom form alters.

acrosman’s picture

I'm not sure if this is an intended side effect or not, but I'll post it for consideration.

The form alter I mentioned before disables the autocomplete field for the node reference that's within the field reference (screenshot attached to try to make it clearer). The goal here is to be able to add new nodes, and re-order nodes, but not add existing nodes. Before the patch in 202 this worked (except the notices), but after the patch new nodes can be created but when you save the parent node their connection to the field collection is lost.

After a little drilling down to get to the right form elements the form alter just does the following for each field:

    $task['target_id']['#disabled'] = True;
    $task['target_id']['#autocomplete_path'] = False;

#autocomplete_path can be disabled safely, but setting #disabled to true triggers the issue above. I may be the only person foolish enough to do this, so it might be enough of an edge case to ignore.

pmichelazzo’s picture

Unfortunately any on these patches works for me. I'm using the Workbench moderation with Entity Reference in some contents and the message remains.

Any clue about it?

Thanks

jpstrikesback’s picture

<meep>I'm still using #8 and loving it...tho I haven't updated IEF for a while :)</meep>

alesr’s picture

I was still getting the notices when applying the patch from #202, but the issue for me was that I used "curl http://drupal.org/files/[patch-name].patch | git apply -", the one-line command to apply the patch and didn't noticed that the files weren't changed.
I just updated to Entity 7.x-1.2 so all the files from this module were "in diff" for me, so I didn't noticed it right away. No error message was printed when applying the patch too.

Do the "wget file_name.patch" and then "patch -p1 < file_name.patch" and check if changes were made.

metametamind’s picture

Bwhahaha. After days of head-banging I discovered the patch had worked and I was still getting a notice because... wait for it... there was actually a views call to non-existent object from a previous project in one of the content areas.

Sigh.

MXT’s picture

@metametamind: please, edit your #206 writing a note that the patch now works for you.

Nileschip’s picture

pmichelazzo’s picture

Hi people,

The patch still have a problem. In my case the notice message occurs with the revisions:

Notice: Trying to get property of non-object in entity_metadata_no_hook_node_access() (line 649 of /var/www/sites/all/modules/entity/modules/callbacks.inc).

This is the code:

if ($node->vid !== $default_revision->vid) {
      return _node_revision_access($node, $op, $account);
    }

Any clue to solve it?

Regards

digitaldonkey’s picture

Version: 7.x-1.x-dev » 7.x-1.2
digitaldonkey’s picture

jweowu’s picture

Version: 7.x-1.2 » 7.x-1.x-dev

I don't know why digitaldonkey felt compelled to re-test the patch from comment #1 against version 1.2, but presumably we don't want the version number left that way.

Kristen Pol’s picture

#202 got rid of the entity_metadata_no_hook_node_access errors for me when using Inline Entity Form. Thanks!

jweowu’s picture

I'd really like to see any of JvE/Mile23/fago weigh in on the changes introduced in #202.

I'll confess to being pretty well confused by what's happening in this issue, as there seemed to be about three different approaches undergoing parallel development. I do gather that there was a great deal of back-and-forth over whether or not strings were a valid argument type, and it seemed that the people most in the know had settled on an approach, so it concerns me a little that #202 is now adding new code checking for a string argument?

diff --git a/modules/callbacks.inc b/modules/callbacks.inc
index d487edd..bd2ceb7 100644
--- a/modules/callbacks.inc
+++ b/modules/callbacks.inc
@@ -632,7 +632,12 @@ function entity_metadata_no_hook_node_access($op, $node = NULL, $account = NULL)
   // First deal with the case where a $node is provided.
   if (isset($node)) {
     if ($op == 'create') {
-      if (isset($node->type)) {
+      if (is_string($node)) {
+        // Support legacy pattern of passing bundle rather than object for
+        // 'create' access.
+        return node_access($op, $node, $account);
+      }
+      elseif (isset($node->type)) {
         return node_access($op, $node->type, $account);
       }
       else {

Is this still in line with #136/#191 ?

Mile23’s picture

It does seem a bit like a regression to have the function allow a string for bundle type, given the other comments from fago that it shouldn't be.

I think basically the maintainers are busy making Drupal 8, so this issue is taking a back seat, or this would have been solved a while back.

jweowu’s picture

Thanks for that, Mile23. So it seems to me that we need to ignore #202, and consider #191 to still be the current patch.

Or if the other changes in #202 are still appropriate and integral to this issue (the ones to EntityMetadataWrapper::access() and EntityStructureWrapper::propertyAccess()), those changes would need to be re-rolled.

wodenx’s picture

As I said in the comment:

Attached patch adds a diagnostic hack supporting the legacy pattern of providing bundle rather than object for create ops. Does this fix the problems you're experiencing.

That part of the patch was never intended to be committed - just as a diagnostic to see if it resolved some of the problems others were experiencing with #191.

That said - I'm not sure I see a problem with supporting the legacy mode. There are probably other modules out there which use this pattern, and what do we gain by breaking them, as long as we make it clear that the old pattern is deprecated?

The other parts of #202 probably deserve a new issue.

Mile23’s picture

wodenx, we've had approximately fifteen hundred patches already in this issue. :-)

Could you finalize a patch that implements the fix you want, and offer it so people can do review and hopefully RTBC? I think that allowing the string is strictly a bad idea, but really: We should get a fix in so we can put this thing to bed.

jweowu’s picture

And if diagnostics are needed, I fancy that logging a debug_backtrace (or portions-thereof) when an invalid (string) argument is passed in would be a more helpful thing to do, so that testers can find out where the fault lies?

sam.spinoy@gmail.com’s picture

Just used the patch in #191, does the trick for me.

wodenx’s picture

Nothing to re-roll. I'm all for #191. I think providing the complete entity is far preferable to overloading the argument. As I say I am unable to reproduce the problems (seemingly related to entityreference fields) reported in #196, #198 and #200.

Still, can we in good conscience move this to RTBC if at least 3 people claim it breaks their sites? There are many, many sites out there which depend on entity api. If a change like this is going to cause issues for them, then I ask again what we lose by supporting both the legacy and the preferred pattern, especially at this late-ish stage in the D7 lifecycle.

Ares_ekb’s picture

I've got this error during editing a node with https://drupal.org/project/inline_entity_form widget enabled.

elvis2’s picture

Ditto #230

lautaro.pastorini@gmail.com’s picture

The same as #230
Please, let us know if there is a solution.
Thanks in advance.

JurgenR’s picture

Ditto #230

Jarek Polok’s picture

Same problem here, I've checked all the mentioned patch proposals in this thread .. no success so far unfortunately ... (#217)

MilanT’s picture

Also having the same issue as #230.

yannickoo’s picture

Status: Needs review » Needs work
msh2050’s picture

FileSize
13.96 KB
73.68 KB

I have the same problem

    Notice: Undefined property: stdClass::$nid in node_access() (line 2999 of C:\inetpub\wwwroot\drupal\modules\node\node.module).
    Notice: Undefined property: stdClass::$nid in node_access() (line 2999 of C:\inetpub\wwwroot\drupal\modules\node\node.module).
    Notice: Undefined property: stdClass::$nid in node_access() (line 2999 of C:\inetpub\wwwroot\drupal\modules\node\node.module).
    Notice: Undefined property: stdClass::$nid in node_access() (line 2999 of C:\inetpub\wwwroot\drupal\modules\node\node.module).

the 2999 line is : $cid = is_object($node) ? $node->nid : $node;
I search for the nodes but when I apply the patch it say that the patch is not apply for the selected context

please inform to which file I have to apply the patch and whhich is the last stable tested patch

with best regard

wodenx’s picture

@msh2050 please try the patch at #191

msh2050’s picture

Dear wodenx thanks alot...

please... for which file I should apply the patch??

best regard..

digitaldonkey’s picture

Also having the same issue as #230 using inline entity form.

msh2050’s picture

FileSize
95.47 KB

I Apply the patch to entity.module but the patch was partially applied as seen in the picture...
I don't want to do try and error without scientific reason..

I will appreciate if you tell me how I can know the patch is for witch file...for this patch and for future batches...

with best regard

jweowu’s picture

msh2050’s picture

thanks a lot jweowu but the language used in the link is for advanced user I did not understand git and I scared from non visual method...may be in near future I can learn that .

many many thanks to wodenx the patch is working perfect ... I just apply it to callbacks.inc after new module installation..
please let me explain what I do by using the beginners language

1- fist of all download the last Entity API module to your computer...
2- unzip the file and copy it to "C:\inetpub\wwwroot\"yoursit file"\sites\all\modules" this will replace the entity folder...

3- then download the patch in at #191 (right click --- save link us)
4-by using netbeans navigate to C:\inetpub\wwwroot\drupal\sites\all\modules\entity\modules

5- right click on callbacks.inc ---then tools ---- apply diff patch.

6- select the file entity-entity_access-1780646-191.patch that you saved us from step 2

still I don't know how I can know to witch file I have to apply the patch may be by using git I don't to specify the file (he will know may be!!)
that's all ...
sorry for English ...
regards

jweowu’s picture

msh2050: in that case, start here: https://drupal.org/node/60108

wodenx’s picture

@ares_ekb, @elvis2, @lautaro.pastori, @jurgenR, @milanT, @digitaldonkey - have you tried the patch at #191?

@jarek polok - can you describe the exact symptoms AFTER applying the patch at #191?

MilanT’s picture

Ok the patch at #191 took care of the entity errors for me. Thanks wodenx!

Now I keep getting notices and warnings regarding field groups when trying to create a new node via the inline entity form field.

wodenx’s picture

@MilanT do you think those warnings are related to Entity API? Can you post them?

msh2050’s picture

FileSize
87.35 KB
39.69 KB

I don't know if it may be Entity API bug
the inline entity form only display the title and status and operation column...
the title is auto generated and I need to add other two fields to the form but I not display that??
I see the tutorial of the e-commerce products it showing all the fields .. but for me it just did not!!??As in attachments...

I open an Issue in Inline entity form also... modifying the Inline entity form listed fields">https://drupal.org/node/2091585

jweowu’s picture

It seems that this issue is getting muddied again. I think wodenx had the right idea in providing a patch for diagnostic purposes, except that it needed to log the details.

This patch is for diagnostic purposes. It is the same patch as #191, with the addition that it will log some useful data to watchdog* when a string is passed in place of a node.

(edit: #191 has since been committed, and these diagnostics are provided as a separate patch at #308)

My understanding (and someone please correct me if I'm wrong) is that this argument type mis-match is the only known problem with this patch. We no longer allow a string argument, but various places are passing one, and people are confused because no one knows who is at fault.

If you are still experiencing problems with #191, please try this patch, and then report back with the data from watchdog*. It will look something like this:

String citation_author passed to entity_access() instead of a node object. Stack data follows:
Array
(
    [file] => sites/all/modules/contrib/entity/entity.module
    [line] => 654
    [function] => entity_metadata_no_hook_node_access
    [args] => Array
        (
            [0] => create
            [1] => citation_author
            [2] =>
            [3] => node
        )
)
Array
(
    [0] => entity_metadata_no_hook_node_access() at /sites/all/modules/contrib/entity/entity.module:654
    [1] => entity_access() at /sites/all/modules/contrib/references_dialog/references_dialog.dialog_widgets.inc:264
    [2] => references_dialog_entityreference_link_helper() at /sites/all/modules/contrib/references_dialog/references_dialog.dialog_widgets.inc:244
    [3] => references_dialog_entityreference_add_link() at /sites/all/modules/contrib/references_dialog/references_dialog.module:312
    [4] => references_dialog_process_widget() at //includes/form.inc:1879
    [5] => form_builder() at /includes/form.inc:1871
    [6] => form_builder() at /includes/form.inc:1871
    [7] => form_builder() at /includes/form.inc:1871
    [8] => form_builder() at /includes/form.inc:1871
    [9] => form_builder() at /includes/form.inc:850
    [10] => drupal_process_form() at /includes/form.inc:380
    [11] => drupal_build_form() at /includes/form.inc:131
    [12] => drupal_get_form() at /modules/node/node.pages.inc:78
    [13] => node_add()
    [14] => call_user_func_array() at /includes/menu.inc:517
    [15] => menu_execute_active_handler() at /index.php:21
)

I didn't want to log really crazy amounts of data, so I'm hoping this format will provide sufficient detail to track down the problems.

[0] will be entity_metadata_no_hook_node_access(), [1] should be entity_access(), and we would expect to identify the source of the problem -- the function which called entity_access() with an invalid argument -- at [2]

In the above example, we can see that the bug lies in the function references_dialog_entityreference_link_helper() which we can see is part of the references_dialog module, so we would head to the references_dialog issue queue to search for an existing issue for the problem (this one in that particular case), or log a new issue there if none existed (with a pointer back to this issue so that the maintainer can understand the problem).

If you have no such errors in your log, then my understanding is that your problem lies elsewhere, and you should search for or log a different issue (again, correct me if I'm wrong).

(*) being admin/reports/dblog for most of you. Filter log messages by Type = "entity" if necessary.

jweowu’s picture

Given the resounding silence, I presume that none of the people previously reporting problems with #191 are seeing any log entries from the diagnostic patch?

Is there anything else we should test for here, or does this more or less suggest that #191 is RTBC?

Mile23’s picture

Status: Needs work » Reviewed & tested by the community

My understanding (and someone please correct me if I'm wrong) is that this argument type mis-match is the only known problem with this patch. We no longer allow a string argument, but various places are passing one, and people are confused because no one knows who is at fault.

That's it exactly. There's timidity on everyone's part to commit a change because no one understands how the API works, and reading a 251-comment issue to understand it is out of the question somehow. :-)

This issue is really about the API, not whether it will break other projects that use the API. That's why it's a total pain to RTBC: It breaks other stuff and that makes people think it's not really a fix.

Also, absent maintainers. :-)

#249 has the diagnostic thing.

#191 has the most recent decent patch (afaik), unless we want to allow people to use the bundle string method, in which case it's #202. I'm going to be bold and say RTBC, and someone can kick that if they want.

Alexander Allen’s picture

The issue queue is being filled up with "critical" bug items and issue requests, just because Entity API is doing the responsible thing and is throwing an Exception when people pass us the wrong parameters - instead of just silently failing, like a lot of modules would do. I think this is the responsible thing for an API developer to do, but instead API implementers get confused and think that the API is broken, when it's the other way around.

Lke #251 said, #249 fixes this, and I for one vote up for this to be committed. That way we'll eliminate all that noise in our issue queue coming from bugs that are really not ours, and also help other developers in the form of providing them with more robust out-of-the box feedback (instead of requiring them to pop-up their debugger just to get some basic feedback).

Mega-BAMP for this one, please commit :-)

wodenx’s picture

This isn't a case of people misusing or misunderstanding an existing API. The API *was* broken and ambiguous -- as the OP said, there was no way to check bundle-specific node access without throwing notices. Now the API is being fixed, and some people report that the fix introduces new problems.

Prior to this thread you could pass the bundle as a string or as part of a node object. Either way it would generate notices, but also either way it would correctly test for access permissions.

Now a decision has been made to enforce the second pattern (passing an object) -- and I think this is absolutely correct. But there may be modules out there which rely on the way it used to work -- especially because that pattern is used in node_access() itself.

That said - I vote for #249 -- it will enable people who do experience problems to track them down and hopefully get the offending client to upgrade to the new pattern. But I do think we need to be clear that this represents a *change* in API -- and this needs to be mentioned in the release notes.

elvis2’s picture

+1 on #253

Mile23’s picture

Please ping @fago with your decision. :-)

jweowu’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
13.45 KB
1.59 KB

Committing the patch with the diagnostic logging seems reasonable to me as well, but as mentioned initially that patch had a dependency on PHP 5.3, and we can't commit it like that, so here's the re-roll which ought to work on 5.2 as well.

I imagine that the only invalid args are going to be strings, but I've also tweaked the code in order to log data when the argument is invalid, regardless of its type.

Please review, and hopefully we can get this committed.

(Any newcomers, please refer to #249 for more information about this patch.)

arosboro’s picture

Thanks for the patch applied in #256

sites/all/modules/contrib/entity$ patch -p1 < ~/patches/entity-node_access-1780646-256.diagnostic.patch
patching file entity.module
Hunk #1 succeeded at 582 (offset -40 lines).
patching file entity.test
Hunk #1 succeeded at 1088 (offset -8 lines).
patching file modules/callbacks.inc
Hunk #1 succeeded at 612 (offset -3 lines).

Errors I experienced, seem to have vanished.

edutrul’s picture

Patch #202


I'm using entity 7.x-1.2 Worked perfectly for me

thanks!

jweowu’s picture

edutrul: #256 is the current patch for review. Can you please use that, or otherwise give some kind of detail on why you are using #202.

Mile23’s picture

Please change status to RTBC if you've applied and tested a patch. Be sure and specify which patch you've tested.

jweowu’s picture

Well "works for me" isn't the same as RTBC (and definitely not when it's an old patch being tested); but we did have RTBC for #249, and there are fairly minor changes between that and #256.

If the people who already looked at #249 could cast their eyes over the code changes between that and #256 (which were primarily because the anonymous function syntax in the older patch required PHP 5.3), we could most likely put this back to RTBC.

drclaw’s picture

Currently testing the patch in #256 and it appears to be working so far. I hadn't tested the one in #249, however.

drclaw’s picture

Issue summary: View changes

Fix markup

hass’s picture

#256 removes all notices I get since I have installed inline_entity_form module.

What's holding this patch back? A lot of people seems to be effected (180 subscribers), ~270 comments.

elvis2’s picture

Unfortunately, I am still having problems after patching with #256. I updated the entity module with 7.x-1.x-dev, as of today, before patching. There is a chance this comment doesn't belong on this thread. My apologies in advance.

Here is a screenshot of what I am seeing. It seems there needs to be more checks to see if $node is being passed and is "not" an object. In the output that you see in the screenshot, the first call to entity_metadata_no_hook_node_access shows there is a value for node, a string, of the node type, normally accessible via $node->type.

I am not sure if this is a brightcove problem (we are using brightcove 7.x-3.4) or something more with the entity module. What I can say is we didn't have any issues 3 months ago, until we upgraded the entity module.

Thoughts?

elvis2’s picture

Disregard comment #264, the access issue was resolved once I used patch #256 and upgraded the brightcove module.

mariusm’s picture

Please update patch 256# for last dev.

Thanks

alexweber’s picture

The patch in #256 applies and works perfectly for me using last dev.

mariusm’s picture

Yes, it's ok

acrosman’s picture

I applied the patch in #256 against both 7.x-1.2 and 7.x-1.x-dev and after reloading the page that displayed the notices instead I got an error message that read:

  • Notice: Undefined index: file in _entity_funcstack_frame() (line 676 of ...[path to site]/entity/modules/callbacks.inc).
  • Notice: Undefined index: file in _entity_funcstack_frame() (line 676 of ...[path to site]/entity/modules/callbacks.inc).
  • EntityMalformedException: Permission to create a node was requested but no node type was given. in entity_metadata_no_hook_node_access() (line 647 of ...[path to site]/entity/modules/callbacks.inc).
elvis2’s picture

@acrosman The "Undefined index" are just warnings.

Can you provide more information regarding the last item (EntityMalformedException)? It seems you were creating or updating a node? Does that node type have other entity related items attached, say, inline_entity_form or something else that relies on the entity module?

acrosman’s picture

It is a node create form for a content type that includes 4 entity reference fields.
Widget is set to autocomplete on two, and check boxes on the other two.
The entities being referenced are nodes and taxonomy terms (two of each).

jweowu’s picture

Those _entity_funcstack_frame() errors are from the diagnostic code I added, so clearly that needs work. I didn't realise that debug_backtrace() frames could be missing a 'file' entry. I'd love to know what those frames actually contained?

Anyway, you should STILL have some diagnostics with stack data in watchdog, so you should look for those. Filter by 'entity' if necessary. As #256 says, see #249 for details.

jweowu’s picture

I've fixed the diagnostic code in _entity_funcstack_frame() so that it won't throw notices in situations with no file or line number. It also now indicates the class name and method call type, where appropriate.

(Any newcomers, please refer to #249 for more information about the diagnostics.)

As before, this is still essentially the patch from #191, which was RTBC as of #251; so please review the current diagnostic code (i.e. just review the interdiff) so we can get this back to RTBC again, and hopefully we can get this committed.

alexweber’s picture

Looks great to me!

acrosman’s picture

The notices are gone, but I still got the MalformedEntityError. That said this got me to the solution for my case. Thanks for the better diagnostics for tracking it down. The notes that follow are for others that might have the same combination of factors.

In my case it was References dialog. It was on the current release (7.x-1.0-alpha4); moving to the current dev resolved all notices and errors (displayed and logged).

With the patch applied I got an exception on the node create and edit page that read:

EntityMalformedException: Permission to create a node was requested but no node type was given. in entity_metadata_no_hook_node_access() (line 647 of ...path/to/drupal/modules/contrib/entity/modules/callbacks.inc).

Watchdog recorded the stack trace as (infographic is a content type):

String infographic passed to entity_access() instead of a node object. Stack data follows:

Array
(
    [file] => /path/to/Drupal/sites/site_name/modules/contrib/entity/entity.module
    [line] => 614
    [function] => entity_metadata_no_hook_node_access
    [args] => Array
        (
            [0] => create
            [1] => infographic
            [2] => 
            [3] => node
        )

)
Array
(
    [0] => entity_metadata_no_hook_node_access() at //path/to/Drupal/sites/site_name/modules/contrib/entity/entity.module:614
    [1] => entity_access() at /path/to/Drupal/sites/site_name/modules/contrib/references_dialog/references_dialog.dialog_widgets.inc:224
    [2] => references_dialog_entityreference_link_helper() at /path/to/Drupal/sites/site_name/modules/contrib/references_dialog/references_dialog.dialog_widgets.inc:204
    [3] => references_dialog_entityreference_add_link() at /path/to/Drupal/sites/site_name/modules/contrib/references_dialog/references_dialog.module:312
    [4] => references_dialog_process_widget() at /path/to/Drupal/includes/form.inc:1872
    [5] => form_builder() at /path/to/Drupal/includes/form.inc:1864
    [6] => form_builder() at /path/to/Drupal/includes/form.inc:1864
    [7] => form_builder() at /path/to/Drupal/includes/form.inc:1864
    [8] => form_builder() at /path/to/Drupal/includes/form.inc:1864
    [9] => form_builder() at /path/to/Drupal/includes/form.inc:843
    [10] => drupal_process_form() at /path/to/Drupal/includes/form.inc:374
    [11] => drupal_build_form() at /path/to/Drupal/includes/form.inc:131
    [12] => drupal_get_form() at /path/to/Drupal/modules/node/node.pages.inc:78
    [13] => node_add()
    [14] => call_user_func_array() at /path/to/Drupal/includes/menu.inc:517
    [15] => menu_execute_active_handler() at /path/to/Drupal/index.php:21
)

As of this morning the site is running D7.24 and entityreference 7.x-1.1, and the errors continued. But the update of references dialog resolved the problem.

jweowu’s picture

Thanks acrosman; that's exactly how this is intended to work :) (and it's good to hear that References Dialog has already fixed it.)

wodenx’s picture

+1 for #273. I'm gonna go out on a limb and set to RTBC

wodenx’s picture

Status: Needs review » Reviewed & tested by the community
antoinetooley’s picture

#273 did it for me. Thanks!

jweowu’s picture

Okay, the patch (#273, incorporating #191 + diagnostics) is RTBC.

I've just reviewed the comments since the first version of the diagnostics (#249, which was originally posted on September 18th) and there are no unresolved problems reported in the 9 weeks between then and now.

acrosman has given us a nice example (#275) of this working in practice to track down the source of the bug in another module.

Let's get this committed! The sooner this code is in a dev release, the sooner the bugs in other modules will be detected and fixed.

jweowu’s picture

Issue summary: View changes
jweowu’s picture

Issue summary: View changes
jweowu’s picture

Issue summary: View changes
pixelwhip’s picture

I was getting similar errors to those reported in #275. The patch in #273 along with updating references_dialog module to the current dev release fixed the issue for me. Thanks to all who worked on this!

Mile23’s picture

Will we make it to #300? :-)

RogerRogers’s picture

Couldn't this just be rolled into the main branch already? #191 doesn't apply to the latest dev release, at least not for me.

Using the latest dev release of entity api, I've now tried all the patches that appear to have worked in the past from this thread - mostly manually - and it doesn't solve the problem. I have solved the problem with these patches in the past, so I assume this never got into the main branch and is not outdated.

jweowu’s picture

RogerRogers: #273 is the current patch. You should be using that.

That said, both #191 and #273 do apply to the current 7.x-1.x branch, so I expect your problem with the former will also occur with the latter.

Are you also applying other conflicting patches?

RogerRogers’s picture

Thanks for your rapid reply jweowu! I was using the latest dev release, and the patch doesn't apply to it. However, moving to the recommended release of Entity API does allow me to apply the patch. However, #191 doesn't resolve the error. Shucks.

I'll try a few earlier patches now. I've applied a patch from this thread a number of times before, with success, so I'm sure it will work with one of the past versions of the module.

RogerRogers’s picture

Oh well. Earlier patches don't apply cleanly. #191 does, but doesn't resolve the Inline Entity Form error I was hoping to resolve, which is:

Notice: Undefined property: stdClass::$nid in node_access() (line 2999 of C:\git\nbwla.com\drupal\modules\node\node.module).

Just in case anyone has an idea. :/

jweowu’s picture

jweowu’s picture

RogerRogers: If you're using #273 (or #191, but seriously: use #273) and you're not getting errors from entity_metadata_no_hook_node_access() then I'm pretty sure this isn't the issue to be looking in.

The error you posted could be anything; you'll need to see a backtrace to figure out where the problem is. I suggest installing https://drupal.org/project/devel and selecting one of the "Krumo backtrace" error handlers at admin/config/development/devel

nicxvan’s picture

Is there a an estimate when this will be committed? I tried applying the patch but I got a pdo exception error when I visited the manage fields pages.

DamienMcKenna’s picture

+1 for this patch fixing the issues, and no noticeable side effects.

jackhutton’s picture

confused abit here: does the patch work with the dev version 7.x-1.x-dev ?
I'm getting a series of errors :

Notice: Undefined property: stdClass::$nid in entity_metadata_no_hook_node_access() (line 634 of /srv/bindings/2c069da8e839437c83018e4c6ca152f3/code/sites/all/modules/entity/modules/callbacks.inc).

reading thru the patches in 273 and the lines of code don't seem to match up w. the dev version.
Thanks for the help here.. I must be missing something.

DamienMcKenna’s picture

@jackhutton: Are you sure you're using the latest dev version? I've just downloaded the module via git, when I apply the patch line 634 of modules/callbacks.inc is a comment. Please verify you're using the latest version and try applying it again.

DamienMcKenna’s picture

For the record, here's what happens when I apply the patch:

$ curl https://drupal.org/files/issues/entity-node_access-1780646-273.diagnostic.patch|patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 14144  100 14144    0     0  13671      0  0:00:01  0:00:01 --:--:-- 26241
patching file entity.module
patching file entity.test
patching file modules/callbacks.inc
Hunk #1 succeeded at 627 (offset 12 lines).

As you can see, part of the patch needed to be offset by 12 lines, but otherwise it it applies fine (I was going to reroll it but didn't think it was really necessary) and works fine.

dalguete’s picture

randell’s picture

$ curl https://drupal.org/files/issues/entity-node_access-1780646-273.diagnostic.patch|patch -p1 --binary
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 14144  100 14144    0     0  10818      0  0:00:01  0:00:01 --:--:-- 10947
patching file entity.module
Reversed (or previously applied) patch detected!  Assume -R? [n] y
Hunk #1 succeeded at 582 (offset -40 lines).
patching file entity.test
Hunk #1 succeeded at 1088 (offset -8 lines).
patching file modules/callbacks.inc
Hunk #1 succeeded at 612 (offset -3 lines).
DamienMcKenna’s picture

@randell: If 'patch' says that it detects a reversed patch, that means that someone already applied the patch to your codebase and it wants to remove the changes.

I've just verified again and the current 7.x-1.x branch of the Entity module still does not have this applied, so someone has already added it to your codebase.

fago’s picture

Status: Reviewed & tested by the community » Fixed

I don't think it makes sense to commit debugging-aids, the exception should be enough to point developers to the problem. Also the "legacy-usage" wasn't indicated to be supported in anyway, so this was just unsupported usage we don't have to account for either.

#191 gets this right and looks good to me. I corrected some minor coding style stuff and committed it - thanks everyone!

jpstrikesback’s picture

This calls for champagne!

Mile23’s picture

OMG. :-)

drclaw’s picture

^^ What Mile23 said =)

jweowu’s picture

fago: Thanks for the commit.

I honestly think the debugging aids would assist in the short term -- there's already evidence that this change can and will confuse people who *don't* know how to track down the cause -- so including the extra logging really would have helped people to help themselves (and hopefully not post new issues here), especially with the much wider audience that the change will have now that it's committed.

I agree that in the long term the diagnostics don't belong in the code base, but I suspect they would save people some headaches in the interim.

But yes; champagne! (or tea :)

jweowu’s picture

Issue summary: View changes

Updated summary to reflect the commit.

JvE’s picture

Wow, after 6.5 months my patch finally landed!

Thanks everyone for sticking with it.

jweowu’s picture

Issue summary: View changes
FileSize
2.23 KB

Providing diagnostics as a separate patch, so that they're still easily available.

See comment #249 for details, and #275 for a real example use-case.

jweowu’s picture

Issue summary: View changes
jackhutton’s picture

Thanks for your patience w me here.
The error was my mistake. I applied the patch using 'patch' vs git this time and that worked for me.
Again, thank you for your work here.

joachim’s picture

Title: entity_access() fails to check node type specific create access » Changed record for: entity_access() fails to check node type specific create access
Status: Fixed » Active

This surely needs a change record.

dropfen’s picture

damn, this issue was created on "September 10, 2012" ...
there are more then 300 comments on it, and more then 1000 men hours I think.

WTF??? Let it in beta and concentrate on real world problems, like critical issues in Drupal8 ;))
Sorry if made someone angry :)

JvE’s picture

This surely needs a change record.

@joachim: why?

1. This is not an issue for Drupal core, but a contrib module.
2. This is a bugfix with no API changes or additions.

joachim’s picture

Title: Changed record for: entity_access() fails to check node type specific create access » Change notice: entity_access() fails to check node type specific create access

> 1. This is not an issue for Drupal core, but a contrib module.

Contrib modules should use change notices too: https://drupal.org/list-changes/entity

> 2. This is a bugfix with no API changes or additions.

It's a big change to the way the API is intended to be used. Contrib (or custom) modules that make use of it may need to change their code.

For instance, if you have a custom entity and you've implemented the entity access callback, you hitherto probably had some special handling for the 'create' op. This needs to be changed, as other modules that call entity_access() will now pass in a new entity for that op.

JvE’s picture

What would the change notice look like?

  • Previously the wrong way of checking node create access with entity_access would generate a PHP Notice. entity_access('create', 'node', 'page', $account); Now it throws an EntityMalformedException('Permission to create a node was requested but no node type was given.').
  • Previously the correct way of checking node create access with entity_access would generate a PHP Notice.
     $node = entity_create('node', array('type' => 'page'));
      $access = entity_access('create', 'node', $node, $account); 

    Now it works as expected.

Andre-B’s picture

just adding that this patch combined with the latest dev version(2013-Oct-01) fixes the notices and (now) exceptions for references_dialog. This Project has an issue opened for a new release as well: #2171515: Create new release maybe we can find other projects that have known problems to be listed/mentioned in the change notice/ update notice?

ifernando’s picture

With entity-7.x-1.3 I found that sometimes the $node parameter is a string and $node-> fails.

I solved it this way:

 function entity_metadata_no_hook_node_access($op, $node = NULL, $account = NULL) {
   // First deal with the case where a $node is provided.
-  if (isset($node)) {
+  if (isset($node->nid)) {
     if ($op == 'create') {
       if (isset($node->type)) {
         return node_access($op, $node->type, $account);
ifernando’s picture

FileSize
597 bytes
ifernando’s picture

I found that with entity-7.x-1.3 sometimes $node does not have an object (but a string).

I solved it with the attached patch:

entity-callback_0.patch

joachim’s picture

Title: Change notice: entity_access() fails to check node type specific create access » entity_access() fails to check node type specific create access
Status: Active » Fixed
jweowu’s picture

ifernando: $node is not allowed to be a string, so the code which is calling it that way is at fault. Please read the issue description. Revert your own patch. Use the patch in #308 to figure out where the bug originated.

nicxvan’s picture

319 works for me, so if 321 needs to happen, how can I help.

jweowu’s picture

nicxvan: well don't use #319 for starters; that's going to break code which is using the API correctly.

Just start with the patch at #308, and follow the links to the older comments for details on the extra logging it provides.

Anonymous’s picture

I got the following error with entity-7.x-1.3 and entity reference 7.x-1.1 EntityMalformedException: Permission to create a node was requested but no node type was given. in entity_metadata_no_hook_node_access().
Patch #319 made it work. Thanks ifernando!

jweowu’s picture

JvE’s picture

@michamilz: do not use patch #319. It is very wrong and breaks things badly without you noticing.

If you get "EntityMalformedException: Permission to create a node was requested but no node type was given" then some other module has a bug and needs to be fixed. If you can't figure out which module it is you can apply patch #308 to find out in the logging.

drupalok’s picture

i know it's not the fault of entity, but could you include that patch #308 in your dev release, so everyone can check whats wrong?

aitala’s picture

So after applying the #308 patch, I get... so where is the issue?

String citation_author passed to entity_access() instead of a node object. Stack data follows:
Array
(
    [file] => sites/all/modules/contrib/entity/entity.module
    [line] => 654
    [function] => entity_metadata_no_hook_node_access
    [args] => Array
        (
            [0] => create
            [1] => citation_author
            [2] => 
            [3] => node
        )

)
Array
(
    [0] => entity_metadata_no_hook_node_access() at /sites/all/modules/contrib/entity/entity.module:654
    [1] => entity_access() at /sites/all/modules/contrib/references_dialog/references_dialog.dialog_widgets.inc:264
    [2] => references_dialog_entityreference_link_helper() at /sites/all/modules/contrib/references_dialog/references_dialog.dialog_widgets.inc:244
    [3] => references_dialog_entityreference_add_link() at /sites/all/modules/contrib/references_dialog/references_dialog.module:312
    [4] => references_dialog_process_widget() at //includes/form.inc:1879
    [5] => form_builder() at /includes/form.inc:1871
    [6] => form_builder() at /includes/form.inc:1871
    [7] => form_builder() at /includes/form.inc:1871
    [8] => form_builder() at /includes/form.inc:1871
    [9] => form_builder() at /includes/form.inc:850
    [10] => drupal_process_form() at /includes/form.inc:380
    [11] => drupal_build_form() at /includes/form.inc:131
    [12] => drupal_get_form() at /modules/node/node.pages.inc:78
    [13] => node_add()
    [14] => call_user_func_array() at /includes/menu.inc:517
    [15] => menu_execute_active_handler() at /index.php:21
)
fearlsgroove’s picture

@aitala:
References Dialog

see #1780626: Entity Reference incorrectly calls entity_access(). Looks like you should be able to resolve by updating to -dev for that module.

emmonsaz’s picture

I tried #308 and other suggestions without luck. I finally hacked a solution by changing modules/entity/modules/callbacks.inc lines 669-670 to:

$default_revision = (isset($node->nid)) ? node_load($node->nid) : NULL;
if ($node !== FALSE && $node->vid !== $default_revision->vid) {
jweowu’s picture

emmonsaz: $node must be a node object and unless $op == 'create', the node must have a nid.

(It's right there in the @param $node header comment.)

All you are doing is masking a symptom of a bug in some other module (and possibly creating a new one in the process), instead of finding and fixing the cause of the problem.

This issue is also specifically about the 'create' operation (as per the issue title), which is why the suggestions here don't help you -- you're looking in the wrong issue. (But also in the wrong project, as the bug is in the calling code, not in entity API).

That all said, you can easily adapt the code added by #308 to log the debug data in the situation you are looking at. I suggest doing that, and raising an issue in the appropriate project's issue queue.

ladybug_3777’s picture

Wow there has been so much discussion on this topic and so many patches suggested that I'm not sure where this ended up. Is it safe to assume that the correct patch to apply is this one? https://drupal.org/files/entity-entity_access-1780646-191.patch?? Is that the official patch that made this issue move to the "resolved" status? (Sorry it's hard to sort through over 330 posts especially when it looks like there is still discussions going on)

ladybug_3777’s picture

OH! I think I just answered my own question and 191 IS the patch that was officially accept.

#301
Posted by fago on January 6, 2014 at 4:43pm
Status: Reviewed & tested by the community » Fixed
I don't think it makes sense to commit debugging-aids, the exception should be enough to point developers to the problem. Also the "legacy-usage" wasn't indicated to be supported in anyway, so this was just unsupported usage we don't have to account for either.

#191 gets this right and looks good to me. I corrected some minor coding style stuff and committed it - thanks everyone!

jweowu’s picture

ladybug_3777: that's correct, and furthermore that fix has been released in 7.x-1.3 ( https://drupal.org/node/2169589 ).

Itangalo’s picture

Itangalo’s picture

If anyone else is using References Dialog, it is worth knowing that the module incorrectly calls entity_access().
A patch is available at issue #1780626: Entity Reference incorrectly calls entity_access() (added as a related issue).

Status: Fixed » Closed (fixed)

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

electrokate’s picture

Itangalo, I just wanted to say THANK YOU SO MUCH for posting your last post about References Dialog. Without it I might not have found that patch, since the error message doesn't say Reference Dialog. My Drupal site and editors are happy again. Thanks a bunch!

kopeboy’s picture

Version: 7.x-1.x-dev » 7.x-1.5
Status: Closed (fixed) » Needs review

Guys has this been fixed in a stable release?

I am getting this error every time I use a 'Entity Reference: Referencing entity' relationship in Views.

Notice: Trying to get property of non-object in entity_metadata_user_access() (linea 712 di /srv/bindings/8036422060ee456b87e0ee13c0f08bad/code/sites/all/modules/entity/modules/callbacks.inc).

The result is I get correct View result rows, but can't use VBO in it (admin can, other users can't).

JvE’s picture

Version: 7.x-1.5 » 7.x-1.x-dev
Status: Needs review » Closed (fixed)

@kopeboy: this issue is fixed
Your error is about entity_metadata_user_access(), not entity_metadata_node_access().