Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
#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. :(
Comment | File | Size | Author |
---|---|---|---|
#10 | project_issue-429792-10.patch | 5.21 KB | slip |
#10 | drupalorg_testing-429792-10.patch | 3.58 KB | slip |
#5 | 429792-5.project-issue-dot_needs_project_api.patch | 5.22 KB | slip |
#4 | 429792-4.drupalorg_testing-dot_needs_project_api.patch | 1.95 KB | slip |
#4 | 429792-4.project-issue-dot_needs_project_api.patch | 5.18 KB | slip |
Comments
Comment #1
dwwI don't have time now for a real solution, but this work-around fixes it -- I just feel dirty going this route. :(
Comment #2
aclight CreditAttribution: aclight commentedWouldn'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.
Comment #3
dwwNot 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.
Comment #4
slip CreditAttribution: slip commentedI'm marking this critical because this bug is making it impossible to cleanly install the profile without patching.
How do these patches look?
Comment #5
slip CreditAttribution: slip commentedproject issue patch was slightly broken... heh. Hopefully all good now.
Comment #6
webchickConfirmed 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.
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.
Comment #7
webchickI guess that's a "needs work." :) Thanks so much for looking into this, though!
Comment #8
dwwOk, 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
Comment #9
slip CreditAttribution: slip commentedwebchick,
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.
Comment #10
slip CreditAttribution: slip commentedOK, 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!
Comment #11
slip CreditAttribution: slip commentedwhoops needs review...
I wonder if the issue I had above means they should be separated again?
Comment #12
dwwGreat, 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
Comment #13
dwwNote, 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...