Skip to content

Conversation

joelwurtz
Copy link
Member

IMO I don't need it here, the standard (PSR7) is too contraignous to have a very different implementation on the message part (Request / Response) and since we use our own stream (no stream factory needed), using a message factory here is over engineering.

WDYT ? ping @sagikazarmark @dbu @Nyholm

@sagikazarmark
Copy link
Member

I would leave it there at least until we see how the PSR MessageFactories end up. Also, I would rather make an overall consensus and this is a BC break.

@@ -12,15 +12,14 @@
"php": "^5.5 || ^7.0",
"symfony/options-resolver": "^2.6 || ^3.0",
"php-http/httplug": "^1.0",
"php-http/message-factory": "^1.0.2",
"php-http/discovery": "^1.0"
"guzzlehttp/psr7": "^1.2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like us to have any dependency on Guzzle.

(except for dev-requirements)

@Nyholm
Copy link
Member

Nyholm commented Jul 31, 2016

I would like to keep the message factories as well. I like the end users to be able to choose what PSR-7 implementation to use. One reason is that Guzzle does not comply with PSR-7 to 100%.

@joelwurtz
Copy link
Member Author

I like the end users to be able to choose what PSR-7 implementation to use. One reason is that Guzzle does not comply with PSR-7 to 100%.

In fact i don't think it matters here, user can still choose it's implementation but it will change anything here.

this is a BC break.

I added a deprecated on the constructor so old way of constructing SocketClient still works

Also i still comply to the contract as i return a object compatible with PSR7.

The only downside of doing this here is someone can have multiple PSR7 implementations in their dependencies, IMO this is something that is affordable in exchange for easier and faster installation / adoption by end-users.

@Nyholm
Copy link
Member

Nyholm commented Jul 7, 2017

This PR has stalled for a year. Both me and Mark are 👎 . All our other clients do follow this pattern.

I will close this. Feel free to open it again if you want continue to discuss it.

@Nyholm Nyholm closed this Jul 7, 2017
@joelwurtz joelwurtz deleted the feature/remove-message-factory branch September 22, 2020 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants