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:

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

Files: 
CommentFileSizeAuthor
#98 789484-hook_access_params-4.patch1.46 KBsven.lauer
#94 789484-node-api-94.patch1.19 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 36,975 pass(es).
[ View ]
#88 doc-followup-hook_node_acces-789484.patch1.13 KBsven.lauer
PASSED: [[SimpleTest]]: [MySQL] 33,846 pass(es).
[ View ]
#86 789484-hook_access_params-3.patch1.46 KBsven.lauer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 789484-hook_access_params-3.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#77 789484-hook_access_params-2.patch1.38 KBsven.lauer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 789484-hook_access_params-2.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#75 789484-hook_access_params.patch1.1 KBsven.lauer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 789484-hook_access_params.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#60 node_api_documentation-789484-55.patch2.17 KBpillarsdotnet
PASSED: [[SimpleTest]]: [MySQL] 33,444 pass(es).
[ View ]
#60 node_api_documentation-789484-58-d7.patch2.56 KBpillarsdotnet
#58 fix-hook_node_access-docs-789484-58.patch2.56 KBpillarsdotnet
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix-hook_node_access-docs-789484-58.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#55 node_api_documentation-789484-55.patch2.17 KBMac_Weber
PASSED: [[SimpleTest]]: [MySQL] 33,433 pass(es).
[ View ]
#51 doc-hook_node_access-arguments-789484-51.patch1.42 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 33,334 pass(es).
[ View ]
#46 doc-hook_node_access-arguments-789484-45.patch1.01 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 33,327 pass(es).
[ View ]
#43 doc-hook_node_access-arguments-789484-43.patch2.77 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] 33,314 pass(es), 21 fail(s), and 1 exception(es).
[ View ]
#38 doc-hook_node_access-arguments-789484-38.patch1.47 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch doc-hook_node_access-arguments-789484-38.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#36 doc-hook_node_access-arguments-789484-36.patch1.48 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch doc-hook_node_access-arguments-789484-36.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#29 doc-hook_node_access-arguments-789484-29.patch1.38 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 33,334 pass(es).
[ View ]
#27 doc-hook_node_access-arguments-789484-27.patch1.48 KBLars Toomre
PASSED: [[SimpleTest]]: [MySQL] 33,314 pass(es).
[ View ]
#22 doc-hook_node_access-arguments-789484-22.patch1.11 KBLars Toomre
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch doc-hook_node_access-arguments-789484-22.patch. See the log in the details link for more information.
[ View ]
#21 doc-hook_node_access-arguments-789484-19.patch686 bytessven.lauer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch doc-hook_node_access-arguments-789484-19.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]
#10 hook_node_access-789484-10.patch702 bytesmichaellenahan
PASSED: [[SimpleTest]]: [MySQL] 33,146 pass(es).
[ View ]

Comments

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.

it is the translation module in core :)

Component:node.module» documentation

Moving to the documentation queue.

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.

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

Status:Closed (works as designed)» Active

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

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?

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

StatusFileSize
new702 bytes
PASSED: [[SimpleTest]]: [MySQL] 33,146 pass(es).
[ View ]

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.

Status:Active» Needs review

Status:Needs review» Needs work

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

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.

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.

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?

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

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.

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:

<?php
* @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'.
?>

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?

Status:Needs work» Needs review
Issue tags:+needs backport to D6
StatusFileSize
new686 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch doc-hook_node_access-arguments-789484-19.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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.

StatusFileSize
new1.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch doc-hook_node_access-arguments-789484-22.patch. See the log in the details link for more information.
[ View ]

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.

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!

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

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.

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

StatusFileSize
new1.48 KB
PASSED: [[SimpleTest]]: [MySQL] 33,314 pass(es).
[ View ]

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

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

StatusFileSize
new1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 33,334 pass(es).
[ View ]

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.

Status:Needs work» Needs review

triggering test bot

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.

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.

+ *   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

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

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.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new1.48 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch doc-hook_node_access-arguments-789484-36.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new1.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch doc-hook_node_access-arguments-789484-38.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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.

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

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?

See #40:

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

Status:Needs work» Needs review
StatusFileSize
new2.77 KB
FAILED: [[SimpleTest]]: [MySQL] 33,314 pass(es), 21 fail(s), and 1 exception(es).
[ View ]

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.

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

Status:Needs work» Needs review
StatusFileSize
new1.01 KB
PASSED: [[SimpleTest]]: [MySQL] 33,327 pass(es).
[ View ]

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.

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.

Status:Needs work» Needs review

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

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

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.

StatusFileSize
new1.42 KB
PASSED: [[SimpleTest]]: [MySQL] 33,334 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community

Looks good to me.

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.

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

Status:Needs work» Needs review
StatusFileSize
new2.17 KB
PASSED: [[SimpleTest]]: [MySQL] 33,433 pass(es).
[ View ]

I agree with @jhodgdon at #54
Sending patch

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

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.

StatusFileSize
new2.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix-hook_node_access-docs-789484-58.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new2.56 KB
new2.17 KB
PASSED: [[SimpleTest]]: [MySQL] 33,444 pass(es).
[ View ]

Oops! Didn't mean to trigger the testbot.

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

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.

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

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

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

Resistance is futile...

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

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.

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.

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

[no longer applicable as the spam was removed]

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.

[redacted because the spam was removed].

Project:Drupal core» Documentation
Version:8.x-dev»
Component:documentation» API documentation files
Status:Closed (fixed)» Needs review
Issue tags:-needs backport to D7
StatusFileSize
new1.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 789484-hook_access_params.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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

Patch attached.

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.

Status:Needs work» Needs review
StatusFileSize
new1.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 789484-hook_access_params-2.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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

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.

Status:Needs work» Needs review

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

(oh. Sorry.)

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

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

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.

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

The line wrapping standards have been in place for ages.

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.

Status:Needs work» Needs review
StatusFileSize
new1.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 789484-hook_access_params-3.patch. This may be a -p0 (old style) patch, which is no longer supported by the testbots.
[ View ]

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

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

Status:Active» Needs review
StatusFileSize
new1.13 KB
PASSED: [[SimpleTest]]: [MySQL] 33,846 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Thanks, that looks good to me.

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?

Version:8.x-dev» 7.x-dev

I'm confused.. What happend with

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

in the latest patch

@aspilicious -- See comments 76-78 above.

Status:Patch (to be ported)» Needs review
StatusFileSize
new1.19 KB
PASSED: [[SimpleTest]]: [MySQL] 36,975 pass(es).
[ View ]

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.

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.

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

StatusFileSize
new1.46 KB

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.

Status:Patch (to be ported)» Needs review

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

Don't we need an @ingroup node_access here?

@jhodgdon: See comments 76-78 above.

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

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.