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
#2989734: PHP 7.3 compatibility made Drupal 8 pass on PHP 7.3 but we introduced some workarounds to overcome differences in PHP 7.3.
The workarounds are:
- Drupal\KernelTests\Core\Entity\EntityQueryTest does not use list()
- PrivateTempStore had to use DependencySerializationTrait - whilst the fix is correct we should work out what has changed in PHP's serialize()
- BrowserTestBase::getDrupalSettings() had to change to use
$this->getSession()->getPage()->getContent()
instead of$this->getSession()->getPage()->getHtml()
in order for it to work
Proposed resolution
Fix the tests or work out why this is happening.
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#42 | 3001920-42.patch | 7 KB | tacituseu |
#40 | 3001920-40.patch | 6.68 KB | tacituseu |
#36 | 3001920-36.patch | 8.17 KB | tacituseu |
#31 | 3001920-31.patch | 7.45 KB | tacituseu |
#29 | 3001920-28.patch | 7.47 KB | tacituseu |
Comments
Comment #2
mikelutzFor MigrationPluginManagerTest, 7.2 and 7.3 are producing different results around the array_multisort call in Drupal\migrate\Plugin\MigrationPluginManager::buildDependencyMigration at line 211
The test tests the order of migrations after sorting them so dependent migrations run first. The broken test is asserting that an set of migrations that do not depend on each other are migrated in a specific order. In this case, the order doesn't actually matter, and the array_multisort call is made with both weighted at 0 (correctly). By the documentation "If two members compare as equal, their relative order in the sorted array is undefined." @see http://php.net/manual/en/function.array-multisort.php. Therefore it isn't correct to test that these particular migrations end up in this particular order.
In fact, if I xdebug pause right before the array_multisort is executed in 7.2 and resume, I get them in a different order and the test fails.
Comment #3
mikelutzI'm throwing a few things against the testbot. Mixing the patch from part one for my own sanity.
Comment #4
mikelutzI feel silly tracking down heisingbugs, but... is it the hasTranslation check, or the just the assertion causing the test to pass...
The Migration failure I was able to duplicate in a docker container - no xdebug, but I could at least poke at it with variable dumps. I haven't been able to replicate the EntityQueryTest issue though.
Comment #5
alexpott@mikelutz++ totally not silly. Thanks for doing this!
Comment #6
mikelutzComment #7
mikelutzComment #8
mikelutzThe more variations on this that I try, the more confused I get...
Comment #9
mikelutzComment #10
mikelutzIs the heisingbug part of this due to using print_r with return set to TRUE in a test? According to the print_r docs , "When the return parameter is used, this function uses internal output buffering so it cannot be used inside an ob_start() callback function."
Because the only difference between #7 and #8 is the addition of the print_r to the end of the message in the assertTrue(FALSE) call. It shouldn't have possibly affected whether that assertion was ran...
Trying with serialize instead...
Comment #11
mikelutzComment #12
mikelutzComment #13
mikelutzComment #14
mikelutzComment #15
mikelutzComment #16
tacituseu CreditAttribution: tacituseu commentedChecking with opcache disabled.
Comment #17
tacituseu CreditAttribution: tacituseu commentedNot convinced... tries harder.
Comment #18
tacituseu CreditAttribution: tacituseu commented:D
Take a pick: b8ffa370.
Comment #19
mikelutzNice find. I figured it was a php bug, but I couldn't find it. I really wish it had occurred to me that I could cut down the test suite too. I need to remember that trick.
This one was driving me nuts all day.
Comment #20
alexpottSo now we need to report a bug on PHP. I'll try to do that with instructions on how to recreate. Testing with opcache on locally shows me that the problem might occur in Symfony/Yaml and sure enough running its tests on PHP 7.3 with opcache enabled fail. Which is great because this is much easier for PHP devs to reproduce. See https://bugs.php.net/bug.php?id=76933
Comment #21
tacituseu CreditAttribution: tacituseu commentedNot sure about it being fixed by bug #76711 as it was committed on Sep 20 and the testbot's environment says:
PHP 7.3.0-dev (cli) (built: Sep 25 2018 18:34:07) ( NTS )
.Comment #22
alexpott@tacituseu yeah I don't think this has been fixed at all. I was just describing my efforts. I was trying to see how https://hub.docker.com/r/drupalci/php-7.3.x-apache is actually built.
If someone can reproduce the bug we're seeing in simple code that'd be amazing.
Comment #23
tacituseu CreditAttribution: tacituseu commented@alexpott: I based it on drupalci_environments, but don't know what triggers the rebuilds.
Comment #24
alexpottI've built PHP7.3 from src and confirmed we still have a problem against PHP7.3 HEAD so I've created a PHP bug report - https://bugs.php.net/bug.php?id=76937. Unfortunately I've still not been successful in creating a script that shows the issue without Drupal being around.
Comment #25
alexpottThere are other PHP7.3 bugs that Drupal's tests have uncovered that are not reported.
There is some change in the serialize that has broken something. The bug exposes that we serialize far too much but it should still work.
I suspect that there is some PHP 7.3 bug here too.
Comment #26
tacituseu CreditAttribution: tacituseu commentedRe #25: Checking first if they go away without opcache.
Edit: #25.2 went away.
Comment #27
tacituseu CreditAttribution: tacituseu commentedDigging into #25.1.
Comment #28
alexpottI've run Symfony's BrowserKit, Behat's Mink and MinkBrowserKitDriver tests on the latest PHP 7.3 dev with opcache enabled and there are no errors so it looks what ever is causing #25.2 is not tested by the stuff we're relying on :(
Comment #29
tacituseu CreditAttribution: tacituseu commentedComment #30
alexpott@tacituseu I dumped the results of the serialization locally before whilst locally into that. It's not pretty. Good luck!
Comment #31
tacituseu CreditAttribution: tacituseu commentedNot pretty indeed, tweaking it a bit to be able to compare to known good.
Comment #32
tacituseu CreditAttribution: tacituseu commentedGood news is that for both good and bad run serialization looks the same (only timestamps and random strings differ), so the problem is in unserializer. Another good thing is that it is consistent as both
MigrateUpgrade6Test
andMigrateUpgrade7Test
fail in the same place:So it is a problem with processing object reference, I've no idea why, maybe it rings a bell somewhere.
PHP's commit log: var_unserializer.c.
Comment #33
tacituseu CreditAttribution: tacituseu commentedReconstructed (replaced references above 1430 with a dummy one) and trimmed
var_dump
up to the failing point:Comment #34
alexpottComment #35
alexpottDrupal\Tests\migrate\Unit\MigrationPluginManagerTest is now fixed in #2989734: PHP 7.3 compatibility and we have a work-around for Drupal\KernelTests\Core\Entity\EntityQueryTest so this issue should now be about investigating the work-arounds introduced in that issue.
Comment #36
tacituseu CreditAttribution: tacituseu commentedChecking what changes with
DependencySerializationTrait
.Comment #37
alexpott@tacituseu the services on the object will not be serialized - ie $this->lockBackend, $this->currentUser, $this->requestStack
Comment #38
tacituseu CreditAttribution: tacituseu commentedI meant "in the structure of serialized string", the patches produce log under
artifacts\run_tests.functional\phpunit-xml
.Comment #39
tacituseu CreditAttribution: tacituseu commentedExactly those mentioned in #37 :), the serialized string is 11K smaller after and the only references left in it are to the
TranslatableMarkup
of the submit button.Comment #40
tacituseu CreditAttribution: tacituseu commentedRe-testing #26 as it only disabled optimizations and not the whole Opcache.
Comment #41
tacituseu CreditAttribution: tacituseu commented@alexpott: Could do a bit search on
opcache.optimization_level
to pinpoint which pass is responsible for https://bugs.php.net/bug.php?id=76937 ? Might help tracking it down.Comment #42
tacituseu CreditAttribution: tacituseu commentedSame as #40 but with Opcache and APC disabled.
Comment #43
tacituseu CreditAttribution: tacituseu commentedThe additional fail in
CommentCSSTest
from #42 is due to race condition withmkdir()
inBrowserHtmlDebugTrait::initBrowserOutputFile()
.Comment #44
tacituseu CreditAttribution: tacituseu commentedLooks like we hit #25.1 again.
@tim.plunkett tracked it down to https://bugs.php.net/bug.php?id=77302:
That would explain why in #31 both on 7.2 and 7.3 serialized strings looked the same.
Comment #45
bojanz CreditAttribution: bojanz at Centarro commentedWe just fixed the serialization crash in Commerce by no longer using $this->logger in a serialized class: #3038430: Unable to checkout on php7.3.
If the logger was retrieved from LoggerChannelFactory, it can't be serialized, since it is invisible to DependencySerializationTrait.
Previously it would get silently corrupted, but PHP 7.3 changed that behavior.
The solution is to either store the factory service instead and retrieve the logger only when needed, or to define a channel-specific logger service, like core does:
Comment #46
aspilicious CreditAttribution: aspilicious commentedAlso see https://www.drupal.org/project/entityqueue/issues/3043100 which points to https://github.com/php/php-src/commit/af324e24df85022314787f6dcf2ac811f5...
Comment #47
joachim CreditAttribution: joachim commentedPHP bug https://bugs.php.net/bug.php?id=77302 is also rearing its head in the Oauth2 Server module, in #3049250: PHP 7.3 Session Handling Issue.
The problem there is basically that a request object can't survive serialization. This code produces an error:
Does the issue summary need to be updated for the object serialization problem?
Comment #49
jungleComment #52
malaynayak CreditAttribution: malaynayak as a volunteer and commentedAfter upgrading to php 7.3 all the custom AJAX callbacks in some custom forms were failing due to php serialisation error:
Notice: unserialize(): Error at offset 28221 of 41088 bytes in Drupal\Component\Serialization\PhpSerialize::decode() (line 21 of /var/www/html/docroot/core/lib/Drupal/Component/Serialization/PhpSerialize.php)
As a quick fix we are patching the /core/lib/Drupal/Core/Form/FormCache.php class setCache function
It though resolved the issue, we need to know the actual root cause and fix for it. Any help would be appreciated.
Comment #56
longwaveComment #59
andypostI bet it outdated!