Subtask of #1569988: [Meta] Project issue settings D7 cleanup

We need to port the 'Display user signatures on issue followups' setting (project_issue_show_comment_signatures) to D7 and make it configurable per node type, not a site-wide global setting.

Comments

senpai’s picture

Assigned: Unassigned » rgristroph
Issue tags: +sprint 2

Assigning and tagging.

iamcarrico’s picture

Assigned: rgristroph » iamcarrico

Assigning

iamcarrico’s picture

Status: Active » Needs review
StatusFileSize
new2.73 KB

Okay, here I moved the setting over, and changed all functions that related to it in its previous life.

dww’s picture

Status: Needs review » Needs work

A) Can you fix the code style bugs in here? I could of course add the missing spaces myself, but if I keep asking you about this, I'm hoping you'll just get in the habit of writing the code to conform to Drupal's code standards in the first place. ;)

B) Have you confirmed that D7's project_issue_preprocess_comment() works the same as D6, and that this actually works as expected?

C) Seems unnecessary to append to variables[] in the first foreach in project_issue_uninstall(), just so we can iterate over each of those again a few lines later. Wouldn't it be clearer (and a bit more efficient) to just call variable_del() directly while iterating over the node types (and move that code block to after where we iterate over $variables)? That'd also make more sense once we have multiple per-issue-node-type settings and we need a nested foreach. See what I mean?

Thanks,
-Derek

senpai’s picture

Issue tags: +sprint 3, +sprint 4
iamcarrico’s picture

Status: Needs work » Needs review
StatusFileSize
new8.05 KB

A) Done, I also fixed a few bugs across the module as a whole.

B) It does.

C) I was merely using what was already there. Personally, I dont see the point of the for_each in the first place, but that is just me. (Unless we make a foreach based on a query on the variable table for anything starting with "project_issue%", which would remove most of the excess code there. But as you say, so shall it be done.

dww’s picture

Status: Needs review » Fixed

A) Cool, although I split off the code-style-only changes to a separate commit so as not to confuse things. Also, you still had this:
foreach($nodetypes as $name => $obj) { instead of this: foreach ($nodetypes as $name => $obj) { (note the space after foreach). ;) Fixed that.

B) Indeed! Also tried it myself and it's working like a charm. ;)

C) Yeah, it's minor crap, I admit. I should probably stop caring at that level of detail. Thanks for humoring me and my ridiculously high standards.

Committed and pushed... yay!

Thanks,
-Derek

iamcarrico’s picture

Assigned: iamcarrico » Unassigned

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