Closed (fixed)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
18 Oct 2015 at 13:26 UTC
Updated:
18 Oct 2016 at 20:34 UTC
Jump to comment: Most recent
Comments
Comment #2
PA robot commentedThere are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxkissjozsef2594193git
We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)
Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #3
kiss.jozsef commentedFixed the syntax and git errors.
Comment #4
rahulbaisanemca commentedHi kiss.jozsef kindly add hook_menu with this format. https://www.drupal.org/node/161085
Thanks,
Rahul.
Comment #5
kiss.jozsef commentedHi rahulbaisanemca, could you please explain why to add hook_menu? or you meant hook_help ? because in the documents there is reference took hook_help.
Comment #6
rahulbaisanemca commentedHi @kiss.jozsef thanks for pointing out my mistake, yes i meant to add hook_help but by mistakenly i have written hook_menu. :)
Comment #7
kiss.jozsef commentedImplemented hook_help and added clear log functionality
Comment #8
rahulbaisanemca commented@kiss.jozsef for implementing hook_help.
Comment #9
neeravbm commentedAutomated Review
Best practice issues identified by pareview.sh / drupalcs / coder.
Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.
Manual Review
The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.
If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.
This review uses the Project Application Review Template.
Comment #10
abhishek.pareek commentedHi kiss.jozsef
Well I saw your code and its good, except a thing:
You are storing some variable in a validation callback, which is not a good way, save your data in form submit not in validation callback.
Add a submit function in that form alter of yours and then save your variable there instead of doing all that during validation.
Comment #11
kiss.jozsef commentedHello abhishek.pareek,
I fixed it, added the callback as submit and not as validate, not sure why i wrote it like that to begin with.
Thanks for the review.
Comment #12
visabhishek commentedModule looks good and working for me. I think we don't have any blocker points. So i am marking as RTBC.
Only one suggestion :
Delete the variable "membership_entity_type_settings"
Comment #13
kiss.jozsef commentedHello visabhishek,
added hook_uninstall
Thanks for the review
Comment #14
mlncn commentedThanks for your contribution, kiss.jozsef! You are now a vetted Git user. You can promote this to a full project.
When you create new projects (typically as a sandbox to start) you can then promote them to a full project.
Here are some recommended readings to help with excellent maintainership:
You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!
Thanks, also, for your patience with the review process, especially my slowness in approving following review. We know it's broken and are trying to fix it.