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 thatentity_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.
Comment | File | Size | Author |
---|---|---|---|
#308 | entity-node_access-1780646-308.diagnostic.patch | 2.23 KB | jweowu |
#191 | entity-entity_access-1780646-191.patch | 12.41 KB | Mile23 |
Comments
Comment #1
Adam S CreditAttribution: Adam S commentedI've included a patch.
Comment #2
Adam S CreditAttribution: Adam S commentedCan I please have this patch review? I might be the only one with this problem but it's still a bug.
Comment #3
Silicon.Valet CreditAttribution: Silicon.Valet commentedI don't see any obvious problems and this clears a notice for me.
Comment #4
Jorrit CreditAttribution: Jorrit commentedWorks fine, thanks!
Comment #5
MXTI can confirm that the patch resolve the issue.
Can this be committed please?
Thank you very much!
Comment #6
wiifm+1, was having the same issue as described in #1780626: Entity Reference incorrectly calls entity_access() - this patch fixed it.
Comment #7
Jorrit CreditAttribution: Jorrit commentedThe 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:
Comment #8
weri CreditAttribution: weri commentedComment #7 is correct. I generated a new patch for that.
Comment #9
rwilson0429 CreditAttribution: rwilson0429 commentedWhen 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.
Comment #10
rogical CreditAttribution: rogical commentedpatch 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.
Comment #11
MXTThe 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
Comment #12
head CreditAttribution: head commentedComment #13
bomarmonk CreditAttribution: bomarmonk commentedThis fixed my issue, but the patch is not committed to head yet (the development version is dated December 26th).
Comment #14
Richard Moger CreditAttribution: Richard Moger commented#8 worked for me. thx.
Comment #15
sergiu.savva CreditAttribution: sergiu.savva commentedwhen Drupal call the next code?
line 715:
Comment #16
sergiu.savva CreditAttribution: sergiu.savva commented#7 and #8 worked for me.
I propose an alternative, to check if node exist.
Comment #17
sandu.camerzan CreditAttribution: sandu.camerzan commented#16: entity-node-access-1780646-16.patch queued for re-testing.
Comment #18
bkmatwa CreditAttribution: bkmatwa commentedThank you 'sergiu.savva' your proposed alternative works perfectly.
Comment #19
Itangalo CreditAttribution: Itangalo commentedPatch in #16 works like a charm. Thanks!
Comment #20
wrodenbusch CreditAttribution: wrodenbusch commentedsergiu.savva's patch works for me. Thanks!
Comment #21
ptitwolfy CreditAttribution: ptitwolfy commentedNot 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
Comment #22
stevieb CreditAttribution: stevieb commentedpatch in #8 works here
Comment #23
timbrandin CreditAttribution: timbrandin commented#16 works!
Comment #24
eiriksm#8 works for me
Comment #25
Itangalo CreditAttribution: Itangalo commentedI 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.)
Comment #26
Itangalo CreditAttribution: Itangalo commentedOk, 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.
Comment #27
gmclelland CreditAttribution: gmclelland commentedPatch in #26 solves the problem for me.
Comment #28
jpstrikesback CreditAttribution: jpstrikesback commentedTesting #26 nothing has blown up yet :)
Comment #29
magicmirror CreditAttribution: magicmirror commented#26 worked for me so far as well, thanks
Comment #30
sergiu.savva CreditAttribution: sergiu.savva commentedHi Itangalo,
node_access() function already includes validation for $op variable
file: modules/node/node.module
line: 2882
We don't need to check $node variable type, because this function is here
file: modules/node/node.module
line: 2895
All these access checks are in the function node_access().
Why not use directly node_access() ?
Comment #32
sergiu.savva CreditAttribution: sergiu.savva commented#30: entity-node-access-callback-1780646-30.patch queued for re-testing.
Comment #34
Itangalo CreditAttribution: Itangalo commented@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.
Comment #35
Rajab Natshah CreditAttribution: Rajab Natshah commentedHi,
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.
Comment #36
knorbi CreditAttribution: knorbi commented#30: entity-node-access-callback-1780646-30.patch queued for re-testing.
Comment #38
betawavetom CreditAttribution: betawavetom commented#26 worked like a charm. Thank you!
Comment #39
sergiu.savva CreditAttribution: sergiu.savva commentedI'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?
For Inline Entity Form it work ok.
Please test new patch.
Comment #40
sergiu.savva CreditAttribution: sergiu.savva commentedComment #41
JvE CreditAttribution: JvE commentedMarked #1879714: Trying to get property of non-object in entity_metadata_no_hook_node_access() as duplicate of this.
Comment #42
philipz CreditAttribution: philipz commented#39 applied cleanly and worked for me. Thank you!
Comment #43
drupov CreditAttribution: drupov commentedYeah, patch from #39 worked for me too!
Comment #44
Itangalo CreditAttribution: Itangalo commented@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.
Comment #45
sergiu.savva CreditAttribution: sergiu.savva commentedHi Itangalo,
Error is excluded.
try this one:
result well be :
node is new
explanation of code:
Comment #46
liza CreditAttribution: liza commentedconfirming same situation: #8 is the only patch that worked for me.
Comment #47
Itangalo CreditAttribution: Itangalo commented@sergiu.savva: You're absolutely right. Thanks for making me see this.
sergiu.savva++
Comment #48
willhowlett CreditAttribution: willhowlett commented#8 worked for me
Comment #49
kaizerking CreditAttribution: kaizerking commentedok 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).
Comment #50
cedewey#39 worked for me.
Comment #51
sergiu.savva CreditAttribution: sergiu.savva commentedHi kaizerking,
Can you to describe workflow ?
You have field collection field and inline entity form, how they are interfere?
Comment #52
kaizerking CreditAttribution: kaizerking commentedCreate 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
Comment #53
jyee CreditAttribution: jyee commented#39 works for me as well.
Assuming that #49 is an issue with Field Collection and not Entity, this should be marked as RTBC.
Comment #54
kaizerking CreditAttribution: kaizerking commentedYes probably it should be marked RTBC
Comment #55
froboy#39 worked for me too. As it seems this has been tested now, I'm going to mark it as so.
Comment #56
fonant CreditAttribution: fonant commentedPatch #39 worked here.
Comment #57
sergiu.savva CreditAttribution: sergiu.savva commentedone more patch )
review test cases for entity node access, and result is a new patch
Comment #58
Yaazkal CreditAttribution: Yaazkal commented#57 works great.
Comment #59
afmdsouza CreditAttribution: afmdsouza commentedPatch #57 works for me too.
Comment #60
spencerthayer CreditAttribution: spencerthayer commentedWhat am I doing wrong? Never had this happen with a patch before.
Comment #61
fagoWhat 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.
Comment #62
Anselm CreditAttribution: Anselm commented#57 Solved the problem!
thanks!
Comment #63
Anselm CreditAttribution: Anselm commentedComment #64
Dave ReidIt'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.
Comment #65
videographics CreditAttribution: videographics commentedI can confirm #64. I'm constantly seeing the same thing on the forms that use inline_entity_form module.
Comment #66
ladybug_3777 CreditAttribution: ladybug_3777 commentedThe 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?
Comment #67
jyee CreditAttribution: jyee commentedYou 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).
Comment #68
jyee CreditAttribution: jyee commentedThe 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?
Comment #69
Rajab Natshah CreditAttribution: Rajab Natshah commentedThanks sergiu.savva your patch at #57 works well.
Comment #70
Mile23Redo of #57 with no whitespace or formatting errors.
Full authorship credit to sergiu.savva please. :-)
Comment #71
Mile23Turns 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.
Comment #72
Mile23Hah.
Wrongy-wrong patch.
Let's try again.
Comment #73
Mile23Comment #75
Mile233rd time a charm?
::hangs head::
Comment #77
Mile23I'm a pro.
Really.
Also: Same problem as #1237014: Notices in entity_metadata_user_access() when loading an anonymous node
Comment #78
Mile23Comment #79
burgs CreditAttribution: burgs commented#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.
Comment #80
sergiu.savva CreditAttribution: sergiu.savva commentedHI 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.
Comment #81
operations CreditAttribution: operations commentedI 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).
Comment #82
Dave ReidI 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.
Comment #83
Dave ReidAlso we're missing tests to confirm this is fixed and it won't regress in the future.
Comment #84
Mile23OK, 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.
Comment #86
kjbowrin CreditAttribution: kjbowrin commentedWhy wasn't this in 1.1? This needs a fix.
Comment #87
Mile23Actually, 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.
Comment #88
Mile23So, 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.
Comment #89
Dave ReidI 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.
Comment #90
Mile23You'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.
Comment #91
jojonaloha CreditAttribution: jojonaloha commentedI'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:
Comment #92
JvE CreditAttribution: JvE commentedI 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:
to:
?
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?
Comment #93
Mile23I'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.
Comment #93.0
tstoecklerCreated a real issue summary
Comment #94
tstoecklerI 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.
Comment #95
Mile23Thanks, tstoeckler.
Comment #96
sergiu.savva CreditAttribution: sergiu.savva commented#90: entity-create_test-1780646-90.patch queued for re-testing.
Comment #97
sergiu.savva CreditAttribution: sergiu.savva commentedWe should let node_access() handle most of the default cases and first check revision access then check node access.
Comment #98
fago@tstoeckler: Thanks for the updated summary!
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.
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.
Yes. That can be discussed, but surely is not a critical bug.
No, see above example.
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.
Comment #99
Mile23Thanks for the clarification, @fago.
Try this on for size.
Comment #100
jamix CreditAttribution: jamix commented#99 worked over here, thanks.
Comment #101
henk CreditAttribution: henk commentedI have the same problem when adding new node (in multiple widget) with Inline Entity Form, after change:
to
patch start work.
Comment #103
wismoyo CreditAttribution: wismoyo commented#8: entity-node-access-1780646-8.patch queued for re-testing.
Comment #104
wismoyo CreditAttribution: wismoyo commented#101: entity-entity_node_access-1780646-100.patch queued for re-testing.
Comment #106
sergiu.savva CreditAttribution: sergiu.savva commentedHi 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.
2) For what to use the user_access if this makes node_access ?
path: modules/node/node.module
line: 3002
node.module
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.
Comment #107
Mile23serigu: 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:
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.
Comment #108
operations CreditAttribution: operations commentedHi 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.
Comment #109
Mile23re: #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.
Comment #110
Offlein CreditAttribution: Offlein commentedI 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.
Comment #111
sergiu.savva CreditAttribution: sergiu.savva commentedHi Offlein,
Try patch #97, it must work.
Comment #112
Mile23Offlein: You might check here: #1990064: Invoke entity_access() properly for create operations
Comment #113
Sinan Erdem CreditAttribution: Sinan Erdem commentedPatch #97 works for me.
Comment #114
bojanz CreditAttribution: bojanz commentedI've committed #1990064: Invoke entity_access() properly for create operations to IEF, now Entity API just needs to fix its node callback.
Comment #115
Offlein CreditAttribution: Offlein commentedMile23, you're right -- that was exactly what was necessary. Thank you!
Comment #116
C-LogemannApplied patch of #107 together with actual release of IEF and my errors are gone. I think everything is fine.
Comment #117
philipz CreditAttribution: philipz commentedI did the same as @C_Logemann and errors are gone too.
Comment #118
mbarcelo CreditAttribution: mbarcelo commentedI did the same as @C_Logemann and @philipz errors are gone too.
Comment #119
Cauliflower CreditAttribution: Cauliflower commentedI also can confirm the errors are gone.
Comment #120
dropfen CreditAttribution: dropfen commentedSorry, but the Issue is not solved.
After I applied the patch #107 , the reference dialog widget had no "Add - Button" more.
Comment #121
Offlein CreditAttribution: Offlein commented@dropfen did you see #110 and #112?
Comment #122
dropfen CreditAttribution: dropfen commentedI 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
Comment #123
Mile23OK, 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. :-)
Comment #124
JvE CreditAttribution: JvE commented@Mile23: For Inline Entity Form there is no need for dev or patch, release 7.x-1.2 contains the proper API usage.
Comment #125
dropfen CreditAttribution: dropfen commented@Mile23, thanks for the reaction ;)
And of course thanks for patches, to all contributers.
dropfen
Comment #126
azinck CreditAttribution: azinck commented#107 works well for me. +1 RTBC
Comment #127
mohamadaliakbari CreditAttribution: mohamadaliakbari commented#107 works well for me, too.
Comment #128
dww#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!
Comment #129
MXTI can confirm that #107 resolves my issues.
Thank you
Comment #130
JvE CreditAttribution: JvE commentedI do not understand why the conditions are inverted but the descriptions are not.
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
then all calling code is responsible for adhering to those demands and no checking should be done.
So functionally the code becomes:
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?
Comment #131
JvE CreditAttribution: JvE commentedComment #132
sergiu.savva CreditAttribution: sergiu.savva commented#97: entity-entity_node_access-1780646-97.patch queued for re-testing.
Comment #133
Nikro CreditAttribution: Nikro commentedThis 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.
Comment #134
Mile23Quoting 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.
Comment #135
JvE CreditAttribution: JvE commented@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.
This should return TRUE for $accounts which may edit any node.
After applying the patch from #107 these situations fail.
Comment #136
JvE CreditAttribution: JvE commentedAttached patch contains a fix for the bug in Entity API as well the additional tests.
Comment #137
sergiu.savva CreditAttribution: sergiu.savva commented#136 works well for me, for inline entity form use dev version [7.x-1.2+2-dev]
Comment #138
vasikeindeed the #136 seems to work -> RTBC.
Comment #139
a.milkovskyHere is my fix of
Comment #140
JeroenTAfter 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).
Comment #141
JeroenTSorry I accidentally deleted the module entityreference. Patch #136 works for me!
Comment #142
a.milkovskyso, RTBC
Comment #143
JvE CreditAttribution: JvE commentedTo avoid confusion: #136 is RTBC.
#139 does not solve anything, it just hides the error.
Comment #144
Mile23Sure wish we could get some movement on this. I like #107, personally. :-)
Comment #145
JvE CreditAttribution: JvE commentedHaha.
#107 breaks existing tests and removes functionality so that's a no-go.
Comment #146
Mile23Pretty sure it does neither, JvE.
Which tests does it break?
Comment #147
JvE CreditAttribution: JvE commentedFrom entity_access():
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:
Comment #148
Mile23@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.
Comment #149
Mile23Changing status. Again. :-)
Comment #150
JvE CreditAttribution: JvE commentedThis 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:
You propose to change this into:
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.
Comment #151
Mile23Resetting the status because clearly there is contention about this, and more than a few patches.
Please remain civil.
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.
#98 seems pretty clear to me. Also, it would be great if Fago would weigh in on this.
Comment #152
JvE CreditAttribution: JvE commentedPlease 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.
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.
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.
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.
Comment #153
Mile23Either 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.
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.
....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.
Comment #154
JvE CreditAttribution: JvE commentedWe'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.
Comment #155
malberts CreditAttribution: malberts commentedSo far the patch in #136 works for me.
Comment #156
Offlein CreditAttribution: Offlein commentedI'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!
Comment #157
Mile23Again: 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.
Comment #158
Dave ReidThe documentation for entity_access() explicitly states:
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.
Comment #159
JvE CreditAttribution: JvE commentedThe documentation for entity_access() explicitly states:
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.
Comment #160
jpstrikesback CreditAttribution: jpstrikesback commentednow now, no sodding flak, that is entirely unnecessary and possibly painful ;)
@fago, do you have an opinion?
Comment #161
fagoAs 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
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.
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.
Comment #162
malberts CreditAttribution: malberts commentedWhat would be the preferred patch or patch direction then? #136 or #148? Or something completely different?
Comment #163
Mile23@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.
Comment #164
JvE CreditAttribution: JvE commented@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.
Comment #165
Mile23Well then let's get on with the committing, or the whatever has to happen before the committing. Please. :-)
Comment #166
sergiu.savva CreditAttribution: sergiu.savva commentedI'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.
Comment #168
dropfen CreditAttribution: dropfen commentedclean some coding standards and add return on line 338
Comment #169
dropfen CreditAttribution: dropfen commentedComment #171
dropfen CreditAttribution: dropfen commentednew try
Comment #173
JvE CreditAttribution: JvE commentedPatches #166, #168 and #171 do not solve the problem, they only break more.
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
Comment #174
dallas_tech CreditAttribution: dallas_tech commentedApplied #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.
Comment #175
dallas_tech CreditAttribution: dallas_tech commented#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.
Comment #176
dallas_tech CreditAttribution: dallas_tech commentedTried # 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:
I have applied all updates and patches.
Comment #177
malberts CreditAttribution: malberts commented@dallas_tech, try with the -dev versions of ER and IEF to see if they make a difference.
Comment #178
Mile23@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.
Comment #179
lauriiiSame 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.
Comment #180
dropfen CreditAttribution: dropfen commented@lauriii
https://drupal.org/node/1780646#comment-7564907
@dallas_tech
I'm using the #97 with entityreference_dialog_widget, and it works.
Comment #181
edutrul CreditAttribution: edutrul commentedI tried patch #97 and works like a charm! thanks!
Comment #182
Sinan Erdem CreditAttribution: Sinan Erdem commentedI applied the patch on #97 to version 7.x-1.1 and the errors were gone. So far, all seems to work ok. Thanks.
Comment #183
fago#136 looks already pretty good, let's proceed with this one. Here some small remarks:
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.
Should be removed.
Let's throw an EntityMalformedException as entity_extract_ids() does.
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.
Comment #184
Mile23Thanks for the input, @fago.
@JvE: Rock on. :-)
Comment #185
joseph.muennich CreditAttribution: joseph.muennich commentedDid 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.
Comment #186
C-Logemann@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.
Comment #187
joseph.muennich CreditAttribution: joseph.muennich commentedThanks 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.
Comment #188
joachim CreditAttribution: joachim commentedThere'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?
Comment #189
Mile23I 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.
Comment #191
Mile23Woops, forgot to uncomment.
Comment #192
yannickooCool, #191 fixed all these errors which I had with Inline Entity Form module :)
Comment #193
Mile23How many needed for RTBC? :-)
Comment #194
yannickooIn 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.
Comment #195
nicholas.alipaz CreditAttribution: nicholas.alipaz commentedAs yannickoo said, wfm, fixes all issues in Inline Entity Form module.
Comment #196
acrosmanI 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.
Comment #197
yogaf CreditAttribution: yogaf commentedAs yannickoo said, wfm, fixes all issues in Inline Entity Form module.
Comment #198
Rob_Feature CreditAttribution: Rob_Feature commentedI'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...
Comment #199
Mile23So 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()
Comment #200
Frippuz CreditAttribution: Frippuz commentedI 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?
Comment #201
shawn_smiley CreditAttribution: shawn_smiley commentedThe patch in #191 worked for me with both releases 1.1 and 1.2.
Comment #202
wodenx CreditAttribution: wodenx commentedI 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.
Comment #203
jrbeemanI'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.
Comment #204
yannickoo@#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.
Comment #205
acrosmanI 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):
Comment #206
metametamind CreditAttribution: metametamind commentedI applied patch #202 and still (intermittently, which is odd) receive this error:Patch #202 works correctly. Above bug report was due to user error.
Comment #207
Frippuz CreditAttribution: Frippuz commentedMy list of modules in use is pretty long. I will try to locate the faulty one.
Comment #208
MXTI 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
Comment #209
acrosmanThe issue I'm seeing with the patch in #202 (reported in #205) is related to the custom form alters.
Comment #210
acrosmanI'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:
#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.
Comment #211
pmichelazzoUnfortunately 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
Comment #212
jpstrikesback CreditAttribution: jpstrikesback commented<meep>
I'm still using #8 and loving it...tho I haven't updated IEF for a while :)</meep>
Comment #213
alesr CreditAttribution: alesr commentedI 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.
Comment #214
metametamind CreditAttribution: metametamind commentedBwhahaha. 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.
Comment #215
MXT@metametamind: please, edit your #206 writing a note that the patch now works for you.
Comment #216
Nileschip CreditAttribution: Nileschip commented#1: 1780646-entity-api-node-access.patch queued for re-testing.
Comment #217
pmichelazzoHi 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:
Any clue to solve it?
Regards
Comment #218
digitaldonkey CreditAttribution: digitaldonkey commentedComment #219
digitaldonkey CreditAttribution: digitaldonkey commented#1: 1780646-entity-api-node-access.patch queued for re-testing.
Comment #220
jweowu CreditAttribution: jweowu commentedI 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.
Comment #221
Kristen Pol#202 got rid of the entity_metadata_no_hook_node_access errors for me when using Inline Entity Form. Thanks!
Comment #222
jweowu CreditAttribution: jweowu commentedI'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?
Is this still in line with #136/#191 ?
Comment #223
Mile23It 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.
Comment #224
jweowu CreditAttribution: jweowu commentedThanks 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()
andEntityStructureWrapper::propertyAccess()
), those changes would need to be re-rolled.Comment #225
wodenx CreditAttribution: wodenx commentedAs I said in the comment:
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.
Comment #226
Mile23wodenx, 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.
Comment #227
jweowu CreditAttribution: jweowu commentedAnd 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?
Comment #228
sam.spinoy@gmail.com CreditAttribution: sam.spinoy@gmail.com commentedJust used the patch in #191, does the trick for me.
Comment #229
wodenx CreditAttribution: wodenx commentedNothing 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.
Comment #230
Ares_ekb CreditAttribution: Ares_ekb commentedI've got this error during editing a node with https://drupal.org/project/inline_entity_form widget enabled.
Comment #231
elvis2 CreditAttribution: elvis2 commentedDitto #230
Comment #232
lautaro.pastorini@gmail.com CreditAttribution: lautaro.pastorini@gmail.com commentedThe same as #230
Please, let us know if there is a solution.
Thanks in advance.
Comment #233
JurgenR CreditAttribution: JurgenR commentedDitto #230
Comment #234
Jarek Polok CreditAttribution: Jarek Polok commentedSame problem here, I've checked all the mentioned patch proposals in this thread .. no success so far unfortunately ... (#217)
Comment #235
MilanT CreditAttribution: MilanT commentedAlso having the same issue as #230.
Comment #236
yannickooComment #237
msh2050 CreditAttribution: msh2050 commentedI have the same problem
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
Comment #238
wodenx CreditAttribution: wodenx commented@msh2050 please try the patch at #191
Comment #239
msh2050 CreditAttribution: msh2050 commentedDear wodenx thanks alot...
please... for which file I should apply the patch??
best regard..
Comment #240
digitaldonkey CreditAttribution: digitaldonkey commentedAlso having the same issue as #230 using inline entity form.
Comment #241
msh2050 CreditAttribution: msh2050 commentedI 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
Comment #242
jweowu CreditAttribution: jweowu commentedmsh2050: read https://drupal.org/project/entity/git-instructions
Comment #243
msh2050 CreditAttribution: msh2050 commentedthanks 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
Comment #244
jweowu CreditAttribution: jweowu commentedmsh2050: in that case, start here: https://drupal.org/node/60108
Comment #245
wodenx CreditAttribution: wodenx commented@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?
Comment #246
MilanT CreditAttribution: MilanT commentedOk 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.
Comment #247
wodenx CreditAttribution: wodenx commented@MilanT do you think those warnings are related to Entity API? Can you post them?
Comment #248
msh2050 CreditAttribution: msh2050 commentedI 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
Comment #249
jweowu CreditAttribution: jweowu commentedIt 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:
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 beentity_metadata_no_hook_node_access()
,[1]
should beentity_access()
, and we would expect to identify the source of the problem -- the function which calledentity_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.Comment #250
jweowu CreditAttribution: jweowu commentedGiven 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?
Comment #251
Mile23That'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.
Comment #252
Alexander Allen CreditAttribution: Alexander Allen commentedThe 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 :-)
Comment #253
wodenx CreditAttribution: wodenx commentedThis 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.
Comment #254
elvis2 CreditAttribution: elvis2 commented+1 on #253
Comment #255
Mile23Please ping @fago with your decision. :-)
Comment #256
jweowu CreditAttribution: jweowu commentedCommitting 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.)
Comment #257
arosboro CreditAttribution: arosboro commentedThanks 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.
Comment #258
edutrul CreditAttribution: edutrul commentedPatch #202
I'm using entity 7.x-1.2 Worked perfectly for me
thanks!
Comment #259
jweowu CreditAttribution: jweowu commentededutrul: #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.
Comment #260
Mile23Please change status to RTBC if you've applied and tested a patch. Be sure and specify which patch you've tested.
Comment #261
jweowu CreditAttribution: jweowu commentedWell "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.
Comment #262
drclaw CreditAttribution: drclaw commentedCurrently testing the patch in #256 and it appears to be working so far. I hadn't tested the one in #249, however.
Comment #262.0
drclaw CreditAttribution: drclaw commentedFix markup
Comment #263
hass CreditAttribution: hass commented#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.
Comment #264
elvis2 CreditAttribution: elvis2 commentedUnfortunately, 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?
Comment #265
elvis2 CreditAttribution: elvis2 commentedDisregard comment #264, the access issue was resolved once I used patch #256 and upgraded the brightcove module.
Comment #266
mariusm CreditAttribution: mariusm commentedPlease update patch 256# for last dev.
Thanks
Comment #267
alexweber CreditAttribution: alexweber commentedThe patch in #256 applies and works perfectly for me using last dev.
Comment #268
mariusm CreditAttribution: mariusm commentedYes, it's ok
Comment #269
acrosmanI 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:
Comment #270
elvis2 CreditAttribution: elvis2 commented@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?
Comment #271
acrosmanIt 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).
Comment #272
jweowu CreditAttribution: jweowu commentedThose
_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.
Comment #273
jweowu CreditAttribution: jweowu commentedI'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.
Comment #274
alexweber CreditAttribution: alexweber commentedLooks great to me!
Comment #275
acrosmanThe 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:
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.
Comment #276
jweowu CreditAttribution: jweowu commentedThanks acrosman; that's exactly how this is intended to work :) (and it's good to hear that References Dialog has already fixed it.)
Comment #277
wodenx CreditAttribution: wodenx commented+1 for #273. I'm gonna go out on a limb and set to RTBC
Comment #278
wodenx CreditAttribution: wodenx commentedComment #279
antoinetooley CreditAttribution: antoinetooley commented#273 did it for me. Thanks!
Comment #280
jweowu CreditAttribution: jweowu commentedOkay, 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.
Comment #281
jweowu CreditAttribution: jweowu commentedComment #282
jweowu CreditAttribution: jweowu commentedComment #283
jweowu CreditAttribution: jweowu commentedComment #284
pixelwhip CreditAttribution: pixelwhip commentedI 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!
Comment #285
Mile23Will we make it to #300? :-)
Comment #286
RogerRogers CreditAttribution: RogerRogers commentedCouldn'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.
Comment #287
jweowu CreditAttribution: jweowu commentedRogerRogers: #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?
Comment #288
RogerRogers CreditAttribution: RogerRogers commentedThanks 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.
Comment #289
RogerRogers CreditAttribution: RogerRogers commentedOh 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. :/
Comment #290
jweowu CreditAttribution: jweowu commentedComment #291
jweowu CreditAttribution: jweowu commentedComment #292
jweowu CreditAttribution: jweowu commentedRogerRogers: 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
Comment #293
nicxvan CreditAttribution: nicxvan commentedIs 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.
Comment #294
DamienMcKenna+1 for this patch fixing the issues, and no noticeable side effects.
Comment #295
jackhutton CreditAttribution: jackhutton commentedconfused abit here: does the patch work with the dev version 7.x-1.x-dev ?
I'm getting a series of errors :
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.
Comment #296
DamienMcKenna@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.
Comment #297
DamienMcKennaFor the record, here's what happens when I apply the patch:
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.
Comment #298
dalguete CreditAttribution: dalguete commented273: entity-node_access-1780646-273.diagnostic.patch queued for re-testing.
Comment #299
randell CreditAttribution: randell commentedComment #300
DamienMcKenna@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.
Comment #301
fagoI 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!
Comment #302
jpstrikesback CreditAttribution: jpstrikesback commentedThis calls for champagne!
Comment #303
Mile23OMG. :-)
Comment #304
drclaw CreditAttribution: drclaw commented^^ What Mile23 said =)
Comment #305
jweowu CreditAttribution: jweowu commentedfago: 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 :)
Comment #306
jweowu CreditAttribution: jweowu commentedUpdated summary to reflect the commit.
Comment #307
JvE CreditAttribution: JvE commentedWow, after 6.5 months my patch finally landed!
Thanks everyone for sticking with it.
Comment #308
jweowu CreditAttribution: jweowu commentedProviding 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.
Comment #309
jweowu CreditAttribution: jweowu commentedComment #310
jackhutton CreditAttribution: jackhutton commentedThanks 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.
Comment #311
joachim CreditAttribution: joachim commentedThis surely needs a change record.
Comment #312
dropfen CreditAttribution: dropfen commenteddamn, 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 :)
Comment #313
JvE CreditAttribution: JvE commented@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.
Comment #314
joachim CreditAttribution: joachim commented> 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.
Comment #315
JvE CreditAttribution: JvE commentedWhat would the change notice look like?
Comment #316
Andre-Bjust 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?
Comment #317
ifernando CreditAttribution: ifernando commentedWith entity-7.x-1.3 I found that sometimes the $node parameter is a string and $node-> fails.
I solved it this way:
Comment #318
ifernando CreditAttribution: ifernando commentedComment #319
ifernando CreditAttribution: ifernando commentedI 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
Comment #320
joachim CreditAttribution: joachim commentedChange notice: https://drupal.org/node/2177497
Comment #321
jweowu CreditAttribution: jweowu commentedifernando: $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.
Comment #322
nicxvan CreditAttribution: nicxvan commented319 works for me, so if 321 needs to happen, how can I help.
Comment #323
jweowu CreditAttribution: jweowu commentednicxvan: 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.
Comment #324
Anonymous (not verified) CreditAttribution: Anonymous commentedI 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!
Comment #325
jweowu CreditAttribution: jweowu commentedComment #326
JvE CreditAttribution: JvE commented@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.
Comment #327
drupalok CreditAttribution: drupalok commentedi 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?
Comment #328
aitala CreditAttribution: aitala commentedSo after applying the #308 patch, I get... so where is the issue?
Comment #329
fearlsgroove CreditAttribution: fearlsgroove commented@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.
Comment #330
emmonsaz CreditAttribution: emmonsaz commentedI tried #308 and other suggestions without luck. I finally hacked a solution by changing modules/entity/modules/callbacks.inc lines 669-670 to:
Comment #331
jweowu CreditAttribution: jweowu commentedemmonsaz: $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.
Comment #332
ladybug_3777 CreditAttribution: ladybug_3777 commentedWow 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)
Comment #333
ladybug_3777 CreditAttribution: ladybug_3777 commentedOH! 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!
Comment #334
jweowu CreditAttribution: jweowu commentedladybug_3777: that's correct, and furthermore that fix has been released in 7.x-1.3 ( https://drupal.org/node/2169589 ).
Comment #335
Itangalo CreditAttribution: Itangalo commentedComment #336
Itangalo CreditAttribution: Itangalo commentedIf 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).
Comment #338
electrokate CreditAttribution: electrokate commentedItangalo, 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!
Comment #339
kopeboy CreditAttribution: kopeboy commentedGuys 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.
The result is I get correct View result rows, but can't use VBO in it (admin can, other users can't).
Comment #340
JvE CreditAttribution: JvE commented@kopeboy: this issue is fixed
Your error is about entity_metadata_user_access(), not entity_metadata_node_access().