Scenario

  1. hook_preprocess*() is invoked for a page.
  2. On the update.php page, all hooks except for an explicit whitelist are not allowed.

Problem

  1. If any kind of code path happens to call any hook that isn't allowed, update.php blows up.

    E.g.: rdf_preprocess_html() tries to invoke hook_rdf_namespaces(). → Exception.

  2. The same code path can be triggered through plenty of other mechanisms; e.g., #pre_render/#process/#after_build callbacks on form elements (that happen to be used on the update.php page), and most likely a lot more call chains.

    Just invoke a hook that happens to be not allowed in any of these, ka-boom.

Proposed solution A

  1. Force-limit module list to System + User module on update.php.

Proposed solution B

  1. Remove the hook limitation on update.php.

    Now that Migrate is used for major upgrades, most of the issues with hook invocations are gone.

    There can still be situations where schema/API updates in contrib modules conflict with each other, but it's reasonable to expect contrib authors to deal with those in their own updates, rather than completely locking the system down for every minor update (including custom deployment stuff).

    hook_update_N() is a typical & precious tool for custom deployment steps on "real life" sites - gaining back the ability to work on entities (e.g. to load and save nodes) is precious.

Proposed solution C

  1. Replace the explicit whitelist with a fuzzy blacklist.

    The only intention is to not invoke secondary reads + writes (primarily concerning entities).

    if (preg_match('/_load|_save|_delete/', $hook) {
  // Do nothing instead of triggering an exception.
  return;
}

Proposed solution D

  1. Move the UpdateModuleHandler replacement service directly before the actual update functions are invoked.

  2. Restore the regular ModuleHandler after calling update functions.

Proposed solution E

  1. Add lock() + unlock() + isLocked() methods to UpdateModuleHandler.

  2. Directly before executing update functions, lock() down hook invocations.

  3. After executing update functions, unlock() hook invocations.

CommentFileSizeAuthor
#17 drupal8-update_module_handler-2233403-17.patch6.08 KBJalandhar
#11 update.modulehandler.11.patch6.12 KBsun
#9 interdiff.txt3.58 KBsun
#9 update.modulehandler.9.patch3.46 KBsun
#6 interdiff.txt1.85 KBsun
#6 update.modulehandler.6.patch2.25 KBsun
#2 drupal8.update-modulehandler.2.patch1.42 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes

Hm. New proposal D might actually make most sense.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Needs review
FileSize
drupal8.update-modulehandler.2.patch1.42 KB

Attached patch implements proposal D.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

Actually, your patch seems to fit the spirit of D but does not match what you describe. I'd call it E and add it to the issue summary, and I'm +1 on this

Status: Needs review » Needs work

The last submitted patch, 2: drupal8.update-modulehandler.2.patch, failed testing.

The last submitted patch, 2: drupal8.update-modulehandler.2.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
update.modulehandler.6.patch2.25 KB
interdiff.txt1.85 KB

Fixed bogus isLocked() call, also lock second batch invocation, added docs.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes

Added code of patch as new proposal E.

Status: Needs review » Needs work

The last submitted patch, 6: update.modulehandler.6.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
update.modulehandler.9.patch3.46 KB
interdiff.txt3.58 KB
  1. Fixed UpdateModuleHandler is not always registered for update.php.
  2. Wrap the actual update function invocation only (instead of entire Batch API).
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

An alternative, it seems catch was considering removing this altogether in #2193673: Let minor updates invoke hooks again

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Issue summary: View changes
FileSize
update.modulehandler.11.patch6.12 KB

Thanks for that pointer! :)

Attached patch implements proposed solution B: Remove UpdateModuleHandler entirely.

Also taking over the meat of that issue into the issue summary (B), and will close that issue as a duplicate.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Major » Critical
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

#1929816: Remove all references to/implementations of hook_schema_0() is also related. I think UpdateModuleHandler was the last thing blocking that one.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Any chance to move forward here?

#2218039: Render the maintenance/install page like any other HTML page was committed but had to be reverted because update.php suddenly threw an exception and is still blocked on this issue.

In light of the conceptual problems that the aforementioned issue happened to reveal, I actually think that removing/reverting UpdateModuleHandler entirely makes most sense — regardless of what the final resolution might be.

That is, because the originally intended concept of the current implementation simply does not work out in practice. Therefore, it's just logical to remove the code. — Afterwards, optionally, IF we decide that a mechanism like this is necessary, we can choose to re-implement it from scratch, investigating the options in the issue summary + with proper test coverage.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Title: UpdateModuleHandler used by update.php does not account for all possible call chains invoking hooks » Let update.php invoke hooks again
Status: Needs review » Reviewed & tested by the community

Better title and +1
Now that migrate is in core it makes complete sense, since update.php will only be used for minor updates

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: update.modulehandler.11.patch, failed testing.

Jalandhar’s picture

Jalandhar CreditAttribution: Jalandhar commented
Status: Needs work » Needs review
FileSize
drupal8-update_module_handler-2233403-17.patch6.08 KB

Updating with reroll. Please review.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

This sounds great to me. I was holding off since I wanted to make sure catch was ok with it, but sun pointed out that catch actually made the duplicate issue where yched, etc. were all +1 sooooo!

Committed and pushed to 8.x. I thought this needed a change record, but sun pointed out that this was behaviour we changed in D8, so simply unpublished the old change notice at https://drupal.org/node/2026515.

  • Commit 93ae08a on 8.x by webchick: 
    Issue #2233403 by Jalandhar, sun: Let update.php invoke hooks again.
anavarre’s picture

anavarre
Primary language French
Location 🇪🇺
CreditAttribution: anavarre commented

FYI https://drupal.org/node/2026515 is throwing an Access Denied here. I guess it's because it's unpublished?

chx’s picture

chx CreditAttribution: chx commented
Title: Let update.php invoke hooks again » Roll back: let update.php invoke hooks again
Assigned: sun » Unassigned
Status: Fixed » Active

Sigh. I hate to step back but: if we allow calling hooks then you might call into code that simply doesn't work before the update. If you allow calling hooks then what is the point of having update.php and the whole subsystem at all? This must be rolled back and a better solution found. Alternatively, declare that in such a situation you must migrate your whole site and remove the update subsystem. No skin off my back.

chx’s picture

chx CreditAttribution: chx commented

Note that in D7 we busted our posterior to avoid firing hooks (hence the reimplementation of field CRUD for example) and the updatemodulehandler was merely a safety net -- if it finds update.php firing hooks that's a bug and needs to be fixed for reasons in #22.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Yes we have that problem for minor updates in Drupal 7, and it's a pain in several cases.

However there are also countless updates that just set configuration, batch over entities to update them etc. that run in isolation many thousands of times per year. Many of those want hooks to run and would be a complete pain to write without.

When we removed hook invocations from update.php, it was primarily for the major upgrade path, there was always a question mark over what we do about minor updates (i.e. at one point someone suggested having upgrade.php and update.php).

Now that migrate has removed major updates, I think it is OK to allow hooks again. It's going to be hard for some contrib modules (especially combinations of them), but easier for the vast bulk of updates that actually get written.

Also saying that if you have a really horrible and fragile hook_update_N() you need to do an 8-8 migration seems reasonable to me - that's precisely why I opened the issue that got marked duplicate of this. There's a way out now for those situations.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Note that in D7 we busted our posterior to avoid firing hooks

For the major upgrade path, which no longer exists as a concept.

chx’s picture

chx CreditAttribution: chx commented

Alternatively, declare that in such a situation you must migrate your whole site and remove the update subsystem

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

I don't think it has to be either/or. We can keep a 7.x-style update system, and also people can run 8-8 migrations when/if they need to.

Contrib authors doing massive schema updates could also do releases that require 8-8 migrations and not provide any upgrade path at all in that case.

chx’s picture

chx CreditAttribution: chx commented

Keeping the separate, convoluted, problematic update subsystem; however; is pointless and is downright misleading. Sure, have a batch page running updates but have it within normal Drupal.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Roll back: let update.php invoke hooks again » Let update.php invoke hooks again

Rolling this back will only replace one regression with another regression.

As outlined in the issue summary (copied from #2193673: Let minor updates invoke hooks again), that regression additionally involved:

hook_update_N() is a typical & precious tool for custom deployment steps on "real life" sites - gaining back the ability to work on entities (e.g. to load and save nodes) is precious.

There are multiple, conflicting interests at stake, so just locking down all hooks is an insufficient answer/solution.

If we want to continue to look into potential solutions for this, then we should investigate a mixture of A + E:

  1. Force-limit the module list to System + User modules in the update.php bootstrap.
  2. Add back an UpdateModuleHandler with new/additional lock() + unlock() methods.
  3. Maintain two module lists (the locked + the unlocked) at the same time.
  4. Ensure that moduleExists() & Co. always works, even when currently locked.
  5. lock() the module list directly before calling an update function + unlock() it directly after.
  6. Allow module updates to unlock() on their own, so e.g. custom site updates can be performed.

I already started to look into something like that at some point earlier, but it turned out to get very complex very fast. I'm also not sure whether it really resolves all use-cases.

Lastly, we additionally have events in D8 now... limiting all of this to hooks doesn't address events, even though the exact same problem exists for them, too.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

@chx oh I might see what you mean. I don't think you mean removing the entire subsystem though.

Since we don't need to support major updates, if we wanted a full environment we could do the following:

1. Move it from a separate file to just a route.

2. Have that route
- (optionally?) put the site into maintenance mode
- collect updates + figure out order/dependencies similar to now
- run the updates in a batch
- take site out of maintenance mode if necessary.

If that's what you meant than yes that sounds like a good follow-up (although we could open a new issue for that, would not be a rollback). We'd lose update.php and a bunch of other hacks, but the update running stays very similar for now.

If that's not what you meant, please try explaining again.

@sun there's also tagged services on top of events.

chx’s picture

chx CreditAttribution: chx commented

#30 agreed , that's what I meant.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Fixed

I think #2250119: Run updates in a full environment was opened as a follow-up to this, setting back to fixed.
If that issue is not adequate, please open a new issue instead of reopening this one.

Status: Fixed » Closed (fixed)

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