Problem/Motivation

#3173180: Add UI for 'loading' html attribute to images discovered an issue with the 9.3.0 database dumps.

#3145563: Route serialization incompatibilities between PHP 7.4 and 7.3 (9.x only) means that a database dumped from PHP >= 7.4 can't be loaded into a site running PHP 7.3, and the dumps were created on PHP 8, this broke tests on PHP 7.3 when we actually used them.

Steps to reproduce

Switch any 9.4.x test to use the 9.3.0 database dumps.

Proposed resolution

Recreate the dumps using PHP 7.3. Only do this for 9.4.x, since we don't want PHP 7.3 to be a development dependency for the entirety of Drupal 10.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#14 3275093-test-only.patch711 bytescatch

Issue fork drupal-3275093

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: Unassigned » larowlan

larowlan opened merge request !2115

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Active » Needs review

Steps taken for each of 'filled' and 'bare'

  • from php8 - ran php ./app/core/scripts/db-tools.php import core/modules/system/tests/fixtures/update/drupal-9.3.0.{type}.standard.php.gz
  • them from php7.3 - ran php ./app/core/scripts/db-tools.php dump-database-d8-mysql |gzip > core/modules/system/tests/fixtures/update/drupal-9.3.0.{type}.standard.php.gz
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +Bug Smash Initiative
bbrala’s picture

bbrala
Location Netherlands
commented

Hmm, was hoping to review this, but not sure how :stuck_out_tongue: Comparing 9.4.x fixtures to the fixture in your MR results in only a small comment change. Which kinda confuses me, how can this be fixed by no real changes but generating a new gz file. Is the GZ format changed? That would be REALLY weird?

vagrant@machine:/mnt/c/projecten/core-fixtures $ diff 9.4.x/drupal-9.3.0.bare.standard.php/drupal-9.3.0.bare.standard.php 3275093/drupal-9.3.0.bare.standard.php/drupal-9.3.0.bare.standard.php
7c7
<  * This file was generated by the Drupal 9.3.0 db-tools.php script.
---
>  * This file was generated by the Drupal 9.4.0-dev db-tools.php script

If that is the only diff in the fixture files, i really dont understand how that would fix the problem.

catch’s picture

catch
he/him
Primary language English
commented

There could potentially be a missing router rebuild step in #6, the problem we're hitting is that the router is serialized differently in PHP 7.3 vs. PHP >=7.4. So we need a PHP 7.3 version of the router table to then export.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

In that case wouldn't it fail on PHP 7.3?

catch’s picture

catch
he/him
Primary language English
commented

We're not running any tests against those fixtures in 9.x yet - they're used in 10.0.x for all the upgrade path tests, but those tests don't run on PHP 7.3, and all our 9.x upgrade path test coverage uses the 8.8 dumps (which would have been dumped on PHP 7.3 probably).

Reverting https://git.drupalcode.org/project/drupal/-/merge_requests/1287/diffs?co... is a good test-only patch.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Hmm nevermind, I thought head was failing here

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: larowlan » Unassigned

I'll redo this tomorrow unless someone else beats me to it

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new 3275093-test-only.patch711 bytes

Here's a test-only patch (the revert of the above hotfix). I think we could even leave this in the final version of the commit. Not sure we need a dedicated 'test the update test database dumps test'.

This should fail on PHP 7.3, but pass on PHP 7.4 and 8.0 per the original issue that broke HEAD.

Then assuming we've diagnosed the issue right, it should pass on PHP 7.3 with the new dumps, as long as the router table has routes serialized via PHP 7.3.

catch’s picture

catch
he/him
Primary language English
commented

btw if anyone recreates the dumps, also need to account for #3261629: Database dumps are no longer driver-agnostic which was not fixed in 9.3.0 (so need to dump from a later-than-9.3.0 version of the script).

Status: Needs review » Needs work

The last submitted patch, 14: 3275093-test-only.patch, failed testing. View results

catch’s picture

catch
he/him
Primary language English
commented

PHP 7.3 test run failed as expected, the 8.1 fail is an unrelated random.

1) Drupal\Tests\image\Functional\ImageLazyLoadUpdateTest::testUpdate
Erroneous data format for unserializing 'Symfony\Component\Routing\Route'
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Steps taken this time for each of 'filled' and 'bare'

  • from php8 - ran php ./app/core/scripts/db-tools.php import core/modules/system/tests/fixtures/update/drupal-9.3.0.{type}.standard.php.gz
  • Run drush php and then \Drupal::service('router.builder')->rebuild();
  • them from php7.3 - ran php ./app/core/scripts/db-tools.php dump-database-d8-mysql |gzip > core/modules/system/tests/fixtures/update/drupal-9.3.0.{type}.standard.php.gz
  • Extract the version of the file from 9.4.x HEAD git show 9.4.x:core/modules/system/tests/fixtures/update/drupal-9.3.0.{type}.standard.php.gz|gunzip > core/modules/system/tests/fixtures/update/drupal-9.3.0.{type}.standard.php.orig
  • Extract the new version of the original file cat core/modules/system/tests/fixtures/update/drupal-9.3.0.{type}.standard.php.gz|gunzip > core/modules/system/tests/fixtures/update/drupal-9.3.0.{type}.standard.php.new
  • Compare the .new file with the .orig - as expected there are changes to the serialized routes
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Needs review
dww’s picture

dww
we/he/they
commented

I did the following:

  1. git checkout 3275093-drupal-9.3.0-dumps
  2. cp drupal-9.3.0.bare.standard.php.gz fixed-drupal-9.3.0.bare.standard.php.gz
  3. cp drupal-9.3.0.filled.standard.php.gz fixed-drupal-9.3.0.filled.standard.php.gz
  4. git checkout 9.4.x
  5. gunzip drupal-9.3.0.bare.standard.php.gz drupal-9.3.0.filled.standard.php.gz fixed-drupal-9.3.0.bare.standard.php.gz fixed-drupal-9.3.0.filled.standard.php.gz
  6. diff -up drupal-9.3.0.bare.standard.php fixed-drupal-9.3.0.bare.standard.php

There are now a ton of changes, so that's progress relative to #8. 😉

Other than this:

- * This file was generated by the Drupal 9.3.0 db-tools.php script.
+ * This file was generated by the Drupal 9.4.0-dev db-tools.php script.

most of the changes look like this:

-  'route' => 'O:31:"Symfony\Component\Routing\Route":9:{s:4:"path";s:1:"/";s:4:"host";s:0:"";s:8:"default
s";a:0:{}s:12:"requirements";a:1:{s:7:"_access";s:4:"TRUE";}s:7:"options";a:4:{s:14:"compiler_class";s:33:
"Drupal\Core\Routing\RouteCompiler";s:8:"_no_path";b:1;s:4:"utf8";b:1;s:14:"_access_checks";a:1:{i:0;s:20:
"access_check.default";}}s:7:"schemes";a:0:{}s:7:"methods";a:2:{i:0;s:3:"GET";i:1;s:4:"POST";}s:9:"conditi
on";s:0:"";s:8:"compiled";O:33:"Drupal\Core\Routing\CompiledRoute":11:{s:4:"vars";a:0:{}s:11:"path_prefix"
;s:0:"";s:10:"path_regex";s:8:"#^/$#sDu";s:11:"path_tokens";a:1:{i:0;a:2:{i:0;s:4:"text";i:1;s:1:"/";}}s:9
:"path_vars";a:0:{}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:1:"/";s:8:"numParts";i:1;}}',
+  'route' => 'C:31:"Symfony\Component\Routing\Route":743:{a:9:{s:4:"path";s:1:"/";s:4:"host";s:0:"";s:8:"
defaults";a:0:{}s:12:"requirements";a:1:{s:7:"_access";s:4:"TRUE";}s:7:"options";a:4:{s:14:"compiler_class
";s:33:"Drupal\Core\Routing\RouteCompiler";s:8:"_no_path";b:1;s:4:"utf8";b:1;s:14:"_access_checks";a:1:{i:
0;s:20:"access_check.default";}}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":297:{a:11:{s:4:"vars";a:0:{}s:11
:"path_prefix";s:0:"";s:10:"path_regex";s:8:"#^/$#sDu";s:11:"path_tokens";a:1:{i:0;a:2:{i:0;s:4:"text";i:1
;s:1:"/";}}s:9:"path_vars";a:0:{}s:10:"host_regex";N;s:11:"host_tokens";a:0:{}s:9:"host_vars";a:0:{}s:3:"f
it";i:1;s:14:"patternOutline";s:1:"/";s:8:"numParts";i:1;}}}}',

Looks like a different format for serializing these routes, which seems legit. I dunno off the top of my head exactly what changed in PHP between 7.3 and PHP 8 for this, but this seems plausible.

The same is true of the diff for drupal-9.3.0.filled.standard.php (except it's 5382 lines of diff, vs. 3258 for the bare fixture).

I was sad to see that the 7.3 test run against the MR failed, but those were all layout_builder random fails. So I just requeued. Of crucial interest, core/modules/image/tests/src/Functional/ImageLazyLoadUpdateTest.php did not fail. Assuming the re-test comes back green, I think this is RTBC. Not sure how else to verify / review / test this.

Thanks!
-Derek

bbrala’s picture

bbrala
Location Netherlands
commented

From what I understood this is pretty much what to expect. But testing it manually is a bit complicated. We can probably trust the tests in this way.

dww’s picture

dww
we/he/they
commented

Just added myself an email notification when the test run completes. If it's green, I'll mark this RTBC. 🤞

Crediting both of us for reviews. 😉

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community

All green. RTBC! 🎉

bbrala’s picture

bbrala
Location Netherlands
commented

;) woop woop

  • catch committed 3aa89b9 on 9.4.x 
    Issue #3275093 by larowlan, catch, bbrala, dww, heddn: Drupal 9.3.0...

catch credited heddn.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Crediting @heddn who between us diagnosed the issue in the original update that uncovered it. The test-only hunk I uploaded was just the original part of the original issue that got reverted, not my patch so I'm OK to commit this one.

Committed 3aa89b9 and pushed to 9.4.x. Thanks!

When I originally opened this issue I thought we should make the same change to the 10.0.x dumps too, but we absolutely should not introduce a PHP 7.3 development dependency for Drupal 10, nor should we make generating these database dumps any more complicated than it already is. With a bit of luck, this will be the last time we change the 9.3 database dumps in Drupal 9.4.

Status: Fixed » Closed (fixed)

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