Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
The recent security release ran into #3026386: Drush fatal error after upgrading to 8.6.6, 8.5.9, or 7.62: PHP Fatal error: Uncaught TYPO3\PharStreamWrapper\Exception
Proposed resolution
Add tests for the situation when a CLI phar is invoked from a shutdown function.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#22 | 3026414-22.patch | 25.48 KB | alexpott |
#22 | 10-22-interdiff.txt | 4.5 KB | alexpott |
#10 | 3026414-5.patch | 25.89 KB | alexpott |
#10 | 3026414-10.test-only.patch | 27.49 KB | alexpott |
#9 | 3026414-5.patch | 25.89 KB | alexpott |
Comments
Comment #2
alexpottHere's a test that runs a phar from the command line without a phar extension and also covers the shutdown function case that failed in the original security release.
Comment #3
alexpottI left these files here because I think it is helpful to have them so we can add other things we find to the phar files and make it easy to rebuild them.
Comment #4
alexpottImproved the test and the code.
Comment #5
alexpottNo longer need to use FileTestBase - whilst working on the patch I needed a real file system and VFS but that is no longer true.
Comment #7
alexpottSo #2 was submitted to the queue prior to #3026386: Drush fatal error after upgrading to 8.6.6, 8.5.9, or 7.62: PHP Fatal error: Uncaught TYPO3\PharStreamWrapper\Exception landing so that failure is expected. The others should be green.
Comment #8
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC - but could we have a tests-only patch (with removed sec patch) as well to see it failing?
Comment #9
alexpottWell fwiw here is a test-only patch with the security stream wrapper not registered but the real test only patch was done in #2 against a HEAD that did not contain the emergency fix.
Re-uploading #5 so the latest patch on the issue is the real patch to review.
Comment #10
alexpottOh here's a test patch with #3026386: Drush fatal error after upgrading to 8.6.6, 8.5.9, or 7.62: PHP Fatal error: Uncaught TYPO3\PharStreamWrapper\Exception reverted as that is helpful.
Uploading #5 again for the same reason.
Comment #13
Fabianx CreditAttribution: Fabianx as a volunteer commentedRTBC - looks great to me -- both cases pass :)
Comment #15
effulgentsia CreditAttribution: effulgentsia at Acquia commentedLooks good to me as well. Crediting @Fabianx for review.
Comment #17
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.7.x. Marking Fixed for now, but should we cherry pick to 8.6.x and backport to 7.x?
Comment #18
alexpott@effulgentsia I would as this is important test coverage and tests are not API.
Comment #19
mondrakeMmm, started getting
Fatal error: PHPUnit\Util\Fileloader::main(): Failed opening required '/home/travis/drupal8/core/../../../../../../../autoload.php' (include_path='.:/home/travis/.phpenv/versions/7.3.0/share/pear') in /home/travis/drupal8/core/tests/Drupal/KernelTests/Core/File/fixtures/cli_phar_builder/index.php on line 16
on TravisCI testing since when this got committed.
See https://travis-ci.org/mondrake/imagemagick/builds/484074305
Comment #20
alexpottoh drats... okay we need to make this work regardless of cwd. I'll revert and then make it work when you run tests from inside the ./core folder.
Comment #22
alexpottThis is caused by the fact we evaluate all files in test folders as test not just *Test.php - see #2296635: Evaluate performance impact of limiting test discovery to *Test.php filename suffix.
So we need to move the fixtures outside of a test discovery location.
To test this try adding
@group Phar
to\Drupal\KernelTests\Core\File\PharWrapperTest
and then running./vendor/bin/phpunit -v -c ./core --group Phar
Before this patch #22 it'll crash the same way @mondrake is reporting and now it won't.
Comment #31
smustgrave CreditAttribution: smustgrave at Mobomo commented@catch was trying to check this one out but appears PharWrapperTest is removed in D10?
Is this ticket still needed?