Remove rebuildAll() and module install from UpdatePathTestBase

Should the Drupal Extension implementation allow $root to be set? It does make sense that potentially an extension could live outside of the drupal install, but that isn't very Drupally.

Extension::serialize() could always set $root to DRUPAL_ROOT, but that doesn't solve the problem of the database dump, which will just serialize it to whatever path the user has setup since the constant will be expanded.

I guess we need to set it to test bot root - /var/www/html.

1.6MB patch. Not sure if it's worth posting a 1.4MB interdiff.

I didn't run the database dump script, but here's diffs of the uncompressed dumps.

Edit: drupalci d.o looks for diff extension too, welp. :(

That's not really an option, that will then cause fails on local installations and possibly on the new testbot as well?

Why is that only a problem for theme data but not for modules? Why do we even need root in there?

Possible solution: what if serialize() checks if root == DRUPAL_ROOT, and if so, sets it to NULL and then if it is NULL, sets it back to DRUPAL_ROOT when it is initialized?

system.theme.data is the only place where Extension objects are serialized in the database dumps.

Yes, I think that @berdir's Extension::unserialize makes sense.

@catch's initial patch probably also fails on the below error most likely because the container is not set, which happens in WebTestBase::resetAll() called from WebTestBase::rebuildAll():

22:58:21 Fatal error: Call to a member function get() on a non-object in /var/www/html/core/modules/simpletest/src/WebTestBase.php on line 1224
22:58:22 FATAL Drupal\system\Tests\Entity\Update\SqlContentEntityStorageSchemaIndexFilledTest: test runner returned a non-zero error code (255).

Extension does not do any sanity check on if the file path exists.

Should Extension::load() do a realpath and return FALSE?
Or should Extension::unserialize() automagically make assumptions if root does not exist?
Or maybe both?

And after adding the container back to the test class, we find that update.php errors out with 500 error in the test run, and none of the database updates are run. There is something in the cache clear that lets the test runner do its thing. I saw this on Monday night when I worked on this before.

For posterity, a patch that will demonstrate the above.

The cache clear is from the commit in #347959: modules_installed is broken during testing.

It also looks like the current behavior is a regression of #1404198: Separate database cache clearing from static cache clearing and data structure rebuilding after reading that Issue Summary. Most likely changed so that the menu/router changes could go through.

To be more verbose about the patch in #12:

Exception Warning    RouteProvider.php  235 Drupal\Core\Routing\RouteProvider->
    Insufficient data for unserializing - 845 required, 845 present
Exception Notice     RouteProvider.php  235 Drupal\Core\Routing\RouteProvider->
    unserialize(): Error at offset 44 of 889 bytes

which is trying to unserialize the update.php route, which unserializes fine if I paste it into unserialize somewhere with the autoloader present.

C:31:"Symfony\Component\Routing\Route":1215:{a:9:{s:4:"path";s:16:"/update.php/{op}";s:4:"host";s:0:"";s:8:"defaults";a:3:{s:6:"_title";s:22:"Drupal database update";s:11:"_controller";s:52:"\Drupal\system\Controller\DbUpdateController::handle";s:2:"op";s:4:"info";}s:12:"requirements";a:2:{s:21:"_access_system_update";s:4:"TRUE";s:7:"_method";s:8:"GET|POST";}s:7
:"options";a:4:{s:14:"compiler_class";s:34:"\Drupal\Core\Routing\RouteCompiler";s:14:"_route_filters";a:1:{i:0;s:27:"content_type_header_matcher";}s:16:"_route_enhancers";a:1:{i:0;s:31:"route_enhancer.param_conversion";}s:14:"_access_checks";a:1:{i:0;s:22:"access_check.db_update";}}s:7:"schemes";a:0:{}s:7:"methods";a:2:{i:0;s:3:"GET";i:1;s:4:"POST";}s:9:"condition";s:0:"";s:8:"compiled";C:33:"Drupal\Core\Routing\CompiledRoute":458:{a:11:{s:4:"vars";a:1:{i:0;s:2:"op";}s:11:"path_prefix";s:11:"/update.php";s:10:"path_regex";s:37:"#^/update\.php(?:/(?P<op>[^/]++))?$#s";s:11:"path_tokens";a:2:{i:0;a:4:{i:0;s:8:"variable";i:1;s:1:"/";i:2;s:6:[^/]++";i:3;s:2:"op";}i:1;a:2:{i:0;s:4:"text";i:1;s:11:"/update.php";}}s:9:"path_vars";a:1:{i:0;s:2:"op";}s:10:"host_regex";N;s:11:"host_tokens";a:0:{}s:9:"host_vars";a:0:{}s:3:"fit";i:1;s:14:"patternOutline";s:11:"/update.php";s:8:"numParts";i:1;}}}}

update.php moved back to a front controller in #2540416: Move update.php back to a front controller - so why are we unserializing the update.php route?

Well, its part of the previous dump, right?

1. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -173,9 +173,6 @@ protected function setUp() {
    
   -    // Install any additional modules.
   -    $this->installModulesFromClassProperty($container);
   -
   

   This is certainly the right step. You have to ship with everything in the dump, if you want to test the update both.

2. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -257,7 +255,9 @@ protected function rebuildAll() {
   +    // Load the container without refreshing cache.
   +    $this->container = \Drupal::getContainer();
   

   Do you mind explaining why without refreshing ca che is the right thing to do?

1. +++ b/core/lib/Drupal/Core/Extension/Extension.php
   @@ -188,7 +189,8 @@ public function serialize() {
   +    // Get the app root from the container.
   +    $this->root = \Drupal::root();
   

   Should we cast it to a string here and in __construct ?

2. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -183,6 +180,7 @@ protected function setUp() {
        /** @var \Drupal\user\UserInterface $account */
        $account = User::load(1);
        $account->setPassword($this->rootUser->pass_raw);
   

   The only way is to execute raw DB queries, right?

It's difficult to read about serialization, but there's always looking at C source in PHP: https://github.com/php/php-src/blob/master/ext/standard/var_unserializer...

Is there an extra byte that is serialized in the route object?

Or is the error in the serialized route with the colon? s:37:"#^/update\.php(?:/(?P<op>[^/]++))?$#s

Fixed a couple of test failures ...

Let's see what happens if we use a custom url generator on opt of my earlier patch.

Tagging beta target per #2341575-45: [meta] Provide a beta to beta/rc upgrade path.

I personally would be fine starting with a later database dump. While we started requiring update hooks in beta-12, we did not broadly announce this to site builders in order to see how beta-13 would go. beta-13 resulted in problems, which means that the earliest beta-to-beta upgrade path that core would support would be beta-14 to beta-15, so starting the dump from beta-14 seems fine.

For me, what's most critical about this issue is if it's surfacing bugs with running update.php at all (due to UpdatePathTestBase doing things that are not done (and cannot be done) when actually running update.php). So, I tried installing beta-12 (from the web UI), then added an article node, then updated to HEAD, and tried running update.php. I'm getting failures like in this screenshot (no CSS to hide the "Skip to main content" link, JS errors preventing the batch from starting). #28 says it was manually tested, but it doesn't get me past these problems. Not sure how related that is to this issue, but it is an indication that update.php (unlike UpdatePathTestBase) isn't clearing caches, so is serving some stale assets and maybe has other problems related to stale caches.

1. +++ b/core/lib/Drupal/Core/Extension/Extension.php
   @@ -166,8 +166,9 @@ public function __call($method, array $args) {
   +    // moved..
   

   Extra period.

2. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
   @@ -57,6 +60,26 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
   +    $routes->add('<front>', new Route('/', ['_title' => 'Home'], ['_access' => 'TRUE']));
   

   I think this is not giving a 200 response for UpdatePathWithBrokenRoutingTest and UpdatePathWithBrokenRoutingFilledTest.

3. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -173,9 +173,6 @@ protected function setUp() {
   -    $this->installModulesFromClassProperty($container);
   

   UpdatePathTestBaseTest, UpdatePathTestBaseFilledTest, BlockContextMappingUpdateTest, BlockContextMappingUpdateFilledTest, and UpdateScriptTest need to call this function.

   I think in UpdatePathTestBaseTest::setUp() right after parent::setUp()

   
   $this->installModulesFromClassProperty($this->container);
   

To be clear, the issues aren't on /update.php

• UpdatePathWithBrokenRoutingFilledTest
• UpdatePathWithBrokenRoutingTest

These two obviously break, because we are going to /, ... we know that this is broken

For me, what's most critical about this issue is if it's surfacing bugs with running update.php at all (due to UpdatePathTestBase doing things that are not done (and cannot be done) when actually running update.php). So, I tried installing beta-12 (from the web UI), then added an article node, then updated to HEAD, and tried running update.php. I'm getting failures like in this screenshot (no CSS to hide the "Skip to main content" link, JS errors preventing the batch from starting). #28 says it was manually tested, but it doesn't get me past these problems. Not sure how related that is to this issue, but it is an indication that update.php (unlike UpdatePathTestBase) isn't clearing caches, so is serving some stale assets and maybe has other problems related to stale caches.

Can you check whether you use aggregated files there? #2538274: DbUpdateController has broken mainteneance mode logic, and update.php doesn't run due to aggregated JS assets had the same kind of problems ...

UpdatePathTestBaseTest, UpdatePathTestBaseFilledTest, BlockContextMappingUpdateTest, BlockContextMappingUpdateFilledTest, and UpdateScriptTest need to call this function.

Well, then all of those need a new partial dump which have those modules installed ...
With those, we know the reason for all those 7 failing tests afaik ...

I'll generate the partials for the modules noted in #41.

This gets UpdatePathTestBaseTest passing. This is based off of #34

(Partial table generated courtesy of #2544972: Add options to limit tables, exclude data, or only export data to DbDumpCommand :)

Oh, I just realized that UpdateScriptTest uses a different set of modules. I'll add those partials too.

er, nevermind UpdateScriptTest is using WebTestBase and loads no db dump scripts.

I'll have a look at the other failures.

Some work.

Let's try again with --binary

I'll see if I can fix more failing tests.

The block context update tests were failing because the language context was not available. This gets enabled during install by the default config in the language module. Even though not strictly necessary for these tests, I also installed the default config from the block_test module.

Since the language dump is binary, this is the change I added there to install the configuration.

// Install configs.
$config_directory = new DirectoryIterator(__DIR__ . '/../../../../language/config/install');
foreach ($config_directory as $file_info) {
  if ($file_info->getExtension() == 'yml') {
    $config = Yaml::parse(file_get_contents($file_info->getRealPath()));
    $connection->insert('config')
      ->fields(['name', 'data', 'collection'])
      ->values([
        'name' => $file_info->getBasename('.yml'),
        'data' => serialize($config),
        'collection' => '',
      ])
      ->execute();
  }
}

Actually, this approach of directly reading in the config probably isn't the best. If those change in the future, things could break, so we probably need these to just be the raw serialized arrays of the current yml files.

Will exclude the route generation changes into its own issue now.
Just wanted to post that this fixes UpdateScriptTest

#2559637: Don't rely on a working router on /update.php for URL generation Is the dedicated issue for that.

Quick reroll

As we no longer access / on update.php + we have a dedicated route provider for /update.php results in no read access to {router} OR {menu_tree}
so I kinda think that #57 at least for now, is not needed. Feel free to disagree :)

Posting the binary patch now.

The other failures ...

Mh --binary

Meh.

1. +++ b/core/lib/Drupal/Core/Extension/Extension.php
   @@ -188,7 +189,8 @@ public function serialize() {
       */
      public function unserialize($data) {
        $data = unserialize($data);
   -    $this->root = $data['root'];
   +    // Get the app root from the container.
   +    $this->root = DRUPAL_ROOT;
        $this->type = $data['type'];
        $this->pathname = $data['pathname'];
        $this->filename = $data['filename'];
   

   I don't really understand why this isn't hardcoded in the first place?

2. +++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
   @@ -57,6 +60,27 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
   +
   +    $routes = new RouteCollection();
   +
   +    $routes->add('<front>', new Route('/', ['_title' => 'Home'], ['_access' => 'TRUE']));
   +    $routes->add('<none>', new Route('', [], ['_access' => 'TRUE'], ['_no_path' => TRUE]));
   +    $routes->add('<current>', new Route('<current>'));
   +    $routes->add('system.site_maintenance_mode', new Route('/admin/config/development/maintenance', ['_title' => 'Maintenance mode'], ['_permission' => 'administer site configuration']));
   +    $routes->add('system.db_update', new Route('/update.php/{op}', ['op' => 'info'], ['_access' => 'TRUE']));
   +
   +    $mock_route_provider = new MockRouteProvider($routes);
   +    $container->set('@router.route_provider', $mock_route_provider);
   

   What if we keep the default route provider and fall back to that if it is not a pre-defined route? Then we know we can get through the initial steps but if an update for some reason wants to load a custom route (e.g., one common thing is to display a message with a link to some settings page or so. Just like we're doing with the maintenance mode.

3. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -274,7 +272,9 @@ protected function rebuildAll() {
   +    // Initialize the container. parent::rebuildall() is not safe to call here.
   

   nitpick: parent::rebuildAll().

Wanted to add test runs for postgresql and sqlite, but apparently DrupalCI doesn't like your patch?

Entering setup_patch().
12:01:45 Patch failed
12:01:45 The patch attempt returned an error.
12:01:45 patching file core/lib/Drupal/Core/Extension/Extension.php
12:01:45 patching file core/lib/Drupal/Core/Update/UpdateKernel.php
12:01:45 patching file core/modules/block/src/Tests/Update/BlockContextMappingUpdateTest.php
12:01:45 patching file core/modules/system/src/Controller/DbUpdateController.php
12:01:45 patching file core/modules/system/src/Tests/Update/UpdatePathTestBase.php
12:01:45 patching file core/modules/system/src/Tests/Update/UpdatePathTestBaseFilledTest.php
12:01:45 patching file core/modules/system/src/Tests/Update/UpdatePathTestBaseTest.php
12:01:45 patching file core/modules/system/system.module
12:01:45 Hunk #1 succeeded at 241 (offset -8 lines).
12:01:45 patching file core/modules/system/tests/fixtures/update/drupal-8.block-test-enabled.php
12:01:45 File core/modules/system/tests/fixtures/update/drupal-8.language-enabled.php: git binary diffs are not supported.
12:01:45 patching file core/modules/system/tests/fixtures/update/drupal-8.update-test-schema-enabled.php

WTF?

Let's postpone on #2559637: Don't rely on a working router on /update.php for URL generation first

So the problem is that DrupalCI uses patch to apply patches, which does not support --binary. Which we have to fix.

The problem is that file isn't actually binary but git treats it as one because of the long lines in there.

Trying to force it to text in the meantime, so we can get the tests to run.

Did this with the following change to .gitattributes, why it doesn't do that with the existing line for *.php I have no idea:

diff --git a/.gitattributes b/.gitattributes
index b377ffb..be9e3f0 100644
--- a/.gitattributes
+++ b/.gitattributes
@@ -40,6 +40,7 @@
 *.txt     text eol=lf whitespace=blank-at-eol,-blank-at-eof,-space-before-tab,tab-in-indent,tabwidth=2
 *.xml     text eol=lf whitespace=blank-at-eol,-blank-at-eof,-space-before-tab,tab-in-indent,tabwidth=2
 *.yml     text eol=lf whitespace=blank-at-eol,-blank-at-eof,-space-before-tab,tab-in-indent,tabwidth=2
+*.php set diff

Will postpone again after posting.

Will postpone again after posting.

Alright

Isntall/Jthorson fixed drupalci - try again. It now uses git apply -v -p1

Let us know if it needs to do more.

Thank you @Mixologic

Let's reroll as this patch is no longer postponed.

With correct patch now. Same interdiff as #78.

Let me try as well :)
There was once this issue where 3 core developers tried to figure out how to create a patch. It was a hard and long journey.

Haha, good try, we need the binary flag + the thing in #74 I think (not sure whether DrupalCI was fixed yet)

Nice! As said on IRC I would have expected $this->container = $this->kernel->getContainer() but if this works fine as well, let's go with it

1. +++ b/core/lib/Drupal/Core/Extension/Extension.php
   @@ -166,8 +166,9 @@ class Extension implements \Serializable {
   +    // moved..
   

   superfluous full stop

2. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -179,16 +179,15 @@ abstract class UpdatePathTestBase extends WebTestBase {
   +    // Set the container. parent::rebuildAll() would normally do this, but is
   +    // not safe to do here because the database has not been updated yet.
   

   but /this/ is not safe to do here

Just fixed those points.

@catch
Yeah posted some quick comment on the main pgsql issue

Committed ec41d08 and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/Update/UpdateKernel.php b/core/lib/Drupal/Core/Update/UpdateKernel.php
index de7da1b..6c5de73 100755
--- a/core/lib/Drupal/Core/Update/UpdateKernel.php
+++ b/core/lib/Drupal/Core/Update/UpdateKernel.php
@@ -10,13 +10,10 @@
 use Drupal\Core\DrupalKernel;
 use Drupal\Core\Session\AnonymousUserSession;
 use Drupal\Core\Site\Settings;
-use Drupal\system\Tests\Routing\MockRouteProvider;
 use Symfony\Cmf\Component\Routing\RouteObjectInterface;
 use Symfony\Component\HttpFoundation\ParameterBag;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
-use Symfony\Component\Routing\Route;
-use Symfony\Component\Routing\RouteCollection;
 
 /**
  * Defines a kernel which is used primarily to run the update of Drupal.
diff --git a/core/modules/system/src/Controller/DbUpdateController.php b/core/modules/system/src/Controller/DbUpdateController.php
index e66a1a5..fa8104d 100644
--- a/core/modules/system/src/Controller/DbUpdateController.php
+++ b/core/modules/system/src/Controller/DbUpdateController.php
@@ -486,7 +486,7 @@ protected function results(Request $request) {
   public function requirements($severity, array $requirements, Request $request) {
     $options = $severity == REQUIREMENT_WARNING ? array('continue' => 1) : array();
     // @todo Revisit once https://www.drupal.org/node/2548095 is in. Something
-    // like Url::fromRoute('system.db_update')->setOptions ... should then be
+    // like Url::fromRoute('system.db_update')->setOptions() should then be
     // possible.
     $try_again_url = Url::fromUri($request->getUriForPath(''))->setOptions(['query' => $options])->toString(TRUE)->getGeneratedUrl();
 
diff --git a/core/modules/system/tests/fixtures/update/drupal-8.block-test-enabled.php b/core/modules/system/tests/fixtures/update/drupal-8.block-test-enabled.php
index 990278d..f3edf28 100644
--- a/core/modules/system/tests/fixtures/update/drupal-8.block-test-enabled.php
+++ b/core/modules/system/tests/fixtures/update/drupal-8.block-test-enabled.php
@@ -2,7 +2,7 @@
 
 /**
  * @file
- * Partial database to mimic the installation of the update_test_schema module.
+ * Partial database to mimic the installation of the block_test module.
  */
 
 use Drupal\Core\Database\Database;

Minor fixes done on commit.