Make it easier to write on4xx() exception subscribers

+++ b/core/lib/Drupal/Core/EventSubscriber/HttpExceptionSubscriberBase.php
@@ -90,6 +90,7 @@ public function onException(GetResponseForExceptionEvent $event) {
+      $method_fallback = 'on' . (string) (int) ($exception->getStatusCode() / 100) . 'XY';

WOAH what? :D

I'm not sure that it's a good idea to rely on PHP's typecasting behavior? :P

Impressive though!

Rather than magic letters, it seems more generic and straightforward to use the 00. That is, on400() will catch any 4**, and then in parallel on500() would catch any 5**, etc. (Obviously in both cases presuming that a more specific version doesn't exist, in which case that gets used.)

Well, 400 is a generic error. So would you want to differentiate at that point? Seems like it would probably be the same code; we can also verify that the actual number is available so on400() can branch internally if it really cares.

XY is pretty confusing without reading the code. That said having 400 being a fallback might confuse people too. I kinda like the idea though... I think.

The (string) in the cast seems repetitive since . is going to cast it anyways. Using (int) to truncate the decimal is clever though :).

The only reason I can think that you would "care" that you're handling a different 4XX error would be because you want to bubble the original response code to the new response.

For example, this loose the 404 and 405 response codes on the response when we sets it to 403 here in the patch.

   public function on4XY(GetResponseForExceptionEvent $event) {
     $response = new JsonResponse(array('message' => $event->getException()->getMessage()), Response::HTTP_FORBIDDEN);
     $event->setResponse($response);
   }

That might be solvable with a optional argument or something though. If we did that and someone absolutely had to handle 400 different from 4XX they could still do so by switching on the new argument.

Don't want to hold this up, but I personally would consider floor($code / 100) . '00' a bit more readable than the cast to int. Not sure what others think...

1. +++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
   @@ -28,46 +28,15 @@ protected static function getPriority() {
   +    $response = new JsonResponse(array('message' => $event->getException()->getMessage()), $exception->getStatusCode());
   

   You could convert this to should array syntax if you wanted ;)

There are like 9k of changes to views and autocomplete that snuck in #11 that aren't clearly connected or in the interdiff. NW since I assume that was an accident?

I really love the DX of this issue and think the kinks have been shaken out so going to be bold here :)

There's a CI error in the log so I think this is still RTBC.

Random fail.

+++ b/core/lib/Drupal/Core/EventSubscriber/ExceptionJsonSubscriber.php
@@ -28,46 +28,15 @@ protected static function getPriority() {
+  public function on400(GetResponseForExceptionEvent $event) {

400 is a specific status code, not a "anything between 400 and 499". In HEAD, we already have a cache_ttl_4xx setting and a 4xx-response cache tag, so what about on4xx() methods?

XY is pretty confusing without reading the code.

Agreed, but is 4xx confusing, given we're already using that for the setting and cache tag?

See #7: A "generic" 4xx error and "400" error (which is a generic 4xx error by definition) would likely be the same, so this avoids duplicating those methods. If you don't want them to be you can still verify the error code itself in the handler method and branch with an if statement if you really need to.

"400" error (which is a generic 4xx error by definition)

What do you mean by "generic" in this case? A 400 error means "The request could not be understood by the server due to malformed syntax". As opposed to, for example, a 403 error, which means "The server understood the request, but is refusing to fulfill it.". I don't see how "could not be understood" is a generic version of "understood, but". Those are semantically disjoint.

So, 4xx is a generic way of referring to 400 and 403, but 400 is not a generic way of referring to 403.

Maybe its really just me that 4xx has a slightly different meaning to 4xy :)

Well, the key is that https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4 refers to it as 4xx, so that's what gives it meaning.

A "generic" 4xx error and "400" error ... would likely be the same, so this avoids duplicating those methods.

Sure. If a particular subscriber doesn't need on400() and on4xx() to be different, then it can implement the latter and not the former.

That pesky randomly failing update test. Looks like concerns are addressed, back to RTBC.

Retitling per #27 also. I agree that 4XY might feel more familiar/accurate with algebraic notation, but I have never seen that before and so I keep thinking this issue is about a new JavaScript library or a men's body spray.

When @effulgentsia, @webchick, and I discussed this issue, we also wondered if this was too magical:

+++ b/core/lib/Drupal/Core/EventSubscriber/HttpExceptionSubscriberBase.php
@@ -90,6 +90,9 @@ public function onException(GetResponseForExceptionEvent $event) {
+      $method_fallback = 'on' . floor($exception->getStatusCode() / 100) . 'xx';

@@ -97,6 +100,9 @@ public function onException(GetResponseForExceptionEvent $event) {
+      elseif (method_exists($this, $method_fallback)) {
+        $this->$method_fallback($event);
+      }

I'm still a bit... unsure about that, but if @effulgentsia thinks it is okay I won't block the issue.

Thanks @dawehner and @neclimdul. Leaving at RTBC for a framework manager.

Never was a big fan of "4xy", happy to see "4xx" now, especially since the spec apparently also uses this.

Random fail.

Actually, we do need to add in-code documentation for this magical method API, explaining how to use it, and a CR for the removal of the statically named ones and the new ones. Thanks!

Good point.

The existing docs for the existing magic:

 * A subscriber may extend this class and implement getHandledFormats() to
 * indicate which request formats it will respond to. Then implement an on*()
 * method for any error code (HTTP response code) that should be handled. For
 * example, to handle 404 Not Found messages add a method:
 *
 * @code
 * public function on404(GetResponseForExceptionEvent $event) {}
 * @endcode

Now we need to document that you can also implement a catch-all public function on4xx.

Really want to get this in so apologies for stealing the novice issues but here is the CR. https://www.drupal.org/node/2776671

gah, well I guess I'm sort of in a writing mood this morning so we are lucky. ;) Docs in the interdiff.

You're totally right sorry.

And that's why we have tests. #2403307: RPC endpoints for user authentication: log in, check login status, log out added a on400 handler which merged cleanly but caused errors because the Symfony Response use was removed.

+ $method_fallback = 'on' . floor($exception->getStatusCode() / 100) . 'xx';

Floating point math? http://i.imgur.com/14pDVr1.gifv

+++ b/core/lib/Drupal/Core/EventSubscriber/HttpExceptionSubscriberBase.php
@@ -90,6 +98,9 @@ public function onException(GetResponseForExceptionEvent $event) {
+      $method_fallback = 'on' . strval($exception->getStatusCode())[0] . 'xx';

Hah, OK that's fair. Since strval adds a FCALL already, wouldn't substr() perform the same and be more readable though?

Same to me as long as you are not using floating point... sooner or later you will end up with the wrong method name when you don't expect it. Are you sure 400 / 100 is not 3.9999999999999999999999999995 deep inside?

Sounds good. There was one more "on4xx" handler added to JsonSubscriber I missed added by #2659070: REST requests without Content-Type header: unhelpful response significantly hinders DX, should receive a 415 response so here we go with one more update.

We might consider a follow up to use this in \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber which provides a big list of duplicated 4xx methods.

Sorry for the spam, but it's very funny to me that we collectively figured out 4 different ways to go from e.g. (int) 403 to (string) '4' in this issue:

1. (int) ($exception->getStatusCode() / 100)
2. floor($exception->getStatusCode() / 100)
3. strval($exception->getStatusCode())[0]
4. substr($exception->getStatusCode(), 0, 1)

I feel weirdly reminded of a first-semester computer science class. This is fun!

Oh, thanks to chx spurring the string approach I've got a few more if you're interested.

1. str_split($str)[0] if you don't like array syntax on strings.
2. sprintf("%.1s", $exception->getStatusCode())if you like obscure c-ish methods of doing things.
3. ((string) $exception->getStatusCode())[0] If you're only using php7 this is the probably the ugliest and definitely the fastest since it avoids all FCALLs! :-D

I nominate

$code = $exception->getStatusCode();
settype($code, 'string');
$code[0];

as well. Bonus points to @neclimdul for sprintf. I haven't thought of that one.

Oh and if we wanted to keep floating point we could do intval($exception->getStatusCode() / 100 + .0005) but we probably do not want to write obfuscated code...

#55: NICE, I never knew settype() existed!

CI broke? Its green now.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/HttpExceptionSubscriberBase.php
   @@ -90,6 +98,9 @@ public function onException(GetResponseForExceptionEvent $event) {
   +      // Keep just the leading number of the status code to produce either a
   +      // on400 or a 500 method callback.
   +      $method_fallback = 'on' . substr($exception->getStatusCode(), 0, 1) . 'xx';
   

   This is only necessary to do if the method doesn't exist - so let's only work out the fallback if on404 (or whatever) does not exist.

2. I think it is fine to do a fallback and on4xx() looks great to me.

3. +++ b/core/modules/rest/src/Tests/AuthTest.php
   @@ -34,7 +34,7 @@ public function testRead() {
   -    $this->assertRaw(json_encode(['message' => 'A fatal error occurred: No authentication credentials provided.']));
   +    $this->assertRaw(json_encode(['message' => 'No authentication credentials provided.']));
   

   Why is this change necessary?

Since the beta deadline has passed this has to go in 8.3.x now. Fortunately 8.3.x is open so this can get done soon.

Why update only \Drupal\Core\EventSubscriber\ExceptionJsonSubscriber?

Why not also update \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber?

Then we update all \Drupal\Core\EventSubscriber\HttpExceptionSubscriberBase subclasses that do generic 4xx error handling.

Also, thanks for reviving this issue! I totally lost track of it. I hadn't forgotten, but couldn't find it anymore!

Investigating.

Turns out RTBC'ing this at #64 (on December 12) didn't actually work, because the patch at #51 no longer applies to HEAD. Because AuthTest no longer exists. Hence the DrupalCI error.

If we look at that no-longer-applicable hunk, we see this:

index 9f4224f..1e570ac 100644
--- a/core/modules/rest/src/Tests/AuthTest.php
+++ b/core/modules/rest/src/Tests/AuthTest.php
@@ -34,7 +34,7 @@ public function testRead() {
     // Try to read the resource as an anonymous user, which should not work.
     $this->httpRequest($entity->urlInfo()->setRouteParameter('_format', $this->defaultFormat), 'GET');
     $this->assertResponse('401', 'HTTP response code is 401 when the request is not authenticated and the user is anonymous.');
-    $this->assertRaw(json_encode(['message' => 'A fatal error occurred: No authentication credentials provided.']));
+    $this->assertRaw(json_encode(['message' => 'No authentication credentials provided.']));
 
     // Ensure that cURL settings/headers aren't carried over to next request.
     unset($this->curlHandle);
 

If we compare that with the test failures of #67, we see this for Drupal\Tests\hal\Functional\EntityResource\Block\BlockHalJsonBasicAuthTest

Drupal\Tests\hal\Functional\EntityResource\Block\BlockHalJsonBasicAuthTest::testGet
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-No authentication credentials provided.
+{"message":"No authentication credentials provided."}

and this for Drupal\Tests\rest\Functional\EntityResource\Block\BlockJsonBasicAuthTest:

Drupal\Tests\rest\Functional\EntityResource\Block\BlockJsonBasicAuthTest::testGet
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"A fatal error occurred: No authentication credentials provided."}
+{"message":"No authentication credentials provided."}

The latter is exactly the same change as we had in AuthTest. The former is new, because of the additional changes in \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber added in #67.

So, let's revert the #67 interdiff for a moment, to prove that even the previous patch would've failed against HEAD (due to new test coverage).

#70 showed that even without the changes to \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber introduced in #67, we have failing tests. Specifically, JSON + Basic Auth tests, like this one:

Drupal\Tests\rest\Functional\EntityResource\Block\BlockJsonBasicAuthTest::testGet
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"A fatal error occurred: No authentication credentials provided."}
+{"message":"No authentication credentials provided."}

This peculiar error response format is something I already noticed while working on #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method. This is why I opened #2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json, to make all that consistent.

Root cause

The root cause is fairly simple: in HEAD, ExceptionJsonSubscriber does not handle 401 HTTP exceptions. So it ends up using \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber::onJson(), whose logic does this:

  protected function onJson(GetResponseForExceptionEvent $event) {
    $exception = $event->getException();
    $error = Error::decodeException($exception);

    // Display the message if the current error reporting level allows this type
    // of message to be displayed,
    $data = NULL;
    if (error_displayable($error) && $message = $exception->getMessage()) {
      $data = ['message' => sprintf('A fatal error occurred: %s', $message)];
    }

    $response = new JsonResponse($data, Response::HTTP_INTERNAL_SERVER_ERROR);
    if ($exception instanceof HttpExceptionInterface) {
      $response->setStatusCode($exception->getStatusCode());
      $response->headers->add($exception->getHeaders());
    }

    $event->setResponse($response);
  }

So that is where that peculiar A fatal error occurred: prefix comes from!

Why does this patch change things?

This patch (including the one in #51) causes \Drupal\Core\EventSubscriber\ExceptionJsonSubscriber to handle all 4xx HTTP exceptions, including 401. So, suddenly, it's not \Drupal\Core\EventSubscriber\DefaultExceptionSubscriber::onJson() generating the error response, but \Drupal\Core\EventSubscriber\ExceptionJsonSubscriber::on4xx(). And hence, the A fatal error occurred: prefix disappears.

So how to make the patch green?

I anticipated this in #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method. So I added a trait, JsonBasicAuthWorkaroundFor2805281Trait, to ensure that all JSON + Basic Auth tests could easily and consistently be updated once we'd fix this in #2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json. Turns out we're fixing it here. That's fine too of course.

This patch is relative to the one in #70.

#71 fixed all failing GET test coverage for all JSON + Basic Auth tests. But, now there's still failing tests in JSON test, for the POST and PATCH test coverage.

Unfortunately, this is actually blocked on #2813755: JSON responses encoded inconsistently: make them all RFC4627-compliant.

All remaining test failures in tests using the "JSON" format are failing like this:

1) Drupal\Tests\rest\Functional\EntityResource\Comment\CommentJsonAnonTest::testPost
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"Unprocessable Entity: validation failed.\nsubject: Subject: this field cannot hold more than 1 values.\n"}
+{"message":"Unprocessable Entity: validation failed.\nsubject: \u003Cem class=\u0022placeholder\u0022\u003ESubject\u003C\/em\u003E: this field cannot hold more than 1 values.\n"}

/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:572

2) Drupal\Tests\rest\Functional\EntityResource\Comment\CommentJsonAnonTest::testPatch
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-{"message":"Unprocessable Entity: validation failed.\nsubject: Subject: this field cannot hold more than 1 values.\n"}
+{"message":"Unprocessable Entity: validation failed.\nsubject: \u003Cem class=\u0022placeholder\u0022\u003ESubject\u003C\/em\u003E: this field cannot hold more than 1 values.\n"}

In other words: inconsistent encoding problems. #2813755: JSON responses encoded inconsistently: make them all RFC4627-compliant makes them consistent, and causes these tests to pass. So, postponing this issue.

#2813755: JSON responses encoded inconsistently: make them all RFC4627-compliant landed, now re-uploading #71 so we can see how that affects the number of fails here.

Interesting, we now have 37 instead of 35 fails. That's more, not less.

But, as expected, all the encoding-related failures have disappeared. Investigating now.

Apparently there are two random fails in Drupal\views\Tests\Update\ArgumentPlaceholderUpdatePathTest. That explains the increase in number of failing tests. But really, it's the same: 35 in #71, 35 in #75.
It's quite understandable the number of failures is the same: the REST-related tests now just are able to get further along in the test scenario until they fail.

Now investigating that.

First, we now observe a new interesting kind of failure in the test output:

Drupal\Tests\hal\Functional\EntityResource\Comment\CommentHalJsonAnonTest
fail: [Other] Line 0 of sites/default/files/simpletest/phpunit-941.xml:
PHPunit Test failed to complete; Error: PHPUnit 4.8.27 by Sebastian Bergmann and contributors.

..FF.

Time: 2.56 minutes, Memory: 3.50Mb

There were 2 failures:

1) Drupal\Tests\hal\Functional\EntityResource\Comment\CommentHalJsonAnonTest::testPost
Failed asserting that 500 is identical to 422.

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:316
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:344
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:568

2) Drupal\Tests\hal\Functional\EntityResource\Comment\CommentHalJsonAnonTest::testPatch
Failed asserting that 500 is identical to 422.

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:316
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:344
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php:760

500 responses instead of 422. That's bad.

The root cause is simple however: #2813755: JSON responses encoded inconsistently: make them all RFC4627-compliant added \Drupal\Core\EventSubscriber\ExceptionJsonSubscriber::on422(), but this issue specifically wants to let \Drupal\Core\EventSubscriber\ExceptionJsonSubscriber::on4xx() handle everything. The on422() referenced a constant in a class that's no longer imported via use, so it results in a fatal error, and hence a 500 response.

So, simply removing that on422() method will fix that.

Ahhh, down to 10 fails! #79 finally proved #71 to be true: all JSON + Basic Auth tests are green now! And we did it simply be removing the temporary work-around trait.

Except there's this one JSON + Basic Auth test that is still failing: RoleJsonBasicAuthTest. Apparently I forgot to use the trait there; it had the same logic as the trait but was never updated to use the trait. So, fixing that now.

This will bring it down to 9 failures, all of which are HAL+JSON + Basic Auth tests.

What remains now: updating \Drupal\serialization\EventSubscriber\DefaultExceptionSubscriber. Only then will have updated all HttpExceptionSubscriberBase subclasses.

This is largely the same interdiff as #67, except without the bug: the #67 interdiff converted all exceptions to a 400 response, this converts them to the appropriate 4xx response.

#82 will introduce zero extra failures. That's because all the REST test coverage only is testing the json and hal_json formats — because the xml format is broken by design (it's being removed in #2800873: Add XML GET REST test coverage, work around XML encoder quirks).

We can remove ExceptionHalJsonSubscriber, so that the hal_json format relies on DefaultExceptionSubscriber for its exception-to-error-responses-handling. But doing so would change the response content type from application/json to application/hal+json, which is an unrelated change and then also requires a bunch of unrelated changes in the test coverage. And actually, that's exactly what #2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json is determined to do. In fact, I've already added a patch at #2805281-10: ?_format=hal_json error responses are application/json, yet should be application/hal+json that applies cleanly after this is committed, which means this is now a blocker.

So, in this issue, I'm not removing ExceptionHalJsonSubscriber. The changes we made to ExceptionJsonSubscriber in all above patches are sufficient (because ExceptionHalJsonSubscriber extends ExceptionJsonSubscriber, and hence it inherited all changes). Hence this lets us remove the similar work-arounds for HAL+JSON + Basic Auth, just as described at the end of #71.

Failures are all across Drupal core, known random fails: #2828559-55: UpdatePathTestBase tests randomly failing. Retesting, back to NR.

Yes, I like this a lot too. Being able to use xx method names as a kind of catch all is nice! Best of both worlds.

It's green now!

HAHA, You know me too well. That snippet has been saved now :)

I do agree!

RANDOM FAIL STRIKES AGAIN.

Retesting…

Committed efb8f71 and pushed to 8.3.x. Thanks!

Nice to get this done at last - the fails on #83 are all unrelated random fails.

WOOOT! This unblocked #2805281: ?_format=hal_json error responses are application/json, yet should be application/hal+json. (Which in turn is blocking #2827084: EntityNormalizer::denormalize should not throw UnexpectedValueException, but \Symfony\Component\HttpKernel\Exception\UnprocessableEntityHttpException, so we get a 422 response instead of 400.)

Updated & published CR.