Closed (won't fix)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
26 Aug 2013 at 12:18 UTC
Updated:
7 Sep 2013 at 10:30 UTC
Jump to comment: Most recent file
Comments
Comment #1
chester_martin commentedHi,
I hadn't heard about notfound.org before but love the idea and your effort for a Drupal integration.
An automated test shows some coding standards issues that needs to be resolved:
http://pareview.sh/pareview/httpgitdrupalorgsandboxpaulvb2071587git
Also I have a few suggestions:
- It might be better to integrate this with the standard 404/403 settings fieldset on admin/config/system/site-information using hook_form_alter().
- Your project page and the description on this issue thread wouldn't suffer from some basic info about notfound.org.
- My personal opinon is also that I would like a way to specify a message that describes what the user is looking at on the 404-page.
Otherwise, I tested on a live site and everything worked as aspected.
Martin
Comment #2
chester_martin commentedComment #3
kasalla commentedmanual review:
not_found.install
------------------------------
on line 11 you set the variable value of "site_404" to "not_found". But in hook_uninstall you check if the variable has the value "notfound404" and then delete it (maybe set it back to default?). I think it should be "not_found" too.
not_found.module
------------------------------
Find no problems, but:
From the Project application checklist:
Kind regards
kasalla
Comment #4
paulvb commented@Chester_martin
Thanks for the feedback. I integrated your suggestions in the project.
- http://pareview.sh/pareview/httpgitdrupalorgsandboxpaulvb2071587git, the test is without errors now.
- I liked the suggestion about putting the config under site-information. It will be better for useability so I changed that also
- I added a description to my project page which I also added to my README
- I added a description field to the config that will be printed above the config
@kasalla
Thanks for your feedback as well.
- I corrected the bug in the .install
- Since i won't be writing code just to get to 120 I think it would be better to promote the project manually.
Comment #5
chester_martin commentedHi paulvb,
Great work! looks awesome. One last suggestion from me:
To make it even more sleek, you might consider the possibility to enhance the settings form with some ajax. Instead of typing in "not_found" in the textfield you could put a checkbox somewhere "Use notfound.org" (or something). When checking that box the original textfield is hidden and your notfound-specific fields appear.
As I said, that's just a suggestion, but probably it will get you closer to the 120 lines at the same time;-)
Good luck!
Comment #6
kasalla commentedCreated a small patch for the AJAX form, maybe you want to use it.
edit:// I made a small mistake at line 53.. =/ shame on me
Comment #7
PA robot commentedWe 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 #8
bohartIf that fails for whatever reason please get back to us and set this back to "needs review".
Regards,
bohart.