Add drupal_alter() after hook_node_access_records()

I was told this was a one-line patch. But it's two. So -1.

Kidding! But seriously, during that fateful day in Szeged Ken convinced me that this alter calls for a corresponding alter after node access grants are compiled. So let's make this truly a two-liner by adding another drupal_alter, then it gets +1. Or has our thinking changed?

This is a beautiful patch. Very much wanted because it will allow custom modules to control the interaction between node access modules, allowing for advanced business logic.

Patch is clean. Tests pass. http://testing.drupal.org/node/14433.

Ok, then. +1 and thanks.

For what it's worth, I think we still need the second patch, since the SQL queries are written based on hook_node_grants(). Unless, of course, we totally revise the current SQL logic.

But this part of the node_access upgrade is ready to go.

Since it would need to be written anyway, can someone please write up the documentation for hook_node_access_records_alter(), including a code example which shows how this hook could actually be used in a practical sense? That'd probably make it easier to grok why this is needed.

Documentation patch.

The idea here is that some sites need more complex node access rules. But we do not necessarily want more rows in the {node_access} table. This hook would allow a module to read the existing grants and write, in theory, a single record that defines the behavior of multiple node access modules at the same time.

Revised docs patch, with Typo Correction [tm].

Sorry, I still feel like this one probably needs a look from Dries since I do not grok the node access system all that well. I will add it to the "hit list" I send off tonight.

IMO, this makes Node Access more Drupal-y, since we fire an alter hook before saving. Thinks of this as the Node Access equivalent to hook_menu_alter(). Normally, you won't need it, but when you do, it is a lifesaver.

Another +1 for this patch from me. Here's an example of an issue in my queue which can't be addressed without this patch: http://drupal.org/node/323360.

(On a side note... Since it requires some momentum to get any patch into core, should we include the call to alter the grants in this same patch and push them both into core at once? That this, this patch allows one to alter the records and affects the records written to the node_access table; I'm talking about another alter for the grants which could simplify the SQL when nodes are queried.)

+1 for that patch (from the author of the above mentionned issue #323360: How to arbitrate priorities between TAC_Lite and Workflow_Access grants?)

From an humble non-Drupal-knight developer point of view, +1 for including the simplified SQL queries.

Both patches still apply. I haven't retested them, though.

Both patches re-rolled for latest head.

Looks good. Will RTBC after the automated test returns.

This looks reasonable, but I'd like to see a bit more documentation in the code as well. It doesn't need to include an example, but it is worthy of a few lines.

Adds extra explanation of the alter hook inside the docblock. Fixes a grammar issue with 'Retain...' capitalization.

We want a test for this, as well as an update to the API documentation that is now part of core.

Tests are beyond me -- not even sure what we would test here. Documentation patch is http://drupal.org/node/309007#comment-1175506.

subscribe

Kind of a weird thing to test, right? All this is doing is allowing the $grant array to be altered. Still, I think it would be a shame if this died on the vine due to a lack of a test, so here's a suggestion:

Write a hook_node_access_records function that gives an arbitrary grant for a particular node. (E.g., make up a realm and give it view access.) Then, write a hook_node_access_records_alter that replaces the $grant array entirely with a wholly different grant (different arbitrary realm). If that latter grant is the only thing that exists in the node_access table after node_access_acquire_grants is called, then the test passes.

Does that sound reasonable?

I decided to go ahead and write my first SimpleTest for this.

Noticed a few typos in the comments. Second version.

Er, the test failed because I didn't include the patch, so of course the SimpleTest failed. I re-rolled it with everything: the API documentation, the patch itself, and the SimpleTest.

Fixed typo in variable_get in node.api.php.

Note the mutliple instances of mutliple.

Sorry about the patch, I do use -up but I misplaced it and didn't notice its absence.

"Note the mutliple instances of mutliple."

I assume you are referring to the fact that the SimpleTest doesn't involve multiple node access modules? I see no reason why it should -- we are testing the alter itself -- what grants are passed to the alter are sort of irrelevant, whether they are from one or seventeen node access modules. This test verifies the functionality of the alter hook -- whether module programmers are capable of merging multiple grants into something else is irrelevant to core, and should be tested within those modules themselves.

keith.smith -- any chance you could elaborate on your reason for switching this to "needs work", or respond to my comment above? Otherwise, I'll switch this back to needs review or RTBC.

Note the single instance of multiple.

;)

I apologize if my earlier comment wasn't clear.

>  * ways that mutliple node access modules interact. The preferred use of 
>  * this hook is in a module that bridges mutliple node access modules

Note the multiple instances of the typo "mutliple".

Ah... I was trying to parse that. I searched the patch for "multiple" and only found one. I was trying to be funny, and also trying to accelerate this issue.

I have no idea why the patch failed testing with a PHP syntax error in node.test, but I've run cvs update and re-rolled the patch with the typos fixed that keith.smith pointed out. We'll see how it goes.

Patch passed all tests and 'mutliple' typos fixed. Otherwise nothing changed since Moshe and Dave RTBC'd this.

Ah, so this is where testing really shines. What happened was that the 'Node RSS Content' test uses the same node_test.module that defines the arbitrary grants for testing hook_node_access_records_alter, but it wasn't passing. So I had to add the proper grants to that module so that other tests using that module don't break.

However, that didn't resolve the issue, because there was a variable typo in node_access_view_all_nodes(). I fixed that typo as well and now both tests run successfully -- this is great, because between these two tests we now have something testing node_access_view_all_nodes().

I've marked this for review since there is now an added fix. Let me know if it's inappropriate to fix the variable typo in this patch -- if so, I'll open another issue for it and mark it as critical. Also let me know if you think there should be yet another test written to test node_access_view_all_nodes() explicitly, rather than between two tests.

Otherwise, hopefully this will pass the testing bot and get marked as RTBC again.

Somehow bad PHP snuck into hook_node_build_alter from another commit and didn't get caught by testing. I made the simple fix, and am switching this back to RTBC.

Actually, let's let the test run again.

S'all good.

My impatience is not a virtue. Someone else fixed the other bug -- I just should've waited it out. Re-rolled.

Passed testing, this is the exact same patch that Moshe last RTBC'd.

Shouldn't the hook_node_access_alter be passed the node, in addition to the grants? That way, logic could be based on node type or other factors, in addition to grants provided by other node_access modules.

This idea was prompted by #463130: Restrict node_access by node type (tac_lite).

I hate to change status away from RTBC. If others feel this is not necessary, I can live without it.

Unfortunately drupal_alter takes only one data parameter, so we'd have to build a composite structure or invoke the hook another way.

Cool. So my suggestion is simply to add the $node after $grants. Makes sense, right?

OK, I'll pop that in later today.

I've added the $node argument to the drupal_alter, and have also added another test that addresses its presence. Setting issue for review.

Nice suggestion, Dave.

Oops, fixed a minor typo.

Lovely. Huge +1 from me, this is exactly what I was planning to do and needed.

So a lot of things have happened since Oct 2008. ;)

1) I'm still no expert in the node access system, but I understand it a lot better than I did back then, including encountering a use case for this very hook.
2) This patch now has lovely documentation.
3) It also has lovely tests!!
4) It's gone through several improvement iterations beyond its initial state, and looks to be pretty solid for real-world use cases now.

Just a couple things (most of them very minor), then I'm cool with committing this:

+function hook_node_access_records_alter(&$grants, $node) {
+  // Check to see if we have enabled complex behavior.
+  if (variable_get('example_alter_node_access', FALSE) && !empty($grants)) {
+    // Our module removes the author grant.
+    foreach ($grants as $gid => $grant) {
+      if ($grant['realm'] == 'example_author') {
+        unset($grants[$gid]);
+      }
+    }
+    sort($grants);
+  }
+}

Remember that this is the example documentation for everyone on earth who wants to assess whether this is the hook they need to do whatever task they have in front of them. Can we please pick a more "real world" use case here, or, if we must be arbitrary, then add more comments to explain why unsetting the gid would be desirable here? Because currently it's very confusing, since that looks like you'd be introducing a bug.

+  function testGrantAlter() {

Needs summary PHPDoc line.

+    $this->assertEqual($records[0]->gid, 1, t('Grant with gid = 1 acquired for node without alteration.'));
+    

Please remove trailing whitespace on the blank line here.

+
+/*
+ * Implementation of hook_node_grants().

Should be /** not /*

+  // Give everyone full grants so we don't break other tests.
+  return array(
+    'article_realm' => array(1),
+    'page_realm' => array(1),
+    'alter_realm' => array(2),
+  );

This could do with some clarification. How does array(1) and array(2) grant full access? Could we get 2-3 more sentences here to explain what's going on?

A more real world use case would be to pick one element of a node such as a taxonomy term or an author and create 'specific' access control based upon that.

IMO the documentation for hook_node_access_records_alter() does not contain documentation to what the $grants array is. It should either document it or provide an @see to where this data structure is documented. Otherwise, a newer user to the node access system will find this to be a highly documented wall, wherein the documentation is insufficient, yet everyone else will go "But see, it's documented!!!"

Otherwise, this patch is fine. I would still really love a non-database method of adding additional node access but at least this can let us do complex changes based upon site needs. It is a start.

I disagree that the 'other' alter hook should be a separate patch. I believe that without being able to alter the grants as well as the records, this patch is far less useful. I think they should be packaged together, because the matching patch is also quite simple.

Working on API docs.

I disagree that the 'other' alter hook should be a separate patch. I believe that without being able to alter the grants as well as the records, this patch is far less useful. I think they should be packaged together, because the matching patch is also quite simple.

I agree with this.

I see this thread as good work by a lot of people, and an example of how difficult it is to get a change into Drupal - even when everyone is in favor of it. I wouldn't stop this patch because it doesn't have the 'other' alter hook. But if we don't do it now I don't expect we ever will.

Yeah, let's do it here. We've got the attention all of the people who understand the node_access system (all ~five of them ;)), plus both core maintainers.

So. What we need:

a) Another well-documented entry in node.api.php for hook_node_access_grants_alter().
b) An implementation of this hook in node_test.module
c) A similar test in node.test to prove that it works.
d) The probably 2 lines of code required in node.module for the actual functionality. ;)

@Dave Cohen: Sorry it's taken so long. It's just that the tweakier and deeper the functionality in Drupal, the more important it is to make absolutely sure it's well-documented and well-tested. The less a subsystem is understood by the wider Drupal community, the more of a chance there are for obscure bugs to break things.

Here are all 4 of webchick's requests. The new test case for hook_node_grants_alter() is deliberately simple, merely checking that the altered array is != to the original for each $op. (I also changed the grants of node_test.module to be test_ for clarity.

The API docs may need a bit of a rewrite. The only other piece I hesitated on is this:

We allow $grants to be passed by reference, so it is possible that some bad author could turn $grants into a non-array. This would force a fail on return array_merge(array('all' => array(0)), $grants);.

So should we be checking that $grants is still an array here?

The documentation for hook_node_access_records_alter() should have an @see to hook_node_access_records I think. This is probably true for the _grants_alter as well.

Also when I looked this up I noted that the help for hook_node_access_records is minorly broken (merely formatting errors) -- this could probably be corrected in either this or a subsequent patch depending upon what people feel more comfortable with. Since it's a documentation patch it may not be an issue either way.

@merlinofchaos: Could you split that off as a separate issue and tag it "Novice"? Always nice to identify little bite-sized chunks like that that we can give to someone new. :)

Added the @see (plus a @see drupal_alter() for additional clarity.

I also noticed that we call the second hook 'hook_node_access_grants_alter'. This is more clear, I think, but a violation of the _alter naming convention, since the original function is 'node_grants.'

So we might rename this to hook_node_grants_alter.

Here is an alternate patch that corrects the name of the _alter hook and forces $grants to be an array, in case someone munges the variable passed by reference.

This patch may be preferred to the one in #71. Let the committer decide!

We had an extensive discussion and edit of the docblock in #drupal today.

Attached is the final (?) language for the patch. Also removed the $grants array check, since we do not normally do so on drupal_alter() calls.

tha_sun caught a few errata in the last patch.

Let's get this in.

here here! This is awesome.

OK, this is absolutely going to be a killer feature for D7!

The docs are looking pretty good, and I think this will open up node access to a whole new generation of module developers. Big ++ all around here.

Eclipse

Wow, looks like I missed all the weekend fun! Great work, everyone -- the latest patch looks solid.

I assume that fail came from the broken HEAD over the weekend?

WTF? This sits for 9 days (again) and breaks?

Ha. Patch was borked by my own docblock patch #472658: Clean up inline comments; check for Capitals and periods..

Resetting, since only the line numbers changes since last review.

Ok, sorry folks. I needed a good half hour to re-review the docs before committing this patch, and half hours are hard to come by lately! :)

The docs are vastly improved, the examples much more "real world," and I can pretty much follow this the whole way through now. There are still places where the node access system is a bit fuzzy for me, but that's not the job of this patch to fix. It'd be great though if hook_node_grants() and hook_node_access_records() could be given a similar documentation treatment, though. Well done.

Committed to HEAD! WOOHOO! :D Please update the 6.x => 7.x page to inform developers of these new hooks.

Thanks.

@webchick, that was more a testbot rant than a comitter rant.

Docs are Node access hooks now have drupal_alter() functions.

Here's a Drupal 6 version of this, in case someone wants it. Trying it to see if it'll let me get Domain Access and User Relationships Node Access to play nicely together.

hefox also mentioned http://github.com/hefox/alter_grants/blob/master/alter_grants.module which attempts to do this without a hack to core.

There's also the module grants module which ANDs grants instead of ORing them: http://drupal.org/project/module_grants