Add proper handling of invalid script paths

[hollodotme]

Expected Behavior

If an invalid script path is requested like in the following example, an exception is thrown.

new PostRequest(`/path/to/dir/../script.php`, '...');

Actual Behavior

The request is processed as it was successful with the response "File not found".

Specifications

• Package version: All
• PHP version: All

Further comments

Check, if there is a specific state returned, but not handled in Socket.php Line 322 ff. or Socket.php Line 441 ff..

If there is no way to distinguish between a valid response with body "File not found" and one caused by an invalid script path, a guard method with path syntax check could be added to AbstractRequest.php

/cc @theseer

[hollodotme]

So, it turns out the root cause is that php-fpm sends multiple packets in case the given script file does not exist and that those packets have different types.

The first packet has the type 7 which indicates that the content in this packet is meant to be sent via STDERR.
The next packet has the type 6 which indicates that the content in this packet is meant to be sent via STDOUT.

Both packets are handled the same way and the internal socket status has an unintended transition from REQ_STATE_ERR to REQ_STATE_OK because php-fpm actually does produce a valid response for STDOUT (Headers + "File not found.") and completes the request.

Furthermore the STDERR content from the first package precedes the header information from the second package (without a line feed) and is therefore not included in the body of the resulting response.

To fix these issues the STDERR output will be collected in a separate string (for a proper error message) and the unintended socket status transition will be replaced and handled correctly as soon as the complete response was read.

A new ProcessManagerException will be raised with the message An error occurred: Primary script unknown.

The STDERR content will still be sent to the pass through callbacks, if there are any, before the exception is thrown.

[hollodotme]

Implemented & successfully tested on PHP 7.1 - 7.3. Released with v2.5.0.

[DjThossi]

Thank you. This makes work much easier.

[andybee]

I've just opened #27 before realising I wasn't on the 2.5 version. This has actually caused us a recent problem whereby we execute PHP scripts which use error_log('foo') and these are now causing the exception to be thrown and the response to be destroyed because PHP FPM sends those logs via STDERR.

Could this approach be made optional so, if we wish to silently monitor STDERR and not abort operation on receipt, we can?

[hollodotme]

@andybee Somehow I knew this (your request) was coming, when I implemented the solution above. 😬

I already have a solution in mind that would:

• separate STDOUT and STDERR output in pass through callbacks
• separate STDOUT and STDERR in the Response

In detail this could mean for a v2.6.0:

1. The pass through callback function declaration gets a second argument:

$callback = function( string $outputBuffer, string $errorBuffer ) {};

2. The Response gets two new methods getOutput() (which is identical to getRawResponse() and returns the output from STDOUT) and getError() which provides the output from STDERR
3. getRawResponse() could be deprecated in favor of consistent naming, and removed in v3.0.0.
4. The newly added ProcessManagerException will be restricted to the Primary script unknown error, as this definitely is unexpected behaviour in any case, IMO.

Once this done users can decide whether to throw exceptions on error output in the response or not.

I'd love to hear your (and maybe also @theseer 's) feedback regarding that approach. Before implementing it.

I think discussion should take place in #27 which I re-opened.

[theseer]

Going to reply in #27 then :)