In the JsonApiParamEnhancer::enhance(), when it sees a 'page' argument, it is not passed to the OffsetPageNormalizer properly as an array. If 'page' is not found, an empty array is passed, but if 'page' IS found it is passed incorrectly as a simple value.

This causes a very obscure problem. When you create a View Block and use Page Manager to place this block on a page, the paginator of the view stops working. This is because Page Manager is looping through all the route enhancers but is trapping any exceptions. Using xdebug I discovered the exception "The page parameter needs to be an array.".

Not sure how to test this in a simpler site setup, but looking at the code the error seems pretty obvious. I'll leave it to maintainers to decide if this needs a specific test, but for now will post the simple patch so I can get my site working again.

NOTE: This *used* to work. Not sure exactly when it broke, but likely during the 8.5.x upgrade process. But not familiar enough with JSONAPI to know exactly what has changed. We are using Lightning distro (which depends on jsonapi) and it uses jsonapi 1.14, but I still see this error in the latest dev branch.

CommentFileSizeAuthor
#14 2961773-13.patch9.97 KBgabesullice
#13 2961773-13.txt9.97 KBgabesullice
#13 interdiff-10-13.txt6.5 KBgabesullice
#10 interdiff-7-10.txt2.92 KBgabesullice
#10 2961773-10.patch5 KBgabesullice
#7 2961773-7.patch4.01 KBgabesullice
#6 2961773-6.patch559 bytesmpotter
#2 2961773-2.patch692 bytesmpotter

Comments

mpotter created an issue. See original summary.

mpotter’s picture

mpotter commented
StatusFileSize
new 2961773-2.patch692 bytes

Here is the patch.

mpotter’s picture

mpotter commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: 2961773-2.patch, failed testing. View results

mpotter’s picture

mpotter commented

Looking at the failing test, is jsonapi expecting a different sort of "page" argument? Even looking at the core Content view, the paginator passes a simple "page=2" url argument. But looking at the test it looks like jsonapi is passing a page array with "offset" and "limit" as a post.

Sounds like I need a maintainer to look at this.

mpotter’s picture

mpotter commented
Status: Needs work » Needs review
StatusFileSize
new 2961773-6.patch559 bytes

Here is a patch version that checks to see if the 'page' argument is already an array. This should pass the tests hopefully.

gabesullice’s picture

gabesullice
Primary language English
commented
Title: JsonApiParamEnhancer not handling Page value as array » JsonApiParamEnhancer interacts with non-JSON API requests.
Issue tags: +API-First Initiative
StatusFileSize
new 2961773-7.patch4.01 KB

Hi @mpotter! Thanks for the excellent investigation and report :) That was very clear and helped me track the problem down easily.

I think the issue here is a complicated interaction between Symfony, Page Manager and JSON API.

Symfony deprecated the RouteEnhancerInterface which had an applies() method on it. When that returned FALSE, the enhancer wouldn't be executed. The new version, EnhancerInterface, removes that method and simply expects the enhancer to "do nothing" if it doesn't apply so it will still call the enhancer regardless of its applicability.

Page Manager is using the new interface and not calling the applies() method to filter out our enhancer. This is a bug in Page Manager I believe, since RouteEnhancerInterface will be around throughout the lifetime of D8.

However, we might as well fix our code now to use the new interface and be forward compatible rather than sticking with its deprecated counterpart.

This should fix the issue for you as a result 🎉

Would you mind testing it out on your set-up?

Note to future reviewers: No interdiff provided because it's an unrelated patch.

gabesullice’s picture

gabesullice
Primary language English
commented
Related issues: +#2953397: Provide BC layer for RouteEnhancerInterface

Whaddayaknow! It's already been fixed in the latest Page Manager version: #2953397: Provide BC layer for RouteEnhancerInterface

Status: Needs review » Needs work

The last submitted patch, 7: 2961773-7.patch, failed testing. View results

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 2961773-10.patch5 KB
new interdiff-7-10.txt2.92 KB

Whoops! Of course the defaults aren't on the request object yet, they're still being determined. This should fix it

Status: Needs review » Needs work

The last submitted patch, 10: 2961773-10.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mpotter’s picture

mpotter commented

Thanks for the details on this...it explains why things suddenly broke during the 8.5 core update. Never thought to look for changes in Symfony. Glad to see it fixed in Page Manager and looks like you are close to having the proper fix here also (I figured it had to be more complicated!)

gabesullice’s picture

gabesullice
Primary language English
commented
Title: JsonApiParamEnhancer interacts with non-JSON API requests. » Update ParamEnhancers to not use deprecated interface to prevent unintentional interactions with requests
Status: Needs work » Needs review
StatusFileSize
new interdiff-10-13.txt6.5 KB
new 2961773-13.txt9.97 KB

Maybe if I don't say, "this fixes it". This will fix it 😅

gabesullice’s picture

gabesullice
Primary language English
commented
StatusFileSize
new 2961773-13.patch9.97 KB

This is embarrassing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2957271: [>=8.5] Fix RouteEnhancerInterface deprecation errors 
+++ b/src/Routing/JsonApiParamEnhancer.php
@@ -16,7 +15,7 @@ use Symfony\Component\Serializer\Normalizer\DenormalizerInterface;
-class JsonApiParamEnhancer implements RouteEnhancerInterface {
+class JsonApiParamEnhancer implements EnhancerInterface {

We can't do this because EnhancerInterface exists only in 8.5.x.

I already created an issue for this several weeks ago: #2957271: [>=8.5] Fix RouteEnhancerInterface deprecation errors.

That issue is an 8.x-2.x-only issue. Because that branch will require >=8.5.0.

#8 says this is already fixed in Page Manager. Is this then still necessary?

gabesullice’s picture

gabesullice
Primary language English
commented
Status: Needs review » Closed (duplicate)

We can't do this because EnhancerInterface exists only in 8.5.x.

Shoot!

I already created an issue for this several weeks ago

Of course you did :) Looking at the patch though, it doesn't do anything about applies no longer applying (pun intended ;)). I'll move that work to your issue.

#8 says this is already fixed in Page Manager. Is this then still necessary?

The fix to applies will be.