Upgrade path is broken since update.php does not enforce empty module list

This sounds like a pretty good idea.

Any module that asks for a full bootstrap and is actually disabled will result in an Exception, so it will not be permitted to run.

Thanks to the dependency system, we can just not run this update (and any update that depend on it). There is no need to prevent updates from other modules from running.

The reason we are throwing exceptions is to prevent misuse. Site specific modules wont be disabled and contrib should not use this.

So what happens if my module's data depends on my module's own info hook being implemented by other modules, and I've to call that hook to remotely be able to update stored data? (since without gathering info from the modules, there is insufficient information about the already stored data, so updates can't be performed)

sun's issue is a whole another bag of hurt, I opened #923432: Disabled modules break update

So something like this.

Quite probably the solution for both problems (which might actually be re-merged into this one now) is to change what list of modules an update can require. The bootstrap will be full anyways (so please disregard sun's patch), but the module_list will be different: none (default), enabled, installed. Also we probably want to provide update helpers that manipulate module_list in these ways.

Hopefully, modules asking for enabled or installed modules to be module_listed will not do major upgrades and the hooks they fire won't hit functions that tank because their tables are not yet updated. We can't do much more than hope. Of course we will first run those updates that need no modules so this hope is well founded.

After applying the outstanding cleanup criticals, I was able to get the BasicUpgradePathTest pass with no modules in module_list (and only field and field_sql_storage needs to be drupal_loaded even).

This is the code that passes the Basic update but does not yet implement the per update module_list functionality. That wont' be hard. This is a lot of change but it's mostly from existing issues so as those get in, this will shrink.

Yeah, something like that. For the other issue, just provide a update_load_installed_modules(), which can be invoked by individual updates (but should be avoided).

One more quick note: The preliminary update.php info/selection forms still have to do a full, regular bootstrap. I (1) have a contrib module that exposes functionality on those forms (do an automated database backup before updating) and (2) once had the need to output a special upgrade warning for a contributed module. Thus, limiting module_list() during actual execution of updates is fine, but please not before or after.

I wish it wouldn't be the case, but I unintentionally and unexpectedly ran into this problem with:
#1215104: Use the non-interactive installer in WebTestBase::setUp()

This time though, it's not a contrib module or similar doing it wrong, it's the very Drupal core update.php script that runs into core module incompatibility and DI service container issues when performing an upgrade from D7.

The one-line summary is: The upgrade path with an enabled Locale module from D7 is broken.

The amount of symptoms I had to go through in debugging the actual upgrade path test failures (after changing the upgrade path tests to produce proper exceptions) were insanely excessive. The bottom line, however, is:

When Locale module was enabled in D7, then update.php ends in an error, because update_prepare_d8_bootstrap() is invoked with all modules enabled and loaded, and t() is being called deeper in there, and because Locale module exists but the database tables haven't been fully upgraded yet, the updater ends in an error. The module list is reduced to system.module only slightly later in update.php when the actual updates are being executed.

[All of this got revealed due to the replacement of drupal_static() with a plain static in module_list() in this patch, which is required, because otherwise any call to drupal_static_reset() in a unit test would reset that module_list() static and thus, the next call into it would attempt to retrieve the list of enabled modules via system_list() from the database -- but there is no database for unit tests.]

After fixing that - and also removing the suppression of errors in update.php - further testing of LanguageUpgradePath revealed that various module update functions are calling into module API functions, which should not be available at all in the first place. Thus, also fixed those module updates.

So:

Attached patch fixes the update.php environment to limit the module list to System module only.

However, it specifically takes #12 into account, which means that the module list has to be "locked down" and "unlocked" more than once within the update process. Turns out that Drupal core requires this itself - specifically, but not limited to the hook_requirements() check that happens within update.php.

because update_prepare_d8_bootstrap() is invoked with all modules enabled and loaded, and t() is being called deeper in there,

What's calling t()?

The call chain was:

update.php: main ()
update_prepare_d8_bootstrap(), update.inc
update_prepare_d8_language(), update.inc
language_default_locked_languages(), bootstrap.inc

  $languages[LANGUAGE_NOT_SPECIFIED] = new Language(array(
    'langcode' => LANGUAGE_NOT_SPECIFIED,
    'name' => t('Not specified'),
    'weight' => ++$weight,
  ) + $locked_language);

t(), bootstrap.inc
locale(), locale.module
WSOD

At first, I tried to adjust the code to not trigger this call chain. But then I realized that this certainly isn't the only possible error that can occur, since we're changing so many early bootstrap things for D8.

So I recalled that we had that discussion in this issue, and that we actually wanted to lock down the module list for D7 already.

Doing so properly resolved the cause for the early update.php bootstrap error. And at the same time, it revealed a couple of module updates that are calling into module API functions during the upgrade path.

I still think it's the right thing to do. It will prevent anyone from writing a bogus update in the future (although, sure, you can hack around it and manually unlock the module list, but then you're doing it really wrong).

This just looks like another case of #1134088: Properly document update-api functions to me. We can't rely on locking down the module list to avoid that issue, since for example a call to url() could trigger a path alias lookup and the db might not be ready for that either.

I don't really understand the relation to #1134088: Properly document update-api functions — that one is about using the right/schema-compatible helper functions to retrieve things in module updates. This change here is about preventing module hooks from being invoked.

You're right in that even with this patch, there are still functions unrelated to module hooks that might trigger DB lookups on a wrong schema. However, for such functions, we either need to provide update helpers, or they might as well be replaced with update process compatible services (e.g., #1269742: Make path lookup code into a pluggable class for the concrete url() example).

The essence of this patch is:

1) No more module hook invocations during the update process.

2) No more access to API functions defined in .module files during the update process.

3) However, retain hook_requirements() checks during update.php.

that one is about using the right/schema-compatible helper functions to retrieve things in module updates. This change here is about preventing module hooks from being invoked.

No that one's also about preventing module hooks from being invoked, since we don't know if the hooks might rely on a schema definition somewhere that hasn't been upgraded yet, which is why we need to document it properly. I'd prefer it if we threw an exception when module_implements() is called in update.php so there's strict errors thrown whenever this happens, rather than have a situation where it's OK to call some non-database/schema API functions but not others since that'll be even more confusing than the blanket rule we have now.

Well, while a conditional exception to module_implements() & co could be easily added to this patch, I don't think it's possible, since the update.php script itself needs at least access to a few System module hook implementations; e.g., to construct a form with form elements, and to theme/render various pieces on the update.php pages.

Specifically regarding schema access, please note that I also filed #1774104: Replace all trivial drupal_write_record() calls with db_merge()

Given the pushback here, I'll try to remove the changes from #1215104: Use the non-interactive installer in WebTestBase::setUp() and work around the LanguageUpgradePathTest failures in some other (yet unknown) way; i.e., effectively retaining the upgrade path error that is currently in HEAD.

I really hate to do this, since it pushes us back up over thresholds, but this is a critical bug as far as I can tell, since it breaks the upgrade path.

The existing patch implements exactly what this issue proposed and discussed since its beginning:

Locking down the module list for the entirety of update.php. Which means that .module files are not loaded in the first place, which in turn means that no API functions can be called, and obviously also that no hooks are invoked.

It sounds like @catch doesn't want to do this anymore. If that's the case, then the proper action to do for this issue is "won't fix." If so, some reasoning would be good to have.

In any case, there's a patch for the proposal, and other issues were able to work around the update.php situation in different ways.

Related: #2001310: Disallow firing hooks during update

As part of the Bug smash initiative, we are triaging issues that are marked 'Postponed (maintainer needs more info)'. Based on the lack of follow up, we believe this issue is no longer relevant so I am marking it 'Closed (outdated)'.

Discussed with @catch and this issue was actually a duplicate of #2001310: Disallow firing hooks during update. The change was later reversed in #2233403: Let update.php invoke hooks again, and we eventually added a workaround for these sorts of problems with the addition of post-update hooks.

So, adding those to the related issues and closing as a duplicate. Thanks!