Add Exception to the Pipeline - Feature Request

[aftabnaveed]

Some times when a middleware does not exist the laravel middleware pipleline breaks with a weird error.

"Function name must be a string" /vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php

                    $parameters = array_merge([$passable, $stack], $parameters);
                } else {
                    // If the pipe is already an object we'll just make a callable and pass it to
                    // the pipe as-is. There is no need to do any extra parsing and formatting
                    // since the object we're given was already a fully instantiated object.
                    $parameters = [$passable, $stack];
                }

                $response = method_exists($pipe, $this->method)
                                ? $pipe->{$this->method}(...$parameters)
                                : $pipe(...$parameters);

                return $response instanceof Responsable
                            ? $response->toResponse($this->container->make(Request::class))
                            : $response;
            };
        };
    }

The above error does not give you a clue of what's wrong in the code. Now that php 7 supports catching fatal errors, it would be great to wrap the pipeline response in a try/catch block like

try {
   $response = method_exists($pipe, $this->method)
                                ? $pipe->{$this->method}(...$parameters)
                                : $pipe(...$parameters);
} catch(\Throwable $e) {
     thorw new PipelineException(sprintf('Method %s does not exists in class %s', $this->method, get_class($pipe)));
}

Above example can be further improved but this just an idea of how it may look like.

[mfn]

a) I'm for improving the message b) Don't think your approach is entirely correct?

About b): This would catch everything and disguise it as a "Method…does not exist…" error, no?

I think the only correct approach an be to check the middleware more diligently before using it. Probably should check performance implications/benchmarks.

[aftabnaveed]

Thanks for considering it please see below my comment:

This would catch everything and disguise it as a "Method…does not exist…" error, no?

$response = method_exists($pipe, $this->method)
? $pipe->{$this->method}(...$parameters)
: $pipe(...$parameters);

That particular line is all about checking the existence of a method and in my opinion the error does makes sense.  What else is pipelines used for? I think I would need to do some more research and sharing any link in that regard would be helpful.

> Probably should check performance implications/benchmarks.

The reason why I think there would be no performance implications is because if the try block succeeds the exception is not thrown hence no performance penalty.  But I will come up with a benchmark to see the exact implications.

[mfn]

I think we talked next to each other, sorry if I wasn't clear.

Your PoC code sample catches all exceptions.

So, what happens to exceptions thrown within a pipe?

Do they all get caught and converted to the generic PipelineException?

So in practice no other exception can bubble up anymore.

If that can be the case (I don't know the details how $pipe calls work here), this would be a no-go as it would break the exception flow.

That's why I mean: I think it's wrong to catch all arbitrary exceptions and rethrow them as something else. A check would need to be implemented (if (…) { throw new PipeLine…) and the performance impact of this should be considered.

But maybe the exception flow doesn't work that way and this is a non-issue.