Problem/Motivation

Bundle services are (apparently) not available in update.php. To provide an upgrade path for #1806334: Replace the node listing at /node with a view, we want to enable the Views module if the D7 site's frontpage is node. Simply enabling Views with update_module_enable() will throw the following exception and completely break the site upgrade:

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "plugin.manager.views.access" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of /Applications/MAMP/htdocs/d7git/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

Attached patch demonstrates this. This is not just an upgrade test issue; I confirmed this happens when manually upgrading a bare D7 install with the standard profile to D8 with this patch applied.

Related issues

Files: 
CommentFileSizeAuthor
#96 update-enable-views-1848998-96.patch6.91 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 53,988 pass(es).
[ View ]
#86 drupal8.views-update.86.patch4.9 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.views-update.86.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#79 drupal8.views-update.79.patch3.38 KBsun
PASSED: [[SimpleTest]]: [MySQL] 50,821 pass(es).
[ View ]
#60 drupal8.views-update.60.patch3.24 KBsun
FAILED: [[SimpleTest]]: [MySQL] 50,349 pass(es), 24 fail(s), and 48 exception(s).
[ View ]
#51 drupal8.views-update.51.patch1.09 KBsun
PASSED: [[SimpleTest]]: [MySQL] 50,706 pass(es).
[ View ]
#34 1848998_34.patch3.05 KBchx
PASSED: [[SimpleTest]]: [MySQL] 50,664 pass(es).
[ View ]
#29 drupal-1848998-29.patch3.06 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 50,767 pass(es).
[ View ]
#25 drupal-1848998-25.patch1.86 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 50,517 pass(es), 84 fail(s), and 105 exception(s).
[ View ]
#23 drupal-1848998-23.patch1.86 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 48,979 pass(es), 18 fail(s), and 36 exception(s).
[ View ]
#21 1848998_21.patch1.81 KBchx
FAILED: [[SimpleTest]]: [MySQL] 48,891 pass(es), 17 fail(s), and 34 exception(s).
[ View ]
#20 drupal-1848998-20.patch1.77 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 49,422 pass(es).
[ View ]
#16 system-1848998-update-hook-only.patch638 bytesxjm
FAILED: [[SimpleTest]]: [MySQL] 48,905 pass(es), 33 fail(s), and 96 exception(s).
[ View ]
update_hook_only.patch638 bytesxjm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

Comments

xjm’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

chx says this will potentially be resolved in the dubiously titled #1798732: Convert install_task, install_time and install_current_batch to use the state system.

sun’s picture

Title:Services not available in update.php» Module/Bundle services not available in update.php
Issue tags:+upgrade path, +D8 upgrade path

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

chx’s picture

Status:Needs review» Closed (duplicate)
sun’s picture

Status:Closed (duplicate)» Active

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.

chx’s picture

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.

sun’s picture

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

chx’s picture

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.

chx’s picture

Note that I tried

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

xjm’s picture

Issue tags:+VDC
xjm’s picture

Category:bug» task
Priority:Critical» Major

Is this a duplicate now, or is this the issue where we're going to handle:

+++ b/index.phpundefined
@@ -30,6 +30,11 @@
+// @todo Remove this once everything in the bootstrap has been
+//   converted to services in the DIC.
+$kernel->boot();
+drupal_bootstrap(DRUPAL_BOOTSTRAP_CODE);
xjm’s picture

Oops, wrong snippet. That's in index.php. It looks like this is the only thing that stands out right now for the upgrade path tests:

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UpgradePathTestBase.phpundefined
@@ -134,6 +134,7 @@ protected function setUp() {
     $this->variable_set('site_mail', 'simpletest@example.com');

     drupal_set_time_limit($this->timeLimit);
+    $this->rebuildContainer();
     $this->setup = TRUE;

catch’s picture

Category:task» bug
Priority:Major» Critical

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

xjm’s picture

#1798832-33: Convert https to $settings shows upgrade path test failures that are potentially caused by this issue:

exception 'Symfony\Component\DependencyInjection\Exception\InvalidArgumentException' with message 'The service definition "config.factory" does not exist.' in /var/lib/drupaltestbot/sites/default/files/checkout/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php:740
lovelykaushik86’s picture

Status:Active» Needs review
Issue tags:-upgrade path, -D8 upgrade path, -VDC

update_hook_only.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+upgrade path, +D8 upgrade path, +VDC

The last submitted patch, update_hook_only.patch, failed testing.

xjm’s picture

Status:Needs work» Needs review
StatusFileSize
new638 bytes
FAILED: [[SimpleTest]]: [MySQL] 48,905 pass(es), 33 fail(s), and 96 exception(s).
[ View ]

Right now that's just choking because we've added more update hooks. Can we get away with doing this, just for purposes of exposing the failure? :P

Status:Needs review» Needs work

The last submitted patch, system-1848998-update-hook-only.patch, failed testing.

chx’s picture

At least the following steps are required:

<?php
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');
?>
chx’s picture

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.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.77 KB
PASSED: [[SimpleTest]]: [MySQL] 49,422 pass(es).
[ View ]

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

chx’s picture

StatusFileSize
new1.81 KB
FAILED: [[SimpleTest]]: [MySQL] 48,891 pass(es), 17 fail(s), and 34 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1848998_21.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.86 KB
FAILED: [[SimpleTest]]: [MySQL] 48,979 pass(es), 18 fail(s), and 36 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, drupal-1848998-23.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.86 KB
FAILED: [[SimpleTest]]: [MySQL] 50,517 pass(es), 84 fail(s), and 105 exception(s).
[ View ]

Just a rerole.

Status:Needs review» Needs work
Issue tags:-upgrade path, -D8 upgrade path, -VDC

The last submitted patch, drupal-1848998-25.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work
Issue tags:+upgrade path, +D8 upgrade path, +VDC

The last submitted patch, drupal-1848998-25.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new3.06 KB
PASSED: [[SimpleTest]]: [MySQL] 50,767 pass(es).
[ View ]

Last patch for today, one upgrade test runs locally.

xjm’s picture

+++ b/core/includes/update.incundefined
@@ -633,6 +633,7 @@ function update_module_enable(array $modules) {
+    module_load_install($module);

Yeah, uhm, yeah. That explains a lot...

+++ b/core/modules/views/views.installundefined
@@ -26,3 +26,19 @@ function views_schema() {
+ * Implements hook_schema_0().

This isn't actually a hook so we should change the docblock.

Status:Needs review» Needs work

The last submitted patch, drupal-1848998-29.patch, failed testing.

webchick’s picture

That looks like a random fail. Re-testing.

webchick’s picture

Status:Needs work» Needs review

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

chx’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new3.05 KB
PASSED: [[SimpleTest]]: [MySQL] 50,664 pass(es).
[ View ]

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

effulgentsia’s picture

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

chx’s picture

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

webchick’s picture

Status:Reviewed & tested by the community» Fixed

chx has a humble day, I see. ;)

Awesome! Thanks so much for figuring this out!!

Committed and pushed to 8.x. YEAH.

echoz’s picture

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

webchick’s picture

Status:Fixed» Needs work

I can replicate. :(

That means we need tests for this. :P

xjm’s picture

chx’s picture

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?

xjm’s picture

Actually I think this might not be a problem; we don't support head-to-head. However, a check to see if the table exists can't hurt.

chx’s picture

Status:Needs work» Active
Issue tags:-Needs upgrade path tests

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.

echoz’s picture

Status:Active» Needs work
Issue tags:+Needs upgrade path tests

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

echoz’s picture

Status:Needs work» Active

didnt mean to change status

chx’s picture

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.

echoz’s picture

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.

dawehner’s picture

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?

echoz’s picture

Status:Fixed» Active

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

dawehner’s picture

Status:Active» Fixed

Okay great, then there is no problem.

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

sun’s picture

Status:Active» Needs review
StatusFileSize
new1.09 KB
PASSED: [[SimpleTest]]: [MySQL] 50,706 pass(es).
[ View ]

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

sun’s picture

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.

dawehner’s picture

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

ups.

xjm’s picture

+++ b/core/modules/system/system.installundefined
@@ -2203,13 +2203,17 @@ function system_update_8046() {
     // This does not fire a hook just calls views.
+    // @todo This is a lie.
+    drupal_load('module', 'views');
     config_install_default_config('module', 'views');
+
+    // Perform hook_install() operations.
+    views_install();

Uh, could we explain this a little more? I don't understand. Also, I'd expect update_module_enable() to install the module too? We should probably update the documentation on that function for what its purpose is and how it should be used.

xjm’s picture

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

  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).
chx’s picture

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.

sun’s picture

StatusFileSize
new3.24 KB
FAILED: [[SimpleTest]]: [MySQL] 50,349 pass(es), 24 fail(s), and 48 exception(s).
[ View ]

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.

xjm’s picture

The installation of Views started out as just a way to demonstrate the bug; I wasn't expecting to commit that update here initially. The underlying issue is basically (I guess) that update_module_enable() doesn't work as expected, apparently in multiple different ways. Most Drupal sites will need to enable Views during the upgrade if they use any listing that's converted to Views.

xjm’s picture

@sun, No, it is not required and should not be required. It is only needed if you want to use a listing that uses it. We've been over this already. :)

Berdir’s picture

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.

xjm’s picture

Title:Module/Bundle services not available in update.php» Problems with update_module_enable()

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.

I don't think this is a good solution; peoples' sites will be suddenly missing features left and right if we do that. Half the blocks in core are going to be converted to Views, so sites that have them should have Views enabled to facilitate this conversion. We can write a views_upgrade_enable() that conditionally enables the module if it's not already on, but I don't think not enabling will be an option. I was expecting to eventually have a list of conditions to check to determine whether Views should be installed, and it needs to depend on what specific Views-to-be a site is using. We can expand the list with each conversion.

sun’s picture

Title:Problems with update_module_enable()» Module/Bundle services not available in update.php

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

xjm’s picture

Title:Module/Bundle services not available in update.php» Problems with update_module_enable()

So there are several possibilities:

  1. The D7 site does not include Views, and does not use any listings that are converted to Views -> Don't enable Views.
  2. The D7 site does not include Views, but does use listings that are converted to Views -> Install and enable Views.
  3. The D7 site already has Views installed but does not use any core listings that are being converted to views -> The user follows the instructions in http://drupal.org/project/views_d8_upgrade.
  4. The D7 site already has Views installed and also uses core listings that are being converted to views -> The user follows the instructions in http://drupal.org/project/views_d8_upgrade, but Views also needs to be enabled during the core upgrade.

We should not forcibly turn on Views on whatever subset of 30% of Drupal sites that don't need it in D8, and we should also not randomly make features disappear on all the rest.

xjm’s picture

@sun, I think everyone but you has agreed in that issue. :)

xjm’s picture

Also, the difference between toolbar and Views is that there are no features of other core modules that are converted from custom code to toolbar in D8. It's a completely different scenario.

Status:Needs review» Needs work

The last submitted patch, drupal8.views-update.60.patch, failed testing.

xjm’s picture

Alright, so we discussed this in IRC. Trying to clarify things:

  1. The fact that you can't enable Views in the upgrade path is a bug, or possibly a composite of several bugs, regardless of when and whether we want to enable it.
  2. This issue is not some sneaky attempt to skip consensus; it is a hard blocker for #1806334: Replace the node listing at /node with a view and has been for two months.
  3. We can either conditionally enable Views based on the site features in use, which is what #1806334: Replace the node listing at /node with a view does (and therefore what was unintentionally committed for this issue because we used that update hook to test the fix), or we can unconditionally enable it and tell 10% or whatever of Drupal sites that have no installed Views after the upgrade is complete that they can uninstall it.
  4. No, we are not making Views required = TRUE. See #1864980: [meta] Figure out how to integrate Views into core for a discussion of why that is not desirable.
  5. Upgrading contrib Views data to D8 will be handled separately, outside of core.
Berdir’s picture

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

xjm’s picture

I don't think we can make a contrib module responsible for updating core data. 30% of Drupal sites don't use Views in D7.

Say I never discovered contributed modules. /node is my frontpage or is linked in my primary navigation menu. I have an aggregator category block configured with 5 items, a recent comments block with 20 items, taxonomy terms with their taxonomy/term/x pages. All these things turn into Views. I don't think it makes sense for there to be 404s all over the place until I install a contrib module, or even until I sort out how to go to the modules page and enable Views.

I realized @sun is right that it doesn't make sense to enable Views conditionally, but not because of Views being pseudo-required or whatever (which I continue to disagree with). @webchick and I realized today that even if you don't use /node as your frontpage, the /node system path is always available from the node module. Since the node module is required in D7, we can just unconditionally enable Views during the upgrade--making sure its config, schema, etc. are installed safely and that its status is set to enabled. That way we only have to do it once, which makes it easier to specify dependencies, at least. Once system_update_8046() is run, then the comment module can convert someone's D7 recent comment block showing 5 comments to the appropriately configured View configuration file. And so on.

xjm’s picture

+++ b/core/modules/system/system.installundefined
@@ -2195,22 +2195,26 @@ function system_update_8045() {
+  if (db_table_exists('cache_views_info')) {
+    db_drop_table('cache_views_info');
+  }
+  if (db_table_exists('cache_views_results')) {
+    db_drop_table('cache_views_results');
+  }

These tables don't exist in the D7 version of views, so we don't need this. Edit: Was the intention here to support upgrading from HEAD as of Oct. 22 to HEAD as of Jan. 19? We don't support that for anything else; why should we here?

+++ b/core/modules/system/system.installundefined
@@ -2195,22 +2195,26 @@ function system_update_8045() {
+  // Install default views.
+  // Note: This calls into APIs and invokes hooks.
+  config_install_default_config('module', 'views');

So this is where we would need a solution for #1856972: config() isn't safe to use during minor updates if there's a change to configuration storage, correct?

xjm’s picture

Assigned:Unassigned» xjm
catch’s picture

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.

sun’s picture

Assigned:xjm» Unassigned

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.

xjm’s picture

Assigned:Unassigned» xjm

I am working on this, which is why I assigned it to myself.

xjm’s picture

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.

The problem I see with this is that there is data tied to core listings. For example, most listing blocks allow you to configure the number of items displayed. If we tell people to put all their listings back themselves, we are losing data about what regions the blocks are displayed in, how many items they show, etc. @berdir suggested writing views.view.* configuration files for such pages and blocks to the active configuration directory with Views still disabled, but this violates a fundamental assumption we have in #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API that we don't let orphaned config sit around in the active config directory.

So I guess we have two options:

  1. Screw writing upgrade paths for listings from D7 core to D8 core views.
  2. Proceed with this patch.
sun’s picture

Status:Needs work» Needs review
StatusFileSize
new3.38 KB
PASSED: [[SimpleTest]]: [MySQL] 50,821 pass(es).
[ View ]

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

dawehner’s picture

Status:Needs review» Needs work
dawehner’s picture

Status:Needs work» Needs review

sorry.

xjm’s picture

Assigned:xjm» Unassigned
xjm’s picture

Issue tags:+Needs manual testing

So, this looks totally sane to me as it stands. However, it needs to be tested with #1806334: Replace the node listing at /node with a view to see if that patch can actually install the view it provides.

sun’s picture

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.

xjm’s picture

Right, so I'm assuming we add node_update_x() to install the configuration file manually, and similarly for each listing we convert in each respective module. I wonder if it is safe to load the configuration from the default config dir and then save it with config->save(), with any changes that are needed? (In the case of the node listing, that's nothing, but there are some configuration settings we'd want to save for other listings. @berdir pointed out that the number of items listed is actually configurable in D7 as well.)

sun’s picture

StatusFileSize
new4.9 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.views-update.86.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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:

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

sun’s picture

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.

sun’s picture

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

xjm’s picture

I think we probably need tests? I agree #86 looks good though.

sun’s picture

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.

chx’s picture

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.

sun’s picture

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

Berdir’s picture

Status:Needs review» Needs work
Issue tags:+Needs tests, +upgrade path, +Needs manual testing, +D8 upgrade path, +VDC

The last submitted patch, drupal8.views-update.86.patch, failed testing.

yched’s picture

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)

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new6.91 KB
PASSED: [[SimpleTest]]: [MySQL] 53,988 pass(es).
[ View ]

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.

Status:Needs review» Needs work
Issue tags:-Needs tests, -upgrade path, -Needs manual testing, -D8 upgrade path, -VDC

The last submitted patch, update-enable-views-1848998-96.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, update-enable-views-1848998-96.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review
Issue tags:+Needs tests, +upgrade path, +Needs manual testing, +D8 upgrade path, +VDC

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

yched’s picture

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.

chx’s picture

Status:Needs review» Fixed

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

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

Anonymous’s picture

Issue summary:View changes

Updated issue summary.