Suggested commit message:

Issue #2001310 by chx, berdir, yched, effulgentsia, YesCT: Disallow firing hooks during update.

Problem/Motivation

In order to get a reliable and testable update environment, we have actively tried to avoid hooks during update and preached against them. It doesn't quite work tho.

Proposed resolution

ModuleHandler is pluggable, replace it with an UpdateModuleHandler during update. UpdateModuleHandler should throw an exception on you if a hook is fired. Also move module_enable,disable and uninstall into the ModuleHandler family, move update_module_enable under UpdateModuleHandler::enable.

Remaining tasks

Commit it the moment the testbot approves. (Aka. run and don't look back.)

User interface changes

None.

API changes

None. Although module_enable/disable/uninstall is deprecated

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

+++ b/core/includes/update.incundefined
@@ -452,6 +453,13 @@ function update_prepare_stored_includes() {
+    // Install/enable the language module. We need to use the update specific
+    // version of this function to ensure schema conflicts don't happen due to
+    // our updated data.
+    $modules = array('language');
+    module_enable($modules);

The comment still mentions the non-longer existing "update specific version of this function"

+++ b/core/includes/update.incundefined
@@ -673,73 +675,33 @@ function update_fix_d8_requirements() {
+    // Views module is required to convert all previously existing listings into
+    // views configurations.
+    // Like any other module APIs and services, Views' services are not available
+    // in update.php. Existing listings are migrated into configuration, using
+    // the limited standard tools of raw database queries and config().
+    module_enable(array('views'));

Why do we need to move this?

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined
@@ -523,4 +525,366 @@ protected function parseDependency($dependency) {
+        // Clear the entity info cache before importing new configuration.
+        entity_info_cache_clear();

This is "interesting", as that would essentially mean that at some point, we might want to inject that. Which is not possible, because this is the thing that's injected into everything else. Fun.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilderStatic.phpundefined
@@ -0,0 +1,14 @@
+ * Definition of Drupal\Core\Routing\RouteBuilder.

Nitpick, Contains...

+++ b/core/lib/Drupal/Core/Routing/RouteBuilderStatic.phpundefined
@@ -0,0 +1,14 @@
+class RouteBuilderStatic {
+  public function rebuild() {
+

Needs some basic documentation, especially because it's an empty class :)

+++ b/core/modules/language/language.installundefined
@@ -117,3 +117,13 @@ function language_disable() {
+    // Load the include files to make constants available for updates.
+    language_negotiation_include();

If we just need the constants and nothing else, should wes simply hardcode the value? That's what I usually do with constants defined in .module

+++ b/core/modules/user/user.installundefined
@@ -658,7 +658,8 @@ function user_update_8011() {
-    $old_schema = update_module_enable(array('image'), 8002);
+    $old_schema = module_enable(array('image'));
+    update_set_schema('image', 8002);

The reason I added this to the function is that we don't want to enforce the schema if image.module was already enabled before, in that case we want to run those updates. So you probably will have to move this into the if below.

chx’s picture

FileSize
57.09 KB

> The comment still mentions the non-longer existing "update specific version of this function"

Nuked.

> Why do we need to move this?

Wait. Are you asking *me*? Please see #1848998-96: Problems with update_module_enable(). It might be familiar ;) This issue contains both that and yched's fixes.

> If we just need the constants and nothing else, should wes simply hardcode the value?

No. There are too many. It'd make the update code even more ugly.

> So you probably will have to move this into the if below.

I so did.

catch’s picture

This issue is very similar to #922996: Upgrade path is broken since update.php does not enforce empty module list.

This is "interesting", as that would essentially mean that at some point, we might want to inject that. Which is not possible, because this is the thing that's injected into everything else. Fun.

I think the CRUD operations for modules should be in their own class, partly for this reason. Also system_rebuild_module_data() is another place we'll easily run into circular dependencies.

+  public function enable($module_list, $enable_dependencies = TRUE) {
+    if ($enable_dependencies) {
+      // Get all module data so we can find dependencies and sort.
+      $module_data = system_rebuild_module_data();
chx’s picture

See #1833592: [META] The road out of module build circular dependency hell for that circular dependency.

The empty module list issue accomplishes two things: hooks can't run and module code is not loaded. Hooks can't run with this patch either but instead of a silent nothing you get a nice and loud exception. Out of the remaining hooks entity_info deserves more investigation -- it'd be good to get an exception on that one. Perhaps switching to the normal handler once the upgrades finished running would accomplish that and further simplify the update handler. I am not quite sure whether it's worth holding up this issue for it, though. Not loading modules is an interesting idea which probably deserves a followup -- it's much easier to do that once this is in place.

Actually, that exception already showed bugs which are fixed -- bugs that berdir and yched fixed but the tests didn't fail before. Well, they would do now.

chx’s picture

FileSize
58.23 KB

Here's a couple lines removed from HEAD that are no longer necessary. Yay, cleanup!

Status: Needs review » Needs work

The last submitted patch, 2001310_5.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review

#5: 2001310_5.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2001310_5.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review

#5: 2001310_5.patch queued for re-testing.

ParisLiakos’s picture

this looks like awesome cleanup!
i did a code review, and i will now test it with some upgrade issues i have:)
some of them might be totally wrong or nitpicky:)

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined
@@ -523,4 +525,366 @@ protected function parseDependency($dependency) {
+  public function enable($module_list, $enable_dependencies = TRUE) {

needs inheritdoc

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined
@@ -523,4 +525,366 @@ protected function parseDependency($dependency) {
+    $module_config = config('system.module');
+    $disabled_config = config('system.module.disabled');

i think we can inject config right?

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined
@@ -523,4 +525,366 @@ protected function parseDependency($dependency) {
+        module_load_install($module);
...
+        module_load_install($module);
...
+      module_load_install($module);

could be $this->loadInclude($module, 'install') i think

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined
@@ -523,4 +525,366 @@ protected function parseDependency($dependency) {
+        if ($kernel = drupal_container()->get('kernel', ContainerInterface::NULL_ON_INVALID_REFERENCE)) {
...
+      drupal_container()->get('kernel')->updateModules($enabled, $enabled);

i am sure we can do something a lot better here, at least get rid of drupal_container() for \Drupal, if we dont want to make module handler container aware..or dunno injecting kernel

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined
@@ -523,4 +525,366 @@ protected function parseDependency($dependency) {
+  function disable($module_list, $disable_dependents = TRUE) {

needs public visibility

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined
@@ -523,4 +525,366 @@ protected function parseDependency($dependency) {
+        module_invoke($module, 'disable');

$this->invoke

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.phpundefined
@@ -523,4 +525,366 @@ protected function parseDependency($dependency) {
+                    $factory = \Drupal::service($definition['factory_service']);

although if we inject container, we wouldn't need this call to \Drupal

+++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.phpundefined
@@ -262,4 +262,81 @@ public function invokeAll($hook, $args = array());
+   * @param $module_list
...
+   * @param $enable_dependencies
...
+   * @return
...
+   * @param $module_list
...
+   * @param $disable_dependents
...
+   * @return

adding the types here would be great

effulgentsia’s picture

FileSize
58.29 KB

Just merges HEAD; does not implement any feedback.

effulgentsia’s picture

I split off the easy move code around part of this patch to #2004784: Move module install/uninstall implementations into ModuleInstaller. That'll make reviewing the rest of this easier.

effulgentsia’s picture

FileSize
23.12 KB

#12 landed, so this is rerolled to exclude it. Note that #2004784-5: Move module install/uninstall implementations into ModuleInstaller contains follow up improvements related to that issue, and when that lands, this will need another reroll.

Status: Needs review » Needs work

The last submitted patch, 2001310-13.patch, failed testing.

chx’s picture

Assigned: chx » Unassigned
alexpott’s picture

Status: Needs work » Needs review

#13: 2001310-13.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2001310-13.patch, failed testing.

chx’s picture

Status: Needs work » Needs review

Finally, this passed!

tstoeckler’s picture

I cannot claim to be an expert in neither the update system nor the module system so the fact that I was able to (I think !) fully grasp such a fundamental patch with wide-reaching consequences is in itself a proof that it is very clearly written and well documented. It makes huge amounts of sense IMO. I especially like that there is a single code path were core can decide which hook implementations to invoke for which hook. Awesome!

That said, I have some minor remarks, mostly related to (additional) comments.

+++ b/core/includes/update.inc
@@ -103,7 +104,7 @@ function update_prepare_d8_bootstrap() {
-  $GLOBALS['conf']['container_bundles'][] = 'Drupal\Core\DependencyInjection\UpdateBundle';
+  $GLOBALS['conf']['container_bundles']['UpdateBundle'] = 'Drupal\Core\DependencyInjection\UpdateBundle';

+++ b/core/update.php
@@ -179,6 +180,20 @@ function update_helpful_links() {
+  unset($GLOBALS['conf']['container_bundles']['UpdateBundle']);

I think this deserves a comment: (assuming I understand this correctly) "Use a named key to be able to unset the bundle after performing the update. @see update_flush_all_caches()"

+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.php
@@ -0,0 +1,132 @@
+      $module_config = config('system.module');
...
+      config('system.module.disabled')

These two could be moved out of the foreach

+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.php
@@ -0,0 +1,132 @@
+      //  @todo: figure out what to do about hook_install() and hook_enable().

Is the assumption correct that this will have to be figured out before commit?

+++ b/core/lib/Drupal/Core/Routing/RouteBuilderStatic.php
@@ -0,0 +1,18 @@
+class RouteBuilderStatic {
+  public function rebuild() {
+    // @TODO: add the route for the batch pages when that conversion happens.
+  }

If I understand this correctly, it seems this is an update-specific version of the router builder, as it only contains routes related to the update. If so, should this be named UpdateRouteBiulder, instead?
Minor: "@todo" instead of "@TODO:"

+++ b/core/modules/language/language.install
@@ -117,3 +117,13 @@ function language_disable() {
+function language_requirements($phase) {
+  if ($phase == 'update') {
+    // Load the include files to make constants available for updates.
+    language_negotiation_include();
+  }
+}

Is this still necessary after #1620010: Move LANGUAGE constants to the Language class?

effulgentsia’s picture

Even though #13 isn't that big in terms of bytes, there's still a lot there that's hard for me to review properly (it could simply be that I'm not proficient enough with the details of our update.php related code). I filed another spin off, #2009484: Move (most of) update_module_enable() into UpdateModuleHandler::enable() in an attempt to make what's needed to review here easier. That said, if others can get this to RTBC and committed without me, I'll happily close the spin off as a duplicate after this lands.

chx’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
648 bytes
23.38 KB

I am superb thank for the review, having a fresh set of eyes and having someone not normally associated with this thorn bush getting entangled is a joy to see.

Enough of the splitting, however. The more the patches, the longer they linger and the more they attract bikeshed. Bigger patches noone can review fly in without review regardless whether they are broken, slow or introduce reflection into the normal code path (although this usually only applies to WSCCI patches but I hope they will apply to update patches too) -- so let's not split this further. Not much would remain of the patch if we were to do that, anyways.

No, let's get this in as fast as possible without much of bikeshed (so far the reviews have actually been useful, miracles!) because this is absolutely critical to make sure the forthcoming update path patches are solid. It already caught an entity system call in file updates that I try to remove in, guess what, another issue.

> These two could be moved out of the foreach
> Is the assumption correct that this will have to be figured out before commit?

We are not changing update_module_enable, we are just moving it. These are coming verbatim from update_module_enable.

> so, should this be named UpdateRouteBiulder, instead?

No, it's a static version of the Route builder which helps the update but there's hardly anythign update specific in there. In the future, the installer might need it too, for example.

> Is this still necessary after #1620010: Move LANGUAGE constants to the Language class

core/modules/language/language.negotiation.inc
15:const LANGUAGE_NEGOTIATION_URL = 'language-url';

Yes, I think so, yes. That issue changed the language constants, this is about a language negotiation constant.

With this said, I am simply setting this to RTBC as there have been no disagreement. If you think this violates core process, well the core process is dead already so let's no try to pretend it is not. If it is not dead, then roll back the router and we can talk.

We can iterate after it is in, and I intend to -- we should quite probably have this override only for the time the upgrades are actually running and have something better than the update_flush_caches but that's no reason not to have this guardian in core.

catch’s picture

Status: Reviewed & tested by the community » Needs review
chx’s picture

I will get #625958: Support Uploading Multiple Files for HTML5 Browsers via #multiple attribute to pass and then stop working on the update path until this is in.

DamienMcKenna’s picture

+1 for this in theory but I've not reviewed the code yet, will try to do that later on using my personal site.

Crell’s picture

On the whole I agree that if hooks are a no-go in update functions we should ensure that is enforced. Swapping out ModuleHandler is a sensible way to do that.

+++ b/core/includes/update.inc
@@ -671,73 +667,33 @@ function update_fix_d8_requirements() {
+function update_set_schema($module, $schema_version) {
+  Drupal::keyValue('system.schema')->set($module, $schema_version);
+  // system_list_reset() is in module.inc but that would only be available
+  // once the variable bootstrap is done.
+  require_once __DIR__ . '/module.inc';
+  system_list_reset();

Shouldn't this be a method on ModuleHandler?

+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.php
@@ -0,0 +1,132 @@
+    $schema_store = \Drupal::keyValue('system.schema');

This should be injected.

+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.php
@@ -0,0 +1,132 @@
+      module_load_install($module);

Shouldn't this function be a method on ModuleHandler at this point?

+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.php
@@ -0,0 +1,132 @@
+          db_create_table($table, $spec);

DB connection should be injected.

+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.php
@@ -0,0 +1,132 @@
+      $module_config = config('system.module');

The config factory should be injected.

+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.php
@@ -0,0 +1,132 @@
+      config('system.module.disabled')

Ibid.

+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.php
@@ -0,0 +1,132 @@
+  public function disable($module_list, $disable_dependents = TRUE) {
+    throw new \LogicException('Disabling modules is not supported during updates');

This actually concerns me. Lots of D6/D7 sites use update functions to enable/disable modules as a deployment mechanism. (Eg, disable a module you're no longer using, enable some newly written Views plugin module, enable a new Feature module, etc.)

With the changes brought by CMI is that no longer necessary? Vis, does CMI offer a way for me to "deploy and enable" or "deploy and disable" such that I don't need update functions anymore?

If so, then we're good. If not, then this becomes a regression.

+++ b/core/lib/Drupal/Core/Routing/RouteBuilderStatic.php
@@ -0,0 +1,18 @@
+
+/**
+ * This builds a static version of the router.
+ */
+class RouteBuilderStatic {
+  public function rebuild() {
+    // @TODO: add the route for the batch pages when that conversion happens.
+  }
+

Uh, it doesn't look like it builds much of anything. :-) What's intended here?

(Also, if this is a real class then we may want to consider adding an interface for this and the normal RouteBuilder to share.)

+++ b/core/update.php
@@ -179,6 +180,20 @@ function update_helpful_links() {
+ *
+ * This needs to be ran once all (if any) updates are ran. Do not call this

Grammar nitpick: "This will need to be run once all (if any) updates have been run."

Also, I hate to throw a wrench in the works, but... what about Events? What happens to those during module update? (I have no idea, personally.)

YesCT’s picture

Assigned: Unassigned » YesCT

patch coming to fix some docs stuff.

YesCT’s picture

Assigned: YesCT » Unassigned
FileSize
4.33 KB
23.66 KB

I dont know how to do most of what @Crell asked for in #25. So that still needs to be addressed.

I did coding style, spelling, and docs changes. The comments I added need to be checked by someone who better understands what is going on.

YesCT’s picture

+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.phpundefined
@@ -0,0 +1,137 @@
+      // @todo Figure out what to do about hook_install() and hook_enable().

Is there an issue for this already?

Anonymous’s picture

re #29 and CMI for modules - yes, CMI will support that.

the code hasn't landed yet, but i'm pretty sure heyrocker considers that functionality a release-blocker for CMI.

chx’s picture

Re #25, thanks for the review. Most of the questions are easy to answer:

> update_set_schema a method on ModuleHandler?

Followup. Has zero to do with firing hooks. #2010376: Add ModuleHandler::setSchema

> module_load_install a method on ModuleHandler?

Followup. Has zero to do with firing hooks. #2010380: Deprecate module_load_install() and replace it with ModuleHandler::loadInstall

> Injection conversions.

Followup. Has zero to do with firing hooks. The normal one isn't converted either. #2010382: Convert ModuleHandler and UpdateModuleHandler to use injection

> disable/uninstall during update

Followup. Has zero to do with firing hooks. #2010386: Resolve module disable/uninstall during update

> If so, then we're good. If not, then this becomes a regression.

Erm, nope. It's a bugfix: right now, module_disable during a major version update is broken. I understand the concern for minor upgrades and we need to address that in a followup, however that's not pressing. Getting in this patch is.

> Uh, it doesn't look like it builds much of anything. :-) What's intended here?

As the comment says: once the path 'batch' is converted to a route, add that here. All of the routebuilding mechanisms are gone, this will be the only place where routes can be stored.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

FileSize
498 bytes
23.63 KB

Let's try this -- entity_info now throws an exception. yay!

Status: Needs review » Needs work

The last submitted patch, 2001310_31.patch, failed testing.

tim.plunkett’s picture

I filed #1998204: config_install_default_config() is not safe to use in hook_update_N() for different reasons than invoking hooks, but it is now doubly a problem.

In each of the failing tests, it is called first by user_update_8011() to install image styles.

I see you fixed system_update_8046(), which was the one that caused me to file that issue in the first place. I guess this will need the same treatment.

EDIT: This process of cherry-picking new config to install will very likely become a common need.
Since we'll already be using it twice it would probably be a good idea to add a helper function in config.inc

function config_install_single_config($type, $name, $config_name){}

config_install_single_config('module', 'node', 'views.view.frontpage');

chx’s picture

Status: Needs work » Needs review

#27 is the one to review/commit. #31 I tried, didn't work yet. Wish it did -- we need to work on that -- shows how important this patch is cos it really shouldn't happen.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/includes/update.incundefined
@@ -631,7 +627,7 @@ function update_fix_d8_requirements() {
-    update_module_enable(array('file'));
+    module_enable(array('file'));

Not having this extra function to confuse people is awesome.

Okay, I went through this line by line, and I think this with the reasonable list of follow-ups (which are normal conversions anyway), is good to go.

We can use the dedicated config_install_default_config() issue to tackle hook_entity_info().

And yes, the patch I reviewed and that is ready to commit is in #27.

tim.plunkett’s picture

Issue summary: View changes

added followups

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

Issue summary: View changes

Updated issue summary.

chx’s picture

I added a suggested commit message to the top because this patch contains one patch from berdir and one from yched.

chx’s picture

Issue summary: View changes

html formatting

chx’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

Overall this looks great, couple of things though:

+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.phpundefined
@@ -0,0 +1,137 @@
+      case 'element_info':
+      case 'stream_wrappers':
+      case 'theme':
+        return array('system');
+      case 'system_theme_info':
+      case 'entity_info':
+        return array();
+      case 'schema':
+        // t() in system_stream_wrappers() needs this. Other schema calls
+        // aren't supported.
+        return array('locale');

These could use comments as to why they're explicitly allowed. Why are some of them returning empty arrays rather than throwing exceptions etc. Any of these hooks we're special casing is likely a case we'll want to clean up separately so it'd be good to document why they're in here.

+++ b/core/lib/Drupal/Core/Extension/UpdateModuleHandler.phpundefined
@@ -0,0 +1,137 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function disable($module_list, $disable_dependents = TRUE) {
+    throw new \LogicException('Disabling modules is not supported during updates');
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function uninstall($module_list = array(), $uninstall_dependents = TRUE) {
+    throw new \LogicException('Uninstalling modules is not supported during updates');
+  }
+

I'm not sure that config staging is going to be enough for this - sometimes you want to run an update, then subsequently disable a module - particularly for custom updates rather than major version upgrades. However I think we can punt on this for now and there's already a follow-up open.

catch’s picture

Followup. Has zero to do with firing hooks. The normal one isn't converted either. #2010382: Convert ModuleHandler and UpdateModuleHandler to use injection

The reason there are so many un-injected dependencies in ModuleHandler is because #2004784: Move module install/uninstall implementations into ModuleInstaller was split out of this issue - but they were in the main added by that patch. Had that not been split out, they'd be in this patch too, so they're not pre-existing in any real sense.

catch’s picture

Status: Reviewed & tested by the community » Needs work
chx’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
24.14 KB
YesCT’s picture

Status: Needs review » Reviewed & tested by the community

Looks like all the concerns have been addressed.

pounard’s picture

+  public function disable($module_list, $disable_dependents = TRUE) {
+    throw new \LogicException('Disabling modules is not supported during updates');
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function uninstall($module_list = array(), $uninstall_dependents = TRUE) {
+    throw new \LogicException('Uninstalling modules is not supported during updates');
+  }

Does this means this will not be possible to disable a module anymore during an update procedure?

DamienMcKenna’s picture

Follow-on from #42: there needs to be a way of enabling & disabling modules during update scripts otherwise that'd be a regression that would affect many developers. If this is the intention then we would need a comparable solution that can be triggered in the correct sequence while other changes are being made.

A common workflow for our (Mediacurrent) dev team today is running two update scripts - one to enable a module and then another to either assign some initial settings via SQL queries / API calls or revert a feature; how would this workflow be handled were this to be committed?

tim.plunkett’s picture

Disabling modules is out of scope for this discussion.
Please feel free to disagree with Dries, catch, and alexpott over in #1199946-148: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed where it was decided that modules will no longer have a disabled state.

DamienMcKenna’s picture

@tim: Ok, so the entire module installing/enabling/disabling/uninstalling process is changing so it'll no longer be possible to "disable" a module. However, it should still be possible to uninstall a module via an update script, right?

Berdir’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.42 KB
24.24 KB

Well, removing that @todo was easier than expected ;)

The UpdateModuleHandler already takes care of installing the default config in a safe way, no need to do it twice for image.module.

Upgrade path tests are passing and I know we have test coverage that the image styles are there.

David_Rothstein’s picture

Status: Needs review » Reviewed & tested by the community

It's in scope if this is the issue that's going to make it impossible to do.... Also, there's definitely no agreement at #1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed about removing disabled modules at the API level (there's not even total agreement about removing them at the core UI level, although that one is closer).

@chx did file a followup for this whole issue already at #2010386: Resolve module disable/uninstall during update so that seems to handle it, but we should probably be prepared for that to be bumped to major (at least)?

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Sorry, crosspost.

Also, #922996: Upgrade path is broken since update.php does not enforce empty module list seems like a related issue (but I guess that one proposes going even further than this one does, which is why this was filed separately?).

Berdir’s picture

FileSize
617 bytes
24.27 KB

Removed one line too much. The case above used the same return array();

chx’s picture

Re #48 -- enforcing an empty module list does not throw exceptions -- it hides the bugs -- this reveals them.

We will deal with events too.

David_Rothstein’s picture

Actually, I thought that issue proposed not loading .module files at all, so in many cases it would be a fatal error rather than a silent one... However, I agree the approach here makes more sense, especially as a first step.

catch’s picture

Not loading module files at all would mean that hooks silently didn't run, so people would still be free to fire hook-invoking functions (assuming they don't live in a non-loaded .module file) - just no hooks would be fired. That was my main concern with the other issue since it's exactly the non-predictability of hook firing with API functions that has led to so many problems in the upgrade path.

Right now trying to uninstall a module during the major version upgrade path is de-facto not supported - we don't have an update_* function for doing that. However I personally in the past couple of weeks have written custom update handlers for uninstalling modules that had dependencies - but that I knew were only going to run in a 7-7 upgrade path in a specific site.

This is where the division between major and minor updates gets us into serious difficulty, if we'd gone the migrate route for the 7-8 upgrade path we'd not have to deal with this all over again. However this patch enforces the actual limitations with the update API, if it's so limited that people can no longer do what they used to, hopefully they'll step in to fix it. The alternative is 2-3 years of broken 7-8 upgrade path again due to all the hidden, hard to reproduce, and easy to introduce bugs this patch exposes.

effulgentsia’s picture

However I personally in the past couple of weeks have written custom update handlers for uninstalling modules that had dependencies - but that I knew were only going to run in a 7-7 upgrade path in a specific site.

Well, presumably a custom scenario like that could swap in yet some other *ModuleHandler service. Do we want a follow up to provide a MinorUpdateModuleHandler implementation in core, or is that ok to leave to contrib?

catch’s picture

I think #2010386: Resolve module disable/uninstall during update covers it well enough. The current patch here enforces the status quo (if you strictly follow not using API functions in hook_update_N()), that issue might make it possible cleanly, so we're not really losing anything here that's not already broken.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I think opening #2010386: Resolve module disable/uninstall during update, and the possibility for something like MinorUpdateModuleHandler, makes this safely RTBC again.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll

curl https://drupal.org/files/2001310_47.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 24849  100 24849    0     0  10587      0  0:00:02  0:00:02 --:--:-- 18488
error: patch failed: core/includes/update.inc:351
error: core/includes/update.inc: patch does not apply
error: patch failed: core/update.php:435
error: core/update.php: patch does not apply
chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
580 bytes
24.23 KB

Yeah because drupal_container()->get('module_handler') was replaced by Drupal::moduleHandler(); meanwhile.

alexpott’s picture

Title: Disallow firing hooks during update » Change notice: Disallow firing hooks during update
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 9d73599 and pushed to 8.x. Thanks!

chx’s picture

Title: Change notice: Disallow firing hooks during update » Disallow firing hooks during update
Status: Active » Fixed

update_module_enable() didn't exist in Drupal 7 and it didn't have a change notice. Nothing needs to be done at this point.

David_Rothstein’s picture

Title: Disallow firing hooks during update » Change notice: Disallow firing hooks during update
Status: Fixed » Active

Don't we need one for the fact that doing anything which invokes a hook during an update function is now prevented?

chx’s picture

Title: Change notice: Disallow firing hooks during update » Disallow firing hooks during update
Status: Active » Fixed

No. You were always told to use the update versions of the functions. This is not new. But, people fail to follow instructions (because it's hard to catch all of the things firing hooks like the infamous field_read_fields in a since-removed file_update_8002) so it's important to force them.

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

Title: Disallow firing hooks during update » Change notice: Disallow firing hooks during update
Status: Closed (fixed) » Active

I still don't see where people were previously told this was forbidden.

In both the hook_update_N() and Update versions of API functions documentation it only says:

...caution is needed when using any API function within an update function - particularly CRUD functions, functions that depend on the schema (for example by using drupal_write_record()), and any functions that invoke hooks...

(And it still says the exact same thing in Drupal 8 now, which I suppose needs to be changed.)

I'll see about writing a quick change notification.

David_Rothstein’s picture

Status: Active » Needs review

Created a change notification: https://drupal.org/node/2026515

We probably need a followup to change the above-mentioned API documentation in the codebase also.

chx’s picture

Status: Needs review » Fixed

Huh. We didn't forbid before? Well. Thanks much for the nice change notice.

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

Title: Change notice: Disallow firing hooks during update » Disallow firing hooks during update
Issue tags: -Needs change record
Fabianx’s picture

We have some problems in the theme system with that.

It seems calling theme() is allowed during updates?

But we want to actively go into a route of depending less and less on the theme registry and push the responsibility to the ModuleHandler instead.

Should we disallow usage of theme() during updates?

Fabianx’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Unfortunately, this causes serious problems, because the update.php page itself still has to be composed and rendered, which involves plenty of call chains that cannot really be controlled in the same way as hook invocations within update functions:

#2233403: Let update.php invoke hooks again