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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mrhanlon’s picture

Just 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.

hctom’s picture

it is the translation module in core :)

ksenzee’s picture

Component: node.module » documentation

Moving to the documentation queue.

jhodgdon’s picture

Status: Active » Closed (works as designed)

To 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.

hctom’s picture

This 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

hctom’s picture

Status: Closed (works as designed) » Active
jhodgdon’s picture

In 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...

hctom’s picture

Ah 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?

jhodgdon’s picture

Title: Wrong documentation for $node argument in hook_access() » Confusing documentation for $node argument in hook_access()/hook_node_access()
Version: 6.x-dev » 8.x-dev
Issue tags: +Novice

Maybe 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...

michaellenahan’s picture

Novice 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.

michaellenahan’s picture

Status: Active » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

I think that has too many commas -- the one after "or" should be removed. Thanks!

hctom’s picture

I 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.

Lars Toomre’s picture

This 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.

jhodgdon’s picture

I 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?

hctom’s picture

@jhodgdon: But why should we hide very useful information to avoid false results? I thought this is what a documentation is for...

jhodgdon’s picture

I 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.

sven.lauer’s picture

I think what @hctom is getting at is that the currently suggested text:

The node on which the operation is to be performed, or a string value representing the type of node to be created.

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:

 * @param $node The node on which the operation is to be performed, or a string value representing the type of node.
*   $node may be a string only if $op is 'create'.
jhodgdon’s picture

I'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?

sven.lauer’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to D6
FileSize
686 bytes

You are right, the additional sentence suffers from just the same problem.

Re-rolled a version of the patch omitting the "to be created" part.

Lars Toomre’s picture

I 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.

jhodgdon’s picture

Status: Needs review » Needs work

The 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!

Lars Toomre’s picture

@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.

jhodgdon’s picture

If 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.

jhodgdon’s picture

Ah. 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

Lars Toomre’s picture

Here 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 ...

jhodgdon’s picture

Git 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. :)

Lars Toomre’s picture

Not 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.

jhodgdon’s picture

Status: Needs work » Needs review

triggering test bot

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

This 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.

Lars Toomre’s picture

Thank 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.

pillarsdotnet’s picture

+ *   The node on which the operation is to be performed, or a string value
+ *   representing the type of node.

might be better as:

+ *   Either a node object or a (machine-readable) content type on
+ *   which to perform the access check.

Some supporting statistics gathered by running grep -r and wc over the includes and modules folders:

Term Core usage count
type of node 2
node type 194
content type 211
machine-readable 69
machine readable 2
machine name 52
sven.lauer’s picture

I 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 a git checkout <branch name>. This makes it easy to work on multiple patches in parallel. (An extended tutorial would be useful for this purpose ...).

pillarsdotnet’s picture

Updated #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.

Lars Toomre’s picture

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

@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'.

Status: Needs review » Needs work

The last submitted patch, doc-hook_node_access-arguments-789484-36.patch, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
1.47 KB

Whoops... 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'.

Status: Needs review » Needs work

The last submitted patch, doc-hook_node_access-arguments-789484-38.patch, failed testing.

pillarsdotnet’s picture

No, 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:

git rebase origin/8.x
git format-patch -C -C -M -B --patience --binary --full-index --stdout origin/8.x > ~/Patches/patch-description-999999-66.patch

(Where 999999 is the issue number, and 66 is the comment number.)

Since I use those format-patch options so frequently, I have the following in my ~/.gitconfig file:

[alias]
        fp = format-patch -C -C -M -B --patience --binary --full-index --stdout
[diff]
        binary = true
        find-renames = true
        break-rewrites = true
        find-copies-harder = true
        full-index = true
        patience = true
        renames = copy

[format]
        binary = true
        find-copies-harder = true
        full-index = true
        patience = true
        renames = copy

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:

git checkout 8.x
git pull
git checkout 999999
git rebase 8.x
git fp 8.x > ~/Patches/patch-description-999999-66.patch
Lars Toomre’s picture

OK... 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?

pillarsdotnet’s picture

See #40:

You need to pull in changes from core that happened since you last submitted.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
2.77 KB

Ah, 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.

Lars Toomre’s picture

#43 is messed up... Somehow I again am including other patches. I need to figure out what I did wrong.

Status: Needs review » Needs work

The last submitted patch, doc-hook_node_access-arguments-789484-43.patch, failed testing.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
1.01 KB

Let'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.

pillarsdotnet’s picture

Status: Needs review » Needs work

What I would do in this case is:

git branch -D 8.x
git checkout 8.x
git pull
git branch -D 789484
git checkout -b 789484
curl http://drupal.org/files/doc-hook_node_access-arguments-789484-29.patch | git apply
$EDITOR modules/node/node.api.php
git diff | $VIEWER
git commit -a -m 'Issue #789484 by Lars Toomre: Fix confusing node_access documentation.'
git format-patch --stdout 8.x > ~/Patches/node_access-docs-789484-46.patch

Of course, you will need to adjust some of that for a Windows environment.

Lars Toomre’s picture

Status: Needs work » Needs review

@pillardotnet #46 came back green and I think this just needs a review before RTBC. What do you think?

pillarsdotnet’s picture

A user object representing the user for whom

Sound a bit awkward to me. Can you find a way to rephrase this without repeating the word "user" ?

sven.lauer’s picture

I agree with @pillarsdotnet's comment ... to my mind

A user object for which the access check operation is to be performed.

(or perhaps "on which"?) is clear and short enough.

Lars Toomre’s picture

Small 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.

pillarsdotnet’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed to 8.x, however this didn't seem to apply cleanly to 7.x.

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Needs work

Could 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 ...".

Mac_Weber’s picture

Status: Needs work » Needs review
FileSize
2.17 KB

I agree with @jhodgdon at #54
Sending patch

pillarsdotnet’s picture

Added machine name and human-readable name to the glossary.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks 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.

pillarsdotnet’s picture

D7 backport patch with @param data types removed as per webchick<.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, fix-hook_node_access-docs-789484-58.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.56 KB
2.17 KB

Oops! Didn't mean to trigger the testbot.

jhodgdon’s picture

Just 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. :)

webchick’s picture

Actually, 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.

jhodgdon’s picture

OK. 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...

pillarsdotnet’s picture

That's what I love about standards -- there are so many to choose from!

webchick’s picture

This isn't a standard; it's a preference. And yes, different people have different preferences. We are not the borg. :)

pillarsdotnet’s picture

Resistance is futile...

Mac_Weber’s picture

Status: Reviewed & tested by the community » Needs review

chat is good, but we forgot to send the backport patch to QA

Shall I send now? oops, I just did it =P

pillarsdotnet’s picture

Status: Needs review » Reviewed & tested by the community

Neither 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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, committed and pushed to 8.x and 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

sven.lauer’s picture

Version: 6.x-dev » 8.x-dev
Status: Needs work » Closed (fixed)

[no longer applicable as the spam was removed]

sven.lauer’s picture

Version: 8.x-dev » 6.x-dev
Status: Closed (fixed) » Needs work

Restoring 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.

sven.lauer’s picture

[redacted because the spam was removed].

sven.lauer’s picture

Project: Drupal core » Documentation
Version: 8.x-dev »
Component: documentation » API documentation files
Status: Closed (fixed) » Needs review
Issue tags: -Needs backport to D7
FileSize
1.1 KB

Moving to the Documentation project, for this is where this documentation is located for 6.x.

Patch attached.

pillarsdotnet’s picture

Status: Needs review » Needs work
+ *  Either a node object or a (machine-readable) content type on which to

Need another space before the word Either.

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
1.38 KB

Darn. Sorry. Re-rolled and fixed another indentation issue I just noticed.

pillarsdotnet’s picture

Status: Needs review » Needs work
- * @see http://api.drupal.org/api/group/node_access/6

That line was removed in the 8.x and 7.x patches because it was functionally replaced by this one:

+ * @ingroup node_access

There are other functions with an @ingroup node_access line, so it should be okay to add one here, as well.

sven.lauer’s picture

Status: Needs work » Needs review

I completely agree, which is why I left the (existing) @ingroup directive where it is ;)

pillarsdotnet’s picture

(oh. Sorry.)

Okay; gotta go read the patched source instead of just looking at the diff.

sven.lauer’s picture

No sweat, a very understandable mistake. Esp. since it is quite unexpected that the @ingroup got lost between D6 and D7 ...

jhodgdon’s picture

Status: Needs review » Needs work

If we're cleaning up this function's documentation, we could bring it up to standards:

+ *   The user object to perform the access check operation on.
  * @return
  *   TRUE if the operation is  to be allowed;
  *   FALSE if the operation is to be denied;
  *   NULL to not override the settings in the node_access table, or access
- *     control modules.
+ *   control modules.

- Blank line between parameters and @return.
- The TRUE/FALSE/NULL lines should either be a list, or they should be wrapped as a paragraph.

pillarsdotnet’s picture

@jhodgdon -- If those standards were not in force when the function was originally written, then according to webchick you shouldn't enforce them afterwards.

jhodgdon’s picture

The line wrapping standards have been in place for ages.

pillarsdotnet’s picture

Thanks. 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.

sven.lauer’s picture

Status: Needs work » Needs review
FileSize
1.46 KB

True, there is no reason to not fix things up while we are at it.

jhodgdon’s picture

Project: Documentation » Drupal core
Version: » 8.x-dev
Component: API documentation files » documentation
Status: Needs review » Active

That 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".

sven.lauer’s picture

Status: Active » Needs review
FileSize
1.13 KB

Attaching 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).

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, that looks good to me.

catch’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 8.x.

Looks like this needs to go back to 7 again?

catch’s picture

Version: 8.x-dev » 7.x-dev
aspilicious’s picture

I'm confused.. What happend with

- * @see http://api.drupal.org/api/group/node_access/6

in the latest patch

pillarsdotnet’s picture

@aspilicious -- See comments 76-78 above.

aspilicious’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.19 KB
sven.lauer’s picture

Status: Needs review » Reviewed & tested by the community

Looks good and is straightforward.

After this is committed, we need to go back to D6 (Documentation project) and merge the two patches for that.

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 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.

sven.lauer’s picture

Project: Drupal core » Documentation
Version: 6.x-dev »
Component: documentation » API documentation files

Yep, 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 ).

sven.lauer’s picture

This 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.

sven.lauer’s picture

Status: Patch (to be ported) » Needs review
jhodgdon’s picture

Regarding
- * @see http://api.drupal.org/api/group/node_access/6

Don't we need an @ingroup node_access here?

pillarsdotnet’s picture

@jhodgdon: See comments 76-78 above.

jhodgdon’s picture

Whoops. I thought those applied to d7. My mistake. :)

jhodgdon’s picture

Status: Needs review » Fixed

Committed and pushed.

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D6, -Novice

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