localgovdrupal / localgov_openreferral

Open Referral integration
0 stars 1 forks source link

Fix tests to support PHP 8.1 #29

Closed stephen-cox closed 2 years ago

stephen-cox commented 2 years ago

The following tests fail when using PHP 8.1

Resource (Drupal\Tests\localgov_openreferral\Functional\Resource)
 ✘ Unpublished access  39616 ms
   ┐
   ├ Exception: Deprecated function: stripos(): Passing null to parameter #1 ($haystack) of type string is deprecated
   ├ Drupal\Core\EventSubscriber\ActiveLinkResponseFilter->onResponse()() (Line: 82)                                 
   │
   ╵ /app/web/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:49
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:204
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:153
   ╵ /app/vendor/guzzlehttp/promises/src/TaskQueue.php:48
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:248
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:224
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:269
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:226
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:62
   ╵ /app/vendor/guzzlehttp/guzzle/src/Client.php:182
   ╵ /app/web/core/tests/Drupal/Tests/DrupalTestBrowser.php:137
   ╵ /app/vendor/symfony/browser-kit/Client.php:404
   ╵ /app/vendor/friends-of-behat/mink-browserkit-driver/src/BrowserKitDriver.php:145
   ╵ /app/vendor/behat/mink/src/Session.php:148
   ╵ /app/web/core/tests/Drupal/Tests/UiHelperTrait.php:334
   ╵ /app/web/modules/contrib/localgov_openreferral/tests/src/Functional/ResourceTest.php:197
   ╵ /app/vendor/phpunit/phpunit/src/Framework/TestResult.php:726
   ┴

 ✘ Permissions  40898 ms
   ┐
   ├ Exception: Deprecated function: stripos(): Passing null to parameter #1 ($haystack) of type string is deprecated
   ├ Drupal\Core\EventSubscriber\ActiveLinkResponseFilter->onResponse()() (Line: 82)                                 
   │
   ╵ /app/web/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:49
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:204
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:153
   ╵ /app/vendor/guzzlehttp/promises/src/TaskQueue.php:48
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:248
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:224
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:269
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:226
   ╵ /app/vendor/guzzlehttp/promises/src/Promise.php:62
   ╵ /app/vendor/guzzlehttp/guzzle/src/Client.php:182
   ╵ /app/web/core/tests/Drupal/Tests/DrupalTestBrowser.php:137
   ╵ /app/vendor/symfony/browser-kit/Client.php:404
   ╵ /app/vendor/friends-of-behat/mink-browserkit-driver/src/BrowserKitDriver.php:145
   ╵ /app/vendor/behat/mink/src/Session.php:148
   ╵ /app/web/core/tests/Drupal/Tests/UiHelperTrait.php:334
   ╵ /app/web/modules/contrib/localgov_openreferral/tests/src/Functional/ResourceTest.php:316
   ╵ /app/vendor/phpunit/phpunit/src/Framework/TestResult.php:726
stephen-cox commented 2 years ago

@ekes @finnlewis Just looking at fixing these test failures.

The error is thrown because there's no 'Content-Type' header when the check is made here https://git.drupalcode.org/project/drupal/-/blob/9.4.x/core/lib/Drupal/Core/EventSubscriber/ActiveLinkResponseFilter.php#L82

This only effects unpublished nodes and looks to be caused by the entity access checks in the routing YAML file. Specifically:

  requirements:
    _format: 'openreferral_json'
    _openreferral_type: 'entity:organization'
    _entity_access: 'entity.view'

Removing _entity_access: 'entity.view' causes the test to fail because it's now possible to access the route. This error is not triggered because the Content-Type header exists.

Changing the _format from openrefferral_json to just json allows these tests to pass, but others then fail.

This looks like it might be an issue with how core handles custom encoder types, but I'm having trouble tracking down the cause. I'm still looking but any thoughts / insights into this would be appreciated.

ekes commented 2 years ago

So when is the Content-Type header being added when it's json? Are we in fact serving it without a Content-Type: application/json?

stephen-cox commented 2 years ago

Some progress, although not a solution. It appears that Drupal core Serialization module doesn't like the openreferral_json request format when handling 4xx errors.

When trying to access the unpublished page a CacheableAccessDeniedHttpException is thrown, which is caught and then used to generate a Symfony\Component\HttpFoundation\Response. This Response object is created in the Drupal\serialization\EventSubscriber->on4xx() method.

The Content-Type header is set by looking up the Symfony\Component\HttpFoundation\Request object's format (openreferral_json) in the Request objects list of formats. Which is set in the Request objects initializeFormats() static method as

static::$formats = [
  'html' => ['text/html', 'application/xhtml+xml'],
  'txt' => ['text/plain'],
  'js' => ['application/javascript', 'application/x-javascript', 'text/javascript'],
  'css' => ['text/css'],
  'json' => ['application/json', 'application/x-json'],
  'jsonld' => ['application/ld+json'],
  'xml' => ['text/xml', 'application/xml', 'application/x-xml'],
  'rdf' => ['application/rdf+xml'],
  'atom' => ['application/atom+xml'],
  'rss' => ['application/rss+xml'],
 'form' => ['application/x-www-form-urlencoded'],
];

So it looks to me like we need to add the openreferral_json to the list of formats. But I have run out of time for this today.

Any thoughts / fixes on this appreciated as it's turning into a time sink. Will return to it next week

ekes commented 2 years ago

Any thoughts / fixes on this appreciated as it's turning into a time sink.

Pretty much answered it. That was all the hard work.

A quick check in jsonapi, which also adds a _format, and it does indeed implement a error repsonse subscriber. It's more involved and returns one of it's response objects. We can be simpler still as there's no standard, but we set the header. So something like #32