search

8p / EightPointsGuzzleBundle

⛽️ Integrates Guzzle 6.x, a PHP HTTP Client, into Symfony
MIT License
440 stars 71 forks source link

Improvements to better support Symfony Flex #125

Closed ruudk closed 7 years ago

ruudk commented 7 years ago

I'm working on a new project that uses Symfony Flex / 4.0 (for testing purposes). I found out that this bundle doesn't really work nicely with it. As Symfony Flex / 4.0 will be released in november I thought it would be good to refactor this bundle a bit to better support it.

This is just a RFC, because I want to know what your vision is for the bundle.

A couple of things I changed:

Todo:

ruudk commented 7 years ago

@florianpreusner Any ideas how to proceed? I'd like to improve this bundle to better support Symfony Flex

florianpreusner commented 7 years ago

Hi @ruudk, first of all: Thanks a lot for your time and effort. Unfortunately I had no time to review this PR. I will try my best to share my opinion asap. My first impression is very good!

ruudk commented 7 years ago

@florianpreusner Thanks! Let me know. I will fix the tests once I have a green light :)

florianpreusner commented 7 years ago

Hi @ruudk! I checked your changes. I like them a lot! 👍

It would be awesome if you can change also the service aliases. We should merge the changes to v7. There will be some other changes too, for example: I want to drop PHP 5 support (https://github.com/8p/GuzzleBundle/pull/100).

ruudk commented 7 years ago

@florianpreusner haha, I was just visiting this PR to check for progress and there is your reply! Great :D

Do you mean the prefixed name of the services? guzzle_bundle > eight_points_guzzle_bundle

florianpreusner commented 7 years ago

@ruudk Right!

ruudk commented 7 years ago

@florianpreusner It's green!

florianpreusner commented 7 years ago

@ruudk Nice! Thanks for your time! What do you think about integrating your changes into v7? So I would like to change the base branch. Unfortunately, that means that some adjustments have to be done to be compatible with v7 (PHP 7) branch 😕

ruudk commented 7 years ago

@florianpreusner Done, rebased on v7.

florianpreusner commented 7 years ago

Going to release v7 by the end of this month. Going to replace master by v7 in two weeks.

gregurco commented 7 years ago

Rename GuzzleBundle to EightPointsGuzzleBundle to better follow the standard. This is not the official > GuzzleBundle and thus should use the namespace prefix. It's also easier for Symfony Flex to automatically install this. Without this change, Symfony Flex tries to install 2 bundles: 'EightPoints\Bundle\GuzzleBundle\GuzzleBundle' => ['all' => true], 'EightPoints\Bundle\GuzzleBundle\EightPointsGuzzleBundle' => ['all' => true],

As I see, this problem with installing 2 bundle was fixed by Fabien 21 days ago. Issue: https://github.com/symfony/flex/issues/160 PR: https://github.com/symfony/flex/pull/164 The test where guzzle-bundle was mentioned: https://github.com/symfony/flex/pull/164/files#diff-7e3f7cc32f32a7d0d94d96dfd18a00b5R49

I do not mind renaming, but let's think one more time. For now it's not in release and is this issue in flex the main reason of renaming? Because the issue was fixed and flex can work with this name and name is accepted by Fabien. @florianpreusner @ruudk any ideas?

ruudk commented 7 years ago

Great! Already thought it was a bug in flex. Still think this change is good to just better follow other bundles. Especially since this is not an official bundle.

florianpreusner commented 7 years ago

Going to rename once we merged new changes into master.