module dependency checks
| Project: | Project issue tracking |
| Version: | 5.x-2.x-dev |
| Component: | Miscellaneous |
| Category: | bug report |
| Priority: | normal |
| Assigned: | hunmonk |
| Status: | closed |
I think this might just be a documentation issue, but I'm not sure. I grabbed the latest of both project and project_issue and ran the upgrade script and got failures because I was missing comment_upload. Is there a way to force admins to get that module before trying to upgrade? Not really sure where that would happen.
At the very least the upgrade.txt file needs to be updated to instruct people to get that module. Additionally, it'd be nice to put that in the release information on the All Releases page.
Also, do people need to save the modules page after uploading the code? I'm not sure that the comment_upload module will get enabled during the upgrade.php.
We' re still in dev, so it's not that big of a deal now. I just had to do a quick rollback on my database and start again. I just wanted to get this issue down now before it gets overlooked. (If this is already addressed somewhere else, my apologies.)
#1
i don't see any better way to do it than what's already in there. drupal's upgrade system doesn't seem to support checking for new module deps during an upgrade afaict.
i have simple dependency checking in there which prevented those updates from running if comment_upload wasn't enabled. this would be a decent step, except that i don't think there's any way to run just a selected set of updates again... :(
what we could do, just to try and keep things sane here for the rest of the 5.x cycle, is to simply fail all subsequent updates if the first update fails, then put instructions in the error messages to enable the required modules, and try running the update again from #5200. i dunno. that might be overkill. but it would be nice to provide some sensible protection here -- i'm sure a lot of people will make this mistake, unfortunately -- even if we do put big red flags in UPGRADE.txt
all that being said, fixing UPGRADE.txt is a decent start.
#2
I agree that something more is needed... when I upgraded a few days ago it was a bit of a pain to figure out why the update was failing and then where I should re-run the updates after I installed comment_upload.module.
#3
attach patch is an effort to provide some sort of sensible failure for the case where dependencies are not installed. it's not the prettiest solution, but it's the best i see atm. i've tested it fairly well on postgres, and it seems to work great. i'd like at least one code review and one person to test on mysql before we commit something like this.
#4
since we can't really guarantee that we can abort cleanly for any updates after the first in the 5200 series, i think it's best to only perform this check at the beginning of project_issue_update_5200() itself. attached patch corrects.
#5
changing the title to better reflect the fixes in this patch -- it also handles some configuration stuff on clean installs.
#6
attached breaks out the changes in the previous patch more sensibly -- the new install fixes are now at http://drupal.org/node/187195
#7
A) On a site without clean-urls enabled, your "re-run update.php" link is bogus. Behold:
http://iskra.local/drupal-5/?q=update.php&op=selection
See the code in update.php for update_finished_page() to see more info on this.
B) I'm not sure these instructions are clear enough. I suspect we're still going to get a bunch of support issues as a result of this. :( Sadly, I don't have time now to elaborate, but I think we should do better... I'll reply with more detail later this afternoon.
Thanks,
-Derek
#8
I talked about this with hunmonk for a little while just now...
We both agree (A) is still a problem, but has an easy solution.
I'm going to give up on (B) and hunmonk's going to handle any support requests that come in as a result of this. ;) However, we should at least add a period at the end of the message. I personally think it should be a drupal_set_message('error'), not just burried in the fine print of the update text itself, but I don't care enough to fight hunmonk over it anymore. ;) Furthermore, the core bug that's addressed by the "Note: you will most likely need to disable the Project issue module temporarily in order to resolve the issues above." is *really* annoying, and will make this much more confusing and annoying for users than this would otherwise be. :( It might be nice to more clearly explain *why* they need to temporarily disable pi to resolve this (as a work-around to this annoying core bug).
New, final problem:
C) This patch needs a new paragraph in UPGRADE.txt and an update to INSTALL.txt.
Otherwise, I'm "happy". ;)
Cheers,
-Derek
#9
i believe i've adequately addressed all of the issues above. committed to HEAD.
#10
Cool, mostly looks good. A few easily-corrected problems I just noticed:
A) The .info file lists the following:
dependencies = project comment comment_upload"upload" itself also belongs in there...
B) project_issue_update_5200() doesn't check for "upload", either, even though it's a new required dependency. If anything, I'd guess it's more likely that sites are already running "comment" than "upload", but we should just check for both.
C) The changes to INSTALL/UPGRADE mostly look great. However, they mention that core's "upload" is a new requirement, but don't mention core's "comment", even though 5200 checks for "comment" but not "upload". ;)
So, in summary, we should go through and unify the dependencies in all 3 places: the documentation, the .info file, and update_5200, so that they contain *all* the new dependencies: comment, upload, comment_upload.
Thanks,
-Derek
p.s. Didn't we say that directly upgrading from 4.7.x-2.* to 5.x-2.* wasn't going to be supported? Oh, maybe I'm thinking we decided that to get from 4.7.x or 5.x to 6.x, you'd have to go through 5.x-2.* separately? Oy vey, my memory is already failing. ;) Seems like we need a good place to record these sorts of decisions and plans. Any suggestions? A wiki on g.d.o, perhaps?
#11
i checked the database stuff, and this will be fine. it's 5.x-1.x -> 6.x that might be a problem
fixed up everything else you mentioned -- nice catch :)
#12
- i reviewed the diffs you committed and it all looks great.
- duly noted about the DB upgrade paths (at least for now). ;)
thanks!
#13
Automatically closed -- issue fixed for two weeks with no activity.
#14
hrm. just noticed a problem on my intel mac:
the array pointer for $_SESSION['updates_remaining'] gets hosed by our foreach loop in the abort function, such that
key($_SESSION['update_remaining'])in core'supdate_do_updates()returns NULL. thus pi's update 5200 is never unset, and we get ourselves into an infinite loop situation.i have no idea how this didn't happen in our initial testing. perhaps it's a weird platform-specific issue with php, i dunno. i only know that on my intel mac, she no worky the way we have it. ;)
attached patch fixes the problem by explicitly resetting the array pointer. this should work perfectly, because any module updates prior to project issue will already have been removed from the array, and the first element will be the pi update for 5200 (ie, the currently running update).
tested on my machine, and it works fine.
#15
I tested this on my intel mac with php 5.2.3 but i wasn't able to replicate the problem hunmonk reported in the first place. However, with patch in place, the upgrades still worked properly. I'm not completely comfortable RTBCing this, since I don't fully understand the problem and wasn't able to reproduce the problem in the first place. However, it doesn't seem to cause any additional problems.
#16
i'm very clear that there was an array pointer issue for me -- that was the problem, clearly. the pointer was fine before the unset stuff in the abort code in pi.install, and not pointing at any element of the array after the foreach loop.
since this addition corrects a potential problem for others, and doesn't harm the process elsewise, i'm going to commit it.
wild guess is that a bugfix regarding the inconsistency of the array pointer in a session var was committed to php at some point, but who knows...
committed to 5.x-2.x and HEAD.
#17
Automatically closed -- issue fixed for two weeks with no activity.