MessageFactory::createRequest signature

[dbu]

while working on the MessageHttpClient, i noticed that we have this signature:

createRequest(
        $method,
        $uri,
        $protocolVersion = '1.1',
        array $headers = [],
        $body = null
    );

I doubt that many people ever need to change the protocol version (if this should not be a setup instruction anyways). i would suggest that if this parameter is needed at all, it should be the last. method and uri are required, headers and body are more likely to be needed to be specified. as is, i would have to pass '1.1' in every createRequest call which feels strange

[hannesvdvreken]

Agreed

[sagikazarmark]

The reason behind this signature is that it is the same as you see it in an HTTP request.

[sagikazarmark]

I agree this is probably not that nice, but this way we enforce a proper request to be created. Keep in mind, that MessageFactories are probably not used widely, because they are rather a common interface for already existing factories (like diactoros and guzzle)

[dbu]

what is the order in the guzzle and diactoros factories?

[sagikazarmark]

Different order, I checked. Can't recall the exact one. Also, parameter types/number are not even the same in some cases.

[dbu]

did we tag an alpha of the message factory? we should decide on this one, potential for a lot of anger if we change after releasing the first alpha... i still tend to reorder, but i can also follow the logic we currently have.

[sagikazarmark]

No, but this is not strictly part of the contract, but we should probably align the basic repositories.

If you think we could profit from reordering then go for it. Keep in mind the following:

• change the documentation, currently it explains the logic I mentioned above
• check the createResponse method as well: the same logic is applied there, but if we rather do it the sane way than the logical one, then we should be consistent about it

[dbu]

guzzle does

    public function __construct(
        $method,
        $uri,
        array $headers = [],
        $body = null,
        $protocolVersion = '1.1'
    ) {

https://github.com/guzzle/psr7/blob/master/src/Request.php#L36-L42

diactoros is

public function __construct($uri = null, $method = null, $body = 'php://temp', array $headers = [])
and
public function withProtocolVersion($version)

https://github.com/zendframework/zend-diactoros/blob/master/src/Request.php#L33
https://github.com/zendframework/zend-diactoros/blob/1037ed1cb4e240eba665597e4c373e57fb9ea24c/src/MessageTrait.php#L71 (so no way to inject in constructor)

the order of guzzle seems good to me. order of headers before body seems good to me, even though you sometimes won't need body. its consistent with https://github.com/php-http/utils/blob/master/src/HttpMethodsClient.php also.

[sagikazarmark]

So basically protocol version gets to the end of argument list. Am I right?

[dbu]

yeah, looks like. then we happen to match the guzzle signature. unless @mtdowling would tell us that he thinks that is a bad idea, i would go for that.