Added error listeners to RequestHandlerRunner

[maurice2k]

Added error listeners to RequestHandlerRunner to be able to log errors/exceptions thrown in ::run() method.
Also made emitMarshalServerRequestException public to it can be called from the outside-

[weierophinney]

The only issue I see with this is that you would not be able to use the same listeners here as you do with the zend-stratigility ErrorHandler, as that class triggers listeners using the following signature:

function (Throwable $error, ServerRequestInterface $requst, ResponseInterface $response)

If we were to do this, we'd need to find a way to either document re-use of listeners, or provide a way to curry listeners to work properly (e.g., providing an empty request instance).

[maurice2k]

Unfortunately yes as the request object is not available. But still offers a chance to get the issue logged somewhere, maybe together with $_REQUEST.

[maurice2k]

Empty request would also be a possibility...

YES, really like the idea. One could create a class implementing ServerRequestInterface which just pipes thru the $_SERVER, $_FILES etc variables. Without triggering any exceptions.

[weierophinney]

$_REQUEST is not available in all SAPI environments; it's also unreliable, as the order in which the various sources are provided is based on php.ini settings. $_SERVER, $_FILES, etc. may or may not be populated depending on the SAPI as well (e.g. async frameworks). I think an empty request makes more sense here, potentially even supplying an empty implementation as part of the package.

The other point remains: we'd need a way of currying arguments for existing listeners. It'd likely look something like this:

class ErrorHandlerListenerAdapter
{
    /** @var callable */
    private $listener;

    /** @param ServerRequestInterface */
    private $request;

    public function __construct(callable $listener, ServerRequestInterface $request = null)
    {
        $this->listener = $listener;
        $this->request = $request ?: new EmptyServerRequest();
    }

    public function __invoke(Throwable $e, ResponseInterface $response) : void
    {
        ($this->listener)($e, $this->request, $response);
    }
}

function adaptErrorHandlerListener(callable $listener, ServerRequestInterface $request = null) : ErrorHandlerListenerAdapter
{
    return new ErrorHandlerListenerAdapter($listener, $request);
}

I'm not sure if that should belong here, or in the zend-expressive package, however.

[maurice2k]

Not at my PC right now. Yes @ $_REQUEST.

But why not just call the listeners in triggerListeners with the second arguments being the new EmptyServerRequest()?

Why an ...Adapter?

[weierophinney]

By currying, we can use existing listeners, and call them using the empty request instance only in this particular context; in other contexts, they get the populated request instance. That's the sole purpose of the approach I outline above.

[maurice2k]

Now I got it; was hard to read on mobile.
I thought I'd just make the listener callback compatible (which I think is easier/requires less code). On the other hand, if you're only using listeners for RequestHandlerRunner you would always have an empty request as the second parameter.
Up to you...

Another idea is to not use an EmptyServerRequest but some kind of Raw/UnitializedServerRequest which returns $_SERVER, $_FILES etc so that just have some bare minimum Request. Not sure if this is a good idea.

[samsonasik]

\ in Throwable is not needed as already in use statement.

[weierophinney]

@maurice2k I'd like to bring this functionality in, but need:

• A class representing the empty server request. This would implement ServerRequestInterface, but provide no-ops and/or default values for all methods.
• A mechanism for providing listeners to the instance (likely via a constructor argument).
• Unit tests.
• Documentation of how to use the feature.

If you cannot provide those, close the issue, and indicate somebody else should pick it up.

Thanks!

[weierophinney]

This repository has been closed and moved to laminas/laminas-httphandlerrunner; a new issue has been opened at laminas/laminas-httphandlerrunner#2.

[weierophinney]

This repository has been moved to laminas/laminas-httphandlerrunner. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

• Squash all commits in your branch (git rebase -i origin/{branch})
• Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
• Run the laminas/laminas-migration tool on the code.
• Clone laminas/laminas-httphandlerrunner to another directory.
• Copy the files from the second bullet point to the clone of laminas/laminas-httphandlerrunner.
• In your clone of laminas/laminas-httphandlerrunner, commit the files, push to your fork, and open the new PR.
  We will be providing tooling via laminas/laminas-migration soon to help automate the process.