Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tests to support PHP 8.1 #29

Closed
stephen-cox opened this issue Apr 12, 2022 · 4 comments · Fixed by #32
Closed

Fix tests to support PHP 8.1 #29

stephen-cox opened this issue Apr 12, 2022 · 4 comments · Fixed by #32

Comments

@stephen-cox
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member

ekes commented Apr 22, 2022

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
Copy link
Member Author

stephen-cox commented Apr 22, 2022

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
Copy link
Member

ekes commented Apr 22, 2022

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

@ekes ekes closed this as completed in #32 Apr 25, 2022
ekes added a commit that referenced this issue Apr 25, 2022
…ent-type

Add an error response event handler to set correct Content-Type header #29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants