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

[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]

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.

[stevenmaguire]

@lorisleiva do you have any further feedback here given the latest changes?

[int3hh]

why not merge ? cool feature to have

[edalzell]

why not merge ? cool feature to have

You can always composer patch this in if you want it now.

[it-can]

really need this, any eta?

[Wulfheart]

really need this, any eta?

Due to the complexity this is a PR only @lorisleiva should merge as I am unfamiliar with it.

[lorisleiva]

Hey guys, I've not come back to you yet because, at first glance, there are still a few smaller design decisions I'm not 100% sure about.

I need to take the time to dig properly into the changes and see if there are a few tweak we can do to mitigate this. Rest assured, this will end up being merged as I think it is a valuable feature. However, this is a fairly large addition to an otherwise stable package and I don't want to rush this. Hope you understand.

That being said, I'll try and allocate some time this weekend for this PR.

P.S.: It looks like CI isn't passing.

[stevenmaguire]

Thanks for the update @lorisleiva. I'll wait for some more specific and granular feedback before making further changes here.

[lorisleiva]

Okay I commented all my thoughts on the current design. It looks like a lot but actually we're much closer to merging point than I thought we were after reflection haha.

Let me know what you think about my comments and do another round of review afterwards.

[stevenmaguire]

Thanks for the continued review and feedback here. Refining and solidifying the functional and style requirements is an important process. Please review the latest changes that should be closer in line with the evolving expectations for this feature addition.

[stevenmaguire]

P.S.: It looks like CI isn't passing.

We'll have to wait until the CI workflow is approved again to see how the latest changes are performing.

[edalzell]

This is so slick now now @stevenmaguire great work!

[stevenmaguire]

Thanks for approving the CI runs. There are two failing builds and I'm scratching my head on one of them. If you have some thoughts, please chime in.

P8.4 - L^11.0 is failing with the error message:

Lorisleiva\Actions\Tests\AsPipelineWithMultipleNonOptionalParametersTest::handle(): Argument #2 ($nonOptionalAdditionalParameter) must be of type int, Closure given, called in /home/runner/work/laravel-actions/laravel-actions/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php on line 209

This is the concern I was mentioning in this thread of the feedback: #304 (comment)

But it is curious that it is only an issue with 8.4. I can reproduce this issue locally on 8.4. Laravel's Pipeline handling logic is detecting the handle method on the Action class - the one that expects two parameters in this failing test case - and is invoking that method instead of the __invoke method on the decorator. Furnishing a handle method on the decorator (like a previous iteration of this solution) does not change this behavior.

After poking at it for a while, it seems - and I could be wrong - that the $pipe in consideration within Laravel's Pipeline logic may start out as an instance of the PipelineDecorator eventually resolves as the direct action instance, which has its own handle method - skipping the decorator logic.

P8.4 - L^10.0 is failing with the error message:

Class "Illuminate\Support\Facades\Pipeline" not found

This one is a head scratcher. Could there be something wrong with the autoloading of the Laravel framework in this specific environment?

[stevenmaguire]

It seems that the Backtrace frame matching is different between <8.4 and 8.4.

Below is the value of $frame->function for the various environments:

26-02-2025 17:10:10 @ 8.2.27 - Illuminate\Pipeline\{closure} 
26-02-2025 17:10:19 @ 8.3.17 - Illuminate\Pipeline\{closure} 
26-02-2025 17:10:28 @ 8.4.4 - {closure:{closure:Illuminate\Pipeline\Pipeline::carry():184}:185} 

Adding this new pattern to the frame matching gets over that hump but is a little smelly.

A couple of other observations from this debugging...

1. The PipelineDecorator needs to ensure that something is always returned when the $closure returns null.
2. The Pipeline stack cannot include a standalone instance of an Action (eg: new PipelineCompatibleAction as opposed to PipelineCompatibleAction::class). There is a test case demonstrating this.
3. The Pipeline stack can include only one explicit container resolved instance (eg: app()->make(PipelineCompatibleAction::class)) at the bottom of the stack, if the handle method is compatible with Pipeline. There is a test case demonstrating this.

New changes are pushed up and ready for review and CI.

Regarding Number 3 above, this feels like a ServiceProvider timing issue that could be overcome (maybe?) but I am not super familiar with that area of the package yet. Perhaps there are some ideas on whether or not that is something we should expect to overcome? Or we can simply assert that this behavior is ONLY expected to work when using the PipelineCompatibleAction::class approach when building the pipeline stack?

This is obviously failing to match the frame match logic in the PipelineDesignPattern because the resolution is happening outside of an expected invocation scope. I only worry about this from a DevExp perspective - managing expectations around using an Action class in Pipeline and it being different (more restrained). Is this something that the docs alone can manage? Should we provide some code-based feedback that might be noticed during development?

To spare you digging into the test cases, here is an example of what works perfectly:

it('can run as a pipe in a pipeline, with an explicit asPipeline method', function () {
    $passable = Pipeline::send(new PipelinePassable)
        ->through([
            AsPipelineTest::class,
            AsPipelineTest::class,
            AsPipelineTest::class,
            AsPipelineTest::class,
        ])
        ->thenReturn();

    expect(is_a($passable, PipelinePassable::class))->toBe(true);
    expect($passable->count)->toBe(4);
});

Here is an example of what gives the impression of working but actually is not:

it('can run as a pipe in a pipeline with only one explicit container resolved instance at the bottom of the stack', function () {
    $passable = Pipeline::send(new PipelinePassable)
        ->through([
            AsPipelineTest::class, // implicit container resolved instance
            app()->make(AsPipelineTest::class), // explicit container resolved instance
        ])
        ->thenReturn();

    expect(is_a($passable, PipelinePassable::class))->toBe(true);
    expect($passable->count)->toBe(2);
});

Here are two examples of failing behavior:

it('cannot run as a pipe in a pipeline with an explicit container resolved instance in the middle of the stack', function () {
    $passable = Pipeline::send(new PipelinePassable)
        ->through([
            AsPipelineTest::class, // implicit container resolved instance
            app()->make(AsPipelineTest::class), // explicit container resolved instance
            AsPipelineTest::class, // implicit container resolved instance
            AsPipelineTest::class, // implicit container resolved instance
        ])
        ->thenReturn();

    expect(is_a($passable, PipelinePassable::class))->toBe(true);
    expect($passable->count)->toBe(2); // 4 is the ideal count here
});

it('cannot run as a pipe in a pipeline as an standalone instance', function () {
    $passable = Pipeline::send(new PipelinePassable)
        ->through([
            new AsPipelineTest, // standalone instance
            AsPipelineTest::class, // implicit container resolved instance
            app()->make(AsPipelineTest::class), // explicit container resolved instance
        ])
        ->thenReturn();

    expect(is_null($passable))->toBe(true);
});

[stevenmaguire]

@lorisleiva do you have a thought here on the fork in the road given the test case support mentioned above?

Should we:

1. Set an expectation that this package will only support pipeline pipes configured as using the ::class syntax so the framework resolves the class consistently where the frame matching logic expects, or
2. Explore the (seemingly remote) possibility of expanding the frame matching logic to support pipeline pipes configured as instances?

I think option 1 is the path of least resistance and will undoubtedly need some careful hand-holding in the documentation to effectively manage the expectation that if any pipeline enabled Action is used, it must follow the convention.

[lorisleiva]

Hi @stevenmaguire, Sorry for the late reply. I needed time to digest this properly.

Firstly, I wanted to mention that we cannot realistically rely on line numbers when matching frames. These numbers (184 and 185 in the snippet below) will update every time some code gets added or deleted in the Pipeline class which is not a robust solution and will be a nightmare to maintain as we always support more than one Laravel version at a time.

    public function recognizeFrame(BacktraceFrame $frame): bool
    {
        return $frame->matches(Pipeline::class, 'Illuminate\Pipeline\{closure}')
            || $frame->matches(Pipeline::class, '{closure:{closure:Illuminate\Pipeline\Pipeline::carry():184}:185}');
    }

Whilst we could use a regex to fix this, it is time to take a step back and compare the benefits of this PR with the cognitive and maintenance overhead it brings.

Option 1

Your first option (only supporting the ::class API when registering actions as pipelines) bring the following benefit to users.

    $passable = Pipeline::send(new PipelinePassable)
-       ->via('asPipeline')
        ->through([MyAction::class])
        ->thenReturn();

And the benefit of not having to define the asPipeline function if and only if they are planning on sticking to a simple "passable in / passable out" kind of pipeline.

The cons of this approach — on top of the maintaining overheads — is that it creates expectations for users that may not be met. What if I need a custom asPipeline method that changes the way the next closure is called? In this case, implementing the asPipeline function on the Action will require a function signature that differs from what I would expect from the framework.

Option 2

Other expectations include the problems you are trying to solve in option 2 such as invoking a pipeline via app()->resolve() or new MyAction.

Solving these problems with complex backtrace frame matching and more hooks on the IoC container may be possible but should we?

By that point, we would have made this library much more difficult to maintain — and much more prone to errors as the framework evolves — for a Nice To Have feature. Which leads me to...

Option 3

We do nothing.

If people wanted to improve the developer experience around pipelines for their actions, they'd just need to create a AsPipeline trait with a solution akin to the one provided here and just use the via("asPipeline") modifier. This seems like a much more user-friendly solution than having to teach users a whole new way of handling Pipeline that is kinda like the framework but also kinda not.

Right now, I am heavily leaning into option 3.

Rest assured, this will end up being merged as I think it is a valuable feature.

I'm sorry for having committed to this too early but, unfortunately, it turns out this innocent little feature brings a lot more baggage than it deserves.

I'll leave this open for a while in case I'm missing something or people want to change my mind but please bear in mind that this sort of things could just as well exist in separate packages.

[stevenmaguire]

Thanks @lorisleiva. That is exactly where I wanted to head in this PR - "is the juice worth the squeeze?"

It was clear from the original Issue and my early commits on the PR that there was not an obvious and easy "drop-in" solution available. I have centered my focus here on finding all the places where this could go wrong so that either 1) someone else with more domain knowledge (you and others) might highlight some kind of "ah-ha" moment to make it click or 2) make sure there is an informed reason why this is NOT a good idea to implement. I think the latter, No. 2, is where we're at based on your comments and I think that is OK.

I do think this support would be valuable and wish it were more viable at this point.

[lorisleiva]

I understand. It is also frustrating for me not to be able to help you push this over the line. I wished I’d been able to identify these issues earlier to avoid you spending as much time on this.

Perhaps this is something we can reevaluate in later versions of the framework where at least there won’t be any backtrack frame discrepancies between PHP versions anymore.

[edalzell]

Hot take I know, but you could ONLY support L12 & php 8.4 if you wanted. If folks don't want to upgrade they won't get this feature. i.e. make it a breaking change.

[lorisleiva]

Only supporting L12/PHP8.4 would only simplify the backtrace frame matching for using AsPipelineTest::class in Pipelines. It won't change the fact that new AsPipelineTest and app()->resolve(AsPipelineTest::class) will cause unexpected behaviour to the user. If fact, since Laravel Actions works by recognising and decorating special classes when they are resolved from the container, there wouldn't be a way of changing this unless we completely overwrote the Pipeline class itself.

The only way forward I can see for this PR is:

• We implement a more robust backtrace frame matching on '{closure:{closure:Illuminate\Pipeline\Pipeline::carry():184}:185}' (e.g. we only check it contains closure:Illuminate\Pipeline\Pipeline::carry()).
• We do not support new MyAction or app()->resolve(MyAction::class) because there's nothing we can do to recognise this. We add one test file for each of these patterns showing they don't work and we keep the rest of the test clean with MyAction::class only.
• We do not add AsPipeline to the AsAction trait by default. It's opt-in. That way, we avoid adding unexpected behaviour to people's apps unless they know what they're doing.
• We add a whole new page in the documentation explaining all this. The documentation PR should be ready to merge before this gets merged.

What do you guys think of this plan?

[lorisleiva]

Side note: I'm curious to understand why this works btw. Is it because if a pipeline class has an __invoke method, the framework will fallback to using this instead of arbitraryMethodThatDoesNotExistOnTheAction?