Posted by joachim on September 12, 2009 at 8:19pm
| Project: | Coder |
| Version: | 7.x-1.0-beta2 |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
I got this:
Line 68: hook_access removed in favor of hook_node_access. (Drupal Docs)
function _trackerlite_myrecent_access($account) {
Comments
#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.
#7
This patch doesn't apply anymore.
I'm also getting false positives on functions like MODULE_add_user()
hook_user has been removed in favour of hook_user_$op functions, for example hook_user_validate. (Drupal Docs)
function acl_add_user($acl_id, $uid) {
#8
Hmmm that shouldn't happen, as it's using the module name to build the hook name.
Can you check what it's building in your case with this line:
$hook = array('#value' => ' ' . $module_name . '_' . $rule['#value'] . '(');#9
I can't find this line in the beta2 code.
#10
That line is in the patch.
#11
Ah, I haven't been able to apply the patch to beta2. Are you sure it still applies?
I haven't tried it with HEAD, but the patch is from last October, well before beta2.
#12
Sorry; my mistake, I completely misread your earlier comment :/ -- I thought you meant you were getting false positives even with the patch. Doh!
I haven't time to reroll this at the moment, but it's not a very big patch, so unless things have changed massively shouldn't be too much work :)