Feature Request: Support for Illuminate Pipelines

[h-sigma]

Relevant Article: https://martinjoo.dev/laravel-pipelines

Illuminate pipelines look like:

app(Pipeline::class)
    ->send('<p>This is the HTML content of a blog post</p>')
    ->through([
        ModerateContent::class,
        RemoveScriptTags::class,
        MinifyHtml::class,
    ])
    ->then(function (string $content) {
        return Post::create([
            'content' => $content,
            ...
        ]);
    });

In the example above, the string $content variable "travels" the pipeline. The signature of each class that it travels through needs to be

public class ModerateContent
{
    public function handle(/* our traveler */ string $content, Closure $next)
    {
        $safeContent = $this->removeBadWords($content);
        $next($safeContent);
    }
}

As you can see, the syntax is fairly similar to the existing Laravel Actions handler. In fact, all I need to do is change the signature to ?Closure $next = null to adapt it for both usages. However, it would be quite neat if Actions could support this directly (the return value of the handler function would be passed to $next in this case).

[h-sigma]

// -- AsAction.php
trait AsAction
{
    use AsActionBase;

    public function asPipeline(...$args): void
    {
        if (count($args) === 0) {
            throw new \Exception('No arguments provided to asPipeline method.');
        }
        if (!(last($args) instanceof \Closure)) {
            throw new \Exception('Last argument must be a closure.');
        }
        $closure = array_pop($args);
        $closure($this->handle(...$args));
    }
}

// later...
app(Pipeline::class)->send('content')->via('asPipeline')->through([ ModerateContent::class ])->thenReturn();

Would there be any shortcomings to this approach?

[edalzell]

I love this approach!

[christopherarter]

You have a PR @h-sigma ? I was about to write this myself in my project but glad I'm not the only one looking for it. Seems like it would be handy for others.

[CeriseGoutPelican]

I was actually looking in the documentation to see if this was a feature and am glad I found this post. I would love this, good approach! Maybe we can work on something together?

[stevenmaguire]

@h-sigma have you been exercising this solution in a production project since proposing here? Does it seem to work well? I am looking for Pipeline support in this package as well and it seems like you might have a solid, and straightforward, approach that would warrant a PR here. Nice work!

[h-sigma]

I've been using it in a couple of smaller production projects. It works fine.
The minor criticism I have is that since I need to call via('asPipeline'), if I already had some classes that weren't actions that I wanted to add to the pipeline, I had to give them a asPipeline method as well.

I don't have a PR in the works right now. What does the maintainer think about adding this as a part of the library? I do not want to bloat it with rarely used features (which pipelines are imo), and it might be better to add it as a tip or advice in the documentation with this code example instead?

[stevenmaguire]

Thanks for the context, @h-sigma.

The minor criticism I have is that since I need to call via('asPipeline'), if I already had some classes that weren't actions that I wanted to add to the pipeline, I had to give them a asPipeline method as well.

I feel this way as well.

I have also been using this implementation in a project to kick the tires and the only modification I've needed to make is detecting the return type of the Action's handle method.

Pipeline steps are expected to always return something and the subsequent steps are expecting that something. Since this package's intent is to use a single class in a variety of contexts, that needs to hold true for the Pipeline use case as well. But, the Pipeline use case is the only one - so far as I have experienced - that mandates the Action's handle method return something. Otherwise a void return type is perfectly fine and pleasant. It feels odd, to me at least, to let the Pipeline use case force a change in that expectation. So, I've updated the implementation to check for a return type before processing the closure. I quite prefer that the Action itself not be expected to return anything.

    public function asPipeline(...$args): void
    {
        if (count($args) === 0) {
            throw new Exception('No arguments provided to asPipeline method.');
        }
        if (! (last($args) instanceof Closure)) {
            throw new Exception('Last argument must be a closure.');
        }

        $closure = array_pop($args);

        $returned = $this->handle(...$args);

        if (! is_null($returned)) {
            $closure($returned);
        } else {
            $closure(...$args);
        }
    }

The assumption being made here is: if the handle method DOES return something, then it is intentional and pass that into the closure. If not, pass the initial params into the closure. This has it's own smell for sure, but it seems to solve the issue and maintain some flexibility in the Action design and consistency.

[edalzell]

OK, I still love this idea but I don't love the implementation.

Taking a look at the laravel pipeline code, if the pipeline is a string (i.e. class name) it resolves out of the container. This means, I believe, we can use the Decorator pattern that's used elsewhere. This would mean we wouldn't need an asPipeline method, nor doing that in the pipeline call.

We could then have an optional closure in the handle and you can deal with as you like. The decorator can handle all the "magic".

[stevenmaguire]

@edalzell that sounds great - especially the whole "no need for explicitly calling asPipeline" and "just like the rest of the package" themes.

How can we help prove something out here?

[edalzell]

How can we help prove something out here?

A PR would be my guess

[stevenmaguire]

Oh, sure. I didn't know if you had some initial implementation ideas to share...

[edalzell]

Oh, sure. I didn't know if you had some initial implementation ideas to share...

Nope, but if ever free up some brain space, this would be fun to do!

I'd start by looking at the other decorators and seeing how they hook into the container resolution. After that it's mostly the code above.

[stevenmaguire]

I went down the road a bit here working on a PR for this following the existing conventions of the package. Admittedly, I am not familiar with all of the inner workings of the existing package behavior, but I think I have it pretty close.

Unfortunately, there is a significant blocker.

When the Action class uses the new AsPipeline trait directly, the testing passes and it seems to work very well.

When the Action class uses the AsAction trait, which is updated to include AsPipeline as well, the testing fails. It appears to fail because the AsController concern and underlying decorator furnishes an __invoke method already which appears to supersede any additional decorators.

I'm still poking around at it, but if I can't get the test passing for that use case, it might not be much I can do alone. Perhaps @lorisleiva has some advice? I can put the PR up as is if it seemed worth following up on.

[stevenmaguire]

I pushed through and got the tests passing - I think things are in reasonably good working order. The PR is here: #304