Recent patch to change array2object() into a cast bring fourth this bug:
users with create new forum topics right are forbiddent to do so.
The problem is in user.module. E.g. node/add page is handled by node_add() function. That function calls node_access('create', $type), where $type is a string, e.g. 'forum'. This is not correct, as node_access() expects node object/array there.
It's weird that it worked before, with array2object().
When node_access('create', 'forum') is called then:
1. array2object('forum') produces 'forum' (a string)
2. (object) 'forum' produces object: class stdClass { var $scalar = 'forum'; }
It worked for 1. because module_invoke(node_get_base($node), 'access', $op, $node);
returned TRUE (didn't checked why).
With 2. that do not return TRUE, and the access fails.
Comment | File | Size | Author |
---|---|---|---|
#9 | node_access-42955_1.diff | 1.56 KB | Cvbge |
#2 | node_access-42955_0.diff | 665 bytes | Cvbge |
Comments
Comment #1
Cvbge CreditAttribution: Cvbge commentedUsers with 'administer nodes' can add new content, others can't.
Comment #2
Cvbge CreditAttribution: Cvbge commentedHere's a proposed patch
Comment #3
Cvbge CreditAttribution: Cvbge commentedComment #4
Cvbge CreditAttribution: Cvbge commentedIn fact this bug was created by the mentioned array2object() patch...
OTOH, there are issues with accessing a string as object ($node->format) with array2object() or accessing not existing object variable with (cast)
Comment #5
chx CreditAttribution: chx commentedThis is a bit weird fix, yet it's working and does not require much code change. Otherwise we would need to fix up the three callers of this, and these type of loops (two of them):
can't really be written in a short manner.
So, I RTC'd this.
Comment #6
Chris Johnson CreditAttribution: Chris Johnson commentednode_access() should not be called with a string as a second argument. It should always be a node, either as an array or an object.
There are 3 places in core where it the API is used incorrectly. It seems like we should fix those 3 places (2 in node.module, 1 in blogapi.module).
There are about a dozen places in contrib/modules in fewer files where the same mistake is made.
It seems like we should just fix core and require correct usage in contrib.
Comment #7
Cvbge CreditAttribution: Cvbge commentedWhat about node_access_type() which would take type as second arg and have similar logic as current node_access()?
Comment #8
Cvbge CreditAttribution: Cvbge commentedOTOH the node_access_type() could only work for 'create' check, so it's better to integrate it with normal node_access() function.
node_access() needs also a doc update.
Comment #9
Cvbge CreditAttribution: Cvbge commentedHere's the patch, not tested.
Comment #10
Cvbge CreditAttribution: Cvbge commentedIt seems the docs for the node_access() function were wrong. The hook_node_access() function has the 'create' $op and string $node described.
Comment #11
Chris Johnson CreditAttribution: Chris Johnson commentedI'm confused. I'm not seeing any hook_node_access() function in core. What am I missing?
Comment #12
Cvbge CreditAttribution: Cvbge commentedI'm sorry, I meant hook_access.
Comment #13
Dries CreditAttribution: Dries commentedI used to be on the other side but now, I come to prefer option #6.
Though, I'm willing to go with the patch if that is considered the cleanest solution.
Comment #14
Cvbge CreditAttribution: Cvbge commentedI currently consider this the best solution. But if someone writes patch for the #6 way, I'll review and *maybe* I'll change my mind (if the patch is really better)
Comment #15
chx CreditAttribution: chx commentedAs create is fundamentally different from others (no nid!) I still like the current approach. How would you fix up the foreaches I quoted above?? Create a $node = array('type' =>$type) just to be able to call up node_access? Let's face it, that's not practical.
Comment #16
Dries CreditAttribution: Dries commentedCommitted to HEAD. Thanks.
Comment #17
Dries CreditAttribution: Dries commentedComment #18
simon rawson CreditAttribution: simon rawson commentedI'm too slow! I was just about to post to say that I thought we should go for the
$node = array('type' =>$type);
solution. It is consistent, at least. Surely it is bad practice to have a function that takes an array/object normally, but takes a string some times.
At the very least the comments need improving! There is not even a mention of 'create' as a possible op.
Comment #19
chx CreditAttribution: chx commentedSimon, we already have such functions. _node_names is one such example.
Comment #20
Cvbge CreditAttribution: Cvbge commentedI've used this approach in my first patch, in #2. I changed it because it creates false "node" object with only type specified for the 'create' op. And the patch would be a bit bigger.
Also, the last patch contains changes that add 'create' $op description.
Comment #21
simon rawson CreditAttribution: simon rawson commented@chx
Thanks for that. I'm learning more about Drupal every day. As a relative new comer, it is often easy to forget that Drupal is a piece of software which has evolved and, as such, sometimes things are always the way you think they might be.
Comment #22
Dries CreditAttribution: Dries commented