Closed (fixed)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
16 Apr 2012 at 14:32 UTC
Updated:
16 May 2012 at 15:10 UTC
Sandbox: http://drupal.org/sandbox/jasperknops/1535526
Git repository: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/jasperknops/1535526.git hooker_hunter
Developed for Drupal 7.x
The hooker hunter enables you to search the usage of hooks throughout your project. If you search for a hook, a list of modules that implement this hook are displayed in the order they will be executed. If you search by module you will see a list of all hooks implemented by that module.
It is a tool for developers to quickly browse trough the life of hookers.
Comments
Comment #1
patrickd commentedwelcome,
Please read about the project application workflow.
You are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
(At least this has to be done before switching back to needs review)
Please take a moment to make your project page follow tips for a great project page.
while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxjasperknops1535526git
Moreover I'm afraid that project is too short to approve you as git vetted user. We are currently discussing how much code we need, but everything with less than 120 lines of (custom) code or less than 5 (functions that are not hooks just providing some information) functions cannot be seriously reviewed. However, we can promote this single project manually to a full project. Alternatively you can try to add more functionality to it so there's more code to review.
We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.
regards
Comment #2
jasperknops commentedCreated new branch: git clone --branch 7.x-1.x jasperknops@git.drupal.org:sandbox/jasperknops/1535526.git
Updated code with violations described at http://ventral.org/pareview/httpgitdrupalorgsandboxjasperknops1535526git
Because it is just a small developers tool project I think the project page doesn't need more explanation?
I also have more ideas for features to add to this module. Do you want me to expand the module first before it can be approved?
Thanks!
Comment #3
patrickd commentedEven if this tool is for developers, please add short installation and usage instructions anyway, maybe also a list of features (even if there's currently only one) and a screenshot (you can upload it here: http://drupal.org/node/add/image).
We can make a full project out of it even if it is small, but it's probably not enough to give you vetted user permissions to promote projects yourself. Therefore, yes, some more features would be good ;-)
Please check the automated review again, it's still complaining about the missing readme and some little coding style issues you may could fix. (you can re-run it by using the repeat review tab).
Comment #4
jasperknops commentedI did everything you asked, I think ;)
Great tool btw, the automated review.
Comment #5
jasperknops commentedI added some extra functionality and ran the automated review again.
Comment #6
pgogy commentedThe git URL should be - git clone --branch 7.x-1.x http://git.drupal.org/sandbox/jasperknops/1535526.git hooker_hunter (i think)
I get an error
Notice: Undefined variable: api_link in hooker_hunter_module_page() (line 28 of C:\xampp\htdocs\drupalclean\sites\all\modules\hooker_hunter\includes\hooker_hunter.admin.module.inc).
when module hunting
Also not sure on the difference between the tabs
"hook" and "hook menu" - perhaps could do with some text to explain?
Nice module though, like it as an idea.
Comment #7
jasperknops commentedI adjusted the git repo, removed the code that triggered the PHP Notice and added some extra form descriptions and a field_prefix.
Comment #8
ClaudeSchlesser commentedHello,
------------------------------------------------------------------------------
The description in the module list is not meaningful "Hunt the hookers by module or hook". There is no description at all in the configuration pannel. If I login to my administration area in a couple of months I might not remember what the module is for.
------------------------------------------------------------------------------
The README.TXT does not need a @file comment.
------------------------------------------------------------------------------
On the page "Enter the name of the hook you want to hunt for."
If the hook has no menu items (example: hook_permission), a recoverable fatal error is thrown:
------------------------------------------------------------------------------
On the page "Enter the name of a module and you will see all menu items defined in MODULE_hook_menu()."
If the module that you enter has no menu hook, nothing appends. A notice should be displayed in this
case or the users will think that you module does not work as expected. Nothing happens? Hmmm?
------------------------------------------------------------------------------
This said, this is a very useful and well thought out module.
I will keep it for further use!
Regards,
Comment #9
jasperknops commentedHi,
I made the changes you asked for. I can't reproduce the error you had. Can you tell me again how I can trigger this. You say you are looking for menu items of module hook_permissions. Not very logical ;-)
Thanks!
Comment #10
ClaudeSchlesser commentedThe issues that I discovered in my previous review seem to have been been fixed.
I can't reproduce the error you had. Can you tell me again how I can trigger this.
The error no longer occurs ;)
Sorry to be picky, but in my opinion you should rename the module to "Hook Hunter" (or something else).
Hooker is a slang word for prostitute ...
This said, I didn't see any other issues.
Comment #11
jasperknops commentedI do know what a hooker is ;-)
So you want me to change the name?
Is there something else I need to do to make it a real project?
Comment #12
ClaudeSchlesser commentedNo, it's only a suggestion ;)
In my opinion it would be ready for a real project with another name.
Comment #13
jasperknops commentedI renamed the module to hook_hunter.
Comment #14
crobinson commentedI know nit-picking over things like names seems tedious, but it's important to be culturally sensitive especially given how diverse Drupal's user base is.
I did a final code review on this project and found nothing blocking its release. I do have some minor comments but while I think they're worth addressing at some point, none of these fit the guidelines for "Needs Work"ing a project. Therefore, I'm marking this project "Reviewed and Tested."
1. In README.txt there are two spelling errors: "quickly browse trough", "to you module folder".
2. There is another one in includes/hook_hunter.admin.hook_menu.inc:49: "doesn 't" has a space before the apostrophe.
3. The same function's comment block doesn't provide a $returns. This isn't a hard requirement in any of the standards documents, and this function doesn't look like something others would really find useful to call since it's so form-related. But you provide an @param so I'm just making the suggestion for consistency to provide an @return. The same goes for the other two include files.
This looks like a really useful module - thanks for the contribution! I'm looking forward to its release.
Comment #15
jasperknops commentedI understand all nit-picking completely.
I adjusted your last remarks. What is the next step to make this an official project? I added a 7.x-1.0 tag to the repo.
Thanks a lot!
Comment #16
crobinson commentedJust wait a day or so. Being marked Reviewed & Tested was the next step. When they have time, one of the Drupal GIT Admins will make sure the reviewers didn't make any mistakes and then mark the ticket "Fixed". That will give you the access to convert your Sandbox project to a full project, and also to make new projects in the future without going through the same procedure. This is outlined here: https://drupal.org/node/532400
While you're waiting, you'd be doing the community a favor if you did the same kind of review on other projects. You don't need it any longer for the "bonus points" but there's a big issue queue and by now you must be familiar with the frustration that comes from such a long process! Here's a link to the queue:
http://drupal.org/project/issues/projectapplications?order=last_comment_...
Comment #17
patrickd commentedComment #18
patrickd commentedWhen I hook-menu hunt for block module I get "Recoverable fatal error: Object of class stdClass could not be converted to string in hook_hunter_hook_menu_page()".
Also It can't find all existing hooks for some modules (eg. better batch implements multiple hooks like hook_menu(), hook_batch_alter but only hook_init() is found) - probably an issue with module_implements().
Some minor things I found (suggestions)
'ajax/' . HOOK_HUNTER_MENU_ITEMI wouldn't prefix the path with ajax/ here as you could possibly collide with other menus here - it's usual to just suffix it with /autocompleteLeaving rtbc as I'm too tired to continue at the moment, sorry.
Comment #19
patrickd commented$items = call_user_func($function);you should check first whether this function exists.$module_data[$module]->info[package] == 'Core';Generally doing some more inline-documentation wouldn't hurt you ;-)
After all (please add the mentioned check_plain()s before creating your fist release) this looks good enough for me.
Thanks for your contribution and welcome to the community of project contributors on drupal.org!
I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best - don't forget this power brings responsibility.
Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.
As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.
Thanks to the dedicated reviewer(s) as well.
Comment #20.0
(not verified) commentedupdated git repo