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.
(There is no standard.profile
component, so I don't know what component to put this in.)
Problem/Motivation
- As of #2797169: Mark BigPipe as stable/non-experimental, BigPipe is marked stable. This means Drupal 8.3 will ship with BigPipe being a stable module.
- BigPipe improves performance without downsides. It's entirely transparent.
- Dynamic Page Cache is the most comparable module in the Standard install profile: improves performance transparently (i.e. layered on top), without downsides.
Why does this make sense?
- All Drupal 8.3 sites that start from the
Standard
install profile, which is arguably mostly non-experts, would get the benefits of BigPipe. Which means evaluations of Drupal would benefit from this. - Existing Drupal sites are not affected.
- Drupal shops/agencies/… usually have their own install profile, or even a per-client install profile. Which means that us changing the Standard install profile does not affect them in any way.
The only reason to not yet do this is to await the experience of Drupal 8.3 sites with BigPipe, since that's the first version of Drupal core in which BigPipe is marked stable. Which means pushing it to 8.5, i.e. Q2 2018.
Proposed resolution
Install the BigPipe module by default in Drupal 8.5 Standard
install profile installations.
Remaining tasks
- Discuss
- TBD
User interface changes
None.
API changes
None.
Data model changes
None.
Comments
Comment #2
Wim LeersAnd patch :)
Comment #3
Wim LeersComment #5
Wim LeersThis fixes most failures: updates the cache context assertions in two tests.
Comment #6
cilefen CreditAttribution: cilefen commentedIf it does happen...
Comment #8
effulgentsia CreditAttribution: effulgentsia at Acquia commentedFor me, that's the key question: do we want to first see what issues are reported when a broader set of sites enable BigPipe (thanks to it being stable in 8.3) prior to adding it Standard? Or is such caution unnecessary? Seems like a release management question to me, so tagging for that review.
Comment #9
xjm@Wim Leers, @effulgentsia, and I discussed this issue this morning and I'd like to postpone this issue to 8.4.x, so that we have the stable module in a shipped core release for testing etc. before we change standard.
Comment #10
Wim LeersWorks for me!
Comment #12
Wim Leers8.4 is open for development now. But I think it's still too early in the cycle. Already tagging though, this should help prevent us to forget to assess this.
Comment #13
xjmWell no, we should only put the tag on issues that are already committed or close thereto. It's already hard enough for release managers to sort through those that do land.
Also, I think it would actually be best for this issue to be active now. In some ways, once we agree on the change, it's best to commit it early. That gives us the longest time possible to catch regressions on new Standard installations that might result from edgecases etc. that were not tested on real sites yet.
So, unless there is a specific issue that this should be postponed on, let's mark it active and start discussing whether we want to do this. :)
Thanks @Wim Leers.
Comment #14
xjmOr well, there is also already a patch.
Comment #15
Wim LeersThanks, and sorry!
Comment #16
Wim LeersReuploading #5, because some of the failures were random update path test failures that have since been resolved.
Comment #17
Wim LeersAnd this updates the expected cache contexts for
NodeTranslationUITest
.Comment #18
Wim LeersAnd this then finally updates
StandardTest
, to execute Javascript, wait for BigPipe to finish sending content before asserting a raw string, and updates the expected raw string from<br />
to<br>
.Now tests pass.
Comment #22
Wim Leers#18 failed due to a malfunctioning testbot:
Reuploading #18.
Comment #23
Wim LeersComment #24
Wim LeersAh, damn, I thought #18 fixed the last problem, but there's one more:
StandardInstallerTest
also fails. Will work on fixing that.Comment #25
Wim LeersWell that's annoying.
The
StandardInstallerTest
failure is legitimate. It uncovered a new edge case, one that we had hitherto assumed could not possibly happen: a message displayed on the very first request to Drupal.After installing Drupal,
\install_finished()
does this:It doesn't show this message on the last page of the installer… but on the first page thereafter.
That first page thereafter is the first page that is rendered by Drupal itself; everything before that is rendered by the installer, which is a special snowflake: it's a limited environment, where advanced features are not available, including BigPipe.
Because it's the first pageload, and because
StandardInstallerTest
usesWebTestBase
which does not support executing JS, we follow the no-JS redirect. Which means that this is what happened:This assertion was added in #2458925: Screen is black and completely unreadable in Configure page after install on standard profile:
… but this was picked rather arbitrarily. We could just as well verify some other text on the default installer page. So, changing that to:
Finally: if you're wondering
, then the answer is: it's something that only ever happen in the installer, because that's the only way you can have your first page load already have a session (because we're coming from the installer, which doesn't use regular rendering system) and a status message. Without a session, no status message. Without a session, no BigPipe.Comment #27
Wim LeersLet's re-test. Because core has changed since Feb 20, but also because the patch in #25 failed, but d.o is not linking to the testbot results for some reason…
Comment #29
yogeshmpawarRe-roll of the #25 patch because it's failed to apply on 8.4.x branch.
Comment #30
Wim LeersLooks good, thank you, @Yogesh Pawar!
Comment #33
JacobSanfordPopping it up again! @YogeshPawar's patch in #29 no longer applied to 8.5.x. A reroll with no further modifications is attached.
Comment #35
cilefen CreditAttribution: cilefen commentedComment #36
Wim LeersExciting, a green patch!
Per #8 and #9, the only thing really preventing an RTBC 8 months ago was the fact that BigPipe should first be shipped with an entire minor cycle: Drupal 8.3. Since then, no significant bugs have been reported. In fact, since creating this issue, only 4 issues were created:
So I think that given that, BigPipe has proved its stability.
Therefore: RTBC'ing since this is green + a release manager decision.
Comment #38
Wim LeersSo apparently there is still a problem here.
Yet the previous testrun was timely and successful: https://dispatcher.drupalci.org/job/drupal_patches/27621/console … so it seems like this is just test infrastructure problems? Retesting…
Comment #39
Wim LeersRe-testing.
Let's get this in Drupal 8.5!
Comment #40
JacobSanfordNow that test shows green, re-escalating to RTBC as per @wim-leers's previous escalation.
Comment #41
Wim LeersThe test took 45 minutes (on Oct 13, 18:41 CEST). The closest D8 HEAD test run occurred on Oct 13, 20:36 CEST and took 48 minutes. Strangely, with different concurrency levels. AFAICT on 2 different bots. In any case, enabling BigPipe for the Standard install profile doesn't seem to make a material difference to test runs.
I don't think there's anything blocking this from becoming RTBC now. BigPipe will have been a stable module in Drupal 8 core for two release cycles (8.3 + 8.4). If material problems arise, we can still revert this. I've gotten the question
many times now.Comment #43
Wim LeersComment #45
JacobSanfordComment #46
xjm> 50% of the test runs on this issue have failed (due to timeouts apparently?), not just a few days ago but also back in August when the patch was last tested. It'd be a surprising coincidence for this patch to have been tested in two windows months apart where testbot happened to be unstable in this way. And, indeed, looking at the branch test history https://www.drupal.org/pift-ci-job/789866 indicates that there was no corresponding testbot problem a few days back for HEAD.
So I'm pretty sure having BigPipe in Standard is making the test suite less stable, in a serious way. Without having an explanation for what exactly is going on, I'd not want to commit this to HEAD, because if I'm right we'd be introducing a 50% random fail into everyone else's patches.
When an issue repeatedly gets marked NW with testbot failures, listen to the bot. :) Sometimes there are random failures or spates of bot issues, but don't just assume it's that -- look into it.The branch tests run every day and several times a day when there's commits so you can go to https://www.drupal.org/node/3060/qa, click on the environment that failed. If you still think it's not your patch's fault, you can add a patch that changes a
.php
file but does not include your change as a test-only patch for the baseline. My default is:Thanks everyone! Setting NR to look more deeply into the testing issue. I'd suggest investigating reasons that BigPipe in Standard might actually make stuff time out, and comparing logs of the problematic runs to logs of HEAD and the regular test runs on the same day. Before we'd commit this I'd also want to see hard evidence that the issue is resolved and an explanation of what was causing this.
Leaving the product manager review tag on because we'd need that signoff still even once the test issue is fixed. And also tagging for release manager review on account of the test concerns and release management implications.
Thanks!
Comment #47
Wim LeersFully agreed, of course.
But getting to the bottom of this is going to be very, very difficult, because the successful test runs are not at all slower than HEAD’s test runs. That’s what I explained and researched in #41. How one test run of the patch can be indistinguishable from a HEAD test run and another shortly thereafter can time out after taking more than twice as long is a complete mystery to me 😥
I’ll work with the testbot maintainers to get to the bottom of this.
Comment #48
Wim LeersPinged mixologic.
Comment #49
MixologicThis is a single test that is failing to complete, the rest of the test run is running fine.
I compared the test output from the console of the most recent successful test: https://dispatcher.drupalci.org/job/drupal_patches/33853/ with the next consecutive failure, and pared it down to just the test names.
This is what I found:
The EntityFormMode tests were new tests introduced in between the 13th, and the 16th, so its no surprise they are showing up. We can ignore those.
The test that never completes is the StandardTest.
I have a pretty strong hunch that this is because you're changing the testbase from BrowserTestBase to JavascriptTestBase, but you are not moving the test into a FunctionalJavascript namespace.
The testbot, and phantomjs, would sometimes hang, and never finish when running javascript tests in parallel, so we separated out the FunctionalJavascript tests and run those separately and serially. Except in this case it is still trying to run the StandardTest along with all the other tests.
Interesting that even just *one* Javascript test can hang if there's any parallelization going on.
So, my suggestion is to update the patch to move the namespace to Drupal\Tests\standard\FunctionalJavascript\StandardTest so that it is tested in the safety of non being parallelized.
Comment #50
Wim Leers😵
Wow, I'd never have been able to do such a clear root cause analysis! Thank you so much, @Mixologic! 👏
(Committers, please credit @Mixologic!)
Comment #51
dawehnerThis is yet another of the instances where the phpunit test runner differs from what we do in
run-tests.sh
, sadly.Is there a reason we could not at least keep some of the existing test coverage in the non javascript bit? I fear we would loose a bit of test coverage when we require JS for these tests.
Comment #52
Wim LeersWhat test coverage do we lose?
Comment #53
Wim LeersI just queued a re-test of #50, and also queued tests in every available combination of environments.
That's a lot of tests, but should help us confirm that the random timeouts are a thing of the past.
(We can then of course still look into @dawehner's concern! Just trying to confirm that Mixologic's analysis was as flawless as I think it is 😀)
Comment #55
Wim LeersPageCacheTagsIntegrationTest
was moved by #2870457: Convert web tests to browser tests for page_cache module.Rebased.
Comment #56
dawehnerWell, we do all kind of stuff here, submitting forms, hitting URLs etc. It would be nice to ensure that those continue to work without JS involved.
Comment #57
Wim LeersOh, of course! Sorry for not realizing that. Will work on reroll that keeps all other tests except one in
Functional
rather thanFunctionalJavascript
.Comment #58
dawehnerNo worries, that's why we have review :)
Comment #59
Wim LeersOn Feb 20, 2017, in #17,
StandardTest
failed. It failed to find certain elements. It looks like since then, our test dependencies/infrastructure changed, because I can now getStandardTest
to pass (at least locally) without changing anything about it. That'd of course be the ideal solution to @dawehner's remark: simply being able to remove the change altogether :)(Ironically, then @Mixologic's research in #49 also would not have been necessary.)
Let's see if this is also the case on testbot.
Comment #60
dawehnerI'm wondering: Do we still need some form of JS test to prove big pipe is actually running in some form or another?
Comment #61
Wim LeersAny test that uses the
standard
install profile that also tests cache contexts will have something like this.We already have extensive, detailed test coverage for BigPipe itself.
I think that's sufficient? Of course, the whole point about BigPipe is that it is a transparent change: if we'd have to change lots of tests, that'd prove that it wasn't transparent!
One thing I can think of that'd be a useful smoke test: we can add explicit test test coverage to verify that e.g. the comment form for an article is served via BigPipe in the Standard install profile.
Comment #62
dawehnerGiven that this actually increases the performance of the site, this sounds like a really nice idea!
Comment #63
Wim LeersAlright :) 👍
Comment #67
webchickReviewed this issue on today's product management meeting.
Making users' sites faster out of the box sounds like a good thing to do. :) So no objections from the product management team. Makes total sense.
We played around a bit with this patch + the Google Chrome network slower-downer thingy to try and evaluate the user experience. It would be awesome to pick up where we left off at #2632750: Interface previews/skeleton screens through optional "preview" or "placeholder" templates (not a pre-requisite for this patch at all, just would make for a noticeably nicer out of the box experience).
Comment #68
Wim LeersThanks, @Bojhan, @Gábor Hojtsy, @yoroy and @webchick! Much appreciated! ❤️
I agree that having #2632750: Interface previews/skeleton screens through optional "preview" or "placeholder" templates would make things even nicer. I'll try to push that one along again too.
Right now, the only blocker here is me working on the expanded/improved test coverage that @dawehner and I discussed in #60–#63. This issue is already assigned to me for that.
Comment #69
ThoreauinSF CreditAttribution: ThoreauinSF commentedI'm worried that dynamic page components - like for example an integrated Hubspot Form - would be invisible to SEO using this method.
Also Big Pipe only applies to logged in traffic - so if your site never uses logged in users but for site content changes - Big Pipe doesn't do anything for you. all but .000001% of my site is unauth nonlogged in users of my site.
Comment #70
Wim LeersWith 100% certainty this is not the case. To convince yourself: install BigPipe, see how it uses JS if your browser supports JS, then disable JS, and see how it still streams the content, but doesn't use JS.
This very concern was already addressed before BigPipe was added to core. If it broke such use cases, it A) never would've been added to core, B) would have caused lots of bug reports in the >1.5 years it's been in core.
Correct. So let's make things faster for site builders :)
Comment #71
Wim LeersImplemented @dawehner's suggested additional test coverage.
Comment #73
Wim LeersFailed due to:
I think this might fix it. (I can't reproduce the failure locally.)
Comment #74
dawehnerThank you @Wim Leers
I think this patch is ready to go. I am a bit hesitant with the premise of the patch itself. Sure it improves performance for the users. One problem I see though is that this makes it harder for people to understand/debug. Content being loaded later, might cause issues with things like google analytics. Making this a conscious choice might help with that.
Comment #75
Wim LeersThe main content is never loaded via BigPipe, for this very reason: serving an empty shell is pointless, we want to ensure that the main content is always available from the very start (i.e. the reason for the user to visit this page/URL).
Furthermore, this only affects new sites using the Standard install profile. We want to show Drupal at its best out of the box, don't we?
Comment #76
Wim LeersProduct manager sign-off in #67:
Patch sign-off in #74:
So I'd RTBC this again, but we still have a
tag.Comment #77
dawehnerFair point :) To be honest I hope the out of the box initiative actually produces something which shows how great Drupal itself is.
Comment #78
Anonymous (not verified) CreditAttribution: Anonymous commentedWoot! BigPipe super rocks!
Comment #79
xjmNice research @Mixologic.
Following that, I queued sufficient test runs to convince myself that this is no longer breaking them.
Committed and pushed to 8.5.x. Thanks! I also created and published a change record: https://www.drupal.org/node/2921159
Comment #80
Wim LeersOMG yay! 🎉
This probably merits a mention in the 8.5.0 release notes?
Comment #81
Gábor HojtsyComment #82
Anonymous (not verified) CreditAttribution: Anonymous commentedIt's great! Many thanks! 🚀🚀🚀
Comment #83
effulgentsia CreditAttribution: effulgentsia at Acquia commentedCongratulations, Big Pipe, for being the first D8 core module to make it through the full pipeline of experimental -> stable -> standard! I'm looking forward to more of our experimental modules following that lead and benefitting from all that's been learned along the way by Big Pipe on how to do it!!
Comment #84
Wim LeersThanks, Alex! 😀
Comment #85
xjmI haven't gotten to posting it yet, but from now on the release notes are only going to contain things that people need to know to upgrade. Cool stuff is going to be tagged "8.5.0 highlights". Instituting that for the first time here. :)
Comment #86
Wim LeersWOOT! 😊
Comment #88
JacobSanfordMany thanks, @wim-leers for your efforts here. This is a fantastic change for 8.5.