egeriis / laravel-jsonapi

Make it a breeze to create a jsonapi.org compliant APIs with Laravel 5.
MIT License
146 stars 27 forks source link

Sorting can result in invalid links from paginator #41

Closed mfn closed 9 years ago

mfn commented 9 years ago

From \EchoIt\JsonApi\Handler::handlePaginationRequest():

        if (!empty($request->sort)) {
            $paginator->appends('sort', implode(',', $request->sort));
        }

In case sorting is done via e.g. +column (needs to be encoded to %2Bcolumn in an actual request), $paginator->appends gets called with +column which will end up in the generate url with sort= column (note the space).

It seems encoding is applied again and thus needs to be escaped first.

Unfortunately \Illuminate\Pagination\AbstractPaginator::addQuery() is sparse on what the expectence is.

But in the end the query ends up in \Illuminate\Pagination\AbstractPaginator::url() which creates $parameters, containing the query and thus the sort, and runs it through urldecode(http_build_query($parameters)).

It seems the following fixes it, but I'm not sure about this approach:

--- a/vendor/echo-it/laravel-jsonapi/src/EchoIt/JsonApi/Handler.php
+++ b/vendor/echo-it/laravel-jsonapi/src/EchoIt/JsonApi/Handler.php
@@ -468,7 +468,7 @@ abstract class Handler
             }
         }
         if (!empty($request->sort)) {
-            $paginator->appends('sort', implode(',', $request->sort));
+            $paginator->appends('sort', urlencode(implode(',', $request->sort)));
         }

         return $paginator;

Could be an oversight in the Paginator implementation?

egeriis commented 9 years ago

Seems like a hack to add urlencode. I didn't implement the pagination, so I'll have to take a brief look :)

egeriis commented 9 years ago

I think #44 will solve this?

mfn commented 9 years ago

I think so too; didn't test the code but it makes sense. No +, no problem :-)