Problem/Motivation

The filled and bare database dumps have some router entries with broken serialized data:

Bare:

<current>
comment.approve
entity.comment.canonical
entity.comment.delete_form
entity.comment.edit_form

Filled:

<current>
comment.approve
content_translation.translation_add_comment
content_translation.translation_delete_comment
content_translation.translation_edit_comment
entity.comment.canonical
entity.comment.content_translation_overview
entity.comment.delete_form
entity.comment.edit_form

Proposed resolution

Correct the entries.

Remaining tasks

Test/review.

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Antti J. Salminen created an issue. See original summary.

Antti J. Salminen’s picture

Status: Active » Needs review
Antti J. Salminen’s picture

Sorry for the duplicate comment, d.o or my connection was acting up...

catch’s picture

Priority: Major » Critical

This would explain some of the many red herrings we ran into when debugging #2561121: Installer fails on postgresql due to uncaught exception and other issues.

Given the hours spent debugging those issues, many of them playing whack-a-mole with that serialisation error, bumping this to critical since it makes our upgrade path tests very untrustworthy.

dawehner’s picture

I'm curious whether this is the only broken route?

dawehner’s picture

Status: Needs review » Needs work

* Imported all the routes from the script
* Used this little script:

$result = db_query('SELECT name, route from {router2}')->fetchAllKeyed();

foreach ($result as $name => $route) {
   print_r($name . "\n");
  unserialize($route);
}
entity.comment.canonical
Insufficient data for unserializing - 1385 required, 1377 present test-script.php:7                          [warning]
entity.comment.delete_form
Insufficient data for unserializing - 1365 required, 1357 present test-script.php:7                          [warning]
entity.comment.edit_form
Insufficient data for unserializing - 1356 required, 1348 present test-script.php:7                          [warning]
comment.approve
Insufficient data for unserializing - 1475 required, 1467 present test-script.php:7                          [warning]
comment.new_comments_node_links
<current>
Insufficient data for unserializing - 845 required, 845 present test-script.php:7                            [warning]
Wim Leers’s picture

Wow, amazing find!

stefan.r’s picture

Status: Needs work » Needs review
FileSize
241.58 KB
2.06 KB
14.15 KB

@dewehner so the filled dump is fine and the bare dump has just those 5 additional broken routes?

Uploading a patch that includes those 5 manually replaced in addition to the ones in #1. Alternatively we could also just do a completely new beta12 dump...

Antti J. Salminen’s picture

I did a couple of diffs between the bare dump scripts generated.

A diff of a newly made bare beta12 dump with and without the patch (attached as fresh_bare_to_patched_diff.txt) only differed for the routes.

A diff for the fixed dump versus what is currently in HEAD (bare_to_patched_diff.txt), has a lot more noise from UUIDs etc. in the diff but going thru it in ediff I didn't see any real differences besides the routes.

In the filled dump dawehner's script from #6 revealed the same 5 and these additional routes failing the unserialize:

content_translation.translation_add_comment
content_translation.translation_delete_comment
content_translation.translation_edit_comment
entity.comment.content_translation_overview

Status: Needs review » Needs work

The last submitted patch, 8: 2565259-8.patch, failed testing.

alexpott’s picture

The test fails in Drupal\config\Tests\ConfigInstallProfileUnmetDependenciesTest look like testbot issues - going to press retest.

Also I think this patch should include #2565225: Database dump script garbles the script contents by using an output formatter - not sure why we have two issues?

alexpott queued 8: 2565259-8.patch for re-testing.

alexpott’s picture

Issue tags: +Needs tests

Also since this was able to occur I guess we should add some test coverage.

The last submitted patch, 8: 2565259-8.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
1.5 KB

Wrote some basic test coverage for that, and indeed these are the routes which cause issues.

Status: Needs review » Needs work

The last submitted patch, 15: 2565259-15.patch, failed testing.

stefan.r’s picture

Should we reset the error handler after the router table check? I think we can also include #8 and #2565225: Database dump script garbles the script contents by using an output formatter in the patch after we see the failure messages are correct - not sure why in the CI results it is not listing the route names, and only one failure instead of the 6 in total we found.

Also do we need further test coverage than this, i.e. check other columns that may hold serialized data?

The last submitted patch, 15: 2565259-15.patch, failed testing.

plach’s picture

Assigned: Unassigned » plach

Working on this a bit

dgv’s picture

@catch:

Given the hours spent debugging those issues, many of them playing whack-a-mole with that serialisation erro

Exactly. The whack-a-mole endless game is the metaphor I wanted to use to reply in
https://www.drupal.org/node/2565529

Drupal needs to take a stand about whether in its code, "standard_conforming_strings" is ON or OFF with PostgreSQL, set that as a rule, and only then find and fix all the code that does not conform to that rule.

Right now what we're doing is backwards, changing the rule when code is found that does not conform to the rule, only to find that another test or code depended on the opposite interpretation.

dawehner’s picture

+++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseTest.php
@@ -38,6 +38,25 @@ public function testDatabaseLoaded() {
+      }
+      catch (\Exception $e) {
+        $this->fail(sprintf('Error "%s" while unserializing route %s', $e->getMessage(), $route_name));
+      }
+    }
+

@plach

Do you mind putting restore_error_handler() and the end here?

Mixologic’s picture

not sure why in the CI results it is not listing the route names, and only one failure instead of the 6 in total we found.

Because you discovered a new bug in drupalCI - Thank you!

https://www.drupal.org/node/2566265

plach’s picture

Ok, this is green locally. It also includes the patch from #2565225: Database dump script garbles the script contents by using an output formatter as suggested by @alexpott.

  • I installed beta12 from scratch and enabled CT
  • I dumped the related DB after applying the fix
  • I applied the differences in the route entries to the bare and filled dumps

I also manually tested the resulting dumps and they were working fine.

Aside from the PG issue we should good here, I think.

plach’s picture

Sorry, wrong patch. The previous interdiff should be ok, though.

The last submitted patch, 23: update-route_serialization_broken-2565259-23.patch, failed testing.

stefan.r’s picture

Issue tags: -Needs tests

Looks good, I guess this is enough test coverage then?

plach’s picture

Title: Fix broken <current> route serialisations in database dumps » Some route serializations in update test database dumps are broken
Issue summary: View changes

Yep, thanks

plach’s picture

The dump diffs as per @dawehner's request.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @plach!

These are indeed the routes I saw when running the tests in #15
Given that I think this should be ready to go.

The last submitted patch, 23: update-route_serialization_broken-2565259-23.patch, failed testing.

The last submitted patch, 15: 2565259-15.patch, failed testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3444108 and pushed to 8.0.x. Thanks!

  • alexpott committed 3444108 on 8.0.x
    Issue #2565259 by plach, Antti J. Salminen, stefan.r, dawehner: Some...

Status: Fixed » Closed (fixed)

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