CacheableHttpException must pass a $headers argument to HttpException

Should we write a test to make sure we don't regress here? Not sure, because it's just a wrong implementation, but seeing as nothing broke in the tests it means that we're not testing this codepath.

These cacheable exceptions don't seem to have many test cases but there is one case we can extend to do this.

Oh, this is going to fail because it requires the Allow header to be set, but CacheableHttpException doesn't support this.

As HttpException accepts an array of headers I think CacheableHttpException should do the same. This is technically a BC break but as this method has never worked I think this is okay to change here? This means we can also get the new test to pass.

Going to add better test coverage instead of piggybacking off that existing test.

This patch fixes the issue originally reported and adds some basic test coverage for all the cacheable HTTP exception classes.

Changing the order of argument for the CacheableHttpException class will break BC.

We can freely add headers as 5th argument and still keep it as 4th argument in the constructor. Doing so, anybody using this as advised would get benifit of this fix. Not 100% sure what we prefer here as it would be a bit odd to mix args.

I realise this is a technical BC break but the current constructor just throws an error, so aren't we just allowed to fix it here? I don't see how anyone can be using it in its current state.

The most basic test case, on current 8.7.x:

>>> new \Drupal\Core\Http\Exception\CacheableHttpException(new \Drupal\Core\Cache\CacheableMetadata());
TypeError: Argument 4 passed to Symfony/Component/HttpKernel/Exception/HttpException::__construct() must be of the type array, integer given, called in /var/www/html/drupal/core/lib/Drupal/Core/Http/Exception/CacheableHttpException.php on line 21

I looked into this.

The exception was introduced 1 year ago and has not been changed since. Back when it was introduced it was breaking as well, meaning no one can use this - so BC should not be an issue.

Drupal has a wide range of specific cacheable http exceptions, like CacheableAccessDeniedHttpException, CacheableBadRequestHttpException etc. Common for all of these, is that the depend on specific symfony counterparts, like AccessDeniedHttpException, BadRequestHttpException etc. The generic HttpException is the only one which allows setting headers. Most likely some one created a bunch of these at the same time, made a c/p error and no one noticed the bug.

I have looked through the patch and the code looks good.

I would like for good measures that you make a test only patch from #10 patch as it's different from the #4 one, so we can validate that it indeed fail as expected.

When you have done this, I would say that this is RTBC. It's really great that you wrote a test for all HTTP exceptions, so we know these wont break again in the future.

Reuploaded #10 plus a test-only patch.

Excellent, assuming we will have a green and red test result, this is RTBC imo.

Excellent, thank you all!

Ha, interesting. Because both the parameters to the original exception and the child exception are wrong, the test case actually passes; the headers are passed through via the incorrect $code parameter. So, if we explicitly test the $code parameter this should actually fail/pass as expected.

This does demonstrate that it is technically possible to use this as-is in core, but you have to supply all arguments to the function with an empty array as $code if you want no headers, so I hope this is still okay to fix.

Funny that it actually works when you misuse the arguments in the constructor. I would still argue that this is a bug, and for this to work for anybody, that would most likely use the class the way it's meant to be used after the change and not the way it's currently meant to be used.

I'm going to repeat myself, the change looks good so if we get red + green we are RTBC.

Spurious fail in a JS test, back to RTBC

So yep this is fundamentally broken...

new \Drupal\Core\Http\Exception\CacheableHttpException(new \Drupal\Core\Cache\CacheableMetadata());
TypeError: Argument 4 passed to Symfony/Component/HttpKernel/Exception/HttpException::__construct() must be of the type array, integer given, called in /Users/alex/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Http/Exception/CacheableHttpException.php on line 21

So the question is shouldn't we remove it. AFAICS it has no usages.

Adding the issue that added the code to #2920001: Add cacheable HTTP exceptions: Symfony HTTP exceptions + Drupal cacheability metadata.

I've also been thinking about the wider issue of class structure here because whilst CacheableHttpException is very broken we have a chance to fix it. Atm CacheableAccessDeniedHttpException inherits from AccessDeniedHttpException inherits from HttpException. You might expect the class chain to look something like CacheableAccessDeniedHttpException -> CacheableHttpException -> HttpException but then instanceof AccessDeniedException won't work.

I wonder if #28 should be solved by:

• JSON:API implementing a CacheableNotImplementedHttpException and removing CacheableHttpException
• or renaming CacheableHttpException to CacheableGenericHttpException so it doesn't look so much like the base class it's not.

Needs to check that headers are required or not in context of #3224420-6: [PHP 8.1] Exception codes should be numeric and not NULL

Confirm that current jsonapi codebase (which came from contrib http://grep.xnddx.ru/search?text=CacheableHttpException) passing [] instead of $code which defined ?int $code = 0 in Symfony

#3224420: [PHP 8.1] Exception codes should be numeric and not NULL has landed.

re-roll

Fix calling code

+++ b/core/modules/jsonapi/src/Revisions/ResourceVersionRouteEnhancer.php
@@ -105,7 +105,7 @@ public function enhance(array $defaults, Request $request) {
+        throw new CacheableHttpException($cacheability, 501, $message, NULL);

@@ -151,7 +151,7 @@ public function enhance(array $defaults, Request $request) {
+        throw new CacheableHttpException($cacheability, 501, $message, NULL);

Should we remove the NULL as well, as it's the default value of $previous?

Good point! Thank you

1. +++ b/core/tests/Drupal/Tests/Core/Http/CacheableExceptionTest.php
   @@ -0,0 +1,80 @@
   +  /**
   +   * @dataProvider providerTestExceptions
   +   */
   +  public function testExceptions($status_code, $class, $argument = NULL, $expected_headers = []) {
   +    $cacheable_metadata = (new CacheableMetadata())->setCacheContexts(['route']);
   +    $message = "$class test message";
   +    if ($argument) {
   +      $exception = new $class($cacheable_metadata, $argument, $message, NULL, 123);
   +    }
   +    else {
   +      $exception = new $class($cacheable_metadata, $message, NULL, 123);
   +    }
   +    $this->assertSame(['route'], $exception->getCacheContexts());
   +    $this->assertSame($message, $exception->getMessage());
   +    $this->assertSame($status_code, $exception->getStatusCode());
   +    $this->assertSame($expected_headers, $exception->getHeaders());
   +    $this->assertSame(123, $exception->getCode());
   +  }
   +
   +  public function providerTestExceptions() {
   +    return [
   +      [400, CacheableBadRequestHttpException::class],
   +      [401, CacheableUnauthorizedHttpException::class, 'test challenge', ['WWW-Authenticate' => 'test challenge']],
   +      [403, CacheableAccessDeniedHttpException::class],
   +      [404, CacheableNotFoundHttpException::class],
   +      [405, CacheableMethodNotAllowedHttpException::Class, ['POST', 'PUT'], ['Allow' => 'POST, PUT']],
   +      [406, CacheableNotAcceptableHttpException::class],
   +      [409, CacheableConflictHttpException::class],
   +      [410, CacheableGoneHttpException::class],
   +      [411, CacheableLengthRequiredHttpException::class],
   +      [412, CacheablePreconditionFailedHttpException::class],
   +      [415, CacheableUnsupportedMediaTypeHttpException::class],
   +      [422, CacheableUnprocessableEntityHttpException::class],
   +      [428, CacheablePreconditionRequiredHttpException::class],
   +      [429, CacheableTooManyRequestsHttpException::class, 60, ['Retry-After' => 60]],
   +      [503, CacheableServiceUnavailableHttpException::class, 60, ['Retry-After' => 60]],
   +    ];
   +  }
   

   I do not understand why this code is added to this patch. This is out of scope for this issue. Could we move this to a followup?

2. +++ b/core/tests/Drupal/Tests/Core/Http/CacheableExceptionTest.php
   @@ -0,0 +1,80 @@
   +  public function testCacheableHttpException() {
   +    $exception = new CacheableHttpException((new CacheableMetadata())->setCacheContexts(['route']), 500, 'test message', NULL, ['X-Drupal-Exception' => 'Test'], 123);
   +    $this->assertSame(['route'], $exception->getCacheContexts());
   +    $this->assertSame(500, $exception->getStatusCode());
   +    $this->assertSame('test message', $exception->getMessage());
   +    $this->assertSame(['X-Drupal-Exception' => 'Test'], $exception->getHeaders());
   +    $this->assertSame(123, $exception->getCode());
   +  }
   

   Can we add a dataProvider method, so that we can test the exception with different kind of parameter values.

#43.1 there are no tests for any of these cacheable HTTP exceptions so to me it made sense to add tests all at once.

#43.2 what sort of values would you want in a data provider? The constructor we are testing is only two lines and the superclass belongs to Symfony; I am not sure what value there is in trying different arguments.

For #29: Maybe we should do that in a followup. We are here fixing a bug.

For #43.1: I still think that it is a bit out of scope, but is only adds testing. So lets add it.

For #43.2: I was a bit worried about calling the exception with its firth parameter being an integer. Only that change has been added in 9.3 and we are still in 9.3, therefor we can change it without it being a BC break.

The bug is fixed and testing has been added.
We can fix it in 9.3 without creating a BC break.
For me it is RTBC.

+++ b/core/modules/jsonapi/src/Revisions/ResourceVersionRouteEnhancer.php
@@ -105,7 +105,7 @@ public function enhance(array $defaults, Request $request) {
         $message .= ' For context, see https://www.drupal.org/project/drupal/issues/2992833#comment-12818258.';
         $message .= ' To contribute, see https://www.drupal.org/project/drupal/issues/2350939 and https://www.drupal.org/project/drupal/issues/2809177.';
-        throw new CacheableHttpException($cacheability, 501, $message, NULL, 0);
+        throw new CacheableHttpException($cacheability, 501, $message);
       }
       return $defaults;

What happens if contrib is doing this? Do we need a bc layer in the constructor for when $code is passed explicitly?

I do not think we need a BC layer:

1. Because the 5th parameter $code was not doing the $code thing, it was doing the $header thing. All contrib and custom code that is using the 5th parameter must be using it as the header thing. It is not possible to set the $code thing to the parent::__construct() method.
2. Because the following piece of code was added in 9.3 and therefor not used in production (#3224420: [PHP 8.1] Exception codes should be numeric and not NULL):

   if (is_array($code)) {
     parent::__construct($statusCode, $message, $previous, $code);
   }
   else {
     parent::__construct($statusCode, $message, $previous, [], $code);
   }		
   

   We can change this in 9.3 as long as 9.3.0 is not released, without it being a BC break.

Both good points.

Committed 5cd5a9d and pushed to 9.3.x. Thanks!