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
All test runs on Postgres / PHP 5.5 for Drupal 8.5.x are segaulting - see https://www.drupal.org/node/3060/qa
Proposed resolution
Discover how this was introduced.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#18 | 2921862-17.patch | 1.14 KB | alexpott |
#8 | lucky-shoot.patch | 626 bytes | Anonymous (not verified) |
#6 | back-to-113c6dc258.patch | 94.81 KB | alexpott |
#2 | back-to-113c6dc258.patch | 95.21 KB | alexpott |
Comments
Comment #2
alexpottSo reverting HEAD back to 113c6dc258 / #2840392: Enable BigPipe by default in the Standard install profile because the test run was passing 2 days ago.
This reverts:
Comment #3
tacituseu CreditAttribution: tacituseu commented@alexpott: see #2871591-167: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts, also @vaplas made it pass with second patch from #2879048-183: Ignore: patch testing issue for #2919863.
Also failing on SQLite (https://www.drupal.org/pift-ci-job/803921).
Comment #4
mradcliffeHmm, the test runs seem to be failing all over the place with #2: https://dispatcher.drupalci.org/job/drupal_patches/36937/consoleFull, https://dispatcher.drupalci.org/job/drupal_patches/36936/consoleFull, https://dispatcher.drupalci.org/job/drupal_patches/36938/consoleFull, https://dispatcher.drupalci.org/job/drupal_patches/36939/consoleFull
Comment #6
alexpottI borked the patch - here's a new one... by including my customised .htaccess.
Comment #7
mradcliffeShoot. It looks like 5.5 test run has the same fail on #6: https://dispatcher.drupalci.org/job/drupal_patches/36959/consoleFull
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedAs @tacituseu said in #3, we already have few issues about this problems, that can be useful here.
Comment #9
alexpottIt's weird that #6 failed whilst https://dispatcher.drupalci.org/job/php5.5_postgres9.1/2567/consoleFull was a pass. That is the log of the last successful branch test on php5.5 / postgres / 8.5.x and the commit hash was:
So the codebase is exactly the same. This suggests that something else has changed. Perhaps on DrupalCI or on the underlying AWS infrastructure.
Comment #10
tacituseu CreditAttribution: tacituseu commentedRe #9: the codebase isn't the same, your revert is missing #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts.
Comment #11
alexpott@tacituseu interesting - according to my git log #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts was committed before 113c6dc
Comment #12
tacituseu CreditAttribution: tacituseu commented@alexpott: my bad, I somehow assumed you ment to revert all of Nov 6.
The fail isn't 100%, e.g. for SQLite:
https://www.drupal.org/pift-ci-job/805468 is green
while
https://www.drupal.org/pift-ci-job/803921
https://www.drupal.org/pift-ci-job/804598
had the same problem
Comment #13
alexpott@tacituseu well the fail seems to be 100% for PHP5.5 + Postgres + 8.5.x at the moment and it wasn't until the commit after 113c6dc. But the fact that revert all the changes doesn't fix it does seem to suggest that something else is at play other than the 8.5.x codebase.
Comment #14
alexpott@vaplas so any ideas why enabling book seems to the issue?
Comment #15
tacituseu CreditAttribution: tacituseu commentedLooking at @vaplas patches it fails 2-8 times per 30 runs of affected tests, both SQLite and PostgeSQL.
Edit:
@alexpott: Retested #6 on PostgreSQL and it passed (https://www.drupal.org/pift-ci-job/806292).
Comment #16
Anonymous (not verified) CreditAttribution: Anonymous commented#14: This patch from here. It is change the set of dependencies, the collection of which leads to random bugs in PHP 5.x. Maybe this bug (found by @xjm, see more in #2919863: Discover why gc_disable() during non-interactive install improves tests stability).
But why for Postgresql and SQLite need different tests? And why it does appears on PHP 5.6 as before? I do not know. See also #2879048-189: Ignore: patch testing issue for #2919863 from @tacituseu.
#15: No, just different modules are differently useful. And 'tour' is useless even here) Three other modules mixed the cards well enough)
Comment #17
tacituseu CreditAttribution: tacituseu commented@vaplas: #15 was meant to be a reply to #13.
Comment #18
alexpottI wonder if we shouldn't just disable garbage collection during non-interactive Drupal install during tests.
Comment #19
Wim Leers#18 looks sensible.
Comment #20
tacituseu CreditAttribution: tacituseu commentedSure beats reverting important patches each time we step on unsupported PHP's toes, all just because of some vintage Ubuntu fetish ;).
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedFunny, the opposite approach in #2879048-202: Ignore: patch testing issue for #2919863 also works!)
Comment #22
catchThe revert patch in the other issue is likely because it's affecting how the tests interact at high concurrency, however it's very likely to result in re-introducing previous segfaults we've already fixed. Moving this to RTBC.
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commented+1. Perhaps large test can cause this bug even with the
gc_disable()
during the installation phase. But this patch is in any case a good strengthening before #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7Comment #24
tacituseu CreditAttribution: tacituseu commented@catch: that revert wasn't meant to be a solution, just a check to see if it will move the segfault out of
EntityTest*JsonInternalPropertyNormalizerTest
, it shows that there's nothing inherently wrong with #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts itself.+1.
Comment #25
Wim Leers#22: which revert patch? One of the patches in #2879048: Ignore: patch testing issue for #2919863? Reverting #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts or #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts would be a massive blow to the API-First Initiative.
Comment #26
Anonymous (not verified) CreditAttribution: Anonymous commented#25: no) @catch said about #21, where I pointed to @tacituseu's patch with demonstration of passing tests after revert #2859704: Intermittent segfaults on DrupalCI (some "did not complete due to a fatal error" with no additional info).
In #24 @tacituseu already confirmed that he did not pursue the goal of pushing this revert. I'm sorry if my #21 comment has caused confusion.
Comment #27
catchYep sorry that wasn't really for tacituseu just for anyone looking at this issue in 4 years who might get confused, but now everyone's confused now...
Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!
Comment #30
Anonymous (not verified) CreditAttribution: Anonymous commented🎉
Hah!) It means that in four years anyone looking at this issue will feel more comfortable with confused :)
I reruned #2879048-178: Ignore: patch testing issue for #2919863 with test-only for PostgreSQL and SQLite, and now both green as expected. Super!
#2919863: Discover why gc_disable() during non-interactive install improves tests stability fixed as support request, because now we know what happened. Thank you all, and especially @tacituseu, who did a lot of work on this, and many times corrected me in my research. 🙏
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedBy the way, @Lendude also made a significant contribution (#2879048-98 - #2879048-109), creating a very useful test at an early stage research #2879048-102: Ignore: patch testing issue for #2919863.
Comment #32
Wim Leers🎉👏
Comment #33
tacituseu CreditAttribution: tacituseu commentedThis would also mean #2900399: Adding test coverage for batch bulk updates can go in now.
Comment #35
catch