lorisleiva / laravel-actions

⚑️ Laravel components that take care of one specific task
https://laravelactions.com
MIT License
2.44k stars 119 forks source link

Implement injectable ADR responders #31

Closed nicolaib closed 3 years ago

nicolaib commented 4 years ago

Thanks for a great package.

I feel an Action from the package already has many responsibilities (maybe too many), and it would be beneficial to avoid having responses return directly from the Action. In my opinion the Action shouldn't create a response if following SRP. If responses could be implemented as injectable ADR-style Responders, they could be composed and it would be a great addition to the simplification of Action implementation in projects.

Is there any chance that we will see this in a release any time soon? Or do you have any recommendations on how these could currently be injected and bound to the routes on where the Action is called?

lorisleiva commented 4 years ago

Hi there πŸ‘‹

Sorry for the late reply. To be honest I'm not 100% sure what you mean by ADR-style responders. Would you be able to provide a small example of how you'd like this to work with Laravel Actions? It would help me understand the feature request a bit better and let you know if there is a current workaround to this.

c17r commented 4 years ago

Not the OP so I'm only guessing but I think they are referring to https://en.wikipedia.org/wiki/Action%E2%80%93domain%E2%80%93responder

lorisleiva commented 4 years ago

Yes, I got that far too. πŸ˜…

ralbear commented 4 years ago

I discover this package not a long ago and sounds very promising, my current way to proceed with a new project ADR + command handler (tactician) sometimes is a bit overkill and time consuming for simple applications.

For example i use to build API's using ADR and what the responder is where i put the login to specify which kind of resource (with a specific status code) or some error code i want to return based on the action response.

Based on this example from the documentation, i guess @nicolaib refers to have the option to have different responses if the action is going to be used to return a blade template or if is going to be used to return a json response, using resources for example.

class PublishANewArticle extends Action
{
    // ...

    public function response($article)
    {
        return redirect()->route('article.show', $article);
    }
}

Let me know your thoughts but maybe something like this can be a valid approach, if no responder is defined, the default one is used.

$action = new PublishANewArticle([
    'title' => 'My blog post',
    'body' => 'Lorem ipsum.',
]);

$action->withResponder(NewArticleApiResponder::class);

$article = $action->run();
lorisleiva commented 4 years ago

Hi @ralbear πŸ‘‹

Thank you for your explanation, that makes a lot more sense now.

Would this custom responder be used only for the HTTP layer (i.e. Actions as controllers only), or would this be used no matter how the action was called?

As usual, before implementing this feature or merging a PR for this, I'll need to assess if this is a relevant one for most developers using Laravel Actions but I think it's a good idea and there's probably good use cases for that.

nicolaib commented 4 years ago

Hey @lorisleiva and @ralbear
You got to the right conclusion about what I was trying to accomplish. I would not recommend the suggested approach, though, as it would not make it possible to use an action as an invokable controller called directly from a route. Auto-resolving the responder from the asController() would be ideal, but that would currently require that a responder implements both JSON and HTML response types (as well as actually calls the responder), which would go against the SRP. A better solution would be to make it possible to auto-resolve a responder from the different response methods (maybe as a third argument). This would probably not be very elegant and almost be no better than making a static responder, like in the documentation examples. Furthermore, it would only be possible to respond to HTTP, which answers the other question: "Yes, responders could be relevant to other call methods - especially commands".

The best solution, I found, was to use a response trait which implements the respond methods and resolves the responder in the asController() method. This also means that I currently do not have the magic solution to the problem.

Due to this and other limitations, I actually stopped using the package for the intended project. I would, though, happily make suggestions on things that could improve the package. But, of course, they should be relevant to the majority of users.

nicolaib commented 4 years ago

The other limitations concern:

These comments belong in a different issue, but in my opinion prevent scalable use of the package. Feel free to delete this comment, as it surely is unrelated to the current thread. :-)

ralbear commented 4 years ago

This ADR approach is more oriented to HTTP and if we check the official documentation from the creators page http://pmjones.io/adr/ we can se than the action have the responsibility of return the responder.

This is an example of how a __invoke() method of an action looks in a project i work on which is based in ADR

class CreateUserAction
{

public function __invoke(Request $request)
    {
        $serviceResponse = $this->createUserService->execute($request->only('id', 'firstName', 'lastName', 'email', 'password'));

        return $this->createUserResponder->withResponse($serviceResponse)->respond();
    }
}

The limitation with this package is the action and the domain is together. A example i face quite often is when you list the same items but with some filtering based on the url. I have for example an endpoint to list all the articles

Route::get('articles', '\App\Actions\IndexArticles');

But when i want to see all the articles related to a specific user, don't want to duplicate all the logic on that articles index action so i do this

Route::get('user/{userId}/articles', '\App\Actions\IndexUserArticles');

In this scenario, the way i do it is, both actions are different but they are using the same service, the only difference is IndexUserArticles have a parameter which specifies the userId which as the same time is an option when you want to see all articles related to an user from the admin panel and do something like articles?userId=123

With this approach is easy even to have a specific URL for the user profile

Route::get('me/articles', '\App\Actions\MeIndexArticles');

Where you get the userId from the request, the user needs to be authenticated, ofcourse!.

And because i have many actions, i can have a different responder for each one.

If you want to preserve the base of the project and need responders, maybe the simple way is to use controllers or other level of simple actions where you do something like this

class IndexArticles
{

public function __invoke(Request $request)
    {
        $action = new IndexArticles([
               'parameters' => $request->filterTheParametersYouWant()
                 ]);

        return $this->indexAllArticles->withResponse($action->run())->respond();
    }
}
class IndexUserArticles
{

public function __invoke(Request $request, $userId)
    {
        $parameters['userId'] = $userId;
        $action = new IndexArticles([
               'parameters' => parameters
                 ]);

        return $this->indexUserArticles->withResponse($action->run())->respond();
    }
}
class MeIndexArticles
{

public function __invoke(Request $request)
    {
        $parameters['userId'] = $request->user()->id;
        $action = new IndexArticles([
               'parameters' => parameters
                 ]);

        return $this->indexMeArticles->withResponse($action->run())->respond();
    }
}
ralbear commented 4 years ago

The other limitations concern:

  • order of authorization and validation execution makes it difficult to use with strict types,
  • same runAs propagation on nested actions prevents reuse,
  • attributes as class properties creates naming conflicts

These comments belong in a different issue, but in my opinion prevent scalable use of the package. Feel free to delete this comment, as it surely is unrelated to the current thread. :-)

I guess will be great if you can give more details of each point, not sure if i understand all of them.

lorisleiva commented 3 years ago

Laravel Actions v2 is much less intrusive which make this feature obsolete to the package. πŸ™‚

c17r commented 3 years ago

Does it though?

Looking at the v2 document site, the "Running as a controller" example is


Route::put('auth/password', UpdateUserPassword::class)->middleware('auth');

/////////////

class UpdateUserPassword
{
    use AsAction;

    public function handle(User $user, string $newPassword)
    {
        $user->password = Hash::make($newPassword);
        $user->save();
    }

    public function asController(Request $request)
    {
        $this->handle(
            $request->user(), 
            $request->get('password')
        );

        return redirect()->back();
    }
}

Doing something like redirect()->back() will only take you so far. More complicated use cases will need to have redirect()->route() with the object just created and the moment you do that, the Action is like v1, locked into this one particular use case and can't be re-used elsewhere. To be truly re-usable, the response portion needs to be injected in from the caller.

lorisleiva commented 3 years ago

I see what you mean. What solution would you have in mind for v2?

lorisleiva commented 3 years ago

I'm going to close that again for now as I don't have any immediate plans to implement this and I'd like to have a more practical explanation of its benefits before investing time on this.

I am not opposed to a PR if anyone wants to give it a shot but it would need testing and a pretty solid description for it to be considered.

Thanks for your understanding. ☺️