Patches for issues found in coder review [#315965]

Hi,

CivicActions is reviewing and upgrading multiple modules for use on client sites. Part of this work is a coding standards review using coder. I realize the code is still in development for Drupal 6, but attached are three patches which may be of use. None of the issues "fixed" are bugs per se, though some of the ones in the "panels_upgradeD6.patch" file might be.

Cheers,
Stella

File	Size	Author
panels_sql.patch	5.04 KB	stella

panels_upgradeD6_minor_0.patch	6.66 KB	stella

panels_upgradeD6.patch	3.14 KB	stella

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

catch

he/him

English

CreditAttribution: catch commented 1 October 2008 at 22:04

Status:

Needs review

» Reviewed & tested by the community

I reviewed all three patches visually and did a short clickaround test to ensure no accidental breakage. Like Stella says they aren't major issues, but fwiw I think they're rtbc for the changes made.

Comment #2

sdboyer CreditAttribution: sdboyer commented 2 October 2008 at 16:18

Assigned:

Unassigned

» sdboyer

I'll look at these during my Panels time this weekend. The parts pertaining to the the form contexts may or may not be included, simply because there's a decent bit of re-thinking that needs to be done with those regardless. I'm also not sure why the query in the sql patch was split into two - I actually find that approach LESS understandable, as it underemphasizes the difference between the two queries. That is, you have to grok the whole thing instead of seeing the parts that are actually different broken out explicitly.

But yeah. I'll fully review this weekend.

Comment #3

stella CreditAttribution: stella commented 2 October 2008 at 17:36

Fair enough point about sql patch - probably a matter of style. However I would still encourage you to adopt the changes in that patch which make use of the new D6 db_placeholders() function.

Comment #4

sdboyer CreditAttribution: sdboyer commented 5 October 2008 at 22:35

Status:

Reviewed & tested by the community

» Fixed

OK, these have been committed, sans the changes from #2. The form-related stuff in the contexts is likely to be changed again before a new version is released, though.

Also, in future - just one patchfile with all the changes, please. If the content of the patch is really different enough to justify different patchfiles, then open separate issues for each.