Problem/Motivation

We want to type hint the ResourceResponseInterface instead of ResourceResponse, so we can use polymorphism. This was uncovered by #2747259: [BUG] strict error.

Proposed resolution

Use the interface instead of the class name.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

e0ipso created an issue. See original summary.

e0ipso’s picture

Issue tags: +php-novice
gabesullice’s picture

Category: Task » Bug report

Just discovered this. This breaks compatibility with Drupal 8.1.

dawehner’s picture

@gabesullice
We could have a different method name than the parent class. This would allow us to not break stuff.

gabesullice’s picture

Looks like the ::renderResponse() method signature got updated again. Just fixing that for now. Not addressing 8.1 compatibility.

gabesullice’s picture

Status: Active » Needs review

kick tests.

gabesullice’s picture

Turns out there were far more changes in core than I realized, just going with a different method name as you suggested, @dawehner :)

  • dawehner committed 3ff97f6 on 8.x-1.x authored by gabesullice
    Issue #2751497 by gabesullice: [CLEANUP] Program to interfaces: use...
dawehner’s picture

Status: Needs review » Fixed
+++ b/src/RequestHandler.php
@@ -82,7 +82,7 @@ class RequestHandler extends RestRequestHandler {
     return $response instanceof ResourceResponse ?
-      $this->renderResponse($request, $response, $serializer, $format) :
+      $this->renderJsonApiResponse($request, $response, $serializer, $format) :
       $response;

@@ -110,7 +110,7 @@ class RequestHandler extends RestRequestHandler {
-  protected function renderResponse(Request $request, ResourceResponseInterface $response, SerializerInterface $serializer, $format) {
+  protected function renderJsonApiResponse(Request $request, ResourceResponseInterface $response, SerializerInterface $serializer, $format) {

+1

Thank you, just committed and pushed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

akoe’s picture

I'm having trouble with this patch Argument 2 passed to Drupal\jsonapi\RequestHandler::renderJsonApiResponse() must be an instance of Drupal\rest\ResourceResponseInterface, instance of Drupal\rest\ResourceResponse given, called in /var/www/html/web/modules/contrib/jsonapi/src/RequestHandler.php
Could this be related to a different core version? Im using 8.1.8 at the moment.

e0ipso’s picture

The minimum supported core version is 8.2.x. Sorry for the inconvenience, you can revert this patch to try to get support on 8.1.x.

Please report back if that worked.

akoe’s picture

@e0ipso you were right it works fine with core 8.2.x and reverting the patch solved the issue.