Fix the filled update tests, they are broken

I'll dig into this.

This still uses the gzipped version, with the same fixes from Gabor. The unzipped version is 5M. I could go either way on leaving them zipped or not.

The failures look like legitimate schema mismatches. The first exception is in regards to adding an index that already exists (perhaps automated entity schema update related).

The 2nd exception is something I have no idea about (but did previously see it in an H2H issue where it wasn't resolved).

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: failed to instantiate user-supplied statement class: CREATE TABLE {cache_config} ( `cid` VARCHAR(255)

The fatals on the last test run were because there are many more classes extending UpdatePathTestBase.

I've dug into the index exception that is thrown in UpdatePathTestBaseFilledTest and it is because the taxonomy_term_field_data table has a stored schema that doesn't actually match what is in the database. Specifically, an index on the content_translation_uid field. I haven't been able to track down the issue that added this index yet.

This should fix the fatals.

That last patch missed some key logic where filled tests were only setting the base dump file, so the parent class needs to be called too.

Still missing a few.

This should allow the test run to complete without fatal errors.

I opened #2555665: When index is added for content_translation_uid, the corresponding stored schema definition is not updated for the taxonomy index issue in #6.

In local testing, the patch in #2555665: When index is added for content_translation_uid, the corresponding stored schema definition is not updated resolves nearly all of the exceptions in #12. The exception mentioned in #4 is still baffling, and I don't know why it doesn't happen on *all* of the tests using the filled db, only on some.

Yep.

I'm going to try to diagnose the pdo exception.

The exception is only thrown from UpdatePathTestBase::testUpdateHookN() (when using the filled database). The actual exception is thrown from DatabaseBackend::ensureBinExists() when Schema::tableExists() is called.

Attached is a backtrace of the exception (it appears to be fast chained backend related, and I haven't tested w/o APC enabled).

Also here is a reroll of #16 since #2555259: Add config schema for image_convert image effect and #2555275: Fix missing schema for URI widget were fixed. It contains the patch from #2555665: When index is added for content_translation_uid, the corresponding stored schema definition is not updated still.

Here's a txt version of the backtrace since the HTML one is a bit hard to read.

Have to give up for tonight.

Here's what I found so far:

* The problem is that we execute queries on a destroyed DB connection.
* destroyed means that destroy() was called and the statement class was reset
* Somehow, that old connection is still injected *somewhere* and the second test method then tries to use the same database connection
* You can confirm this by commenting out the setAttribute() in Connection and/or a debug(spl_object_hash($this->connection)); in DatabaseBackend::createTable(). The hash is the same for both runs.
* Commenting out that line makes the test path but obviously that is not the solution.

I haven't been able to figure out why that connection is still used and what service/object is carried over. Spent a lot of time debugging setUP()/building containers and we seem to initialize with a clean empty container.

Edit: The primary difference seems to be language.module/language outbound processing. As you can see in the backtrac,e it's the language url negotation that is loading config which then results in that exception.

It'd be good to split out only the syntax error fix for the dump into a new issue so we can commit that asap - would make it easier to review patches here.

I added #2558063: Fix syntax error in filled database dump and will post the updated db there shortly.

This approach disables language negotiation during the runUpdates() method. If we go this way, we should probably add a follow-up to pursue the issue detailed in #30.

Note, the patch above still contains the fix from #2555665: When index is added for content_translation_uid, the corresponding stored schema definition is not updated, but I'm not sure what this is since the other issues mentioned in #16 have been committed.

+++ b/core/config/schema/core.entity.schema.yml
@@ -171,6 +171,17 @@ field.widget.settings.uri:
+field.widget.settings.uri:
+  type: mapping
+  label: 'URI field'
+  mapping:
+    size:
+      type: integer
+      label: 'Size of URI field'
+    placeholder:
+      type: label
+      label: 'Placeholder'
+

The problem is here:

  function __construct($test_id = NULL) {
    parent::__construct($test_id);
    $this->zlibInstalled = function_exists('gzopen');

    // Set the update url.
    $this->updateUrl = Url::fromRoute('system.db_update'); // This leaks the old url generator into all tests.
  }

    // Set the update url.
    $this->updateUrl = Url::fromRoute('system.db_update');

Resolution is to move that into the setUp() routine. Then it should work fine.

I'll try to fix the remaining 2 fails. Thanks Fabianx for tracking that down!

The fail in SqlContentEntityStorageSchemaIndexFilledTest is due to a subtle change to entity schemas that was introduced in #2498919: Node::isPublished() and Node::getOwnerId() are expensive. I already added an update hook for the change to head2head in #2550489: Upgrade path for 2498919, so I guess the easy fix there is to pull that into core?

Still looking into the fail in UpdatePathWithBrokenRoutingFilledTest, which seems to be different.

This contains the fix found by Fabianx, and I added an assertion to runUpdates to make sure there are no failed updates.

@jhedstrom isn't there already a fix for #2498919: Node::isPublished() and Node::getOwnerId() are expensive in #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state?

@catch oh, you're right! That's great.

The remaining fail in UpdatePathWithBrokenRoutingFilledTest is due to an assumption that the front page is accessible before running updates. From UpdatePathWithBrokenRoutingTest::testWithBrokenRouting():

    // Make sure we can get to the front page.
    $this->drupalGet('<front>');
    $this->assertResponse(200);

the filled DB fails with this exception on the front page (before updates are run)

InvalidArgumentException: You must provide the context IDs in the @{service_id}:{unqualified_context_id} format. in Drupal\Core\Plugin\Context\LazyContextRepository->getRuntimeContexts() (line 76 of core/lib/Drupal/Core/Plugin/Context/LazyContextRepository.php).

I think we can just ditch that line - the 500 assertion and the one after the updates have run are the important ones.

Given #40 I think we should not postpone #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state on this issue - will post to same effect there.

This removes the test of the homepage before running updates.

Postponed on #2555665: When index is added for content_translation_uid, the corresponding stored schema definition is not updated.

#2555275: Fix missing schema for URI widget already resolved the URI schema. #2555665: When index is added for content_translation_uid, the corresponding stored schema definition is not updated is about to resolve the existing index problem. Rolling those two out of the patch.

@stefan.r: crossposted the same thing, sorry :D

Just putting back to CNR so the tests can run.

Yes, but it'd be nice to see that that's the only fail, and that it's the same after #2542748: Automatic entity updates can fail when there is existing content, leaving the site's schema in an unpredictable state.

#57: Does that mean we should simply reupload #43?

@Wim Leers: no, the fix included in #43 for #2555665: When index is added for content_translation_uid, the corresponding stored schema definition is not updated is debated in that issue (its not the right fix, it only fixes for the extent of our tests), so trying to get that shoehorned in here would not work overall. :/ Since we have that issue for that part of the problem with attention from the right folks, it looks sane to try and resolve it there (even though there is no tests there yet, but the tests here apparently do not cover the whole spectrum of the situation either, hence the debate).

Discussed with @webchick, @catch, @alexpott, and @effulgentsia. This issue is critical because we cannot have confidence in our upgrade path for a release candidate if we are not testing it properly.

Also re-postponing on #2555665: When index is added for content_translation_uid, the corresponding stored schema definition is not updated.

#2555665: When index is added for content_translation_uid, the corresponding stored schema definition is not updated landed, so unblocked now.

Reuploading #49 so testbots have a chance to retest it in the new setup :)

ALL of the fails seem to be missing config schema for some new D6 migrations. Haha. Do we want to track that as yet another critical for issue management? :)

It is 90 instances of “migrate.migration.*:load failed with: missing schema" repeated all over and over. Discussing with @mikeryan.

Gabor alerted me to this, opened up an issue to fix the migration tests: #2559355: Migration tests reference defunct load plugins.

So migrate removed the load key from migrations since the dump was made. Migrate needs an update function for removing the load key from migrations in config.

Issue for the migration update path at #2559381: Migrate needs core update path.

Having trouble with this locally, want to see how testbot handles the migrate update with this patch...

This may be a silly question, but why are we testing Migrate at all in the dump file, given core does not support it yet?

This contains the updated fix from #2559381: Migrate needs core update path. Uploading here to make sure it resolves the issue.

This may be a silly question, but why are we testing Migrate at all in the dump file, given core does not support it yet?

My understanding is that the filled tests represent something that could possibly be done (enable *every* module), so since core ships with migrate, it can be enabled, and might need update before it is officially supported.

Ok, that's probably fine for now as long as it's not holding anything up here for too long, but over in #2313651: [policy, no patch] Clarify policy on the inclusion of Migrate in Core for 8.0.0 we are discussing marking Migrate as "Experimental" and if that happens I think we maybe change the rule "Turn on all modules except the experimental ones."

I think it would be fine to not turn migrate on in the filled database dump too. Although we might still want tests and test coverage for migrate upgrade paths in core so it's a bit of a trade-off - that could be a major to add them back though at the moment.

Re-uploading the patch from #62.

For a short moment I thought about something like $this->baseDatabaseDump or so, which would make it a bit easier to override,
but meh, this would be out of scope here, I think.

I braved a peek at this patch and surprisingly everything here looks straight-forward!

The one thing that stood out was this:

-    // Make sure we can get to the front page.
-    $this->drupalGet('<front>');
-    $this->assertResponse(200);
-

...but that's covered in #41/#42 above (the assertion is out of date since changing the way update.php runs in another patch). Therefore, let's get 'er done! :D

Committed and pushed to 8.0.x. Thanks!

(If I over-stepped my bounds here, feel free to revert, but this seemed like a pretty easy/important one.)