Skip to content
This repository has been archived by the owner on Sep 15, 2022. It is now read-only.

Suggest guzzle/guzzle instead of require it #223

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iamluc
Copy link
Contributor

@iamluc iamluc commented Jan 30, 2017

The hard dependency on Guzzle 3 blocks the usage of newer versions.

I would like to suggest the package instead of require it as an hard dependency.

@Nek-
Copy link
Contributor

Nek- commented Jan 31, 2017

I agree this is a problem. A more serious fix is to switch to guzzle 6.

Also this fix is invalid because there is nothing in the code that test that guzzle is actually here in the good version. Usage of FriendlyContext will result in weird errors.

A last thing about this patch is that people using Symfony3 will never be able to use he API part (which is probably the best part of friendlycontext). This will generate frustration, issues, and so on.

@iamluc
Copy link
Contributor Author

iamluc commented Jan 31, 2017

I used the EntityContext succesfully using my fork, so the extension is loaded correctly by behat even if guzzle is not installed.
But I agree we could need more checks to avoid some crashes.

The ApiContext will still be usable, but users will have to require guzzle themself. That's why I modified the documentation: https://github.com/KnpLabs/FriendlyContexts/pull/223/files#diff-b89a5bb9165e004541d763d27d5c7da4

For Guzzle 6, I started the migration (#224) but I don't have the time right now (And I don't use the ApiContext).

Finally, to avoid similar issues (What I do if I use guzzle 5 on my project ?) in the future, I think it is a good idea to make some dependencies optionals.

@Nek-
Copy link
Contributor

Nek- commented Jan 31, 2017

I agree with this PR. Agree with the fact that a fork is needed. Agree with guzzle 6 update. Agree with dropping dependencies...

I just think dependencies missing errors really need to be handle. :-)

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

Successfully merging this pull request may close these issues.

2 participants