Confusing error response set by ContentTypeHeaderMatcher when forgetting the Content-Type request header

1. +++ b/core/tests/Drupal/Tests/Core/Routing/ContentTypeHeaderMatcherTest.php
   @@ -101,4 +102,21 @@ public function testNoRouteFound() {
   +   * Confirms that the matcher throws an exception for missing request header.
   +   *
   +   * @expectedException \Symfony\Component\HttpKernel\Exception\UnsupportedMediaTypeHttpException
   +   * @expectedExceptionMessage No "Content-Type" request header specified.
   

   We use setExpectedException() now, see https://thephp.cc/news/2016/02/questioning-phpunit-best-practices. Mh ideally we would have a @covers here

2. +++ b/core/tests/Drupal/Tests/Core/Routing/ContentTypeHeaderMatcherTest.php
   @@ -101,4 +102,21 @@ public function testNoRouteFound() {
   +    $this->fail('No exception was thrown.');
   

   We really don't need this with set expected exception

Well, feel free to choose :) I just don't believe in adding bad new code. I know you value consistency more

re #5
1) Is new. Megh but it is the new way. If you're going to do it just put it right before the method going to throw the exception so you test only the method you're testing not some other code that might have thrown the exception.

2) Well ContentTypeHeaderMatcherTest is wrong then. fail() will bypass the expected exception expectations because it runs after the test method finishes. This means the failure message doesn't include the exception type unless it throws the wrong exception type.

1) Is new. Megh but it is the new way. If you're going to do it just put it right before the method going to throw the exception so you test only the method you're testing not some other code that might have thrown the exception.

Yeah exactly, its also describing for users where the exception is thrown. All in all just improvements.

all of that is just copy/pasted from the existing test at

Everytime someone comes up with that, similar (it would not be worth to test), its suspicious :P

Is this what we're looking for?

Also writing this I found it interesting that one of these 2 exception messages has a period and one doesn't. I don't actually know what our standard is for such things.

I personally don't care about the dots :) The discussion might better end up in its own issue + a fix for all the exception messages.

Committed 97f0d43 and pushed to 8.3.x. Thanks!

Also I think this is actually a bug and should be backported to 8.2.x - but I'll leave that to those on the issue to decide.

diff --git a/core/lib/Drupal/Core/Routing/ContentTypeHeaderMatcher.php b/core/lib/Drupal/Core/Routing/ContentTypeHeaderMatcher.php
index 973037b..13e18f3 100644
--- a/core/lib/Drupal/Core/Routing/ContentTypeHeaderMatcher.php
+++ b/core/lib/Drupal/Core/Routing/ContentTypeHeaderMatcher.php
@@ -43,7 +43,7 @@ public function filter(RouteCollection $collection, Request $request) {
     // \Symfony\Component\Routing\Exception\ResourceNotFoundException here
     // because we don't want to return a 404 status code, but rather a 415.
     if (!$request->headers->has('Content-Type')) {
-      throw new UnsupportedMediaTypeHttpException('No "Content-Type" request header specified.');
+      throw new UnsupportedMediaTypeHttpException('No "Content-Type" request header specified');
     }
     else {
       throw new UnsupportedMediaTypeHttpException('No route found that matches "Content-Type: ' . $request->headers->get('Content-Type') . '"');

Remove dot on commit for consistency. Also we've already discussed and agreed no dots.

This broke 8.3.x tests: https://www.drupal.org/pift-ci-job/503665

Rolled back for now. Late here so didn't look into why.

For me, it's the period, but weird.

Oh, it's simple. #16 removed the period from the logic but not from the test.

That'd do it!

Okay alexpott--

As per #16

Also I think this is actually a bug and should be backported to 8.2.x - but I'll leave that to those on the issue to decide.

Committed 8bbdbed and pushed to 8.3.x. Thanks!

Discussed with a @catch and we both agreed this is fine for 8.2.x

Committed 4d97701 and pushed to 8.2.x. Thanks!