netgen-layouts / layouts-core

Netgen Layouts enables you to build and manage complex web pages in a simpler way and with less coding. This is the core of Netgen Layouts, its heart and soul.
https://netgen.io/layouts
MIT License
36 stars 6 forks source link

php-http client dependancy #16

Closed wizhippo closed 4 years ago

wizhippo commented 4 years ago

Currently there is a requirement on php-http/socket-client.

Should this not be php-http/client-implementation?

php-http/socket-client can be a require-dev for testing.

emodric commented 4 years ago

Maybe. I'll test it! I remember that I had some errors before if I used php-http/client-implementation, but maybe I was at blame here.

emodric commented 4 years ago

Oh, I know why this is in requirements. friendsofsymfony/http-cache 2.x requires one php-http/client-implementation and we require php-http/socket-client in order to provide it for FOS HTTP Cache.

It was done to cut down on install instructions and not having to document that to use caching features in Layouts, you need to choose your own HTTP client, since by default (when I tested it originally, at least), none was installed by defualt. php-http/socket-client has minimal dependencies (no need for Curl e.g.) so I chose that one.

Does this makes sense? Is there a better way this could be handled?

wizhippo commented 4 years ago

For me this seems wrong as the end user should be able to pick their client of choice unless you are using some specific functionality of socket-client.

Right now as you state you are using php-http/socket-client which only exits in beta right now so if beta is not not part of part of the minimum-stability you get composer failing only to have to add a line like php-http/socket-client: ^2.0@beta or add beta to minimum-stability anyway which breaks the convenience you mentioned above.

emodric commented 4 years ago

That is true, yes. As long as php-http/socket-client is in beta, you will have to add it to your project manually.

What do you suggest? I would definitely not want to go through the trouble of explaining in docs need for installing additional packages to use some built in functionality.

wizhippo commented 4 years ago

Sorry no solution. php-http has no fallback to install a client if none has been required. Composer doesn't have a require this default if some other dependency is not met that I know of.

I think this is a shortcoming of php-http in general if you decide to opt in to using it. If you don't wish to use php-http as it is intended for your convenience I certainly can understand.

But strictly speaking I personally don't think a dependency should be dictating which client to use unless it is using some function specific to that client which layouts-core is not. That really is the whole point of php-http is it not?

But my current reason for the issue will disappear when socket-client is out of beta, though not sure what issue may arise if two clients end up installed because of the projects requirements and layouts-core.

emodric commented 4 years ago

I think this is a shortcoming of php-http in general if you decide to opt in to using it. If you don't wish to use php-http as it is intended for your convenience I certainly can understand.

The thing is, we don't wan't to use it, but have to, since FOS HTTP Cache bundle requires an implementation of it.

But strictly speaking I personally don't think a dependency should be dictating which client to use unless it is using some function specific to that client which layouts-core is not. That really is the whole point of php-http is it not?

Yes, I do agree. The point of requiring php-http/socket-client was not to dictate which implementation to use, though. It was to prevent exceptions when installing Layouts when no other implementations are required.

But my current reason for the issue will disappear when socket-client is out of beta

Coincidentally, there's an issue recently opened on php-http/socket-client to tag 2.0 stable: https://github.com/php-http/socket-client/issues/47 . We even require it specifically in our site repos due to @beta requirement: https://github.com/netgen-layouts/layouts-sylius-site/blob/master/composer.json#L31

though not sure what issue may arise if two clients end up installed because of the projects requirements and layouts-core.

Yep, this is something I thought of too. I'd like to think there would be no issue since you would still need to configure the client in Symfony DI (via project config) somehow to use it.

wizhippo commented 4 years ago

Thanks for looking. I'll close this as it's not a big deal to add the dependency for now and it will disappear when the beta is updated.

emodric commented 4 years ago

I have played a bit with the setup again, and it turns out that php-http/socket-client is not used at all, instead async-client-implementation was used, even if it is not required in FOS HTTP Cache composer.json, which is weird in itself. I presume this changed in FOS HTTP Cache sometime in the past, as I distinctly remember that using socket-client helped previously.

Anyway, to get it all working now and use e.g. Symfony Http Client, at minimum, I had to install a PSR-17 factory (e.g. provided by nyholm/psr7) and Guzzle Promises (guzzlehttp/promises).

This all makes it too much in my opinion to install by default, so I will just remove php-http/socket-client dependency and fallback to properly documenting how to configure FOS HTTP Cache.

emodric commented 4 years ago

Hm. I spoke too soon. It seems that FOS HTTP Cache bundle indeed cannot be installed without at least one php-http/client-implementation, even if it uses async client. I think this was the whole idea behind me requiring it: to allow composer install to pass.

eddie@abyss ~ $ composer require friendsofsymfony/http-cache
Using version ^2.9 for friendsofsymfony/http-cache
./composer.json has been updated
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
    - Installation request for friendsofsymfony/http-cache ^2.9 -> satisfiable by friendsofsymfony/http-cache[2.9.0].
    - friendsofsymfony/http-cache 2.9.0 requires php-http/client-implementation ^1.0 || ^2.0 -> no matching package found.

Potential causes:
 - A typo in the package name
 - The package is not available in a stable-enough version according to your minimum-stability setting
   see <https://getcomposer.org/doc/04-schema.md#minimum-stability> for more details.
 - It's a private package and you forgot to add a custom repository to find it

Read <https://getcomposer.org/doc/articles/troubleshooting.md> for further common problems.

Installation failed, reverting ./composer.json to its original content.

What I can do is replace php-http/socket-client with symfony/http-client. It can be installed on Symfony 3.4 & eZ Platform 2.5, so that's definitely a plus.

What do you think @wizhippo ?

wizhippo commented 4 years ago

symfony/http-client is the direction I'm moving with all my code.

emodric commented 4 years ago

Good! Let it be symfony/http-client then :)

emodric commented 4 years ago

@wizhippo Tagged 1.1.4 with the fix!