Add support for AsPipeline concern #304
Conversation
|
This is awesome @stevenmaguire, thanks for your work on this. How come the |
Do you mean in the |
|
|
|
Thanks. That's what I thought you were asking about. Yes, I clocked that implementation detail but originally did not include it because if that method was not available, the Pipeline resolution chain would be broken anyway. I'm not sure how someone would configure things in such a way where the Decorator was in play without also having the trait, which furnishes a |
Nope, but in general you should match how a package works. Consistency is important. Hopefully @lorisleiva chimes in here and guides us/makes changes. |
I tend to agree with that, but not as a dogmatic approach. If the code path can't be accessed or exercised with a test (that replicates what a consuming project could do), it shouldn't be included just because another class in the project does it. I'm still scratching my head on how I could exercise that code path, even though I've now added it. Do you have a suggestion on how that might be exercised with a test? |
Totally agree, mine was only a suggestion to be considered. If it isn't right then we shouldn't do it.
Not off the top of my head, but this was only a quick review. If it's dead code path then it def shouldn't be included, but I know as a user of this package I would expect having only a I'll take a look later when I have a few moments. Thanks again for your work on this, it's awesome. |
|
I just tried to test some use cases that would exercise that code path and was unable to get there. I tried a pipe class which explicitly used another trait from the package but not I tried a generic pipe class and naturally, it did not get there. In the meantime, I've removed the fallback code path from the decorator. I would love a review from @lorisleiva or anyone else with close working knowledge of this package. At the moment, there are two main test cases that are passing. |
|
Hey guys, thanks for this! I'll have a proper look at it over the weekend and read all your comments. On little thing I've noticed on a first quick look is that the Sorry if thats mentioned in your conversation though, as I said I'll have proper read through everything soon. |
|
Thanks @lorisleiva! Regarding the /**
* Typical pipeline behavior expects two things:
*
* 1) The pipe class to expect a single incoming parameter (along with
* a closure) and single return value.
* 2) The pipe class to be aware of the next closure and determine what
* should be passed into the next pipe.
*
* Because of these expectations, this behavior is asserting two opinions:
*
* 1) Regardless of the number of parameters provided to the asPipeline
* method implemented here, only the first will be supplied to the
* invoked Action.
* 2) If the invoked Action does not return anything, then the next
* closure will be supplied the same parameter. However, if the
* invoked action does return a non-null value, that value will
* be supplied to the next closure.
*
* Also, this logic is implemented in the trait rather than the decorator
* to afford some flexibility to consuming projects, should the wish to
* implement their own logic in their Action classes directly.
*/Basically, the whole value of the concern here is that you (the consuming package) don't need to worry about handling the Pipeline's expectations around the callback closure, this concern will take care of it for you. The opinionated logic that is now here in the PR should be furnished by the package somewhere. If that is the case, then furnishing it in the trait leaves it accessible to consuming projects to override the default behavior if they please. Without this opinionated logic in the package it will be left to the consuming project to implement every time and if they don't and it falls back to the The demonstrate, if there exists an Action that is capable of doing everything else this package offers, it would look something like this: class PublishPost
{
use AsAction;
public function handle(Post $post): void
{
// do the things that publish the post.
}
}That is simple and pure and will work for all the existing use cases. But, it alone is not compatible with Pipeline. Here is what would be needed to make it compatible with Pipeline: class PublishPost
{
use AsAction;
public function handle(Post $post, ?Closure $next = null): mixed
{
// do the things that publish the post.
if ($next) {
return $next($post);
}
}
}Pipeline expects that the closure will never be null and the method will return the result of invoking that closure. In this example, I made the obvious assumption that since the Closure won't be supplied for other use cases - like So, the So while this may not be an obvious match for the style and mental model here, I think there is a reasonable justification to consider the deviation in order to add more value and convenience. At the end of the day, I am very new to this package and if you feel strongly about moving this logic to the decorator - or somewhere else 🤔 - I'm all ears. |
There was a problem hiding this comment.
I think there is a misalignment on how this package handles AsX decorators and this PR.
You are saying that, in order to make the handle method "compatible" with Pipeline one would have to do the following:
class PublishPost
{
use AsAction;
public function handle(Post $post, ?Closure $next = null): mixed
{
// Do the things that publish the post...
if ($next) {
return $next($post);
}
}
}I disagree.
To illustrate this, let's see how AsController handle this situation because, it too, has some specific requirements — e.g. route model bindings.
Here's a simple example that uses an action as a controller to publish a post.
class PublishPost
{
use AsAction;
public function handle(Post $post, PublishPostRequest $request): Response
{
$request->validate();
// Do the things that publish the post...
return redirect()->route('posts.published', [$post]);
}
}This works but because we're using the handle function, we are "locking" ourselves to only use this action as a controller. Sometimes that's what we want, but most of the time, you want to extract the "main logic" of your action in the handle method. That way, you can use asX functions as adapters to plug in your action in various places in the framework.
To improve on the previous design, we would need to use the handle method exclusively for the "domain logic" of our action, and refer to it inside the asController method whose responsibility is purely to make this action accessible in web/api routes. Here's our updated example.
class PublishPost
{
use AsAction;
public function handle(Post $post): void
{
// Do the things that publish the post...
}
public function asController(Post $post, PublishPostRequest $request): Response
{
$request->validate();
$this->handle($post); // Notice how we delegate to the domain logic here.
return redirect()->route('posts.published', [$post]);
}
}Now, with that in mind, how would you make this action also available as a pipeline? With your current design, we cannot do it anymore because you are forcing the handle method to be pipeline-specific. However, if we follow the same pattern as controllers, we can add a new asPipeline method that again delegates the handle method for the domain logic.
Which gives us the following code:
class PublishPost
{
use AsAction;
public function handle(Post $post): void
{
// Do the things that publish the post...
}
public function asController(Post $post, PublishPostRequest $request): Response
{
$request->validate();
$this->handle($post);
return redirect()->route('posts.published', [$post]);
}
public function asPipeline(Post $post, ?Closure $next = null): mixed
{
$this->handle($post);
if ($next) {
return $next($post);
}
}
}And just like that an action can be used as a pipeline and as whatever else the user needs. And if the user decides they only want this action as a pipeline, they can use the handle method as a fallback for the pipeline decorator (just like we did initially with the controller) and we end up with your original design which also works.
This is my main design change request on this PR. I wouldn't feel confortable merging something that deviates from the mental model of the whole package.
An additional smaller concern I have is the signature (Post $post, ?Closure $next = null): mixed. Particularly the input/output selection of the pipeline. If you could add more tests showing that a single action can accept multiple different inputs and return multiple different outputs without the usage of mixed, I'd feel more confortable with the function signature. 🙏
src/Concerns/AsPipeline.php
Outdated
| $passable = array_shift($arguments); | ||
| $closure = array_pop($arguments); | ||
|
|
||
| return $closure($this->handle($passable) ?? $passable); |
There was a problem hiding this comment.
This code should live in the decorator.
src/Decorators/PipelineDecorator.php
Outdated
| if ($this->hasMethod('asPipeline')) { | ||
| return $this->resolveAndCallMethod('asPipeline', $arguments); | ||
| } |
There was a problem hiding this comment.
This code should resolve the passable, closure, etc. and then pass it to the asPipeline function or fallback to passing it to the handle function.
|
@lorisleiva thank you very much for the detailed and thorough feedback. I appreciate it! I think we can make something work here, especially within the constraints of the explanation you provided. I've pushed up an update that - I think - puts this closer to what you might consider acceptable. I'm not afraid of another round of refinement if needed. I gleaned a few of assumed truths from your feedback that were most productive in the latest revision. I want to put them next to numbers so it might be easier to confront and/or refine each assumed truth independently, if needed.
I did add more test cases here to try to exercise these assumed truths relative to the Pipeline concern. Here is the Pest output for that: Hopefully between those Pest test cases and the assumed truths listed above, the current state of the PR is more clearly aligned. Please do let me know if I am still missing something. |
| $returned = null; | ||
|
|
||
| if ($this->hasMethod('asPipeline')) { | ||
| $returned = $this->callMethod('asPipeline', [$passable]); |
There was a problem hiding this comment.
100% personal preference here but I'd prefer:
return $closure($this->callMethod('asPipeline', [$passable]) ?? $passable);Here to remove the else
|
There we go, great work @stevenmaguire, that's what I meant by "copy the patterns" in a package. Now it looks like all the other action types. |
As discussed in Feature Request: Support for Illuminate Pipelines #279 there seems to be some interest and appetite in allowing our Action classes to function well within a Pipeline workflow, in addition to all the other already support Laravel conventions.
This PR aims to do that.
The main thing to note are the two opinions being asserted by the new
AsPipelinetrait. An explanation and justification for those opinions are captured in a code comment in the trait file.If there are any critiques or comments, please share them. I am happy to collaborate to get this to a production-ready and mergable state.
Fixes #279