(There is no standard.profile component, so I don't know what component to put this in.)

Problem/Motivation

  1. 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.
  2. BigPipe improves performance without downsides. It's entirely transparent.
  3. 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?

  1. 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.
  2. Existing Drupal sites are not affected.
  3. 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.4, i.e. Q4 2017.

Proposed resolution

Install the BigPipe module by default in Drupal 8.3 Standard install profile installations.

Remaining tasks

  1. Discuss
  2. TBD

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Status: Active » Needs review
FileSize
435 bytes

And patch :)

Wim Leers’s picture

Title: Enable BigPIpe by default in the Standard install profile » Enable BigPipe by default in the Standard install profile

Status: Needs review » Needs work

The last submitted patch, 2: big_pipe_standard_default-2840392-2.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
1.39 KB

This fixes most failures: updates the cache context assertions in two tests.

cilefen’s picture

Issue tags: +8.3.0 release notes

If it does happen...

Status: Needs review » Needs work

The last submitted patch, 5: big_pipe_standard_default-2840392-5.patch, failed testing.

effulgentsia’s picture

The only reason to not yet do this is to await the experience of Drupal 8.3 sites with BigPipe

For 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.

xjm’s picture

Status: Needs work » Postponed
Issue tags: -8.3.0 release notes, -Needs release manager review

@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.

Wim Leers’s picture

Works for me!

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Issue tags: +8.4.0 release notes

8.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.

xjm’s picture

Status: Postponed » Active
Issue tags: -8.4.0 release notes

Well 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.

xjm’s picture

Status: Active » Needs work

Or well, there is also already a patch.

Wim Leers’s picture

Thanks, and sorry!

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.81 KB

Reuploading #5, because some of the failures were random update path test failures that have since been resolved.

Wim Leers’s picture

And this updates the expected cache contexts for NodeTranslationUITest.

Wim Leers’s picture

And 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.

The last submitted patch, 16: big_pipe_standard_default-2840392-5.patch, failed testing.

The last submitted patch, 17: big_pipe_standard_default-2840392-17.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: big_pipe_standard_default-2840392-18.patch, failed testing.

Wim Leers’s picture

FileSize
3.5 KB

#18 failed due to a malfunctioning testbot:

00:33:21.038 Drupal\system\Tests\Module\InstallUninstallTest              3202 passes                                      
00:33:42.775 Drupal\views_ui\Tests\PreviewTest                            237 passes                                      
00:33:46.108 Drupal\views_ui\Tests\HandlerTest                            516 passes                                      
00:34:00.017 Drupal\views\Tests\Plugin\ExposedFormTest                    131 passes                                      
00:34:32.145 Drupal\views_ui\Tests\DisplayTest                            361 passes                                      
01:50:00.019 Build timed out (after 110 minutes). Marking the build as aborted.
01:50:00.024 Build was aborted
01:50:00.024 [CHECKSTYLE] Skipping publisher since build result is ABORTED
01:50:00.024 Archiving artifacts
01:50:00.034 Checking console output
01:50:00.038 Recording test results
01:50:00.056 ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error?
01:50:00.062 Finished: ABORTED

Reuploading #18.

Wim Leers’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs review » Needs work

Ah, damn, I thought #18 fixed the last problem, but there's one more: StandardInstallerTest also fails. Will work on fixing that.

Wim Leers’s picture

Well 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:

  $success_message = t('Congratulations, you installed @drupal!', array(
    '@drupal' => drupal_install_profile_distribution_name(),
  ));
  drupal_set_message($success_message);

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 uses WebTestBase which does not support executing JS, we follow the no-JS redirect. Which means that this is what happened:

  1. Step 1: request the frontpage. Rendered by BigPipe, using JS BigPipe placeholders. Includes rendered messages. But includes the no-JS redirect.
  2. Step 2: since this browser does not support JS, we follow the redirect.
  3. Step 3: request the frontpage. Rendered by BigPipe, using no-JS BigPipe placeholders. There are no messages to render, because they were already rendered in step 1!

This assertion was added in #2458925: Screen is black and completely unreadable in Configure page after install on standard profile:

    $this->assertRaw(t('Congratulations, you installed @drupal!', array(
      '@drupal' => drupal_install_profile_distribution_name(),
    )));

… but this was picked rather arbitrarily. We could just as well verify some other text on the default installer page. So, changing that to:

$this->assertRaw('No front page content has been created yet.');

Finally: if you're wondering but shouldn't we fix this in BigPipe?, 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.

Status: Needs review » Needs work

The last submitted patch, 25: big_pipe_standard_default-2840392-25.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

Let'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…

Status: Needs review » Needs work

The last submitted patch, 25: big_pipe_standard_default-2840392-25.patch, failed testing.

Yogesh Pawar’s picture

Status: Needs work » Needs review
FileSize
4.11 KB

Re-roll of the #25 patch because it's failed to apply on 8.4.x branch.

Wim Leers’s picture

Looks good, thank you, @Yogesh Pawar!

Status: Needs review » Needs work

The last submitted patch, 29: enable_bigpipe_by-2840392-29.patch, failed testing.