Node access example comment; Improve discussion of grants

I thought to myself: This is a good opportunity to learn more about the node access API stuff. And so here's a patch.

Please, someone with more node_access insight please double check the content. :-)

This patch 'deviates' the D6 code of the module from the D7 branch. I'd suggest to change function order and array attributes to stick as much as possible as their D7 versions to make module maintaining easy. i.e. in D7 we do have $grants['node_access_example_..'] where .. are any of the following: 'author', 'view' or 'edit', and the patch introduces $grants['example_private_realm'] for view and edit operations, and a $grants['example_author_realm'] for authors.

To make this go in, I'd first make these changes in the D7 branch also and then continue with this issue. Mile23, Randy, what do you preffer?

I'm in doubt, is this really a but report or a task?

Yah, good point. I'll muck about with it and do a couple patches.

Great Mile23, thanks so much for taking this one :)

OK: The D6 version lacks a test, so I can't say I didn't introduce any bugs. I changed some names to match the D7 version. Also, same for the D7 version, because even though it has a test, I don't have a D7 install at hand.

Ok, the D7 patch does not touch a line of code, and it is only changing comments. Very complete, I'd say. I've fixed the only issue I've seen no the patch: comments exceding 80 chars.

I do have one concern.. the module currently says:

* Note that current best practice is probably to interoperate with the
* @link http://drupal.org/project/acl ACL module @endlink rather than directly
* implementing node access.

Really is that a 'best practice' or just a good? really this should be in a module comment deep into the file and not in the header section?

The D7 passess all the tests locally, verified. Lets see if the testbot thinks the same. I'm moving the changes to D7 first so we can go back for D6 later.

I forgot to mention, very great work Mile23. I hope you finally get this issue fixed at all! :)

Regarding the ACL module stuff... I really don't know whether it's best practice or just good. I think it might be a holdover from D6, which doesn't have a hook_node_access() and therefore can't deny access, only grant it. However, ACL has a D7 version in the pipeline. None of the linked documentation is clear on why you'd want to avoid node access in favor of another module. But it was in the D7 @file block before, so I kept it in order to have exactly this discussion.

Lets wait for rfay's point on it, also he introduced it in the patch at http://drupal.org/node/768054#comment-3057126. I'm pretty sure that he will have a whole rationale for that.

About the D7 patch, IMO it is ok to go, could someone else make an additional review?

Mile23, will you do the honor patch working on these last issues? I totally agree that instead of a '1', we can use a constant, but the document must explain that WE are defining them, they are not provided by Drupal.. or.. do they?

rfay: Going down the list here... :-)

Defining a node grant constant is a good idea, probably a best practice. I think the important point to get across is that grant IDs are arbitrary and can be set up however you want. In fact, if I were coding this for a real project, I'd define one realm and use different grant IDs for view vs. edit access. But that's just me.

Will kill the typo.

If someone needs to read about hook_menu(), then here's some text for them to read. :-) Seriously, though, that page shows us how to run some SQL while taking account of the access system. The comment tells us that this is the menu definition for the page that demonstrates this. It's easy enough to tell our newbie audience that this is scaffolding for the real demonstration.

'When' is better English, because it's accurate. When the conditions are met, it does the thing.

Same with the last one: We're making a list of all of the user's realms and grants based on operation. Any grants could be included, and in fact the authorship one is always included in the example code. But I decided to change that all around anyway, having re-read it.

Still wondering about the ACL module stuff... But really all the concerns raised are addressed by hook_node_access(). In fact, if I were going to write a D7 node access module, I'd toss out the carryovers from D6, and base it solely on hook_node_access(). I assume hook_node_grants/records() still exist in D7 because they're based in the database, and an upgrade path to to hook_node_access() can only be coded by the module author. I'll take the ACL module reference out of the D7 example, and someone can put it back if they please.

About the ACL stuf, I'd simply change 'Best practice' with a 'good or common' practice notice. Probably a person reading the node_acces example code is wondering how does it work, otherwise they would be reading the ACL's module code. However, you already exposed the reason: first grant wins, so probably a line stating something like 'Anything that you can do here, you will find done in ACL module, so if you just can't use ACL module then keep reading'.

What say ye?

Mile23, comment lines exceeding 80 chars again :)

Appart of this, I enjoyed the detail of choosing 23

Someone should make a tool to format doxygen comments.

Based on the non-commit of this patch, I'll leave node_access_example out of #767204: API.drupal.org: Rework Doxygen comments so people can navigate the comments.

No worries. :-)

Backported D7 docblock comments to D6, with minor changes to reflect differences in code functioning. (D6 doesn't have view vs. edit distinction, for instance.) Changed the grant names from 'example' and 'example_author' to 'node_access_example' and 'node_access_example_author'.

And committed: http://drupalcode.org/project/examples.git/commitdiff/29aa395423795ab2ce...

Yell and scream if this is the wrong way to go. :-)

Well if I explained it incorrectly, give a shout and I can fix it.