Follow-up for #2678568: Ensure good UX & DX even when A) rendering of placeholder fails, B) some response event subscriber fails.

The code committed there uses the \Drupal::service() pattern in exception handlers. We should not do that anymore. See #2678568-44: Ensure good UX & DX even when A) rendering of placeholder fails, B) some response event subscriber fails.

CommentFileSizeAuthor
#9 interdiff-2702001-4-9.txt1.06 KBolafkarsten
#9 2702001-9.patch4.54 KBolafkarsten
#7 2702001-7.patch4.21 KBolafkarsten
#4 2702001-4.patch4.25 KBolafkarsten

Comments

Wim Leers created an issue. See original summary.

olafkarsten’s picture

olafkarsten commented
Assigned: Unassigned » olafkarsten
Issue tags: +DrupalBCDays

Working on this, during DrupalBCDays Codesprint.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Yay, thanks! :)

olafkarsten’s picture

olafkarsten commented
StatusFileSize
new 2702001-4.patch4.25 KB
olafkarsten’s picture

olafkarsten commented
Status: Active » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Just one nitpick:

+++ b/core/modules/big_pipe/src/Render/BigPipe.php
@@ -91,13 +92,16 @@ class BigPipe implements BigPipeInterface {
+   *   A config factory for retrieving required config objects.

Let's use a shorter description, the same one used in other places.

olafkarsten’s picture

olafkarsten commented
StatusFileSize
new 2702001-7.patch4.21 KB

Actually picked that one from 'BigPipeResponseAttachmentsProcessor'

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work 
+++ b/core/modules/big_pipe/src/Render/BigPipe.php
@@ -91,13 +92,16 @@ class BigPipe implements BigPipeInterface {
     $this->eventDispatcher = $event_dispatcher;
+    $this->configFactory = $config_factory;
   }

We should document the property itself, sorry :)

olafkarsten’s picture

olafkarsten commented
Status: Needs work » Needs review
StatusFileSize
new 2702001-9.patch4.54 KB
new interdiff-2702001-4-9.txt1.06 KB

Next try.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

Perfect!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Thanks! :)

catch’s picture

catch
he/him
Primary language English
commented

Can we open a follow-up, or link to the existing issue if there is one, to something which would allow the message to be printed in the same request? For example could we show error messages in the footer as a final fallback?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +code cleanup, +Quickfix

#12: You mean a follow-up to #2678568: Ensure good UX & DX even when A) rendering of placeholder fails, B) some response event subscriber fails, not to this, right? This is solely code clean-up. What you describe sounds feasible, but I'm not sure if it's desirable. It'd mean appending HTML to the HTML just before the closing </body> tag. And that HTML may simply end up not being visible depending on the theme. I think it's good enough as it is.

catch’s picture

catch
he/him
Primary language English
commented

Well I still think the config setting for error display leaking into bigpipe is a bit smelly, I'm not aware of anywhere else we have to deal with that.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Let's continue #12/#13/#14 in #2712935: Messages are not rendered last, meaning those messages set within placeholders only appear on the next request to not move this issue in a more off-topic direction.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Given that we want to explore the issue further as per #15 should we consider committing this to 8.1.x and 8.2.x. Big pipe is experimental and code divergence at this point might add more pain and make development / bug fixes slow.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Version: 8.2.x-dev » 8.1.x-dev

+1.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Inject config factory, even in exception handlers » Inject config factory for BigPIpe (including in exception handlers)

Agreed that this change would be okay for 8.1.x since it only touches BigPipe, per: https://www.drupal.org/core/d8-allowed-changes#experimental

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Inject config factory for BigPIpe (including in exception handlers) » Inject config factory for BigPipe (including in exception handlers)

(Typo, sorry.)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed bdb434a and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 4c11b7a on 8.2.x 
    Issue #2702001 by olafkarsten, Wim Leers: Inject config factory for...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Weird, no commit message for 8.1.x, even though it's there (commit bdb434ad0751d30af1a4778dc9c543144341cdac).

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Also committed & pushed to the contrib module for Drupal 8.0: http://drupalcode.org/project/big_pipe.git/commit/2df41d8

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.