Tag specific alter hook

I'm fine with this. Attached patch just moves one of the alter unit tests to its own function as a test mechanism, and still passes, so I'm pushing up to RTBC.

And this one doesn't have my htaccess file in it...

This introduces a second mechanism. Do we want to remove the first mechanism?

Removing the main query_alter() hook would make it impossible or very difficult to do multi-tag-based or non-tag-based alteration, just like not having form_alter() would make node forms or "all forms" more difficult.

I can see how this patch would be very nice, and Moshe is correct that it is consistent with the way hook_form_alter_X() is done.

a) It doesn't make sense to me why you are pulling out just that one check into its own hook implementation, when any of those would also suffice. It makes it look at first glance like there's a hole in the testing code, until you read much further and see it there as a separate function. I'd prefer to not "cheat", and instead make a separate test entirely to test this new functionality.

b) I'd prefer to see that second function (tag-specific alter hook) documented like we document preprocess functions, for consistency. So how about:

* Implementation of hook_query_alter_TAG().

c) Do we need to benchmark this, or is the performance impact relatively small?

Nah, for a) I don't mean anything too fancy at all. Just another testX function specifically for testing a tag-specific alter hook rather than the global one.

Something like, in database_test.module, add an alias like "tag_specific_alter_hook" and in database_test.test, have a testTagSpecificAlterHook, probably in DatabaseAlter2TestCase, checking to make sure it's there. It's just a simple test to make sure the hook keeps firing through the ages.

Your points on b) are true, but are moot if we also document the tag-specific hook in core.php, which will happen when I set this CNW after committing and say "Docs please!" ;) This is a must IMO; I don't even think anyone who doesn't read form.inc on a regular basis is aware that hook_form_alter() works this way too... I'll file a second issue to document that one as well, if there isn't one already.

The docs can basically be a single sentence of description and then @see hook_query_alter().

And yes, hook_query_TAG_alter(). Sorry!

Hm. This is something worth considering with this patch:
#330784: With hook_form_$form-id_alter() module's weight might be insignificant

I can imagine this messing up forms pretty bad. I'm pretty terrified to think about how that same behaviour could mess up modules governing access control. ;)

Dramatic yes, but worth pondering. For now, I guess we just document the hell out of it along the lines of #331832: Document hook_form_FORM_ID_alter().

The correct solution to both APIs is chx's hook weighting endeavors. That is not something that we will be able to solve in the DB system specifically, or in FAPI, so it's not really a concern here. We can document it as appropriate in a follow-up patch.

#15 looks fine to me. It doesn't split off a tag-specific-check-only test, but I don't see how we can do that in a non-contrived fashion (more contrived than most unit tests are by nature) so I'm OK with it.

Thanks for the docs in system.api.php, but you're missing the most important part -- an example of how it works! :)

There has to be a reason you're adding this hook, so stick a few lines in each of the functions that illustrate why one would use it.

After that, I'm good to go on this one.

Actually, no. I think the example could use some more documentation. Specifically, I would add some inline code comments to explain different steps.

Either way, seeing this example code makes one eyebrow raise -- the code, and the metadata handling, is a bit of a hack. Is that something we can clean up? Why do you need to specify 'op' and what is 'op'? Why can't we use isSelect() or isUpdate() or something more clean than this meta data? It looks a bit redundant to me.

+    if (!$op = $query->getMetaData('op')) {
+      $op = 'view';
+    }

Moshe, hadn't we discussed breaking the node_access tag up into separate tags for each op rather than sticking that into metadata?

It looks from the patch like the only place the function differs by $op is the last line, and it's just a string replacement, so I guess I'm OK with it being a single tag.

We should also update the handbook page to mention specifically that the node_access tag needs an $account metadata. I can see that being non-obvious. That's separate from committing this patch, though.

Hm. The override ability should then be documented to tell people they don't need to pass $account most of the time. :-)

Committed the last patch to CVS HEAD. I'm marking this 'code needs work' (i) because this needs to be documented in the upgrade documentation and (ii) because of Crell's comment #24. Thanks!