Add support for AsPipeline concern

[stevenmaguire]

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 AsPipeline trait. 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

[edalzell]

This is awesome @stevenmaguire, thanks for your work on this.

How come the handle method isn't used? The "standard" pattern in this package is to check for the as... and use that if found but if not, use the handle method.

[stevenmaguire]

How come the handle method isn't used? The "standard" pattern in this package is to check for the as... and use that if found but if not, use the handle method.

Do you mean in the PipelineDecorator::__invoke method? If not, where were you expecting to see that? I'd like to know what context you are asking about before responding to the "why" of your question.

[edalzell]

[stevenmaguire]

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 asPipeline method. But, as I acknowledged I don't know the complete inner workings of the rest of the package and I will take it your might know something I don't, so better safe than sorry. The PR has been updated with more guard rails in place as you appeared to be expecting.

[edalzell]

I will take it your might know something I don't, so better safe than sorry.

Nope, but in general you should match how a package works. Consistency is important. Hopefully @lorisleiva chimes in here and guides us/makes changes.

[stevenmaguire]

Nope, but in general you should match how a package works. Consistency is important.

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?

[edalzell]

I tend to agree with that, but not as a dogmatic approach.

Totally agree, mine was only a suggestion to be considered. If it isn't right then we shouldn't do it.

Do you have a suggestion on how that might be exercised with a test?

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 handle method to work.

I'll take a look later when I have a few moments.

Thanks again for your work on this, it's awesome.

[stevenmaguire]

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 AsPipeline - I tried AsController - and it failed. Likely because the PipelineDesignPattern didn't find a match and did not put the decorator in play.

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.

[lorisleiva]

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 asPipeline method is defined on the trait. Other patterns tend to make the decorator resolve from the asX method first and fallback to the handle method. I can see the asPipeline as some specific requirements but it would be nice for the same mental model to apply here.

Sorry if thats mentioned in your conversation though, as I said I'll have proper read through everything soon.

[stevenmaguire]

Thanks @lorisleiva!

Regarding the asPipeline method being on the trait, I explained the justification in the code comment on the method itself.

    /**
     * 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 handle method there is no value being added by this package because a handle method is not likely to be compatible with the Pipeline expectations.

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 AsController or AsJob, it is now nullable. Also, the return type of handle is now mixed. It's no longer elegant and all for the purpose of being compatible with Pipeline.

So, the asPipeline method can abstract this messiness. But it is essential it exists, otherwise there is no compatibility with Pipeline if left to handle alone. Therefore, if it's existence is essential, you could update the trait to include an abstract method signature forcing the implementation upon the consuming project or just implement the basic opinionated logic that will be appropriate 9 times out of 10, leaving it available for overwriting for the outlying 1 time out of 10. That's what exists in this PR.

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.

[lorisleiva]

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. 🙏

[lorisleiva]

This code should live in the decorator.

[lorisleiva]

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.

[stevenmaguire]

@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.

1. It is OK to isolate Pipeline closure resolution logic within this package.
2. Each "critical" method in the Action (handle and asPipeline in this case) should be expected to declare unambiguous type hinting for parameters and return types.
3. Much like the AsController flow, the asPipeline method should be used to "coerce" a single Pipeline parameter into the parameters expected by the handle method if the handle method is not already Pipeline compatible.
4. If the handle method is Pipeline compatible (in that it only requires one non-optional parameter to function properly) it is not essential for the Action to furnish an asPipeline method.

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:

   PASS  Tests\AsPipelineTest
  ✓ it can run as a pipe in a pipeline, with explicit trait
  ✓ it can run as a pipe in a pipeline, with implicit trait
  ✓ it can run as a pipe in a pipeline, without an explicit asPipeline method
  ✓ it can run as a noop/passthrough pipe in a pipeline, without a handle or asPipeline method
  ✓ it can run with an arbitrary via method configured on Pipeline
  ✓ it cannot run as a pipe in a pipeline, with an explicit asPipeline method expecting multiple non-optional params
  ✓ it cannot run as a pipe in a pipeline, without an explicit asPipeline method and multiple non-optional handle params

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.

[edalzell]

100% personal preference here but I'd prefer:

return $closure($this->callMethod('asPipeline', [$passable]) ?? $passable);

Here to remove the else

[edalzell]

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.