Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.When project_issue_update_5200() runs, it checks to see if comment_upload is installed, and if not aborts the updates of project issue. The problem is that when the user then enables comment_upload and goes back to update.php, the project issue update number select box will be set for 5201. The user is warned of this and warned to set it back to 5200, but I'll admit that this is sub-optimal workflow, but it appears that core doesn't really provide any other options.
What we can (and probably should) do is to, at the end of project_issue_update_5200(), set call variable_set('project_issue_update_5200_completed', TRUE). Then, at the top of project_issue_update_5201() and project_issue_update_5202() check to make sure that variable is set to true, and if not then abort the update with a big error message. At the end of project_issue_update_5202() we can then delete this variable.
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | 231713_form_alter_solution.patch | 1.27 KB | dww |
| #4 | 231713_4_pi_install.patch | 5.31 KB | aclight |
| #3 | 231713_pi_install.patch | 5.06 KB | aclight |
Comments
Comment #1
dwwSounds good to me. We definitely need to fix this -- the existing IFAC upgrade workflow is confusing a lot of folks, and leading to data loss in cases. :(
Comment #2
aclight commentedI'll take a crack at this.
Comment #3
aclight commentedUntested patch.
Comment #4
aclight commentedThis patch also deletes the variable when we're done with it.
Comment #5
aclight commentedFYI, I've now tested this with MySQL on a db dump from a real site. It works as promised and prevents the user from really messing up the database tables.
Comment #6
dwwMaybe this is an insane use-case, but I just tried to be dumb and kept re-trying the update.php, seeing the error, not quite fixing the problem, and trying again. Each time, it bumped the schema number (the evil problem we're trying to work around). :( After just 3 iterations of stupidity, I hit 5204(), which dropped some columns that hadn't been converted yet. Not sure if we care.
However, I just investigated and got the following alternate approach working. IMHO, it's pretty slick... hook_form_alter() to the rescue. ;) Try it out for yourself. No new variable, extra checks in each update, and no chance of brain-dead data-loss. It's only 6 lines plus comments, so while it's not ideal to have to bloat our live codebase to hack around core's limitations, it's certainly not the worst 6 lines of code we spent on solving a problem.
Thoughts?
Comment #7
aclight commentedYeah, I was aware of that insane use case but figured it would be pretty unlikely. But this is a more elegant solution to the problem and provides more protection as well. Tested on a site upgrade and it did the trick. One might argue that a combination of our two patches might be ideal, since with the form alter patch someone could still set the wrong update and mess up their database. However, I think this is pretty unlikely, and since the form alter solution is a smaller patch and more elegant let's go with that.
Comment #8
dwwCommitted to HEAD and DRUPAL-5--2. Thanks for working on this, sorry I didn't think of this alternate approach before you spent the time on the variable-based solution.
Comment #9
rcross commentedFWIW - I am someone who bumped against this. I ended up being able to redo the upgrade and the second time went smoothly - though i didn't try to replicate the problem.
Thanks for the help
Comment #10
dwwComment #11
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.