false positives for hook sniffing
joachim - September 12, 2009 - 20:19
| Project: | Coder |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Description
I got this:
Line 68: hook_access removed in favor of hook_node_access. (Drupal Docs)
function _trackerlite_myrecent_access($account) {
#1
I'll look into this and provide a patch, I'll also check for other false positive hook matches for functions that start with "_" which raises the question, I know it's probably highly unlikely but what if someone creates a module called "_my_modulename"?
#2
I've not looked at the code for this module, but the way to ccover for this would be to grab the module's root name from its filename or the system table and search for ROOTNAME_HOOKNAME. Then you're covered for that.
I wrote some hooksniffing regexps for module_builder which you might want to look at :)
#3
So having had a chat about this on IRC, I see the current code says this:
array('#type' => 'regex',
'#value' => 'function\s+[a-z0-9_]+_(access)\s*\(',
'#function-not' => '^\w+_node_access$',
'#warning_callback' => '_coder_review_7x_hook_access_warning',
),
It seems to me we could abstract some of this.
Otherwise, every single data key in hook_reviews that looks for a hook is reimplementing the same regexp, bar a hook name.
So instead:
array('#type' => 'hook',
'#value' => 'access',
'#function-not' => '^\w+_node_access$',
'#warning_callback' => '_coder_review_7x_hook_access_warning',
),
Whatever uses hook_review then has to build a regexp like:
'function\s+$modulerootname_$hookname\s*\('#4
This patch makes '#type' => 'hook' available for use in implementations of hook_reviews() enabling simpler and more accurate sniffing for API hooks. This is useful when you need to test for obsolete hooks and can be done like this:
// In the $rules array in /coder/coder_review/includes/coder_review_7x.inc for examplearray(
'#type' => 'hook',
'#value' => 'access',
'#warning_callback' => '_coder_review_7x_hook_access_warning',
),
#5
Patch is good.
But was missing any actual implementation to try it.
So I have changed the hook_access check to use this new thingy instead, and run trackerlite through a coder review.
Success:
function _trackerlite_myrecent_access($account) {
not picked up.
Added this to my code
funtion trackerlite_access() {
and this IS picked up.
Here's an updated patch.
I think however this needs some documentation on how to use it in a hook_reviews file.
In particular, it's important to say whether you need to give the #value as 'access' (which is the case) or 'hook_access' (which isn't, but one could suppose it is).
However, the place for this says:
// @TODO : document hook_reviews()
so that's another issue really -- #598600: // @TODO : document hook_reviews() in fact. We can't add to documentation that doesn't exist at all yet.
#6
Yes, good points, thanks joachim.
We can remove '#function-not' => '^\w+_node_access$', too, this is only used for regex type reviews.