Skip to content

Dependency on psr/http-message-implementation #104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Nyholm opened this issue Jan 5, 2016 · 13 comments
Closed

Dependency on psr/http-message-implementation #104

Nyholm opened this issue Jan 5, 2016 · 13 comments
Labels
Milestone

Comments

@Nyholm
Copy link
Member

Nyholm commented Jan 5, 2016

I feel like we need a dependency on psr/http-message-implementation but I can not decide what package that should have that dependency. Technically no package require an implementation because that is one of our goals, but in practice you can not use any package without an implementation of PSR-7.

The scenario is that an API library uses Httplug it has dependencies like:

    "require": {
        "php": "^5.5|^7.0",
        "php-http/client-implementation": "1.0",
        "php-http/httplug": "v1.0.0-beta",
        "php-http/message": "^0.2.1",
        "php-http/discovery": "^0.6"
    },

The user tries to install the library and will first get an error like:

Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - The requested package php-http/client-implementation could not be found in any version,
    there may be a typo in the package name.

So he googles the error and finds our our documentation saying he need a client. He tries to install the socket-client.

$ composer require php-http/socket-client

Everything installs but when one tries to use the library which have auto discovery you will get an exception:

Uncaught exception 'Http\Discovery\NotFoundException' with message 'Resource of type "Http\Message\MessageFactory" not found' in /Users/tobias/Workspace/tmp/vendor/php-http/discovery/src/ClassDiscovery.php:114

This will happen because we have no implementation of PSR-7 installed. The user has faced a lot of issues and the experience of Httplug is that it is hard do use and understand.

I would suggest we add a dependency on psr/http-message-implementation on one of following:

  • Httplug
  • Message factory
  • Discovery
  • All clients/adapters
@sagikazarmark
Copy link
Member

If a client needs a dependency, the actual dependency in the class is a message factory. In an ideal case, a message factory is shipped with a message implementation.

So I would say, message implementation should be required in clients, where we actually need them (either through message factories or not).

However, this will still have a problem open: if there is no message factory in the package, or no puli config is present, the problem will still persist. So if discovery is used, a message implementation is not enough: there must be a message factory available as well (for Guzzle and Diactoros available in the message package, the error message contains instructions for installing it) AND it must be registered in puli.

To mitigate this problem (apart from adding message implementation requirement to clients where needed), we should clearly document this and have an easy way, where we just instruct the user to install php-http/message and one of guzzle or diactoros.

@Nyholm
Copy link
Member Author

Nyholm commented Jan 5, 2016

So if discovery is used, a message implementation is not enough: there must be a message factory available as well AND it must be registered in puli.

So the php-http/discovery should have a dependency on a new virtual package php-http/factory-implementation. And a package that provides php-http/factory-implementation (like our php-http/message) should depend on psr/http-message-implementation.

This will also solve the issue with the API library having a dependency on php-http/message.

@sagikazarmark
Copy link
Member

So the php-http/discovery should have a dependency on a new virtual package php-http/factory-implementation

I think that adds unnecessary complexity. Also:

In an ideal case, a message factory is shipped with a message implementation.

Guzzle and Diactoros are two special cases, funny or not, they are the most problematic and the most popular ones.

@Nyholm
Copy link
Member Author

Nyholm commented Jan 5, 2016

So the php-http/discovery should have a dependency on a new virtual package php-http/factory-implementation

I think that adds unnecessary complexity.

Yeah, it is complex, but that's how it is. php-http/discovery needs a factory to work. And our factory needs a psr/http-message-implementation.

In an ideal case, a message factory is shipped with a message implementation.

Okey. But that does not address the issue with discovery..


Correction from above:

So the php-http/discovery should have a dependency on a new virtual package php-http/factory-implementation. And a package that provides php-http/factory-implementation (like our php-http/message) should depend on psr/http-message-implementation.

So the php-http/discovery should have a dependency on a new virtual package php-http/factory-implementation. And our php-http/message should depend on psr/http-message-implementation.

@sagikazarmark
Copy link
Member

Yeah, it is complex, but that's how it is

Another problem: what if you don't need messages, only clients? Are you still forced to install a message factory?

Following this logic, httplug and a client-implementation should be required too.

The fact that a message factory is not found by the message factory is not necessarily wrong. It indicates that you don't have one installed which is an error case YOU have to solve.

Factory implementation virtual package does not really make sense if it is essentially the same as the message virtual package. Again, guzzle and diactoros are unfortunate exceptions.

The message package does not need a message implementation either. The only justification would be having guzzle and diactoros factories in the package, but you can't install any implementations to use those. For the rest of the package, an implementation is not necessary.

@Nyholm
Copy link
Member Author

Nyholm commented Jan 5, 2016

Another problem: what if you don't need messages, only clients? Are you still forced to install a message factory?

Hm. I did not think of that scenario. You are correct.

Factory implementation virtual package does not really make sense if it is essentially the same as the message virtual package. Again, guzzle and diactoros are unfortunate exceptions.

That is correct, but as stated: Those exceptions are the vast majority of the cases.

I'm convinced that my idea with php-http/factory-implementation has some flaws. Thank you.

To mitigate this problem, we should clearly document this and have an easy way, where we just instruct the user to install php-http/message and one of guzzle or diactoros.

Lets!

@sagikazarmark
Copy link
Member

That is correct, but as stated: Those exceptions are the vast majority of the cases.

Yes, that concerns me as well. I think most of people will want to use Guzzle 6 anyway. We already have place for a quick start guide (tutorials) where we can create examples. Hopefully people will use example code instead of just installing stuff without understanding. You and me/we could generally just install stuff, but that's a LogicException: leads to the solution immediately if something is wrong.

@joelwurtz
Copy link
Member

In psr/log, psr/cache and other figs standard they don't require the implementation in the composer.json so i'm also against this.

@dbu
Copy link
Contributor

dbu commented Jan 26, 2016

the set up can be tricky in both cases. either you end up with cryptic composer messages or with cryptic class not found errors. as long as composer does not specifically explain what went wrong when a virtual package is required i think i can live with not specifying it.

if composer starts to improve there, we should reconsider.

@sagikazarmark
Copy link
Member

Httplug does not require an implementation, only the interface.

Also, this issue is not really about requiring an implementation, but simplifying the installation process. With the recent changes of discovery and message this is probably a bit different now.

@sagikazarmark sagikazarmark modified the milestone: 1.0.0 Jan 26, 2016
@Nyholm
Copy link
Member Author

Nyholm commented Jan 26, 2016

It is a bit different now and I understood the reasoning why we are doing right.

We need to make sure to document this clearly until we can merge factory classes to Guzzle/Diactoros.

@sagikazarmark
Copy link
Member

I would say open a documentation ticket then and close this as in it's current form, it's a blocker for the stable.

@Nyholm
Copy link
Member Author

Nyholm commented Jan 26, 2016

👍

Nyholm pushed a commit to Nyholm/httplug that referenced this issue Dec 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants