Problems with update_module_enable()

Yeah. This will get very, extremely hairy. The new DrupalKernel is not at all prepared for that.

Let's have fun in #1849004: One Service Container To Rule Them All

Neither #1798732: Convert install_task, install_time and install_current_batch to use the state system nor #1849004: One Service Container To Rule Them All is touching update.php.

It's also pretty clear that update.php does not follow any of the regular bootstrap flows and that it cannot. We need to figure out

1) if it is possible at all to register any bundles in update.php

2) if it is possible: at which exact point it is possible to register bundles in update.php

3) whether it is legit to make update_module_enable() rebuild the kernel or whether it is not.

Services are APIs, and for the same reason we don't invoke APIs and hooks in update.php, it is probably a very bad idea to register and invoke any services.

Neither of them does it yet because the current 'remove drupal_container' patch is less than a day old and simply I needed to get to other issues first. However, unlike with hooks where the problem is we do not know what gets fired services are limited in scope and if we have a kernel we can have proper overrides for them. So I would recommend going back to duplicate and get the other one complete.

Who guarantees that the storage/schema of a new/current/updated service will never change?

Hrm, OK, let's keep this one open for a while, the other one passed, hopefully we can get that one in and continue on a better footing here.

Note that I tried

function system_update_8037() {
  module_enable(array('views'), FALSE);
}

which of course is not correct, but, it does pass with my patch. Once my patch is in we can take a look at update_module_enable and get this fixed.

As far as I can see update_module_enable() is still broken with Views, discussed with xjm and moving this back to critical/bug.

update_hook_only.patch queued for re-testing.

At least the following steps are required:

module_set_weight('views', 10);
$module_list = drupal_container()->getParameter('container.modules');
$module_list['views.module'] = 'core/modules/views.module';
drupal_container()->get('kernel')->updateModules($module_list);
// This does not fire a hook just calls views.
config_install_default_config('module', 'views');

Also make sure views.module is loaded before config_install_default_config is called so that the module_invoke / module_hook does not go haywire.

Yeah that worked out after some small changes to the code :)

Except you missed update_module_enable. Re-added. Someone needs to write a better test.

Regarding the upgrade test: It should explicit check whether the frontpage is now a view, though we sort of need to get the actual conversion in first. Do you have a better way in mind to check whether a module is enabled?#

Fixes the function naming but not sure about the actual problem, calling update_module_enable seems to have some odd side effects.

Just a rerole.

#25: drupal-1848998-25.patch queued for re-testing.

Last patch for today, one upgrade test runs locally.

That looks like a random fail. Re-testing.

#29: drupal-1848998-29.patch queued for re-testing.

Docblock, whitespace errors fixed, this is ready, dawehner you are awesome.

+++ b/core/modules/system/system.install
@@ -2195,6 +2195,25 @@ function system_update_8045() {
+    drupal_container()->get('kernel')->updateModules(array_keys($module_list), array('views' => 'core/modules/views/views.module'));

Whoa!! I was so surprised that this worked, since I didn't realize that there was a 'kernel' service during update.php. But I see that HEAD now has a DRUPAL_BOOTSTRAP_KERNEL phase, which kind of blows my mind, and I don't know if it will make sense to keep (I haven't figured out yet whether that's genius or insanity), but that's not this issue's concern. Very nice that such a small patch unblocks Views progress for now.

#35, let me help: I added that phase. So, genius.

chx has a humble day, I see. ;)

Awesome! Thanks so much for figuring this out!!

Committed and pushed to 8.x. YEAH.

I'm not changing the status since I don't know if it's just me, but update #8046 fails on my vanilla standard install. I ran update.php twice. I'll just reinstall but wanted to report this.

Update #8046
Failed: Drupal\Core\Database\SchemaObjectExistsException: Table cache_views_info already exists. in Drupal\Core\Database\Schema->createTable() (line 662 of /PATH/TO/drupal/core/lib/Drupal/Core/Database/Schema.php).

I can replicate. :(

That means we need tests for this. :P

I think #38 was trying to run from an earlier installed Drupal 8, which is not supported. echoz, what was the original Drupal version, 7 or 8?

Adding a test/check for that is a whole another issue. For now I am keeping this at active until we hear from echoz or someone else.

#41, from up to date git pull.
Occasionally I do neglect running update.php every time but I'm pretty consistent with that. Maybe this is what happened to both me and webchick, skipping over a database update just prior to this one.

didnt mean to change status

echoz, I do not understand, at all. You installed, some time in the past, a Drupal, but was it Drupal 7 or Drupal 8? and then run update.php on it, yes, but what was the Drupal that did the install? If D7, we need a dump of the failing database or something like that, if D8, please close the issue.

I got the error running update.php on an install that was originally D8, a test environment with only core, in which I frequently drop the database and reinstall.

I got the error running update.php on an install that was originally D8, a test environment with only core, in which I frequently drop the database and reinstall.

Just to be sure, you don't get that error on a fresh installation?

#48, no. I got the error running update.php after pulling only this commit and the minor one before it (only a text file)

Okay great, then there is no problem.

Drupal Core doesn't support HEAD to HEAD updates before alpha/beta-phase.

So much time spent on discussing whether a bug is a bug, whereas it's clear that there is a bug.

Additionally, I agree with @effulgentsia in #35. I already mentioned in #4 that this will cause us quite some headaches.

Services are APIs, in the same way module functions and hooks have been APIs. Up until now, modules were not allowed to use any APIs and services other than the Database service in module updates.

+++ b/core/modules/system/system.installundefined
@@ -2203,13 +2203,17 @@ function system_update_8046() {
+    drupal_container()->get('kernel')->updateModules($module_filenames, $module_filenames);

Oh yeah it's confusing that the first parameter of updateModules are the module names keyed by module name.

+++ b/core/modules/system/system.installundefined
@@ -2203,13 +2203,17 @@ function system_update_8046() {
+    // Perform hook_install() operations.
+    views_install();

So we set the weight of views, I'm wondering whether we actually still need this.

ups.

1. config_install_default_config() calls into the entity system/API, which invokes plenty of entity CRUD hooks. That's why that code comment is a lie. We should either fix the comment and clarify why it is OK to call into plenty of APIs there, or remove the installation of default configuration.
2. Off-hand, I don't really understand why we need to install Views 1) at all during the upgrade path to D8, and/or alternatively, 2) why we need to install its default configuration. The update does not document why it installs Views. Normally, we do not install new modules, unless they are new required modules.
3. The subsequent loading of views.module clarifies further that this update is calling into module API functions, whereas, like any other update function, it isn't allowed to do that.
4. update_module_enable() already contains a @todo that it probably should invoke hook_install(), too. We've never been really sure about what this update helper function is supposed to do, and what it is allowed to do. In general, updates are on their own, so update_module_enable() actually shouldn't perform too many things. E.g., views_install() might be changed in the future to contain function calls that are not compatible with update.php anymore (e.g., calling into APIs).

We install Views because we want to convert listings to Views. Right now Views is there but nothing uses it because nothing can without an upgrade path.

Why only conditionally then?

With the previously contained condition, we have to "re-install" it for every other module + default view.

It sounds like you guys want to make Views required, so let's be honest, and just enable that thing unconditionally. A follow-up issue to mark it as required would probably make sense.

I didn't understand why the 0-schema didn't include the full schema definition of views_schema(). I checked all existing System and Views module updates, and there isn't any that would perform the additional tweaks to those two cache tables, so views_schema_0() diverges from the actual views_schema() when upgrading to HEAD now.

And since no one seems to know why that default weight of 10 exists, I also removed that.

Right, but if it's not required then we are not required to enable it during the upgrade path and the site should still continue to work. We haven't enabled the new optional/standard-profile modules like toolbar in the 7.x - 8.x either.

IMHO, the upgrade path should instead clear the default frontpage configuration if it points to node and display a message that users might want to enable views.

Especially since views will require a contrib upgrade path module anyway.

Thanks @Berdir, that mirrors exactly my train of thought.

We've been over this already.

I don't see an indication of #1864980: [meta] Figure out how to integrate Views into core being anywhere near a community agreement and resolution?

Note that I don't disagree with anything. I'm just noting that this patch here feels a lot like infiltrating and ignoring the entire meta discussion and just doing something else under the hood, while others are discussing. In any case, we need a clear path forward — and if that means to make it required, or to make it not-required-but-actually-required, then we should announce that clearly and file the appropriate major follow-up tasks for that direction. If so, IMHO, we should aim for no less than to eliminate views.module and move it into a Drupal\Core\View component (there's very little procedural code left).

Ok, I think I was not quite clear in my comment. My argument has not much to do with views being a required module or not or even views at all. Let me to write down my thoughts.

- Writing a working upgrade path is hard.
- Enabling modules during a (core) upgrade is probably even harder. #1239370: Module updates of new modules in core are not executed has been open for a year, we have added update_module_enable() as a stop gap, but to quote @sun: " We've never been really sure about what this update helper function is supposed to do, and what it is allowed to do.". So we already have a critical issue for/about that function anyway...
- So far, we have only used this function to enable modules that were absolutely required to maintain data integrity, new modules that are required by already installed ones and modules that contain tables that were previously part of system.module like file.module and ban.module
- Views is the first example that's different, not enabling it will not result in a fatal error or lost data.
- Enabling it during the upgrade has implications on the upgrade path from now on. Unlike module_enable(), update_module_enable() means that update functions will be executed. That has consequences that we might not be able to think of now. One that we will need to solve anyway at some point is what happens if one of those update-enabled modules adds an update function. Doing that currently messes up our upgrade path tests completely :)

So, those are not reasons to *not* do it, more like "are we sure we can deal with that" and is it worth it to provide a (IMHO slightly) better experience to people upgrading from 7.x. I'm not so sure, as I expect all sites that did not limit themself to core modules and a core theme will suffer from "missing features left and right if we do that" anyway. 5. in #70 means that all sites that were using views before will be missing their views anyway until they download a contrib module to migrate it anyway, which I think is going to be the majority of upgraders. But I'm not right person to ask that :)

I think it's OK to have a note somewhere that tells people to install views after the upgrade path to get their listings back. If it's core listings only they don't need a contrib module, just need to enable whichever default views.

We supposedly guarantee data integrity after upgrades, but we definitely don't guarantee anything else.

#1856972: config() isn't safe to use during minor updates if there's a change to configuration storage is slightly different to the issue with config_install_default_config(). That issue was mainly about the possibility of the format changing after RC. I'd be OK with broadening the scope of that issue a bit, but heyrocker suggested leaving the format change issue unfixed - i.e. just say we'll never, ever change the format for the entire 8.x lifecycle, in which case that issue would be 'fixed' but config_install_default_config() still wouldn't be.

Use of undefined constant VIEWS_API_VERSION - assumed 'VIEWS_API_VERSION'
Notice: Drupal\Core\Config\Entity\ConfigStorageController->create():236

So the installation of config actually depends on views.module. Although if it's that constant only, then we ought to be able to move that into the View entity class.

Or even better - drop it entirely. Within a major core version, there'll only be a single API version.

Oh, and now I remember that I created #1677258: Configuration system does not account for API version compatibility between modules a very long time ago for this. We should probably bump that to major or even critical. An eco-system module like Views, Panels, CTools, etc will need that in order to deal with differing configuration schemata across its own major versions.

Given the direction of #1864980: [meta] Figure out how to integrate Views into core, I'd suggest to do this.

@sun #1894784: Remove api_version from views

sorry.

Please note this comment:

+    // 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().
+    update_module_enable(array('views'));

What we actually want to do for #1806334: Replace the node listing at /node with a view and potentially related issues is to provide a update helper function that essentially copies a config file from module Foo into the active config directory. We cannot call into Views API in updates.

Or to put it differently, two possible upgrade paths, on a case-by-case decision:

1. Copy new D8 default config files into the active config directory. If necessary, perform manual adjustments on the raw config object via config() afterwards.
2. Create a new config object from scratch, using config() only.

As with default configuration of modules, configuration created by the upgrade path should be able to simply ignore and omit all properties and keys that have sane defaults. The config entity files will be rewritten into full-blown objects including all keys upon the first time they're updated.

That's why I think that 2) is actually doable in upgrade paths. Spelling out an entire views config file with all keys + subkeys obviously would not, but if you can scale it down to the keys you actually want to set (only), it should be doable.

load the configuration from the default config dir and then save it with config->save(), with any changes that are needed

Yep, way to go. I'd imagine a update helper function to perform the copy step:

function update_config_copy_default($module, $name);

- Copies the config object $name from the $module's ./config directory into the active config directory.
- Throws an exception if $name doesn't exist.
- Does not overwrite $name in the active config directory, if it already exists.
- Does not care for manifest files.

update_config_copy_default('node', 'views.view.node');
config('views.view.node')
  ->set('pager.limit', config('node.settings')->get('items_per_page'))
  ->save();
update_config_manifest_add('views.view', array('node'));

Would you prefer to introduce that here or in a separate issue?

That said, we might still need to do something about (at least static/non-entity) default configuration of a module. For already existing modules, the individual @ingroup config_upgrade updates will copy default configuration files via update_variables_to_config(). But for modules that get installed from scratch in the upgrade path (File module, Views module), we might actually have to copy the entire ./config directory contents into the active config directory in update_module_enable().

I've added that to update_module_enable() in attached patch. Doesn't contain anything for the new update helper mentioned above.

I think that #86 is RTBC.

Given that the existing system update in HEAD calls into module APIs/services + hooks during update.php, this patch might also help to contribute to fixing #1893800: Something is very, very wrong with update.php / upgrade tests... demons suspected — manual testing of reverting individual core commits (including this one) didn't show an improvement, but in any case, this follow-up patch presents a correction and clean-up to an update in the upgrade path that technically should trigger all kinds of weirdness, so this definitely moves us into the right direction.

#86: drupal8.views-update.86.patch queued for re-testing.

Not sure what would/could needs tests? I'd rather suggest to quickly move forward here, so we can advance with the views listing conversions.

This might work with Views -- but is it generic? I mean, config_install_default_config calls the owner module to save the config and who knows what the owner module is doing. Disclaimer: this is config import. I do not know and I do not want to know anything about that but it's a critical so I am almost forced to follow up.

The latest patch in #86 does not call config_install_default_config(), so I'm not sure how #91 is related.

#86: drupal8.views-update.86.patch queued for re-testing.

Stumbled on my own problems with update_module_enable(), can't really decide whether it's a dupe of this one, but the latest patch here seems to go in entirely different directions :

#1941000: update_module_enable() does not update ModuleHandler::$moduleList (with patch)

Re-roll.

- Changed the installation of the default view to only install that specific file but with the same method as update_module_enable()
- Moved the enabling of the language module a few lines up because otherwise it now blows up because the default config already exists. Not sure what's the right thing there, move this up or do not blow up.

#96: update-enable-views-1848998-96.patch queued for re-testing.

#96: update-enable-views-1848998-96.patch queued for re-testing.

As per @chx instructions, closing #1941000: update_module_enable() does not update ModuleHandler::$moduleList as a duplicate.
The patch over there has a fix that doesn't seem to be included here, should probably be merged.

Resetting to fixed and continuing in #2001310: Disallow firing hooks during update which includes #96