Skip to content
This repository has been archived by the owner on Oct 3, 2023. It is now read-only.

Add PSR-17 factory support #1

Open
mindplay-dk opened this issue Jun 26, 2019 · 3 comments
Open

Add PSR-17 factory support #1

mindplay-dk opened this issue Jun 26, 2019 · 3 comments

Comments

@mindplay-dk
Copy link

I've been looking at PSR-18 implementations, and this looks like exactly the sort of thing I was hoping for :-)

My only issue is the dependency on a specific PSR-7 implementation.

Would you consider adding support for PSR-17 factory interfaces?

If so, I might contribute a PR.

But I wanted to check with you first, because I figured maybe having no constructor dependencies for the client itself was a stated goal for you?

(It wouldn't matter to me personally, because everything we build uses constructor injection - we already have PSR-17 factories in our standard container registrations, and intend to register your client implementation as the default service behind the PSR-18 interface.)

Introducing it would be a breaking change, since the factories would need to be provided as constructor arguments to the client.

Let me know if you're interested? :-)

@fain182
Copy link
Owner

fain182 commented Jun 26, 2019

I am glad you appreciated my project :-)

But I wanted to check with you first, because I figured maybe having no constructor dependencies for the client itself was a stated goal for you?

The idea is to remove all the accidental complexity and suprises, to let the developer focus on making the http request. I don't think that choosing a PSR-7 implementation is a decision worth making for every project.
So I prefer to rely on a single PSR-7 implementation, but I am open to substitute it if you find some problem in the one that we are using now 🙂

Anyway I could be wrong, so I am interested in understanding: why do you want the ability to choose a PSR-7 implementation?

@mindplay-dk
Copy link
Author

So I prefer to rely on a single PSR-7 implementation, but I am open to substitute it if you find some problem in the one that we are using now

No, it's the one we've chosen as well.

The main point is, we don't want our code coupled to a particular implementation of PSR-7, because we can't control what third-party libraries use - and of course PSR-17 helps with that; but if we introduce other dependencies with a hard dependency on a particular PSR-7 implementation, the purpose of PSR-17 is defeated.

why do you want the ability to choose a PSR-7 implementation?

You can read about it in the PSR-17 meta and spec documents.

In our case, we've already once had to port dozens of libraries from hard dependencies on Zend Diactoros (for reasons I don't go into) and switch to Nyholm's package - switching was a lot of work and breaking changes, because the PSR-7 standard doesn't cover constructors, so we bridged it by switching to PSR-17 factory interfaces, rather than just switching to a different set of arbitrary class-names and constructor signatures, which ensures it's the last time we'll need to do this.

So it's in part about mitigating risk - though the main argument for PSR-17 is interoperability; we don't want a situation where another team (or third party) picked and different PSR-7 implementation, and we end up with two full implementations loaded at the same time. We also don't want inconsistent constructor calls which increases the risk of bugs - it's better if these follow a standard (PSR-17) and look and work the same everywhere.

Anyhow, if none of those things are a concern to you, it might be possible to keep it backwards compatible - for example, by using suggests for the PSR-7 implementation, and adding a withFactories() factory-method, with a built-in default for Nyholm's implementation - while allowing to optionally substitute any other implementation?

@fain182
Copy link
Owner

fain182 commented Jun 28, 2019

Ok, I understand your thought now.
I think it could be a good idea to have a default implementation and the option to substitute using a withFactories() method, like you said.
A PR on this issue is welcome 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants