PSR-7 Compatibility?

[mtymek]

Looks like this library comes with it's own implementation of HTTP messages, almost identical to those defined in PSR-7 standard, but not compatible on interface level. I can see that:

• UriInterface can be used directly
• RequestInterface and ResponseInterface are almost compatible
• streams are different, as they need to support async operations

I think it is worth to build building some kind of bridge between this implementations, for the sake of compatibility. It won't be very elegant if you think about good OOP practices, but it's going to be very pragmatic - say someone develops PSR-7 library for detecting mobile devices using headers, it could be used directly from within icicle/http app.

This "bridge" can be implemented in many different ways. As an example, Request class could have following methods:

public function getBody() 
{
    throw new RuntimeException("Synchronous stream is not supported");
}

public function getAsyncBody() // ...or whatever name would be better
{
    return $this->stream;
}

You get incompatible getBody(), but everything else (headers, uri) can be used as always.

What do you think? I could create a PR with example bridge implementation.

[trowski]

I really like the idea of having a PSR-7 adaptor available, but I'm not sure it should be included in this package. If the user only writes asynchronous code, then there would be no reason to have the adaptor available or have a dependency on psr/http-message. It is incredibly easy to block a script if one starts using code that was written to be synchronous, so I want to make it more obvious to the user what the consequences of using such code could have. However, having it in as a separate package that is linked from this package's documentation sounds like a fantastic idea.

I could set up a separate repo for the adaptor and push a basic project skeleton. You could do a PR against that, does that sound reasonable?

Here's some of what I had in mind to get you started:

use Icicle\Http\Message\MessageInterface;

class SynchronousMessage implements \Psr\Http\Message\MessageInterface
{
    private $message;

    private $body;

    public function __construct(MessageInterface $message)
    {
        $this->message = $message;
        $this->body = new SynchronousStream($message->getBody());
    }

    // Other PSR-7 MessageInterface methods...

    public function getBody()
    {
        return $this->body;
    }

    public function getAsyncMessage() // Or some other name
    {
        return $this->message;
    }
}

use Icicle\Loop;
use Icicle\Stream\ReadableStreamInterface;

class SynchronousStream implements \Psr\Http\Message\StreamInterface
{
    private $stream;

    public function __construct(ReadableStreamInterface $stream)
    {
        $this->stream = $stream;
    }

    public function read($length)
    {
        $promise = $this->stream->read($length);

        // Blocks until promise is resolved.
        while ($promise->isPending()) {
            Loop\tick(true);
        }

        $result = $promise->getResult();

        if ($promise->isRejected()) {
            throw $result;
        }

        return $result;
    }

    // Similar methods for write and seek.

    // Other PSR-7 StreamInterface methods...

    public function getAsyncStream() // Or some other name
    {
        return $this->stream;
    }
}

[mtymek]

Interesting approach. Sure, prepare a skeleton and I will send a PR.

Still thinking, is dependency to psr7 that bad? You could at least reuse URI interface, as it is exactly the same as in PSR7 implementations.

[trowski]

I avoided a dependency on psr/http-message because I didn't want to confuse users, thinking that the library was completely PSR-7 compatible, but I also wanted the interface to be familiar so I went with a similar interface. Note that there are some differences in thrown exceptions, since I think throwing InvalidArgumentException for everything is rather silly, especially when the library has to actually build HTTP messages, not just import arrays from the SAPI.

Additionally, I intend to release v1 and v2 of Icicle (and all components for Icicle) simultaneously with v1 for PHP 5.5+ and v2 for PHP 7. The PHP 7 version will have type hints on methods and massive performance improvements. Assuming I did this to all the HTTP message interfaces, UriInterface would not be compatible with the PSR-7 interface.

I created a new repo and committed a project skeleton and a little bit of code based on what's above at https://github.com/icicleio/Psr7Adaptor. Write any adaptor classes you think would be useful. Message, Request, Response, and Uri will all need to be wrapped. Also, something to convert PSR-7 messages back to Icicle messages would make sense, but an async stream would have to be provided with the message.

Feel free to send me an email, DM me on twitter, or we could talk on gitter.im.

[mtymek]

I still think that sharing code with PSR-7 implementation is not bad - they are focused on processing HTTP messages which are almost the same here, they solve common problems like security (see (here)[https://github.com/zendframework/zend-diactoros/blob/master/src/HeaderSecurity.php] and (here)[http://framework.zend.com/security/advisory/ZF2015-04]). I feel that if someone is able to use your library, he will also read the manual and understand that it is different :)
Anyway, it's your call :-)

Before I go into coding, could you rename this package to something like psr7-bridge? "Bridge" is common term for package connecting two others, while adapter (btw, usually spelled with "e", at least in most what I've seen in PHP: ZF, Symfony...) is usually reserved for something that can be plugged in and replaced (see Auth adapters in ZF2: https://github.com/zendframework/zend-authentication/tree/master/src/Adapter)

[trowski]

I think that having a bridge package to interfaces that are 100% compatible with the synchronous interfaces of PSR-7 is the best solution. I've contributed to the zend-diactoros project before and probably will contribute again. :) I plan on keep a close eye on that package to ensure I incorporate any security fixes and contribute any fixes I make here.

Most of what the linked class does is taken care of by this line: https://github.com/icicleio/Http/blob/master/src/Message/Message.php#L326 However, I will definitely take another look to make sure I've covered that vulnerability.

I very much agree with the name change, bridge is a much better term, and doesn't have the spelling ambiguity problem. Already done: https://github.com/icicleio/Psr7Bridge. I might change React Adaptor to React Bridge as well.

[mtymek]

Understood :)

BTW, you can use regex from my latest patch to zend-diactoros - it works faster than their current implementation:

// validate
if (preg_match('/[^\x09\x0a\x0d\x20-\x7E\x80-\xFE]/', $value)) {
    return false;
}

[mtymek]

Closing this issue, to be followed up in new repo.

[Crell]

Hm. Thanks to @mtymek for pointing me here. I have to disagree with twowski, though. Having an interface that looks and acts almost like PSR-7, but isn't, is more confusing that just being totally different.

I don't follow why the streams area an issue. As for PHP 7 and scalar typing, I am all for adding that in time but that is best done PHP-globally. I specifically pushed for PSR-7 to have single, predictable return types specifically to make a PHP 7 version of it in the future easier. If anything, I'd petition FIG to start releasing PHP 7-ified versions of its interfaces for those who want the extra pedantry (including me).

I think the current approach actually makes it more confusing, not less.

[trowski]

I went with a PSR-7-like interface because I thought it would be familiar to users and immutability seemed like a good idea.

Streams are an issue because in asynchronous code you cannot call StreamInterface::read() and block while waiting for a string. Streams could be made non-blocking (immediately returning an empty string if no data is available), but that's not enough: the stream object needs some method to poll for available data. I could extend the PSR-7 stream interface with a method for polling, but then you'd be required to use only streams of that interface in async code, defeating the whole point (interoperability).

I'm open to suggestions on how to make the interfaces different to avoid confusion, however, the interfaces of PSR-7 in general make a lot of sense. This package might benefit from mutable messages, simply because message objects are built in parts, requiring many clones before producing the final object to be sent to the client or server.

Adding types in PHP 7 is a minor nicety of having interfaces specific to this package and was not a major contributing factor in the decision. I too would like to see PHP 7 versions of the FIG interfaces with all the necessary types added to prototypes.