Closed (fixed)
Project:
Notices
Version:
5.x-1.x-dev
Component:
Miscellaneous
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
8 Oct 2008 at 19:47 UTC
Updated:
6 Sep 2009 at 05:40 UTC
Attached patch fixes the following
| Comment | File | Size | Author |
|---|---|---|---|
| notices-coder_and_misc.patch | 18.69 KB | deekayen |
Comments
Comment #1
liquidcms commentedwas about to apply your patch.. but noticed a few of these things aren't correct so i might need to do this a bit later when i have time to go through them one by one and apply manually.
* coder module notice fixes
o remove trailing spaces - don't care
o add @file block - not sure what this does, other modules dont use this
o spacing on string concats - yes, thought i had run this through coder so not sure; but cant find example in your patch
o missing use of SQL replacements - yup, should do this
o ?> at the end of .install - you shouldnt actually add ?> at the end of your files it makes it possible for trailing lines to kill php
o indent docblocks one space - dont care
o put brackets around block statements (if, while, foreach) - my personal pref i guess; i dont like verbose code
o booleans should be all caps - yup
o use db_query_range() instead of db_query with LIMIT in the SQL - yup
* version and project shouldn't be in .info... project module takes care of those things when it makes packages for release
- hmm, never knew this, i guess since most of my modules don't make it to d.org cvs, but i still want them to have ver info
* remove useless hook_help() with 4.7 module description
- this is valid for 5.x http://api.drupal.org/api/function/hook_help/5
* use t() in drupal_set_messages
- yup
* drupal_set_message only has error and status params for the type and it was using 'notices'
- nope, you can add your own and this was done for a reason
* standardize spacing in hook_menu() array elements
* use automatic form _validate() hook instead of an if statement in the _submit()
- yup
* _notices_page_form() was referring to nodes in the form vars - updated to notices for clarity
- sure
* use XHTML standard br tags
- yup
plus a couple other things that were just your personal preference; which i didnt change
i'll try and wade through these soon and add manual ones that i agree with
thanks for being so thorough
Comment #2
deekayen commentedWould it help if I broke this out into smaller patches in other issues?
Comment #3
liquidcms commentedi have done the coder ones (normal level) and sql ones, likely a few others.. coming in next rel later today
Comment #4
deekayen commented