Problem/Motivation
Upgrading drupal/core-composer-scaffold from any 8.8.x version to any 8.9.x version causes a PHP fatal error.
Example:
$ composer create-project drupal/recommended-project:8.8.4
$ cd recommended-project
$ composer require drupal/core-composer-scaffold:8.9.x-dev
./composer.json has been updated
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 0 installs, 1 update, 0 removals
- Removing drupal/core-composer-scaffold (8.8.4)
- Installing drupal/core-composer-scaffold (8.9.x-dev 16a4227): Cloning 16a4227460 from cache
Package zendframework/zend-diactoros is abandoned, you should avoid using it. Use laminas/laminas-diactoros instead.
Package zendframework/zend-escaper is abandoned, you should avoid using it. Use laminas/laminas-escaper instead.
Package zendframework/zend-feed is abandoned, you should avoid using it. Use laminas/laminas-feed instead.
Package zendframework/zend-stdlib is abandoned, you should avoid using it. Use laminas/laminas-stdlib instead.
Writing lock file
Generating autoload files
Deprecation Notice: Non-static method Drupal\Composer\Plugin\Scaffold\Operations\ScaffoldFileCollection::process() should not be called statically in /Users/ganderson/tmp/drupal-upgrade-test/recommended-project/vendor/drupal/core-composer-scaffold/Handler.php:167
PHP Fatal error: Uncaught TypeError: Argument 1 passed to Drupal\Composer\Plugin\Scaffold\Operations\ScaffoldFileCollection::process() must implement interface Composer\IO\IOInterface, instance of Drupal\Composer\Plugin\Scaffold\Operations\ScaffoldFileCollection given, called in /Users/ganderson/tmp/drupal-upgrade-test/recommended-project/vendor/drupal/core-composer-scaffold/Handler.php on line 167 and defined in /Users/ganderson/tmp/drupal-upgrade-test/recommended-project/vendor/drupal/core-composer-scaffold/Operations/ScaffoldFileCollection.php:133
Stack trace:
#0 /Users/ganderson/tmp/drupal-upgrade-test/recommended-project/vendor/drupal/core-composer-scaffold/Handler.php(167): Drupal\Composer\Plugin\Scaffold\Operations\ScaffoldFileCollection::process(Object(Drupal\Composer\Plugin\Scaffold\Operations\ScaffoldFileCollection), Object(Composer\IO\ConsoleIO), Object(Drupal\Composer\Plugin\Scaffold\ScaffoldOptions))
#1 phar:///Users/ganderson/bin/composer/src/Composer/Plugin/PluginManager.php(196) : eval()'d code(70): Drupal\Com in /Users/ganderson/tmp/drupal-upgrade-test/recommended-project/vendor/drupal/core-composer-scaffold/Operations/ScaffoldFileCollection.php on line 133
Once there is a stable release of Drupal 8.9.0 / 9.0.0, the same error will happen when using composer update
to upgrade Drupal.
The Drupal 9 upgrade path is supposed to be very easy; encountering a PHP fatal error during the process would cause a lot of confusion and dissatisfaction with the upgrade process.
Proposed resolution
This upgrade bug was fixed by #3104922: Guard against changes to Scaffold Plugin that might cause upgrade problems ; however, that patch was not back-ported to the 8.8.x branch. That omission should not have been cause for any trouble (c.f. #35), EXCEPT for the fact that the test that should have guarded against changes that caused problems did not work correctly (failures passed instead of being flagged).
The fix, therefore, is twofold:
- Fix the test so that it correctly fails when there is a problem in the upgrade path.
- Fix the problem in the upgrade path that was committed to 8.9.x / 9.0.x.
Remaining tasks
- Write a failing test
- Fix the failing test
User interface changes
None
API changes
None
Data model changes
None
Release notes snippet
A PHP fatal error that was triggered when upgrading the Core Composer Scaffold plugin has been fixed.
Comment | File | Size | Author |
---|---|---|---|
#21 | 3134606-8.9.x-21.patch | 6.93 KB | greg.1.anderson |
#19 | 3134606-9.0.x-19.patch | 6.69 KB | greg.1.anderson |
#19 | 14-to-19-interdiff.txt | 1.83 KB | greg.1.anderson |
#14 | 3134606-8.9.x-14.patch | 6.76 KB | greg.1.anderson |
#14 | 3134606-9.0.x-14.patch | 6.52 KB | greg.1.anderson |
Comments
Comment #2
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere is a failing test to show the problem.
Comment #3
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedSetting to "needs review" to run the failing test.
Comment #4
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere is a patch that fixes the test from #2.
Comment #5
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedMade my patches against 9.0.x instead of 8.9.x. Re-queued on the correct branch.
Comment #6
xjmOy.
Comment #7
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's a failing test that should fail the correct way.
Comment #8
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's the fix. Interdiff is the same as #7.
Comment #9
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThe failing test in #7 now matches the symptoms reported in the IS, and the patch in #8 is green, so this is ready for review now.
Comment #10
junglecomposer update
does not work as expected on my local as well.Excpected version of
drupal/core
after running the commands above, is 8.8.5, but the actual version is8.9.x-dev
assertStringContains()
working, the traitPhpunitCompatibilityTrait
should be used inScaffoldUpgradeTest
.Comment #11
catchWe should mark this @internal, we can't really deprecate it because it still has to be called and there's no way remove those calls until the new release. It will also need to be retained until Drupal 10 (i.e. remove in Drupal 10.0.0) - since we theoretically allow updates from Drupal 8.8 to Drupal 9.LAST.
The 8.9-dev issue that @jungle saw has its own issue at #3134648: [backport, needs scheduling] Don't pin the composer/installers version in drupal/core-recommended.
Comment #12
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI think that it is more correct to call this method
@deprecated
rather than@internal
. The static process method should never be called in any 8.9.x or 9.x+ version of Drupal. The@deprecated
tag lets you know that it should not be used. This method is not deprecated in Drupal 8.8.x; due to a bug, when the 8.8.x code calls the static process method, it might first load the 8.9.x or 9.0.x version of the class. This is the only time that the static function might be called.If we tagged this method
@internal
, that would imply that it is okay to call the method from within other parts of the Core Composer Scaffold plugin, which is not correct.I updated the comment to be more clear about the situation, and to update the advise to remove in Drupal 10 rather than in Drupal 9.1. I also made an 8.9.x version of the patch following the advice in #10.
Comment #13
Kristen PolThanks for the update. Only looked at the interdiff and noticed a tiny nitpick:
Nitpick in interdiff:
Change "8.8.x." to "8.8.x" (remove period at end).
Comment #14
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedComment #15
Kristen PolConfirmed that #14 fixes the nitpick in #13 but leaving as "Needs review" as I don't understand the patch well enough to review better. :)
Comment #16
longwaveDoes the
--no-ansi
switch tocomposer require
help? This should remove the colour encoding which might be the source of the binary string.Comment #17
alexpottThe binary string thing happens because PHPUnit does
in \SebastianBergmann\Exporter\Exporter::recursiveExport(). See https://github.com/sebastianbergmann/phpunit/issues/736 for more.
Comment #18
longwaveYeah, and I think --no-ansi should remove those non printable characters from the output - we use it in many other composer command lines in build tests.
Comment #19
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThanks, @longwave; that is, in fact, cleaner.
Comment #20
longwaveDiscussed with @greg.1.anderson on Slack after trying to manually reproduce; the only way to do so is have a git repository containing the updated plugin, as you have to remove the old directory from vendor first before switching to a path-based repository, and then the bug isn't triggered. Then I realised that this is exactly what the test does, and that passes, so the 9.0.x patch is RTBC. We could do with an 8.9.x patch to match though, as the one in #14 needs updating to include the
--no-ansi
switch.Comment #21
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedHere's the 8.9.x version of #19. Interdiff for 14-to-21 is the same as 12-to-19.
Comment #24
catchCommitted/pushed to 9.1.x and cherry-picked to 9.0.x
The 8.9.x backport is still with DrupalCI, so waiting for green there.
Comment #26
catchCommitted 0423e3b and pushed to 8.9.x. Thanks!