WIP v2

[zackify]

Changes

• Remove resource keys if not paginated
• Parse and eager load in the background

Here's my proposed syntax changes:

From this:

$includes = $this->transformer->getEagerLoads($this->fractal->getRequestedIncludes());
$courses = $this->eagerLoadIncludes($this->courseModel, $includes);
$courses = $this->applyParameters($courses, $request->query);

return $this->respondWithPaginatedCollection($courses, $this->transformer);

to this

return $this->respondWithPaginatedCollection();

All that is required is setting the model and transformer in your constructor:

public function __construct(Course $courseModel, CourseTransformer $transformer)
    {
        parent::__construct();
        $this->transformer = $transformer;
        $this->model = $courseModel;
    }

Need custom logic like searching for filter by a company id? Just pass in the modified builder:

if($request->company_id) $model = $model->where('tc_id', '=', $request->company_id);
if($request->search) $model = $model->where('title', 'ILIKE', '%' . $request->search . '%');

$model = $model->groupBy('id');

return $this->respondWithPaginatedCollection($model);

Respond with item:

From:

 $includes = $this->transformer->getEagerLoads($this->fractal->getRequestedIncludes());
 $courses = $this->eagerLoadIncludes($this->model, $includes);
 $course = $courses->findOrFail($id);

return $this->respondWithItem($course, $this->transformer);

to:

return $this->respondWithItem($course, function ($course) use ($id) {
    return $course->findOrFail($id);
});

//if you dont care about parsing includes just return the item:
return $this->respondWithItem($course->findOrFail($id));

[joshforbes]

Few things as I think of them:

It would be cool if the transformer was set as:

$this-setTransformer($whatever)

Then it could be chained set in the constructor as in your example or chained inline:

$this-setTransformer($transformer)->respondWithItem($course)

Also we could take this same idea further and remove the resource key from the response and instead chain it:

$this-setTransformer($transformer)->setResourceKey('course')->respondWithItem($course)

Would make the method signature of respond with item simpler and we could just default the resource key to false if it wasn't explicitly set.

[zackify]

Oh awesome I like the ideas!

…

On Mon, Feb 13, 2017 at 19:13 Josh Forbes ***@***.***> wrote: Few things as I think of them: It would be cool if the transformer was set as: $this-setTransformer($whatever) Then it could be chained set in the constructor as in your example or chained inline: $this-setTransformer($transformer)->respondWithItem($course) Also we could take this same idea further and remove the resource key from the response and instead chain it: $this-setTransformer($transformer)->setResourceKey('course')->respondWithItem($course) Would make the method signature of respond with item simpler and we could just default the resource key to false if it wasn't explicitly set. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAbacH6A8LieYNRnKRYKz5RgTPtM7XXeks5rcPHCgaJpZM4L-i3n> .

[joshforbes]

There is one thing I am not fond of. Supporting the setting of a default model to $this-model in the constructor and then it making it optional to the resondWith methods. It is such a narrow use case that you would want an endpoint that just returns all of the items with no restrictions. To me it just makes everything more readable to specify it inline.

$this-respondWithCollection($this->courseModel)

or

$this->respondWithCollection(new Course)

or

$this->respondWithCollection($courses)

That being said I think it could be appropriate to have it work with '$this-model' in the template controller you are planning on implementing.

[joshforbes]

I am really liking this sytax overall. I think the way I would typically use it would be:

    $this->setTransformer(new CourseTransformer)
        ->setResourceKey('courses')
        ->respondWithPaginatedCollection($courses)

I think that's super readable. Of course I would set the transformer and resource key in the constructor if it was going to be the same for all the endpoints in a controller.

[joshforbes]

Also as a note so I don't forget. We should make sure that all of these 'respondWith' methods can take either a query builder or a collection/item. If a builder is passed in then we automatically try to eager load available includes. If a collection/item is passed in then we just skip the eager load step and go straight to transformation.

I guess the only exception would be the 'respondWithPaginatedCollection' method - which really needs a builder to do pagination efficiently. If we get really ambition we could could make it fall back on less efficient pagination if a collection is passed in (it does not do this currently).

[zackify]

I guess you can just let me know what parts you'd like me to implement and I'll go for it. It all sounds good to me.

…

On Tue, Feb 14, 2017 at 19:51 Josh Forbes ***@***.***> wrote: Also as a note so I don't forget. We should make sure that all of these 'respondWith' methods can take either a builder or a collection/item. If a builder is passed in then we automatically try to eager load available includes but if a collection/item is passed in then we just skip that step. I guess the only exception would be the 'respondWithPaginatedCollection' method - which really needs a builder to do pagination efficiently. If we get really ambition we could could make it fall back on less efficient pagination if a collection is passed in (it does not do this currently). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAbacDTErDPrOrNuGqIyVIcXb0V4aUwEks5rckv0gaJpZM4L-i3n> .

[zackify]

Here's the latest. I added everything except checking if we can accept a builder or an item, and also a couple methods havent been updated either. But it works for respondwithcollection and item ;)

use NavJobs\Transmit\Transmitters\Show;
use NavJobs\Transmit\Transmitters\Update;
use NavJobs\Transmit\Transmitters\Destroy;


class CourseController extends Controller
{
    use Show, Update, Destroy;

    public function __construct(Course $model, CourseTransformer $transformer)
    {
        parent::__construct();
        $this
            ->setTransformer($transformer)
            ->setModel($model);
    }

We can't extend and overwrite a class because it enforces you to have the same function parameters of whatever it is you overwrite, so we have to use traits. Would love to find a way to do this without four use statements... lmk what you think.

[joshforbes]

I don't understand this change. Few things:

• If you do this - then the remaining lines of this file do not execute correctly.
• Breaks some tests as we were making assertions about the expected structure of the data.
• The way it is setup before your change allows the most configurable default (false for no key, null for data, and any value to set that as the key)
• Your change only ever allows it to be no key or whatever value - but that is misleading as this class is called 'dataArraySerializer'. The general point of this class is to serialize with a data key unless the user overrides it.
• I know what you are getting at - in your projects you don't ever want a 'data' key. Did you know that as it is currently setup it is as simply as doing an artisan config:publish and then changing the 'default_serializer' config option in transmit.php to 'arraySerializer'. Or alternatively make your own super simple serializer on a project by project basis and just give the path to it in the config file.

Now that I am thinking about it - I think I will update the arraySerializer class to basically do what you have here. Since even with that serializer the user would need the option to set a resourcekey when they were doing pagination.