CivicActions is reviewing and upgrading multiple modules for use on client sites. Part of this work is a coding standards review. Attached you will find a patch based on a review with the coder module and a careful examination of the code. Thanks!

CommentFileSizeAuthor
#2 filefield-codereview.patch9.73 KBwmostrey
filefield-codereview.patch32.44 KBwmostrey
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

drewish’s picture

drewish CreditAttribution: drewish commented
Status: Needs review » Needs work

looks like the wrong file.

wmostrey’s picture

wmostrey CreditAttribution: wmostrey commented
Status: Needs work » Needs review
FileSize
filefield-codereview.patch9.73 KB

You are correct, sorry about that. This is the correct one.

drewish’s picture

drewish CreditAttribution: drewish commented

i'm not sure dopry will be into the capitalized TRUE/FALSE. i'd changed that once and it got changed back.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Took a look at this patch and it looks fine to me. I personally prefer the D7 string concatenation but that's not an issue here. Obviously it's down to you/dopry as maintainer whether you use lower or upper case TRUE/FALSE, but on the basis that the patch makes things coder compliant, and applies fine, marking RTBC. We could probably re-roll without those if it's a no ;)

Note, I ran into #301398: Install error: Call to undefined function content_notify(). when installing.

dopry’s picture

dopry CreditAttribution: dopry commented
Status: Needs review » Closed (won't fix)

I can run coder.module as easily as the next guy. Frankly if the only input you can give me is based on the case/whitespace in my code you're really wasting your time. I'm not going to make changes just to adhere to coder.module. When I work on the noted sections of the code I may change them, but It's unwise to make changes simply for aesthetics sake.

Why not try to give people real reviews of their code... Look at how variable initialization is handled, confusing variable names, note places needing better documentation, do a security analysis checking, check out concatenation, can you spot the forgery exploit in the ajax callback? Maybe write a few functional test cases for what seem like key functions. Immediately path and url generation come to mind, tests for hook_file? You know there are a lot of ways to contribute besides running coder.module.