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

CommentFileSizeAuthor
panels_sql.patch5.04 KBstella
panels_upgradeD6_minor_0.patch6.66 KBstella
panels_upgradeD6.patch3.14 KBstella

Comments

catch’s picture

catch
he/him
Primary language English
commented
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.

sdboyer’s picture

sdboyer commented
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.

stella’s picture

stella commented

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.

sdboyer’s picture

sdboyer commented
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.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.