Hi @all,
I just mentioned that there is a wrong documentation for the $node argument of hook_access().
The documentation says:
The node on which the operation is to be performed, or, if it does not yet exist, the type of node to be created.
This is the correct documentation for the most cases, but the translation module passes in a full node object for $op = 'create' in translation_node_overview() (File: translation.pages.inc, Line: 49). If you only do a string comparison in your own hook_access() for the "create" operation, you compare the object with your node type string... and this results in a false result
In the node module itself, this is handled like this:
function node_content_access($op, $node, $account) {
$type = is_string($node) ? $node : (is_array($node) ? $node['type'] : $node->type);
// rest of the node access code...
}
So the documentation should say that it might also be a full node object, when the "create" operation is queried.
Cheers
hctom
Comment | File | Size | Author |
---|---|---|---|
#98 | 789484-hook_access_params-4.patch | 1.46 KB | sven.lauer |
#94 | 789484-node-api-94.patch | 1.19 KB | aspilicious |
#88 | doc-followup-hook_node_acces-789484.patch | 1.13 KB | sven.lauer |
#86 | 789484-hook_access_params-3.patch | 1.46 KB | sven.lauer |
#77 | 789484-hook_access_params-2.patch | 1.38 KB | sven.lauer |
Comments
Comment #1
mrhanlon CreditAttribution: mrhanlon commentedJust ran into this myself. I'm not sure which module is responsible, but same thing. Where $node sometimes is the full node object, other times is just the node type as a string.
Comment #2
hctomit is the translation module in core :)
Comment #3
ksenzeeMoving to the documentation queue.
Comment #4
jhodgdonTo me the doc looks correct. It says that the input is either a full node object, or a type string. It is one or the other, correct?
If you feel strongly about this needing to be changed, then please reopen as a Drupal 8 issue, because it should be fixed on
http://api.drupal.org/api/drupal/modules--node--node.api.php/function/ho...
and then have the fix backported to drupal 7/6.
Comment #5
hctomThis issue deals with the Drupal 6 version of the documentation:
http://api.drupal.org/api/drupal/modules--node--node.module/function/nod...
There is no hint for that.
Cheers
hctom
Comment #6
hctomComment #7
jhodgdonIn the D6 hook_access and the D7/8 hook_node_access, the doc says (as you pasted in the original issue report):
The node on which the operation is to be performed, ...
That implies a node object, doesn't it?
And then it goes on to say:
or, if it does not yet exist, the type of node to be created.
If you think this is not clear, please propose a clearer wording that would say what you want it to say? Otherwise, I think the doc is fine...
Comment #8
hctomAh okay... now I understand whar you wanted to say ;-) Yes you are right, that it is almost good to go. But
or, if it does not yet exist, the type of node to be created.
is a little confusing, because you would normally think that the translation node does not already exist, when you hit "Add translation" (as you are about to create that node). Perhaps an add-on like this would be useful:
The node on which the operation is to be performed, or, if it does not yet exist, the type of node to be created. When a node is going to be translated, a full node object is passed for the 'create' operation.
What do you think?
Comment #9
jhodgdonMaybe we should just take out the "or if it doesn't exist" part? Then it would just say it's either a node object (we can add the word object too... although maybe it can be an array? not sure) or a type string?
That sounds reasonable... moving to d8 as it needs to be done there first on hook_node_access, then ported back to d7 hook_node_access and d6 hook_access (sorry, that will delay it getting fixed, but that's how we do things).
Good project for a novice patch contributor...
Comment #10
michaellenahan CreditAttribution: michaellenahan commentedNovice patch contributor here! :)
Does this work:
* @param $node
* The node on which the operation is to be performed, or, a string value
* representing the type of node to be created.
Comment #11
michaellenahan CreditAttribution: michaellenahan commentedComment #12
jhodgdonI think that has too many commas -- the one after "or" should be removed. Thanks!
Comment #13
hctomI still think that
* @param $node
* The node on which the operation is to be performed, or a string value
* representing the type of node to be created.
is still not clear enough. When reading this, I would think that I can do a string comparison for $op = 'create' on $node for the type... but as I mentioned, the full node object is passed in for adding a translation node. Perhaps something like this?
* @param $node
* The node on which the operation is to be performed, or a string value
* representing the type of node to be created. For translation nodes there
* is always a full node object passed.
Comment #14
Lars Toomre CreditAttribution: Lars Toomre commentedThis is a great example where type hinting would be a real help. I would suggest that we use either '@param mixed $node' or '@param object|string $node' depending on what our yupe hinting standard is.
Comment #15
jhodgdonI don't get why this is not clear. It says $node can be an object or a string. How again is this unclear? You just can't assume it's a string if it says it can be an object or a string?
Comment #16
hctom@jhodgdon: But why should we hide very useful information to avoid false results? I thought this is what a documentation is for...
Comment #17
jhodgdonI don't get it, what information are we hiding? You mean your suggestion of "For translation nodes there
is always a full node object passed."? Hmmm... I'm not against adding that information (although it needs to be written in a more grammatical way)... just needs a new patch.
Comment #18
sven.lauer CreditAttribution: sven.lauer commentedI think what @hctom is getting at is that the currently suggested text:
Still suggests (though more weakly then before) that, in case of node creation, the argument will always be a string.
Perhaps a good solution would be something like:
Comment #19
jhodgdonI'm fine with dropping "to be created" from the "type of node". I think that adding a clarification that it's a string only if it's being created will cause the same confusion that is being discussed here, that sometimes $node is an object even if it's being created?
Comment #21
sven.lauer CreditAttribution: sven.lauer commentedYou are right, the additional sentence suffers from just the same problem.
Re-rolled a version of the patch omitting the "to be created" part.
Comment #22
Lars Toomre CreditAttribution: Lars Toomre commentedI never have have used git to create a patch before. Hence, this is my first ever attempt.
This patch duplicates the change in #21 and adds type hinting for the @param and @return directives as I suggested in #14. I am particularly interested to see if this applies correctly and hence passes all tests.
Thanks for bearing with me on this first patch journey.
Comment #23
jhodgdonThe patch in #22 looks pretty good to me (test bot is not done with it but I think it should be fine). One thing I just noticed (not your fault - was in the original function) is that we normally put the @ingroup at the very bottom of the docblock, and we also normally have a blank line before the @param section starts. All of the examples on our standards page do that: http://drupal.org/node/1354, although it may not be explicitly stated as a standard.
So if you could make a new patch with those changes, that would be great! I think the type hinting is a good idea here by the way, and thanks!
Comment #24
Lars Toomre CreditAttribution: Lars Toomre commented@jhodgdon - Thanks for your review and comments.
I am curious about why the test bots did not come back. I wanted to make sure that I was using the correct -p1 format (not understanding what the difference from -p0 is).
I am happy to roll an updated patch as you requested. However, let me ask a couple of questions first:
1) In several places in that file, the @see directives come before the @param directives. Should those be moved in this patch?
2) The coding standards page indicates that there should be no blank line between @see and @ingroup directives with a blank line after the @return directive. Again several places in this file violate that standard. Should those be corrected in this patch?
3) I believe type hinting should be added at some point to this file. Should I do it now or wait until another issue?
I will re-roll the patch once I receive guidance on what should be included. Thanks again.
Comment #25
jhodgdonIf the test doesn't run at all (which appears to be the case), it's because the test bots are having problems. If your patch had formatting issues, the test would run but it would fail.
Regarding your other questions... In general, when we are making doc patches for a particular function, we try to bring that particular function up to the doc standards. We don't usually try to bring the entire file up to doc standards, because that would make the patch rather large and harder to review. So please just fix up hook_access/hook_node_access, and leave the rest of the file alone for now.
We're going to have a "clean up the docs" sprint in a couple of weeks (I need to announce it on http://groups.drupal.org/documentation-team and with a Drupal Planet post)... that would be a good time for us to take care of this type of thing for entire files.
Comment #26
jhodgdonAh. Apparently Drupal 8 itself has a test failure, so no Drupal 8 patches are being tested right now:
http://qa.drupal.org/8.x-status
Comment #27
Lars Toomre CreditAttribution: Lars Toomre commentedHere is the revised patch limited only to the docblock for hook_node_access().
My git skills are still rather rudimentary. I could not figure out how to modify the changes only applicable to the above patch, so I started over again from scratch. I hope I got everything. (I had worked on another patch in the interim and that was being added to this patch too. I have no clue why.) Anyways, here goes ...
Comment #28
jhodgdonGit help: http://drupal.org/documentation/git or IRC channel #drupal-gitsupport
Sorry about this: I just noticed that that @link line above the @ingroup is totally unnecessary. It's pointing to that group/topic page. So let's just take that line out. At least that will give you one more practice at making a patch. :)
Comment #29
Lars Toomre CreditAttribution: Lars Toomre commentedNot a problem this time... as I had not yet tried to roll another patch yet!!
Here is re-rolled patch with the original line with the @link directive removed as well.
Comment #30
jhodgdontriggering test bot
Comment #31
jhodgdonThis patch looks fine to me, thanks for the rework! Hooray for your first Drupal Core patch (I think)!
Committers: Please note that this needs to be backported to D6 after applying to d7/8.
Comment #32
Lars Toomre CreditAttribution: Lars Toomre commentedThank you @jhodgdon. This is definitely my first core patch and a major accomplishment because it caused me to bite the bullet about getting git working in my Windows development environment. Thanks for your feedback.
Comment #33
pillarsdotnet CreditAttribution: pillarsdotnet commentedmight be better as:
Some supporting statistics gathered by running
grep -r
andwc
over theincludes
andmodules
folders:type of node
node type
content type
machine-readable
machine readable
machine name
Comment #34
sven.lauer CreditAttribution: sven.lauer commentedI like he quantitative approach, in particular with respect to "machine-readable" vs "machine readable", and I agree that "type of node" sounds awkward in this context. However, the genitive alternation (X of Y vs. Y's X vs. YX) in English is complex, and raw counts are unlikely to help. I agree, though, that "node type" or "content type" would be preferable to "type of node" here ... but in your suggested replacement, the comma after "type" should be omitted. In any case, I am not sure that this (rather stylistic) issue should keep the patch from being committed (though if Lars would be willing to re-roll, that would be awesome).
Also, @Lars, with respect to your problems mentioned in #27: It seems that now you figured out how to get git working, you should check out how to use ("feature") branches. Some info is at http://drupal.org/node/1054616#creating-patches . Using the steps outlined there, you can go back to the main branch via
git checkout 8.x
(or whatever main branch your are working on) after you have submitted the patch. You can re-follow the steps for another patch and go back to your previous branch by doing agit checkout <branch name>
. This makes it easy to work on multiple patches in parallel. (An extended tutorial would be useful for this purpose ...).Comment #35
pillarsdotnet CreditAttribution: pillarsdotnet commentedUpdated #33 to remove the comma and also replace "the" with "a" for consistency.
Note that I quite deliberately did not change the status to needs work.
Regarding git and feature branches: Another bonus of this approach is that you can submit patches via "git format-patch" rather than "git diff", which makes the committer's job easier.
Comment #36
Lars Toomre CreditAttribution: Lars Toomre commented@pillarsdotnet - I liked your suggestion and re-rolled the patch using your suggested description for $node. Your English is far better than mine! While I was at it, I also slightly word-smithed the description of the $account object. The resulting 'cummulitive' patch is attached.
@sven.lauer - Thank you for your guidance about git. My mistake mentioned in #27 was working on two completely different patches out of the same branch. I fixed the problem by completely starting over and now have different branches for each core patch. I will have to look into using 'git format-patch' going forward. As a start, I have been using 'git diff'.
Comment #38
Lars Toomre CreditAttribution: Lars Toomre commentedWhoops... I think that I should have used 'git diff --no-prefix' to create the patch instead of just 'git diff'. @pillarsdotnet, do you have a suggestion for full command line options to use with 'git format-patch'?
Here is a patch created with 'git diff --no-prefix'.
Comment #40
pillarsdotnet CreditAttribution: pillarsdotnet commentedNo, you should use "git diff" rather than "git diff --no-prefix".
You need to pull in changes from core that happened since you last submitted.
Here's the sequence I normally use:
(Where
999999
is the issue number, and66
is the comment number.)Since I use those format-patch options so frequently, I have the following in my
~/.gitconfig
file:Also, I maintain a local copy of the 8.x branch called 8.x, and a local copy of the code for an issue in a branch named after the issue number.
So actually I do this:
Comment #41
Lars Toomre CreditAttribution: Lars Toomre commentedOK... I am at a loss as to what the problem is. Any ideas why the test bot is not happy with either #36 or #38?
Comment #42
pillarsdotnet CreditAttribution: pillarsdotnet commentedSee #40:
Comment #43
Lars Toomre CreditAttribution: Lars Toomre commentedAh, I did forget that important step to first pull in recent core changes. I have now done so and re-rolled the patch using 'git diff'. I will focus in a bit on 'git format-patch'. Thanks for your tips @pillarsdotnet.
Comment #44
Lars Toomre CreditAttribution: Lars Toomre commented#43 is messed up... Somehow I again am including other patches. I need to figure out what I did wrong.
Comment #46
Lars Toomre CreditAttribution: Lars Toomre commentedLet's try this again. It appears that the patch #29 was committed to 8.x core without a comment in this issue. The current state of the file node.api.php can be viewed here: http://drupalcode.org/project/drupal.git/blob/HEAD:/modules/node/node.ap....
Hence, the attached patch is technically new rather than a re-roll, which completely confused me. Please review and confirm my understanding.
Comment #47
pillarsdotnet CreditAttribution: pillarsdotnet commentedWhat I would do in this case is:
Of course, you will need to adjust some of that for a Windows environment.
Comment #48
Lars Toomre CreditAttribution: Lars Toomre commented@pillardotnet #46 came back green and I think this just needs a review before RTBC. What do you think?
Comment #49
pillarsdotnet CreditAttribution: pillarsdotnet commentedSound a bit awkward to me. Can you find a way to rephrase this without repeating the word "user" ?
Comment #50
sven.lauer CreditAttribution: sven.lauer commentedI agree with @pillarsdotnet's comment ... to my mind
(or perhaps "on which"?) is clear and short enough.
Comment #51
Lars Toomre CreditAttribution: Lars Toomre commentedSmall victories on road to becoming proficient with git... This patch uses both format-patch and squashes multiple small commits into one patch. Anyways, here is the revised patch with the language change suggested in #49 and #50.
Comment #52
pillarsdotnet CreditAttribution: pillarsdotnet commentedLooks good to me.
Comment #53
webchickCommitted and pushed to 8.x, however this didn't seem to apply cleanly to 7.x.
Comment #54
jhodgdonCould we possibly change "machine-readable content type" to say "machine-readable content type name", or better yet "the machine name of a content type"? I am pretty sure that throughout the user interface, it is just called "machine name", but if not, "machine-readable name of a content type" is fine. What's missing now is the word "name".
Also, my inner grammar nerd cannot stand "A user object for which...". This is a specific user object, so it should be "The user object...". But how about:
The user object to perform the access check operation on.
It is considered actually good usage in modern American English to end a sentence with a preposition, especially if it avoids things like "on which the ...".
Comment #55
Mac_Weber CreditAttribution: Mac_Weber commentedI agree with @jhodgdon at #54
Sending patch
Comment #56
pillarsdotnet CreditAttribution: pillarsdotnet commentedAdded machine name and human-readable name to the glossary.
Comment #57
jhodgdonThanks for the follow-up patch! This is d8 only, and then the whole thing that was committed to D8 needs to be ported to d7.
Comment #58
pillarsdotnet CreditAttribution: pillarsdotnet commentedD7 backport patch with @param data types removed as per webchick<.
Comment #60
pillarsdotnet CreditAttribution: pillarsdotnet commentedOops! Didn't mean to trigger the testbot.
Comment #61
jhodgdonJust a thought... It is normally considered wise to wait until a patch has been committed before backporting it, in case the committers set it back to needs work and the original patch needs further changes (saves doing the work again). It also decreases confusion among committers as to which patch they should be reviewing/committing. :)
Comment #62
webchickActually, I like when there's two versions cos then I know I'm waiting a maximum of a day or something after committing to D8 if the D7 patch doesn't apply. It's not a requirement, though.
Comment #63
jhodgdonOK. I guess my comment would only apply in the case of a long and complicated patch that's likely to be rejected by the committers then, and only to save time creating something and then having to re-create it. :)
My policy in reviewing is usually to only review the current version's patch until it's committed, and then move on to the next version, though. That is because I only have so much time in the day...
Comment #64
pillarsdotnet CreditAttribution: pillarsdotnet commentedThat's what I love about standards -- there are so many to choose from!
Comment #65
webchickThis isn't a standard; it's a preference. And yes, different people have different preferences. We are not the borg. :)
Comment #66
pillarsdotnet CreditAttribution: pillarsdotnet commentedResistance is futile...
Comment #67
Mac_Weber CreditAttribution: Mac_Weber commentedchat is good, but we forgot to send the backport patch to QA
Shall I send now? oops, I just did it =P
Comment #68
pillarsdotnet CreditAttribution: pillarsdotnet commentedNeither patch changes code, so qa.drupal.org is just checking whether it applies. Can't check a d7 patch in a d8 issue. The d8 patch is RTBC. I assure you that the d7 patch applied when I made it; dunno about tomorrow.
Comment #69
webchickOk, committed and pushed to 8.x and 7.x. Thanks!
Comment #72
sven.lauer CreditAttribution: sven.lauer commented[no longer applicable as the spam was removed]
Comment #73
sven.lauer CreditAttribution: sven.lauer commentedRestoring the settings, I realize that we should not have let this one be closed---we still need to port the patch to 6.x. A good novice task, so the tag is appropriate.
Comment #74
sven.lauer CreditAttribution: sven.lauer commented[redacted because the spam was removed].
Comment #75
sven.lauer CreditAttribution: sven.lauer commentedMoving to the Documentation project, for this is where this documentation is located for 6.x.
Patch attached.
Comment #76
pillarsdotnet CreditAttribution: pillarsdotnet commentedNeed another space before the word
Either
.Comment #77
sven.lauer CreditAttribution: sven.lauer commentedDarn. Sorry. Re-rolled and fixed another indentation issue I just noticed.
Comment #78
pillarsdotnet CreditAttribution: pillarsdotnet commentedThat line was removed in the 8.x and 7.x patches because it was functionally replaced by this one:
There are other functions with an
@ingroup node_access
line, so it should be okay to add one here, as well.Comment #79
sven.lauer CreditAttribution: sven.lauer commentedI completely agree, which is why I left the (existing) @ingroup directive where it is ;)
Comment #80
pillarsdotnet CreditAttribution: pillarsdotnet commented(oh. Sorry.)
Okay; gotta go read the patched source instead of just looking at the diff.
Comment #81
sven.lauer CreditAttribution: sven.lauer commentedNo sweat, a very understandable mistake. Esp. since it is quite unexpected that the @ingroup got lost between D6 and D7 ...
Comment #82
jhodgdonIf we're cleaning up this function's documentation, we could bring it up to standards:
- Blank line between parameters and @return.
- The TRUE/FALSE/NULL lines should either be a list, or they should be wrapped as a paragraph.
Comment #83
pillarsdotnet CreditAttribution: pillarsdotnet commented@jhodgdon -- If those standards were not in force when the function was originally written, then according to webchick you shouldn't enforce them afterwards.
Comment #84
jhodgdonThe line wrapping standards have been in place for ages.
Comment #85
pillarsdotnet CreditAttribution: pillarsdotnet commentedThanks. I will have to assume, absent any documentation, that by "for ages" you mean "since before this function was written." It's too bad that we have to depend on you to supply answers like this instead of actually writing them down somewhere.
Comment #86
sven.lauer CreditAttribution: sven.lauer commentedTrue, there is no reason to not fix things up while we are at it.
Comment #87
jhodgdonThat looks good, except there is an extra space in this line:
+ * - TRUE if the operation is to be allowed.
But. I'm putting this back to D8 ***again***. Can we please put the word "name" in the doc as I requested in #54? It is still confusing, and the word "name" got dropped from the fix-up patch that was committed. The $node parameter now says:
$node Either a node object or a (machine-readable) content type on which to perform the access check.
It should say "machine-readable content type name".
Comment #88
sven.lauer CreditAttribution: sven.lauer commentedAttaching a D8 patch. I went with "the machine name of the content type" instead of "machine-readable content type name", as the latter sounds less awkward ... "the machine name of the content type on which to perform the access check" may still not be the nicest prose, but it is quite clear, at least.
While I was at it, I also transformed the @return documentation into a list (otherwise, it would have violated the standards just as the D6 version above).
Comment #89
jhodgdonThanks, that looks good to me.
Comment #90
catchCommitted/pushed to 8.x.
Looks like this needs to go back to 7 again?
Comment #91
catchComment #92
aspilicious CreditAttribution: aspilicious commentedI'm confused.. What happend with
in the latest patch
Comment #93
pillarsdotnet CreditAttribution: pillarsdotnet commented@aspilicious -- See comments 76-78 above.
Comment #94
aspilicious CreditAttribution: aspilicious commentedComment #95
sven.lauer CreditAttribution: sven.lauer commentedLooks good and is straightforward.
After this is committed, we need to go back to D6 (Documentation project) and merge the two patches for that.
Comment #96
webchickCommitted and pushed to 7.x. Thanks!
Marking back to 6.x. I'm not aware of us tracking core API documentation for 6.x in the Documentation project, so I left that alone. But if I'm mistaken, feel free to move it.
Comment #97
sven.lauer CreditAttribution: sven.lauer commentedYep, D6 node hooks are defined in the Documentation repo at developer/hooks/node.php (which from D7 on is modules/core/node/node.api.php ).
Comment #98
sven.lauer CreditAttribution: sven.lauer commentedThis is a re-roll of #86, fixing the whitespace issue pointed out by jhodgdon in #87 and changing the wording to "the machine name of the content type", as in the latest D7/8 patch.
Comment #99
sven.lauer CreditAttribution: sven.lauer commentedComment #100
jhodgdonRegarding
- * @see http://api.drupal.org/api/group/node_access/6
Don't we need an @ingroup node_access here?
Comment #101
pillarsdotnet CreditAttribution: pillarsdotnet commented@jhodgdon: See comments 76-78 above.
Comment #102
jhodgdonWhoops. I thought those applied to d7. My mistake. :)
Comment #103
jhodgdonCommitted and pushed.