SapiStreamEmitter, consistent method arguments to call from Server::listen()

[sasezaki]

currently (1.3.10), in Server class.

$this->getEmitter()->emit($response, $bufferLevel); 

but, SapiStreamEmitter::emit() defines

public function emit(ResponseInterface $response, $maxBufferLength = 8192)

despite setting $server->setEmitter(new \Zend\Diactoros\Response\SapiStreamEmitter());

emit method argument is $response, $bufferLevel (not $maxBufferLength).

[weierophinney]

Currently, cannot merge this as-is, due to BC breakage, and lack of tests for the new feature. That can be solved with one slight change, as noted.

[weierophinney]

For backwards compatibility, the last two arguments MUST be swapped in order. Otherwise, somebody providing a $maxBufferLength value with the existing signature will now be providing a $maxBufferLevel value instead, which could have unexpected repurcussions.

[weierophinney]

Okay, after looking at Server.php, SapiEmitter.php, and SapiStreamEmitter.php, I finally see what's going on. Server calls emit() with the response and buffer level, which is fine for the SapiEmitter, but cause problems in the SapiStreamEmitter, as that emitter is expecting the buffer length.

More problematic is we're passing this value, but the EmitterInterface::emit() definition only defines a single argument. I have a vague recollection that when we added output buffering awareness to the SapiEmitter, we'd already released a stable 1.0, which is why we made the argument optional.

I'm okay with this change, but will schedule it for 1.5.0 so we can draw attention to the breakage. I'll add tests when merging, as I have an idea on how to do it.

[weierophinney]

Right here is a good indication of why the order must be swapped; any change to how existing tests are written means breakage. If you swap the order, these tests will not need to change.

[weierophinney]

I need a test for this new behavior.

[sasezaki]

I opend #246 for flush test

[sasezaki]

I open other PR #245 for another approach.

[moufmouf]

Hey @MWOP !

Just ran into this issue today. The fact that $bufferLevel is passed instead of $maxBufferLength is really problematic and makes SapiStreamEmitter almost unusable.
In my experience, $bufferLevel === 2. This means that PHP will flush files 2 bytes per 2 bytes. On a gigabyte connection, that means that transferring a 90MB file took 1 minute (instead of expected 1 second).

I understand that you don't want to break backward compatibility. However, the other workaround proposed by @sasezaki in #245 is less clean (see https://github.com/sasezaki/zend-diactoros/blob/4bd9618567bec75ef52c11232eb74a25ab727937/src/Server.php#L172-L176 => @sasezaki is testing the instance of the class to know what parameters to pass... not really OO)

A quick check at Packanalyst tells me that the SapiStreamEmitter class is almost not used in OO projects.

http://packanalyst.com/class?q=Zend%5CDiactoros%5CResponse%5CSapiStreamEmitter

I'm pretty sure usage must be similar with close source projects.

Given the fact that the SapiStreamEmitter is currently too slow to be of any use and that it is almost never used outside of Diactoros, is it possible to break backwards compatibility on purpose?

Alternatively, a solution might be to deprecate the SapiStreamEmitter and replace it with a SapiStreamEmitter2 class?

[weierophinney]

Merged to develop for release with 1.5.0; thanks @sasezaki and @moufmouf .