This is the flip side to this issue: http://drupal.org/node/80526
According to chx on IRC (and webchick on the issue above) - all core updates MUST be in system.install - anything else risks serious loss of user data because updates for versions more than 2 ago are dropped - if you disable a module (e.g. comment module on Drupal 5) and then re-enable is much later (e.g. on Drupal 9) than the update code will be gone. Since the updates in system.install always run - even for disabled modules - the problem is avoided.
Perhaps in D7 a mechanism can be devised to circumvent this problem, but for D6 it's unavoidable at this point.
Comment | File | Size | Author |
---|---|---|---|
#113 | update-t.patch | 1.34 KB | JirkaRybka |
#108 | comment_install_1.patch | 967 bytes | Gábor Hojtsy |
#107 | comment_install.patch | 653 bytes | chx |
#106 | comment_install.patch | 10.56 KB | chx |
#101 | run_updates_disabled-194310.patch | 5.92 KB | chx |
Comments
Comment #1
pwolanin CreditAttribution: pwolanin commentednote, the modules that seem to have their own updates are: book, locale, comment
Comment #2
pwolanin CreditAttribution: pwolanin commentedeven worse -the locale module update code does not use the schema API
Comment #3
chx CreditAttribution: chx commentedTo be more precise they need to be in a module that can not be disabled. system,user etc.
Comment #4
Gábor HojtsyOh, this sounds quite unfortunate, but also understandable. Indeed, there should be a better solution in D7. For now we can move the update functions to system.install, but that would completely break any possible beta upgrades (which use these modules), unless we have some funky checks at each update function (on whether the actually updated module's matching update was run). This might be a bit overkill to support upgrades from betas. Also all these updates need to be wrapped in table existence checks.
Comment #5
JirkaRybka CreditAttribution: JirkaRybka commentedSo do we support upgrades from betas, or not? Do we require updates to be in system, or just in required modules? http://drupal.org/node/120960 seems to bite me on this point.
Comment #6
Gábor HojtsyThroughout the development cycle we did go in and modify previous updates and so on, so I would say we generally not support upgrades from betas. Especially if it leads to so ugly code.
Comment #7
catchI reckon it's a lot more important to support real updates cleanly, people running live (or even development) sites on beta should either know enough to deal with the changes or not do it.
Comment #8
JirkaRybka CreditAttribution: JirkaRybka commentedPoint is, that I had a simple and clean update from betas in that other issue (just another preg_replace() on data already processed for other reasons), and I was asked to REMOVE that. Is that right?
Comment #9
Gábor HojtsyWell, with moving the update functions to system.install, all beta updates will break anyway (they will be offered these functions to run in update.php), so I say it is fine not to support them. But keep focused on this issue? Anyone up for a patch moving the files? This is a critical issue without any patch, and not exceptionally hard, so ... :)
Comment #10
catchQuick cut and paste find and replace. Untested.
Comment #11
Gábor HojtsyHuh, did not expect you jump on the issue this fast :) I committed a fix to locale.install in the meantime, so let's update that.
Comment #12
catchI read the commit messages too :)
Comment #13
catchComment #14
Gábor Hojtsypwolanin: your comments on locale not using the schema API (this update was written way before schema API was added AFAIR) is about basically replacing the CREATE TABLEs with schema API table creation based on the schema copied to that update function? Anything else where schema API should be used there?
Comment #15
chx CreditAttribution: chx commentedDrupal supports two kind of installations: clean database or over the database of a previous stable. Nothing else.
Comment #16
chx CreditAttribution: chx commentedOh, doh have not seen catch's patch and coded independently. However, something is amiss with his because I roll with bzr which results in smaller file headers thus smaller patches and yet mine is significantly bigger. Rerolled against HEAD.
Comment #17
catchmine missed the system_install() chunk for uid, fwiw. Also I shortened a couple of comments for their new context.
Comment #18
Gábor HojtsyHm, merge the comment touchups and the missing stuff then.
Comment #19
chx CreditAttribution: chx commentedOh doh! Disregard my patches then because that uid stuff is committed already. Dunno why the latest reroll have not omitted that. Sorry for the noise. Also, do not credit me in here.
The patch to review is http://drupal.org/node/194310#comment-637778
Comment #20
Dries CreditAttribution: Dries commentedMmm. While we can fix this problem in core by moving everything to system.install, how are contributed module owners supposed to deal with this? As far as I can see, the code in
install.inc
andupdate.php
does not differentiate between core and contrib modules. Are contributed modules affected by this problem as well?Thought: why don't we run all updates, regardless of the fact a module is enabled/disabled?
Comment #21
Gábor HojtsyWell, there is no solution for contrib modules for sure. If we were about to run updates for disabled modules, we would need to only run them if the module was ever installed AND if there is no gap in the update functions. Unfortunately there is no way to detect if there is a gap or not. Modules are not required to have sequential updates and we also do jumps in the numbers between major versions. Running updates when there are gaps could easily leave the data in an inconsistent state. Gaps could appear if a module was enabled previously, then disabled, and after that updates were removed in the name of removing unsupported update code, just as explained above. On the other hand we have a policy of only supporting upgrades from the minor releases of the previous major stable release *officially*. So if people actually do what we document, they should not have any problems with these jumps, and running all updates could still work.
Comment #22
catchI reckon the main issue with this is that we're talking about major version upgrades. With core, even if the modules are disabled, they're still there in the folder when you upgrade. If I install a 4.7 contrib module, then switch it off, then upgrade to 5.x - the chances of me installing the 5.x version of the module in the folder and not ever switching it on are very slim. By 7.x or 9.x it's unlikely the module author will be including smooth 4.7-9.x upgrade paths (and core explicitly doesn't).
If someone enables the contrib module at least once each major version, then there's no issue here right? I think if you skip two major core versions with a module then you're pretty likely to be starting from scratch anyway by that point.
Comment #23
pwolanin CreditAttribution: pwolanin commented@Gabor - the comment about locale and schema API was indeed to define the new table that way.
Also - is there a reason these locale updates need to be several separate update functions?
Comment #24
Gábor Hojtsypwolanin: Great, that makes this needs work, just as Dries' note. Anyway, wherever the locale update is, it needs to use schema API. The reason that it has multiple update functions is that we were building as we went along. Just as system.install does not have one update function per module.
Comment #25
pwolanin CreditAttribution: pwolanin commented@Gabor - I guess the question was more whether there is any reason to not roll the local module updates into one function? They all look pretty simple and not batched.
Comment #26
Gábor HojtsyIf we move them anyway, then we can certainly roll them together. (Although I think this is the most minor point here).
Comment #27
catchIn that case back to review since afaik it's not been tested yet.
The only active patch with a schema change in system.install that I know of is #164532 which is one change away from RTBC.
Comment #28
pwolanin CreditAttribution: pwolanin commentedok - here's a re-roll to use schema API for the new locale table. based on the patch in #12. I've also added checks using db_table_exists(), and consolidated some of the local update functions.
Comment #29
chx CreditAttribution: chx commentedI got a tip from Steven: regardless of a module enabled/disabled state we know when it needs an update -- the schema version is more than -1 if it was installed and not uninstalled. So updates can stay in their respective files but it's update.php that needs patching.
Comment #30
chx CreditAttribution: chx commentedAn untested attempt.
Comment #31
Gábor Hojtsychx: that's what we talked about above:
A problem with running updates for disabled modules, is
- they might not be updated in code for 6.x (ie. core compatibility flag is not there, or in the future: is less then the current compatibility required) -> need to check for this
- users might not download and replace disabled contrib modules, possibly they disable and remove the file
So if we go down this route, people need to be told that they need to download and replace their disabled modules too or uninstall them before upgrades for safe upgrades in the future.
Comment #32
chx CreditAttribution: chx commentedThat's all OK but that should not stop people from reviewing this patch :) (Side note: discuss statuses in the future)
Comment #33
Gábor Hojtsychx: Eh. Let me try to reword parts of what I tried to get through to you above. While the module_list() you remove ensured that all modules returned are compatible with this Drupal version (because update_fix_compatibility() executed previously disabled all modules which are not, and module_list() only returns enabled modules), your code offers running update code for modules not compatible with the current Drupal core. These might use unsupported API functions, and can do any kind of havoc in the update. This makes your code needs work.
Comment #34
chx CreditAttribution: chx commentedAh. Yes, this means that the update functions needs to be self-contained, that's true. I forgot to quote Steven
Comment #35
catch#28 applies cleanly, and I successfully upgraded a clean 5.x site to D6, which passed schema.module testing. So marking back to review since it deals with the immediate issue and is consistent with other updates this release cycle.
Comment #36
Gábor HojtsyAgreed with catch that until a generic solution evolves, we should clean up the inconsistencies here. However, system module got another update in the meantime (every update function number needs to be incremented) and the book.install patch also gives me errors when trying to apply.
Comment #37
chx CreditAttribution: chx commentedI wish Steven actually participated here but ah well, so #28 it is, resetting title. Could we just use some numbers from , say, 6050 so we do not need to reroll every time...? The patch anchors itself to the @end. Update system works well with holes in the update numbering, we know and utilize that.
Comment #38
bjaspan CreditAttribution: bjaspan commentedsubscribe
Comment #39
Steven CreditAttribution: Steven commented"I wish Steven actually participated here but ah well, so #28 it is, resetting title."
Fine. There are so many very loud warning bells:
We cannot throw all that away just because of minor problems with missing updates or with incompatible modules. It seems that in each case, the answer is "Do not update that particular module, and warn the user".
If there is a problem with gaps in schema versions, why don't we examine that area then?
Comment #40
Gábor HojtsyGood analysis. I guess we can detects gaps when we find that the schema version is less then any update number we have in the .install file. If the that update exists, we don't need to run that but the next one, although we need that one kept so we can check. (I hope 'this' and 'that' is not so ambiguous here).
Docs to contrib people: always leave at least one last update in your .install file if you ever had updates. Even if your last two major versions had no updates, do not remove the latest update at least. Is this sane?
But then we had a practice of committing most updates to system module, so we either leave them at that (they work now as is), or move them to separate install files and remove the table exists. In the interest of not muddying the waters too much, I think we do no harm with leaving them where they are.
Comment #41
Steven CreditAttribution: Steven commentedI said "additional hook", not "overloading an existing, easy to grok mechanism with additional, invisible behaviour".
Comment #42
Gábor HojtsySteven: sorry for not reading the entire post. Smart idea. Unfortunately it has a bit of a margin for human error, if the number is not updated, but for already removed updates, we cannot do better, granted.
Comment #43
chx CreditAttribution: chx commentedComment #45
chx CreditAttribution: chx commentedIs this what we are talking of?
Comment #46
chx CreditAttribution: chx commentedExtra underscore removed thx pwolanin.
Comment #47
pwolanin CreditAttribution: pwolanin commentedlogic of this patch looks reasonable - but needs to be tested.
I'll roll some of the improvements to locale.install from #28 into http://drupal.org/node/164532
Comment #48
Gábor Hojtsymodule_invoke() only runs the hook, if the module is loaded, right? Which means it is enabled.
Comment #49
pwolanin CreditAttribution: pwolanin commented@Gabor - I think you are mistaken - module_invoke() checks if the function exists, but does not check if the module is enabled. The only question I have is whether all module .install files are actually in memory?
Comment #50
catchI installed 5.x - enabled all core modules. Disabled them all again. Ran patched upgrade and only system updates were called.
edit:
Comment #51
chx CreditAttribution: chx commentedpwolanin: no. But that's easy to patch, isn't it?
Comment #52
chx CreditAttribution: chx commentedcatch tells me comment constants are used by the update. So we need to be moved into an inc and included frm both places.
Comment #53
chx CreditAttribution: chx commentedtypofix.
Comment #54
chx CreditAttribution: chx commentedPHPdoc by catch.
Comment #55
catchI tested this by installing 5.x, enabling every core module. Disabling every core module, then running the patched update.
The update goes completely smoothly, as expected, with the comment/locale etc. updates run as they would be if the modules were enabled. Especially at this stage in the cycle, it seems like a sane and unobtrusive change to get this working. So RTBC.
Comment #56
Gábor HojtsyWhy would inclusion of comment.module in comment.install hurt? This constants.inc solution seems rather awkward to me. Otherwise everything look sane and good :)
Comment #57
chx CreditAttribution: chx commentedComment #58
bjaspan CreditAttribution: bjaspan commentedNo doc for new argument to drupal_get_installed_schema_version().
I don't have time to review/think about this carefully right now but I'm wondering what the failure mode is if someone forgets to update their "last remove patch" hook. If it is bad, I'm worried, because people will screw that up. But I'm CNW'ing this issue for my doc comment, not this concern, because I do not know that my concern is valid.
Comment #59
chx CreditAttribution: chx commentedNot even the second parameter was documented but my patch is held back on this?. Of course, now that you have raised, it won't be committed because of this nuance. I hate when people hold back my patches for stuff like this -- this is a function noone ever calls!
About that concern, yes, there can be quite some problem if people do not update that hook but I think this is a rarity. Most modules won't need to remove their updates because they are usually quite a bit simpler --ie just simple schema changes-- than core's. Also, we can't really doany better as it is already mentioned in the issue...
Comment #60
chx CreditAttribution: chx commentedHuh, during rerolls poor module_invoke has biten the dust. Why/how dunno. Of course, testing does not fail because of that...
Comment #61
Gábor HojtsyTested patch #60 on a 4.6 to 6.0-dev upgrade, and it did update all my disabled modules. I have three notes though:
- the update_last_removed hook is optional, module_invoke() returning a value equivalent with zero when compared to the current $default
- $default is badly named, it should be $current_versions or something like this, right? why $default?
- Steven suggested displaying warnings for modules with update gaps, to let the user know that he should first update those modules to the current Drupal version before stepping ahead to the next.
Comment #62
Gábor HojtsyTwo more things here:
- the updates I tried enabled all modules which had updates, even if those were not enabled before the update run
- also, we might run disabled module updates here, numerous update functions are designed to do stuff only for enabled modules, which may or may not be completely out of scope for this issue. Look at system_update_6027() (issue http://drupal.org/node/197500) our current poster child for not too nice stuff. It updates all blocks with their cache properties for enabled modules only. So some updates use module_list() and/or friends which limit their reach to enabled modules.
Comment #63
chx CreditAttribution: chx commentedIf we load all the disabled (but once installed) modules I am not sure what memory limits we will hit. I do not know what/how your modules got enabled, you sure? I rummaged the source for place that change system.status for modules and it's only module_enable as far as I can see and that's in turn only called from system.admin.inc which is not loaded and drupal_install_modules which in turn is only called from install_configure_form_submit so I do not readily see how that happens. Error warning added, $default got a better name where appropriate.
Comment #64
catchAttached patch fixes a typo and specificies the full comment.module path.
With those changes I did the same upgrade run as #55 and it runs clean - all updates as expected, schema testing passed, cursory comment testing (posting, viewing) all fine. I can't reproduce the "enables disabled modules" thing.
Comment #65
chx CreditAttribution: chx commentedAttached patch fixes a bug intro'd in #63 which caused the system to try to update non-installed updates.
Comment #66
Gábor HojtsyTurns out I am not the best guy to do testing on this, as I have a Drupal 4.6 data set handy, which obviously throws schema_version nonexistence errors on this patch now. (Do not bother fixing this obviously :) Anyway, while testing how this behaves with the removed updates, I found an UI bug. The message looks nice when the fieldset is expanded but not when it is collapsed. Collapsed is the default.
Either find a better place for this error in the fieldset, or set a dsm() error IMHO.
Comment #67
catchMarking to CNW for the error message.
Comment #68
catchThis moves the message into a DSM. You can manually change the "system_update_last_removed" value to see what it does if you don't have a 4.6 database :)
Comment #69
catchOK so I tried to go ahead anyway with the update - since it's still offering me comment etc. and got this:
Which is a bit odd, system is a special case of course. I guess even though they're core modules, we should add module_update_last_removed to those other modules to prevent them from being displayed on the form maybe?
Comment #70
chx CreditAttribution: chx commentedPlease observe that drupal_set_message is not used anywhere in update.php . I suspect that message is the cause for the above error.
Comment #71
catchGave it some thought and a nice green drupal_set_message is probably not the way to go anyway. Here it is with just a div prefix/suffix around the form item which fixes the display issue Gabor reported.
Comment #72
keith.smith CreditAttribution: keith.smith commentedIt's should probably be Its.
I wonder if this is too technical -- would I know what this is referring to if I haven't read the issues in the queue about removing older updates? Of course, if someone has an older Drupal site, they've obviously been to this rodeo before.
What about:
However, that's arguably clunky as well. And, it should be noted, I have no idea if this "two release-cycle" thing is accurate. That's just what I thought it was based on no research.
Comment #73
catchKeith, we may remove 4.7 updates from D6 as well, so there's no guarantee of a two release cycle update, and it's not officially supported even if available - this also applies for contrib modules as well as core, where there may be no update path either. In other words we can't rely on it :)
Attached patch fixes the 'Its'. I think the slightly more technical language ought to be ok since we're dealing with:
1. a drupal site upgrading to it's third major release version
2. user 1 and/or someone who's just edited settings.php
Comment #74
chx CreditAttribution: chx commentedLet's uncollapse only when it's needed.
Comment #75
webernet CreditAttribution: webernet commentedMaybe something like this might be better:
It may also be wise to have different messages for core and non-core modules...
Comment #76
catchHere's an updated version of the text, taking into account webernet's comments. We have quite a few possible cases here (required core, optional core, contrib) but I think this text will work for all cases.
<em>$module</em> module can not be updated. Its schema version is <em>$schema_version</em>. Updates up to <em>$last_removed</em> have been removed in this release. In order to update $module, you will first need to upgrade to the last version in which these updates were available.
Patch also puts class="warning" in the div to draw additional attention to it.
Comment #77
catchSlightly improved version.
Comment #78
webernet CreditAttribution: webernet commentedSounds good, but it needs to be
"Updates up to and including <em>$last_removed</em>"
since $last_removed is the last update that existed, but was removed.Comment #79
catchWell spotted.
Comment #80
Gábor HojtsyLooked through the patch and the current wording and message use seems to be fine. I would however link "need to upgrade" to http://drupal.org/upgrade where there can be more docs for these kinds of issues too (that's a doc team job). I am not 100% sure about this though, as people are already directed to the upgrade guide on the screen before this one, but a direct link might be handy when people wonder where to get more info on this.
Comment #81
catchLink seems sensible. I had to move the warning in to a variable and t() to get everything working, not sure how coding standards conformant that is.
Here's a patch, and a screengrab of the message.
Comment #82
catchNo, no I didn't need to do that. Attached patch is identical minus the completely unnecessary variable.
edit: missing the conditional collapsed fieldset, sorry.
Comment #83
catch...and the right one this time.
I think this is ready. Warning text is identical to screenshot in #81 in case people don't want to test manually.
Comment #84
Gábor HojtsyGreat. The problem of gaps in updates is solved now. What about incompatible modules? Before this patch, core checks for all enabled modules, and disables all the incompatible ones on update. So when you run the update, you actually get modules which run on your core version. update.php has update_fix_compatibility(), which disables incompatible modules.
Now that we don't care about incompatible modules, we can end up with a process where the user sees that the modules were updated, but those were not compatible with the Drupal version used. Let's discuss whether this is a problem or not.
- Suppose you update from 4.7, and that we *don't* drop direct updates from 4.7.
- With this patch, you can put up Drupal 6, which will run its updates fine.
- But some of your modules dropped their direct updates from 4.7.
- So you can copy the Drupal 5(!) version of your modules under Drupal 6, run those updates.
- Then copy the Drupal 6 versions and run them too.
This sounds like a "feature", but it can very well be a point of misunderstanding and faulty upgrades, if people think they are done just by running update.php once.
Before this patch, their incompatible modules would not run their updates.
This could be a problem in the other direction too. You have a 6.x site running fine, and you need to upgrade a module (told you so by update status module). You accidentally grab the latest 5.x version, and voila, it will offer its updates on the update page (although the "No updates available" select item will be selected).
Comment #85
KarenS CreditAttribution: KarenS commentedIf people were to add D5 CCK code to their D6 installation and run the updates, I don't know what would happen. The D5 code is not designed to run right in D6 and there are significant differences between the D5 version of CCK and the D6 version. And even worse things would happen if someone had the D6 version of the content module and the D5 version of one or more of the field modules and tried to run them together.
So I would definitely want to be sure that D5 updates can *not* be run in D6. I sure wouldn't want the job of picking up the pieces afterward to try to fix whatever damage that might do.
Comment #86
Gábor HojtsyLet's add back code to not run updates for incompatible modules then. (They are disabled already, so they will not run on the site, but their updates are run after this patch is committed as far as I see).
Comment #87
catchIn that case...
Comment #88
catchOK I took a look at this this evening, but got stuck. Looks like the cleanest way to do this would be re-use _system_is_incompatible - but that's a private function inside system.admin.inc
Comment #89
Gábor Hojtsycatch: as I have said, update_fix_compatibility() does what we need but it is now not effective, as it disabled incompatible modules, but then we run those updates anyway. After the module is run, we don't know whether a module was disabled before the update or in the update because of incompatibility. Either we need that function refactored to an update API function to get a list of incompatible functions (move the last four line to after where it is called, and rename the function), or somehow make up a system to have a different status for incompatible disabled modules (this can be much more tricky).
Comment #90
catchTo expand on #88 I was trying to re-use the section from system_modules() which checks both core and php version compatibility of every module's .info file regardless of status then disables the form options based on that.
refactoring update_fix_incompatibility() sounds a lot simpler (although possibly not simple enough for me), and we only need modules that are installed, so that part is fine. Even though disabling the modules doesn't do anything to prevent the updates running, it's got value in itself for data integrity I guess.
Comment #91
KarenS CreditAttribution: KarenS commentedI'm not sure I'm looking at this right, but it look like update_fix_compatibility() sets the status to 'off' for any incompatible modules. After that we go to update_script_selection_form() which uses module_list() to select a list of module updates. That list still includes the modules we just marked as incompatible because it wasn't refreshed after we ran update_fix_compatibility(). So don't we just need to force the module list to be refreshed after that step to keep incompatible modules off the list?
Comment #92
Gábor HojtsyKarenS: the patch here removes usage of module_list() in two places. One is the selection form you mention. The goal of this patch to run update code for disabled modules. So module_list() will not be used as it returns enabled modules only.
Comment #93
catchI can't see module_list() being used anywhere here, but could we maybe modify drupal_get_installed_schema_version to get status as well? Then it'd be easy to add an extra check where we warn about updates being removed to warn admins the module is incompatible as well.
edit: cross posted.
Comment #94
Gábor Hojtsycatch: we don't know whether a module is disabled because it was disabled before the requirements check or disabled as a consequence of the check.
Comment #95
KarenS CreditAttribution: KarenS commentedAs you said Gabor, the solution is to add a new function where we can pass in a module name and it will return true or false as to whether it is compatible, then have update_fix_incompatibility() use that function to create its array. We can then test each module in our update list using that new function to keep incompatible modules off the list.
I'll work on a patch.
Comment #96
KarenS CreditAttribution: KarenS commentedUnfortunately, I don't have time to really test this right now, but this patch should work.
Comment #97
KarenS CreditAttribution: KarenS commentedOops, got my check for compatibility in the update list backwards. New patch attached.
Comment #98
Gábor HojtsyCode looks good but otherwise it does not sound like fun to call module_rebuild_cache() and system_theme_data() for each module. These are expensive functions.
What about returning the list of incompatible modules from update_fix_compatibility() and pass that on to update_selection_form() and then down to the form builder?
Comment #99
KarenS CreditAttribution: KarenS commentedThat would make the rest of the code messier. What if we just save the values as static variables so they only get called once and the value is retained?
Comment #100
KarenS CreditAttribution: KarenS commentedHow about this?
Comment #101
chx CreditAttribution: chx commentedWhile patching http://drupal.org/node/201479 I found that
drupal_load
is also inbootstrap.inc
so it can be used everywhere, like in comment.module, the functionality is the same just it's faster because of a static cache in drupal_load.Comment #102
catchTook these steps to review:
installed drupal 5. Enabled all modules. Disabled forum, comment, tracker. Installed panels 2.x alpha 8.
Before running update.php in 6.x, I put in panels 2.x-alpha14 (which has db updates) into my sites/all/modules directory and patched with #101.
comment updates were run.
panels wasn't shown on the form, and nothing ran for it.
Everything else smooth.
I also upped system_update_last_removed to 7000 afterwards and the warning was shown as before on update.php
Should be good to go now.
Comment #103
Gábor HojtsyThanks for all the effort and testing, I committed the latest patch to 6.x.
Comment #104
Gábor Hojtsychx's drupal_load() usage from #101 actually broke the installer:
http://drupal.org/node/110981#comment-662117
Comment #105
PasqualleYes, the installer is broken
Please revert this patch..
Comment #106
chx CreditAttribution: chx commentedD'oh!
Comment #107
chx CreditAttribution: chx commentedComment #108
Gábor HojtsyAdded a bit of a comment, and committed, thanks.
Comment #109
Pasqualletested the installer only, it works again
Comment #110
PasqualleComment #111
catchOops, didn't think to check normal D6 install when reviewing this.
Comment #112
Gábor HojtsyThere is another minor bug I just noticed. We should not use t() at all in update.php. This patch introduced the only t() used there. So let's fix that.
Comment #113
JirkaRybka CreditAttribution: JirkaRybka commentedI rolled a patch for that, rather easy (and probably no need to play with theme() more than absolutely needed ('placeholder'), this runs pre-update in fact).
But this is untested, I'm quite unfamiliar with this whole patch and no time to learn now. I only just checked that it doesn't do a WSOD, but no clue how to bring that text to my screen on UI.
Comment #114
Gábor HojtsyLooks like you did not apply the em tags to all strings which were placeholders before. Was this intentional?
Comment #115
JirkaRybka CreditAttribution: JirkaRybka commentedI thought about this a while, but finally saw no point to do
<em>
on numbers, these are already visible enough in the message, so why clutter the line more than needed. I also dropped url() on a hardcoded external link BTW.Comment #116
catchDouble checked this on update.php and it all seems fine. Don't think we need the
<em>
on numbers - looks pretty clear without and the actual update numbers won't mean an awful lot to many people anyway - it's pretty much always going to be a full major Drupal version's difference.Comment #117
Gábor HojtsyThanks, committed.
Comment #118
Gábor HojtsyBack to critical as it was.
Comment #119
(not verified) CreditAttribution: commentedAutomatically closed -- issue fixed for two weeks with no activity.