#405478: Do a proper page/code split of project_issue broke this install profile. The reason is that we're being lame and directly invoking form submit handlers to do things, instead of having a sane API, e.g. to add new issue status values. :(

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dww’s picture

Status: Active » Needs work
FileSize
846 bytes

I don't have time now for a real solution, but this work-around fixes it -- I just feel dirty going this route. :(

aclight’s picture

Wouldn't we still have to manually include admin.issue_status.inc if we had an API for this, unless the API function was in project_issue.module? In a sense that would diminish the benefit of the code split itself.

dww’s picture

Not if that API function was tiny (which it would be) compared to all the FAPI code to generate the form, validate and submit.
And of course, in D7 with the registry, we could even put the API function where ever we wanted.

slip’s picture

Priority: Normal » Critical
Status: Needs work » Needs review
FileSize
5.18 KB
1.95 KB

I'm marking this critical because this bug is making it impossible to cleanly install the profile without patching.

How do these patches look?

slip’s picture

project issue patch was slightly broken... heh. Hopefully all good now.

webchick’s picture

Confirmed the patch fixes the problem. However, instead of "magical" parameters that are one big glomp of properties, it'd be nice to split these out a little.

I'd recommend instead of:

+function project_issue_admin_states_add_status($status) {

We do:

+function project_issue_admin_states_add_status($name, $options = array()) {

If options aren't specified, they just default to whatever makes the most sense.

Update is a bit trickier though, since it could be any of those properties that are different.

+++ project_issue.module	8 Nov 2009 06:29:03 -0000
@@ -590,6 +590,60 @@ function project_issue_add_auto_followup
+  $state = db_fetch_object(db_query('SELECT name, weight, author_has, default_query FROM {project_issue_state} WHERE sid = %d', $sid));
+  // Check to see whether the record needs updating.
+  if (($state->name != $status['name']) || ($state->weight != $status['weight']) || ($state->author_has != $status['author_has']) || ($state->default_query != $status['default_query'])) {
+    db_query("UPDATE {project_issue_state} SET name = '%s', weight = %d, author_has = %d, default_query = %d WHERE sid = %d", $status['name'], $status['weight'], $status['author_has'], $status['default_query'], $sid);
+    return TRUE;
+  }
+
+  return FALSE;

Also, I know you're just moving code around, but this chunk doesn't really make any sense to me. Why not select where name = %s and weight = %s and... and just count the number of rows coming back. If it's 0, then you know you need to perform the update.

And finally, I don't think return FALSE is the right thing to do here. It's not that the update /failed/, it's that there was nothing /to/ update.

Personally, I would just always perform the update without this extra check, since that way you're just doing one query instead of 2. Dunno how dww feels about that, though.

And actually, in thinking about this a bit more, I wonder if we should just do a blanket "project_issue_admin_states_save($name, $options = array())" function that does its behaviour based on whether the $name parameter already exists in the DB? This would be similar to the convenient node_save() and user_save() pattern in core, where you don't need to call separate functions for add vs. update, it just always does the right thing.

This review is powered by Dreditor.

webchick’s picture

Status: Needs review » Needs work

I guess that's a "needs work." :) Thanks so much for looking into this, though!

dww’s picture

Title: Call to undefined function due to project_issue page split » Add API for manipulating issue status options
Project: Drupal.org Testing » Project issue tracking
Component: Code » Issues
Category: bug » task

Ok, this is silly. I've had #1 applied in my local copy for months, and completely forgot the profile was broken all this time. :(

So, I just committed #1 to HEAD of the profile. So, the bug is resolved. Now, this issue is about adding an API to project_issue to manipulate the status options, which is most of what we've been discussing here.

Sorry for leaving the profile broken for so long, and thanks to slip and webchick for reviving this thread!

Cheers,
-Derek

slip’s picture

webchick,

Thanks for the thorough review :)

I was thinking of those functions as a quick fix to get around issues in #1 and not as an API that people would actually use, although that's what it's supposed to be designed for! Working on a new patch now.

slip’s picture

OK, here it is.

I think the code is a lot closer thanks to webchick's suggestions. I've tried to combine the update and save functions and I think it works pretty well.

One thing that was a little tricky though was that when you add a status option you shouldn't be specifying a sid (you would need at least the name). And when you're updating and modifying the name of a status option, the function has to rely on the sid.

So I didn't do
+function project_issue_admin_states_add_status($name, $options = array()) {

But instead the function accepts an array that can have all the columns of the db table specified as keys... Is that considered "magic" still?

thanks again!

slip’s picture

Status: Needs work » Needs review

whoops needs review...

I wonder if the issue I had above means they should be separated again?

dww’s picture

Status: Needs review » Needs work

Great, thanks. Definitely getting closer.

A) project_issue_status_option_save() should just use drupal_write_record() instead of duplicating it.

B) Likewise, it should try to reuse the API for drupal_write_record() for the $update argument to handle insert vs. update. See the above api.d.o link for details. While it's not necessarily the best possible API for handling this, it's in core, and therefore, it's more consistent DX for this function to work the same way.

Thanks!!!
-Derek

dww’s picture

Note, if this ends up being a trivial one-line wrapper around drupal_write_record(), maybe we don't even need an API function for this at all, and we should rename this issue "use drupal_write_record() for manipulating issue status options". ;) But, even if it is a trivial wrapper, it's probably nicer to make it a real API function so that we could change the implementation details later if we needed to add something to it...