dingo / api

A RESTful API package for the Laravel and Lumen frameworks.
BSD 3-Clause "New" or "Revised" License
9.32k stars 1.25k forks source link

Enable default response transformation with bound transformers #1711

Closed xwiz closed 4 years ago

xwiz commented 4 years ago

Using $this->response->paginator or $this->response->collection currently requires user to specify transformer binding which may already be registered according to the documentation.

This enhancement enables dingo to use the default resolution method to resolve and return transformed data.

specialtactics commented 4 years ago

Thanks @xwiz, only thing is I'm a bit worried that people will use this without a bound transformer, get an error, and be unsure as to why. Can you add a check there that if there is no bound transformer found, it should throw an exception saying that no transformer specified nor bound.

xwiz commented 4 years ago

@specialtactics Yes I noticed this but forgot. Will do, thanks .

xwiz commented 4 years ago

So just to be sure I'm in line, currently Transformer getBinding throws an error saying:

"no transformer specified nor bound."

currently. Is this enough? Another approach could be to return null if there is no binding for the class and then throw an appropriate message that says:

"Please specify a transformer or bind transformer in routes"...

specialtactics commented 4 years ago

I think the second option is better.

xwiz commented 4 years ago

@specialtactics OK so apparently, throwing an error if binding is null causes 5 of the tests to throw errors: testMakingCollectionRegistersUnderlyingClassWithTransformer, testMakingCollectionResponseWithThreeParameters along with the collection and paginator examples. Is this expected coz I'm not sure I understand how the tests bind the transformers.

xwiz commented 4 years ago

Alternatively one can just update the error message in the current approach to cater for the new scenario.

specialtactics commented 4 years ago

@xwiz Yeh, it seems like it will not be a nice solution to go around changing those things, let's just merge for now.

specialtactics commented 4 years ago

Tagged in 3.0.0 final

xwiz commented 4 years ago

Nice one @specialtactics

Meanwhile maybe the Wiki should be updated? (not familiar with how to create PR for it):

Responding With A Collection Of Items.

class UserController extends BaseController
{
    public function index()
    {
        $users = User::all();

        return $this->response->collection($users, new UserTransformer);
    }
}

Responding With Paginated Items

class UserController extends BaseController
{
    public function index()
    {
        $users = User::paginate(env('PAGINATE_COUNT', 25));

        return $this->response->paginator($users, new UserTransformer);
    }
}

The transformer parameter may be omitted if already bound/registered.

specialtactics commented 4 years ago

@xwiz I updated it in the registering a transformer for a class section with the rest of that info.

xwiz commented 4 years ago

Thank you @specialtactics Yet to see in the wiki though.

Meanwhile using this with a new project I realized something weird. In the PR I created, the transformer parameter is not set to null by default. So as it is now it's not allowing the transformer to be omitted for item and paginated response though it correctly checks for null in the method itself.

specialtactics commented 3 years ago

I am not sure what you mean on the wiki? If you are talking about pagination, it has actually always been documented here (even before this): https://github.com/dingo/api/wiki/Responses