coding standards violations
toemaz - April 16, 2008 - 17:05
| Project: | Latest Members |
| Version: | 5.x-1.0 |
| Component: | Code |
| Category: | task |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed |
Description
I downloaded the module code from http://www.hygen.net/Malaysia/index.php?q=node/32 and checked out the code.
I'm sorry to tell you but this module has a lot of violations against the Drupal coding standards.
Just a few examples:
- HTML output should always be preformed in a theme_ function so it can be overridden by the theming system
- The display_latest_members() function is violating the modules name space
- The doxygen comment style should be revised
- Use two space indents
- Always use a space after a comma
- Never use capitals in PHP variable names
- Instead of printing your own HTML for the user picture, you should reuse Drupal core functions like theme_user_picture
Consider this list as help. Anyhow, thanks a lot for sharing the code!
If you check in your code, I will send you patches.
#1
I took the liberty to rewrite the module.
Notice the following changes to the business logic of the code:
Other than that, I cleaned all the coding standards violations.
#2
@toemaz.
Thanks for the module rewrite! I went ahead and downloaded your rewrite and have it working on my site!!! Awesome!
Although the module itself is now working, I wanted you to be aware of some issues.
1. This 1st issue is still present.
For some reason, it now shows up twice on my "Modules" page (site.com/admin/build/modules) in the "Other" section.
(EDIT: There are other modules that show up twice, so this issue isn't something specific to "Latest Member", but I thought I'd point it out just in case you can fix it.)
This is what I see:
Other
...
(checkbox) Latest Members
(checkbox) Latest Members
...
2. This 2nd issue is now resolved.
After clicking on the 1st "Latest Members" checkbox on the "Modules" page to enable it and clicking "Save configuration", I then went to the "Blocks" page and the module wasn't there. I double checked and it was nowhere to be seen. I went back to the "Modules" page and unchecked the 1st "Latest Members" (see my #1 above) and checked the 2nd "Latest Members". I clicked "Save configuration" and got the following error message with nothing else on my screen (i.e., a white screen except for the following error):
Fatal error: Cannot redeclare latest_members_uninstall() (previously declared in /home/.chagall/balance10/wellnesscorps.com/sites/all/modules/latest_members/latest_members_1.install:6) in /home/.chagall/balance10/wellnesscorps.com/sites/all/modules/latest_members/latest_members.install on line 8
I clicked back to try and see if the issue would go away and yes, I was back at my "Modules" page and both "Latest Members" boxes were unchecked. I again clicked on the 2nd "Latest Members", clicked "Save configuration", and this time I received no errors. Not sure what happened the 1st time I tried enabling the 2nd "Latest Members" module, but I'm glad the error message went away. I went to the "Blocks" page, saw that the "Latest Members" block was now present, reviewed its configuration settings and found them OK, went back to the "Blocks" page and, finally, selected "right sidebar". I clicked on "Save blocks" and am happy to report that the "Latest Members" block now correctly appears on the right side!
Thanks again for your help! I really appreciate it!
#3
@dangibas.
Thanks for the really helpful module and I can see it becoming really popular!!!
To answer your email to me from 4/15/08 that referred to when you updated the module back on 4/14/08 (http://www.hygen.net/Malaysia/index.php?q=node/32#comment-10), unfortunately I was never able to get your version of "Latest Members" to work. I tried various "Path to files directory" but all I usually saw was a big red X where the member's avatar was supposed to be in the "Latest Members" block. And it was hit or miss where sometimes I would see an avatar and at other times I would see a red X along with some writing (e.g., User xyz profile) where the avatar should be.
Here's what I tried typing in to the "Path to files directory" box to no avail:
/system/
/
/files
and I also tried not having anything in the box (it just being blank - no slash or anything).
Nothing worked.
However, I now have the module working as you can read from my post above. I hope you can take advantage of the helpful rewrite in order to more easily get the code checked into the Drupal CVS.
Keep up the good work and thanks again! I really appreciate you having taken the time to develop your module!!!
#4
Hi toemaz,
Brilliant thanks! Thats just what the module needed prior to going into Drupal CVS. I will go through the code and upload it to Drupal. Thanks for taking the time to clean this up!
Hi Walt,
Very welcome! And it looks like toemaz gas done a great job on fixing it up. If you still have problems I will take some time to help out. Have you tried the new rewrite toemaz posted above?
Lets get this module up first ;)
#5
Hi Dan,
Yes, I tried the new rewrite toemaz posted above. I responded at #2 above:
http://drupal.org/node/247465#comment-810435
Thanks again!
#6
Thx for the feedback ;-) Always fun to get positive reactions.
@Walt
The issues you mention (1 & 2) have probably nothing to do with the code itself. The double appearance of the module might have to do with the fact that you uploaded the module to two different folders (e.g. all/modules & mysite.com/modules). I'm just guessing.
Anyway, I guess it worked out for you.
@dangibas
Before you try it out, check out the changes to the theming part. The css you used for the original module will certainly not work anymore. If you have issues, report them asap so I can have a look.
Don't hesitate to check it in. The more eyeballs we can have on it, the better.
#7
@toemaz
After some thought, I agree that issue 1 (in my post above) probably has nothing to do with the "Latest Members" code itself.
After an extensive search of drupal.org for a possible cause on issue 1 where several modules show up twice (and even 3 times!) on my "Modules" page, I have no idea what causes the duplicates. I only uploaded the modules in question once, and only to the folder at:
mysite.com/sites/all/modules
I opened an issue here, attached various screen shots of the duplicate modules, and titled it:
Various modules on my "Modules" page show up twice and even three times
Regarding issue 2, I found Fatal error: Cannot redeclare blah_function() in ../modules/blah.module. This may be of help, although I'm not smart enough to make sense of it and will simply provide the link for you smarter types. :)
#8
@dangibas
Please check in the code. It should normally take a couple of minutes.
If it is new to you, you may follow the Step-by-step: Create a CVS project guide.
#9
@dangibas
I'd like to ask you to check in the code. If you don't feel comfortable doing so, I can help you out. Perhaps you could make me co-maintainer so I could initialize the code import.
#10
@dangibas
Since I'm not getting any feedback, I'd like to inform you that I plan to create a new project and check in the code since I want to deploy it on a production server. I wish to use the drupal update mechanism to track the module and make use of the issue tracking.
Checking in the code takes a couple of minutes. So, if you still consider doing it, don't wait.
#11
please do not create a duplicate project. My suggestion is to apply for co maintainership of this module so that you can make the changes necessary.
#12
Co maintainership requested via a new issue: http://drupal.org/node/254230
Will come back to this issue to close it when the module has been checked into the CVS.
#13
Finally the code has been checked in. The releases (5.x-1.0 & 5.x-1.x-dev) will be made during the next Drupal packaging run.
For those who are interested how a creating a module works on Drupal CVS, follow the handbook for TortoiseCVS
#14
Automatically closed -- issue fixed for two weeks with no activity.