Fix sortable method in APIHandler class

marcelgwerder / laravel-api-handler

Package providing helper functions for a Laravel REST-API

Other

152 stars 45 forks source link

Fix sortable method in APIHandler class #55

Closed sixty-nine closed 5 years ago

sixty-nine commented 5 years ago

Fix #54

Make the APIHandler::sortable() method work.

marcelgwerder commented 5 years ago

Hey, It's been a while but I think the sortable function is probably old code. I changed that for all the other config options and handle it in the __call : https://github.com/marcelgwerder/laravel-api-handler/blob/next/src/ApiHandler.php#L503

Try whether just removing the method fixes the problem.

sixty-nine commented 5 years ago

Hey Marcel, thank you for your reply :)

If I simply remove the function then indeed the __call method works. But then it will fail if you pass an array of params (see error in the issue).

With the code of this PR everything works.

But now I wonder if the problem with the __call method is that I don't pass the params the right way..

->sortable(['col1', 'col2']) does not work

But I didn't try ->sortable('col1', 'col2'). Might actually be the problem. Will try that tomorrow.

marcelgwerder commented 5 years ago

Ups, missed to read the linked issue, yeah we probably have to extend the call function to:

Check if the config type is an array, if yes allow passing a single array or a list of parameters
If it is not an array, append instead of overriding the options to allow multiple calls with single additions. (I still think passing an array should override and not be merged)

And yeah the params are supposed to be passed either as an array or individually, that is how Laravel does it for many of its APIs so I thought I follow that path.

sixty-nine commented 5 years ago

So indeed I was wrong, the right way to define multiple sortable columns is:

        ->sortable('id', 'type', 'recipient', 'created_at')

This will work as long as you remove the legacy definition of the "sortable" method.

Thanks for the hints.

This PR has been adapted so that it just removes the legacy sortable() method.

This does not implement more argument interpretation as suggested in your comment, it's just the good old feature you implemented, i.e. either there is a single string parameter or multiple coma separated string params.

As far as I can see any subsequent call to ->sortable() will overwrite the previous setting.

As it is now it works perfectly for our needs.

sixty-nine commented 5 years ago

Thank your for the merge. I switched back my project to your "next" branch but for some reason this composer line will always get the version before the PR merge:

    "marcelgwerder/laravel-api-handler": "dev-next",

Whatever I do (remove the vendor folder, remove the composer.lock), it will always install the commit "687c456457f92a8ec73940c07bf8b66f6a5ef444" (just before my PR) instead of the latest one "bf4b2c6b4470086b29e22d95cbab2fe2f741ef75".

Is there some tagging work to do on your side?