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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

note, the modules that seem to have their own updates are: book, locale, comment

pwolanin’s picture

even worse -the locale module update code does not use the schema API

chx’s picture

To be more precise they need to be in a module that can not be disabled. system,user etc.

Gábor Hojtsy’s picture

Oh, 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.

JirkaRybka’s picture

So 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.

Gábor Hojtsy’s picture

Throughout 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.

catch’s picture

I 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.

JirkaRybka’s picture

Point 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?

Gábor Hojtsy’s picture

Well, 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 ... :)

catch’s picture

Status: Active » Needs review
FileSize
28.5 KB

Quick cut and paste find and replace. Untested.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Huh, 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.

catch’s picture

FileSize
28.5 KB

I read the commit messages too :)

catch’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

pwolanin: 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?

chx’s picture

FileSize
30.47 KB

Drupal supports two kind of installations: clean database or over the database of a previous stable. Nothing else.

chx’s picture

FileSize
30.47 KB

Oh, 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.

catch’s picture

mine missed the system_install() chunk for uid, fwiw. Also I shortened a couple of comments for their new context.

Gábor Hojtsy’s picture

Hm, merge the comment touchups and the missing stuff then.

chx’s picture

Oh 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

Dries’s picture

Mmm. 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 and update.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?

Gábor Hojtsy’s picture

Well, 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.

catch’s picture

I 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.

pwolanin’s picture

@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?

Gábor Hojtsy’s picture

Status: Needs review » Needs work

pwolanin: 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.

pwolanin’s picture

@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.

Gábor Hojtsy’s picture

If we move them anyway, then we can certainly roll them together. (Although I think this is the most minor point here).

catch’s picture

In 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.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
28.98 KB

ok - 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.

chx’s picture

Title: all core updates must be in system.install » Updates need to update all modules where schema_version > -1
Status: Needs review » Needs work

I 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.

chx’s picture

Status: Needs work » Needs review
FileSize
764 bytes

An untested attempt.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

chx: that's what we talked about above:

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.

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.

chx’s picture

Status: Needs work » Needs review

That's all OK but that should not stop people from reviewing this patch :) (Side note: discuss statuses in the future)

Gábor Hojtsy’s picture

Status: Needs review » Needs work

chx: 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.

chx’s picture

Ah. Yes, this means that the update functions needs to be self-contained, that's true. I forgot to quote Steven

The only tricky thing would be that a module can no longer rely on being enabled during its updates, which means that code shared between a .install and a .module needs to be moved into a shared .inc file.

catch’s picture

Status: Needs work » Needs review

#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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Agreed 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.

$ patch -p0 < system-updates-194310-23.patch 
patching file modules/book/book.install
Reversed (or previously applied) patch detected!  Assume -R? [n] n
Apply anyway? [n] n
Skipping patch.
1 out of 1 hunk ignored -- saving rejects to file modules/book/book.install.rej
patching file modules/comment/comment.install
patching file modules/locale/locale.install
patching file modules/system/system.install
Hunk #1 succeeded at 2736 with fuzz 1 (offset 33 lines).
chx’s picture

Title: Updates need to update all modules where schema_version > -1 » all core updates must be in system.install

I 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.

bjaspan’s picture

subscribe

Steven’s picture

"I wish Steven actually participated here but ah well, so #28 it is, resetting title."

Fine. There are so many very loud warning bells:

  • It introduces special rules for core modules, ensuring that the problem is not solved for contrib modules and extended installation profiles.
  • Functionally independent updates are needlessly forced into a single, linear update chain. It is a direct step backwards from the componentization of core, which started with moving modules into their own directories and adding separate .info files.
  • It requires the addition of special, manual db_table_exists() checks for every optional module update, instead of transparently relying on the schema version tracking which exists exactly for this purpose.
  • Manual checks do not scale elegantly if you factor in table renaming or dynamically named tables.

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?

  • Schema versions may have gaps, but they are still monotonously increasing.
  • It thus only makes sense to remove updates in a FIFO manner rather than randomly.
  • This causes a possible update gap for a module if and only if the current schema_version is less than the first defined update.
  • Updates are still safe to perform, if and only if the last removed update was less than or equal to the current schema_version.
  • It is trivial to provide information about the last removed update by defining an additional .install hook to return this schema version number and to disambiguate modules into "safe to update" or "not safe to update".
  • From a common sense point of view, it's smart to replace the practice of "drop updates from an .install file and cross your fingers" with "drop updates from an .install file and tell Drupal about it, so it can be taken into account".
Gábor Hojtsy’s picture

Title: all core updates must be in system.install » Check / run updates of disabled modules

Good 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.

Steven’s picture

I said "additional hook", not "overloading an existing, easy to grok mechanism with additional, invisible behaviour".

Gábor Hojtsy’s picture

Steven: 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.

chx’s picture

Assigned: Unassigned » chx
chx’s picture

Status: Needs work » Needs review
FileSize
1.97 KB

Is this what we are talking of?

chx’s picture

FileSize
1.97 KB

Extra underscore removed thx pwolanin.

pwolanin’s picture

logic 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

Gábor Hojtsy’s picture

module_invoke() only runs the hook, if the module is loaded, right? Which means it is enabled.

pwolanin’s picture

@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?

catch’s picture

Status: Needs review » Needs work

I installed 5.x - enabled all core modules. Disabled them all again. Ran patched upgrade and only system updates were called.

edit:

the update process was aborted prematurely while running update #6036 in system.module.

Fatal error: Call to undefined function search_install() in drupal6/modules/system/system.install on line 2678
chx’s picture

Status: Needs work » Needs review
FileSize
2.31 KB

pwolanin: no. But that's easy to patch, isn't it?

chx’s picture

FileSize
8.23 KB

catch tells me comment constants are used by the update. So we need to be moved into an inc and included frm both places.

chx’s picture

FileSize
8.23 KB

typofix.

chx’s picture

FileSize
8.3 KB

PHPdoc by catch.

catch’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Why would inclusion of comment.module in comment.install hurt? This constants.inc solution seems rather awkward to me. Otherwise everything look sane and good :)

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.66 KB
bjaspan’s picture

Status: Reviewed & tested by the community » Needs work

No 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.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.88 KB

Not 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...

chx’s picture

FileSize
2.97 KB

Huh, during rerolls poor module_invoke has biten the dust. Why/how dunno. Of course, testing does not fail because of that...

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Tested 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.

Gábor Hojtsy’s picture

Two 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.

chx’s picture

Status: Needs work » Needs review
FileSize
3.56 KB

If 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.

catch’s picture

FileSize
3.58 KB

Attached 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.

chx’s picture

FileSize
3.6 KB

Attached patch fixes a bug intro'd in #63 which caused the system to try to update non-installed updates.

Gábor Hojtsy’s picture

FileSize
18.78 KB

Turns 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.

catch’s picture

Status: Needs review » Needs work

Marking to CNW for the error message.

catch’s picture

Status: Needs work » Needs review
FileSize
4.09 KB

This 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 :)

catch’s picture

OK so I tried to go ahead anyway with the update - since it's still offering me comment etc. and got this:

n unrecoverable error has occurred. You can find the error message below. It is advised to copy it to the clipboard for reference.
Please continue to the error page

An HTTP error 200 occurred. http://example/update.php?id=1&op=do

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?

chx’s picture

Status: Needs review » Needs work

Please observe that drupal_set_message is not used anywhere in update.php . I suspect that message is the cause for the above error.

catch’s picture

Status: Needs work » Needs review
FileSize
4.36 KB

Gave 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.

keith.smith’s picture

Status: Needs review » Needs work
+          '#value'  => "$module can not be updated. It's schema version is $schema_version and updates up to $last_removed have been removed. You need to install an older version of Drupal and update this module first if you want to use $module in the future.",

It'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:

An update from $module ($schema_version) is not supported. This version of Drupal supports updating older modules that fall within a two release-cycle window only. It may be possible to temporarily install an older version of Drupal and run a database update as an interim step, and then reinstall and run the update from this Drupal version.

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.

catch’s picture

Status: Needs work » Needs review
FileSize
4.36 KB

Keith, 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

chx’s picture

FileSize
3.77 KB

Let's uncollapse only when it's needed.

webernet’s picture

Maybe something like this might be better:

$module module can not be updated. Its database schema version is $schema_version. Older updates, up to and including $last_removed, have been removed in this release. In order to ensure that this module is properly updated, you will need to install an older version of this module first.

It may also be wise to have different messages for core and non-core modules...

catch’s picture

FileSize
3.83 KB

Here'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.

catch’s picture

FileSize
3.85 KB

Slightly improved version.

webernet’s picture

Sounds 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.

catch’s picture

FileSize
3.86 KB

Well spotted.

Gábor Hojtsy’s picture

Looked 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.

catch’s picture

FileSize
12.39 KB
4.61 KB

Link 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.

catch’s picture

FileSize
4.58 KB

No, no I didn't need to do that. Attached patch is identical minus the completely unnecessary variable.

edit: missing the conditional collapsed fieldset, sorry.

catch’s picture

FileSize
4 KB

...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.

Gábor Hojtsy’s picture

Great. 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).

KarenS’s picture

If 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.

Gábor Hojtsy’s picture

Let'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).

catch’s picture

Status: Needs review » Needs work

In that case...

catch’s picture

OK 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

Gábor Hojtsy’s picture

catch: 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).

catch’s picture

To 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.

KarenS’s picture

I'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?

Gábor Hojtsy’s picture

KarenS: 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.

catch’s picture

I 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.

Gábor Hojtsy’s picture

catch: 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.

KarenS’s picture

As 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.

KarenS’s picture

Status: Needs work » Needs review
FileSize
6.28 KB

Unfortunately, I don't have time to really test this right now, but this patch should work.

KarenS’s picture

FileSize
6.28 KB

Oops, got my check for compatibility in the update list backwards. New patch attached.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Code 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?

KarenS’s picture

That 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?

KarenS’s picture

Status: Needs work » Needs review
FileSize
6.42 KB

How about this?

chx’s picture

While patching http://drupal.org/node/201479 I found that drupal_load is also in bootstrap.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.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Took 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.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks for all the effort and testing, I committed the latest patch to 6.x.

Gábor Hojtsy’s picture

Status: Fixed » Needs work

chx's drupal_load() usage from #101 actually broke the installer:

http://drupal.org/node/110981#comment-662117

But for the moment I have troubles installing drupal HEAD with default profile.
After entering database name, user and password I get:

Warning: Table 'tmp01.system' doesn't exist query: SELECT filename FROM system WHERE name = 'comment' AND type = 'module' in .../public_html/drupal-HEAD/includes/database.mysqli.inc on line 167

Pasqualle’s picture

Yes, the installer is broken
Please revert this patch..

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
10.56 KB

D'oh!

chx’s picture

FileSize
653 bytes
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
967 bytes

Added a bit of a comment, and committed, thanks.

Pasqualle’s picture

Status: Fixed » Reviewed & tested by the community

tested the installer only, it works again

Pasqualle’s picture

Status: Reviewed & tested by the community » Fixed
catch’s picture

Oops, didn't think to check normal D6 install when reviewing this.

Gábor Hojtsy’s picture

Priority: Critical » Minor
Status: Fixed » Active

There 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.

JirkaRybka’s picture

Status: Active » Needs review
FileSize
1.34 KB

I 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.

Gábor Hojtsy’s picture

Looks like you did not apply the em tags to all strings which were placeholders before. Was this intentional?

JirkaRybka’s picture

I 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.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Double 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.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, committed.

Gábor Hojtsy’s picture

Priority: Minor » Critical

Back to critical as it was.

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.