Ensure good UX & DX even when A) rendering of placeholder fails, B) some response event subscriber fails

Clean-up.

As of this reroll, the logic is in place to handle broken response event subscribers.

This issue is now blocked on reviews.

Rerolled now that BigPipe is in core.

Straight reroll.

Straight reroll. Conflicted with #2670444: Embedded AJAX responses aren't application/json, but application/vnd.drupal-ajax.

Discussed with catch to get on the same page. Went with #2, because otherwise debugging is still significantly impaired, which itself already largely goes against what the issue title says.

Writing test coverage to verify that errors do in fact get logged was extremely painful, but I managed to do it.

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

#25: because this is for handling uncaught exceptions. i.e. when something fails: when code is broken.

Injecting this service means it's always initialized, and therefore always slows down this code. We only need that service in case of broken code, so there's no need for that service to be injected always.

But not rethrowing the exception will still mean it's painful to debug. The whole point of this issue is to ensure it's equally easy to debug with BigPipe as it is without.

Or am I missing something here?

The trigger_error() will still show the exception in the UI during development, so it's not really much more painful than throwing the exception itself is it?

No, that doesn't work, because:

1. trigger_error() only allows for E_USER_* errors to be triggered
2. _drupal_error_handler_real() treats all E_USER_* errors (except if thrown from a __toString() method) as non-fatal errors
3. non-fatal errors are only displayed (even with verbose error logging) as messages
4. … and it was already determined before rendering these placeholders that in fact no messages were to be displayed on the current HTML page

Therefore you will only get to see this trigger_error()-triggered error message on the next page you visit!

The consequence of doing #30 is therefore that the DX is worse than it is today in HEAD, and that only UX improves.

Thoughts/ideas?

To be clear, this is what AFAICT @catch is asking for in #30, and I explained in #34 why that does not work.

@dawehner in IRC agrees with me:

13:02:35 <WimLeers> dawehner: yes, but catch is arguing to NOT re-throw an exception, and just always use trigger_error()
13:02:49 <dawehner> oh I see, yeah I disagree with that
…
13:04:59 <dawehner> WimLeers: well, maybe catch is arguing for a better error handling
13:05:04 <WimLeers> I don't think so
13:05:31 <WimLeers> I think he just didn't realize the error handling system's use of drupal_set_message() and how that doesn't help in this situation
13:06:03 <dawehner> WimLeers: well, many systems convert any kind of errors to exceptions in development mode

I'm +100 for that, but that's a much bigger undertaking. I'd like to see BigPipe's DX fixed before Drupal 8.1.0 is released.

So, in light of #34–#36, I think the most sensible thing for me to do is to figure out why the patch in #20 that was committed and then reverted is failing for PHP7. This is the patch in #20, plus some debug output to figure that out. I suspect PHP7 simply logs things differently.

So, #37 clearly shows the difference between PHP 5 and PHP 7. The failed assertionsare:

$this->assertTrue(FALSE !== strpos((string) unserialize($records[0]->variables)['@message'], "exception 'Exception' with message 'Oh noes!'"));
…
$this->assertTrue(FALSE !== strpos((string) unserialize($records[1]->variables)['@message'], "exception 'Exception' with message 'You are not allowed to say llamas are not cool!'"));

i.e. the expected strings are:

1. exception 'Exception' with message 'Oh noes!'
2. exception 'Exception' with message 'You are not allowed to say llamas are not cool!'

These expectations are met on PHP 5. But on PHP 7, the resulting strings are:

1. Exception: Oh noes!
2. Exception: You are not allowed to say llamas are not cool!

So, rather than asserting the presence of the exact entire error log string generated in PHP 5, we can just assert on the presence of the custom message. This then is still testing what we care about: that the actual expected exceptions are logged correctly. We don't care about the exact format that PHP 5 or 7 log them in.

The net difference between the patch here and #20 is then (and this is the entire interdiff):

-    $this->assertTrue(FALSE !== strpos((string) unserialize($records[0]->variables)['@message'], "exception 'Exception' with message 'Oh noes!'"));
+    $this->assertTrue(FALSE !== strpos((string) unserialize($records[0]->variables)['@message'], 'Oh noes!'));
     $this->assertEqual(RfcLogLevel::ERROR, $records[0]->severity);
-    $this->assertTrue(FALSE !== strpos((string) unserialize($records[1]->variables)['@message'], "exception 'Exception' with message 'You are not allowed to say llamas are not cool!'"));
+    $this->assertTrue(FALSE !== strpos((string) unserialize($records[1]->variables)['@message'], 'You are not allowed to say llamas are not cool!'));

That's it. And just for PHP 7 compatibility. I believe that makes it okay for me to re-RTBC this patch, since the changes are so utterly trivial.

the disruption from this change is still acceptable

To be more accurate, there is no disruption. It does not break anything. It does not change any APIs.

Also note that 80% of this patch is test coverage.

+1 — thanks for all the detailed insight into your considerations!

Fair enough. Thanks for committing it so that the improvements will definitely be available in 8.1.0. I filed #2702001: Inject config factory for BigPipe (including in exception handlers) to address that.

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