I was playing around with the JSON API module and Ember, and I came across this problem.
I wanted to load a single node by nid, so I call this Ember Data fetch (after doing the work to make the two talk to each other):
store.queryRecord('node--page', { filter: { nid: 1 } });
This causes a request to the Drupal backend, using the URL http://development.site/jsonapi/node/page?_format=api_json&filter%5Bnid%5D=1 – which fails, and Drupal gives this traceback in return:
The website encountered an unexpected error. Please try again later.
Recoverable fatal error: Unexpected PHP error: Argument 2 passed to Drupal\jsonapi\Routing\Param\Filter::expandItem() must be of the type array, string given, called in /Users/mikl/Work/Drupal/jaeger/web/modules/contrib/jsonapi/src/Routing/Param/Filter.php on line 76 and defined in Drupal\jsonapi\Error\ErrorHandler::handle() (line 44 of modules/contrib/jsonapi/src/Error/ErrorHandler.php).Drupal\jsonapi\Error\ErrorHandler::handle(4096, 'Argument 2 passed to Drupal\jsonapi\Routing\Param\Filter::expandItem() must be of the type array, string given, called in /Users/mikl/Work/Drupal/jaeger/web/modules/contrib/jsonapi/src/Routing/Param/Filter.php on line 76 and defined', '/Users/mikl/Work/Drupal/jaeger/web/modules/contrib/jsonapi/src/Routing/Param/Filter.php', 98, Array) (Line: 98) Drupal\jsonapi\Routing\Param\Filter->expandItem('nid', '1') (Line: 76) Drupal\jsonapi\Routing\Param\Filter->expand() (Line: 39) Drupal\jsonapi\Routing\Param\JsonApiParamBase->get() (Line: 160) Drupal\jsonapi\Query\QueryBuilder->configureFilter(Object) (Line: 136) Drupal\jsonapi\Query\QueryBuilder->configureParam('filter', Object) (Line: 112) Drupal\jsonapi\Query\QueryBuilder->configureFromContext(Array) (Line: 81) Drupal\jsonapi\Query\QueryBuilder->newQuery(Object, Array) (Line: 548) Drupal\jsonapi\Controller\EntityResource->getCollectionQuery('node', Array) (Line: 254) Drupal\jsonapi\Controller\EntityResource->getCollection(Object) call_user_func_array(Array, Array) (Line: 98) Drupal\jsonapi\Controller\RequestHandler->Drupal\jsonapi\Controller\{closure}() (Line: 574) Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 99) Drupal\jsonapi\Controller\RequestHandler->handle(Object, Object) call_user_func_array(Array, Array) (Line: 123) Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 574) Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124) Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97) Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() call_user_func_array(Object, Array) (Line: 144) Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64) Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 51) Asm89\Stack\Cors->handle(Object, 1, 1) (Line: 57) Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47) Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 207) Drupal\page_cache\StackMiddleware\PageCache->fetch(Object, 1, 1) (Line: 121) Drupal\page_cache\StackMiddleware\PageCache->lookup(Object, 1, 1) (Line: 75) Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47) Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50) Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23) Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 652) Drupal\Core\DrupalKernel->handle(Object) (Line: 19)
Obviously, something goes off the rails with the argument handling, but I wasn't able to come up with an immediate fix, but I hope this report is useful none the less.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | jsonapi-add-test-coverage-to-filters-2852259-12.patch | 7.13 KB | spleshka |
Comments
Comment #2
miklComment #3
dawehnerI had basically the same issue while working on #2852272: Make it possible to call jsonapi from within templates
I'm investigating this now.
Comment #4
dawehnerWhat I figured out, it works when you specify more than one
vs.
Comment #5
dawehnerI put a breakpoint into
ParameterBag->get()so the particular piece of code which involved to fetch the 'fields' query parameter.In case
'title,nid'is in the URL it contains:In case just the title is specified it looks like the following:
... and now after some heisen-debugging, it works. "Nice"
Comment #6
miklHmm, so a bug in ParameterBag. I apparently didn't go deep enough, good find.
Comment #7
dawehnerYeah I'm not entirely sure where the bug is though to be honest.
Comment #8
wim leersComment #9
e0ipsoComment #10
spleshkaAssigning to myself, will provide the first patch soon.
Comment #11
wim leers<3
Comment #12
spleshkaLanding the first patch. A couple of thoughts and notes:
1. I haven't added much tests to the current patch, there's mostly clean up. The reason for this is simple: the current
\Drupal\jsonapi\Routing\Param\Filterjust expands shorthand requests likefilter[0][path]=foo&filter[0][value]=barinto more verbose output (something like[0 => ['condition' => ['path' => 'foo', 'value' => 'bar', 'operator' => '=']]], which eventually can be used byDrupal\jsonapi\Query/QueryBuilderto build an entity query. This case is now thoroughly covered by Unit tests.2. Do we want to cover the whole filtering process with Kernel tests? As mentioned above, the current job for the
\Drupal\jsonapi\Routing\Param\Filterclass is just about kind of "internal normalization". What would make more sense is to cover the whole filtering operation by Kernel tests, not just the class mentioned in the issue title.3. The only proper way to cover
\Drupal\jsonapi\Routing\Param\Filterwith Unit tests is to provide comprehensive filter params validation first (#2878654: [FEATURE] Implement validation of filter params) and then enhance the Unit tests. At the moment we can't cover with Unit tests advanced filtering offilter[condition]orfilter[group]groups, because the current code simply bypasses these request to theDrupal\jsonapi\Query/QueryBuilderwithout validation or any modifications.4. The reason I push for Unit tests now and for Kernel tests later is performance. As soon as we know that
\Drupal\jsonapi\Routing\Param\Filterwill return only valid options for entity query, Kernel tests become more nice to have option rather than the only way to cover filtering.Having this all said, the proposed action plan is:
1. If no objections, treat the current patch as clean-up & comprehensive test coverage by Unit tests of the current implementation of
\Drupal\jsonapi\Routing\Param\Filter, get the patch committed & close the current issue.2. Create a follow-up to cover the whole filtering process (not just the
\Drupal\jsonapi\Routing\Param\Filterclass) by Kernel tests.3. Act on #2878654: [FEATURE] Implement validation of filter params. There we will be able to provide more comprehensive Unit test coverage, which will include all possible filtering options, groups and exceptions.
4. Act on the follow-up created in #2.
Comment #14
spleshkaTests fail due to the issue with d.org test bot. Awaiting for resolution.
Comment #15
e0ipsoI don't see any new functionality in this patch, just some comment max length changes an dropping some unnecessary parameters. Is that correct?
I think that this proves that the current coverage is good.
I'm keeping this opened and blocked until #2871499: Support comma-separated and plus-separated multi-value filters and #2878654: [FEATURE] Implement validation of filter params land, so we can test those features in this patch.
Comment #16
e0ipso@Spleshka, I do not think we need other coverage using Kernel tests, unless there's something you want to test that is easier with kernel testing.
Comment #17
e0ipsoOn second thought as soon as tests are green I can merge the current patch. Testing of those features should happen in those patches.
Comment #18
spleshkaYep, this is correct. I haven't found any new tests which would make sense to add to cover the existing functionality.
Gotcha. It will certainly be easier to cover this with Unit tests.
Completely agree here.
The current dev branch for PHP7 fails for the module in general: https://www.drupal.org/node/2723491/qa. I've created a separate issue to resolve it #2880987: Fix failing tests with PHP7 / D8.4.x in the dev branch
Comment #19
spleshkaRe-sent #12 for testing against php7 / drupal 8.4.x.
Comment #20
spleshkaNow they pass, great. Awaiting for manual final review.
Comment #22
e0ipsoThanks for the cleanup @Spleshka. Also thanks for determining that we don't need extra coverage for that class.
Comment #23
spleshkaWelcome. Though we certainly need to implement a good filters validation :)