8p / EightPointsGuzzleBundle

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

[UPDATE] Add plugins #183

Closed Neirda24 closed 6 years ago

Neirda24 commented 6 years ago
Q A
Bug fix no
New feature no
BC breaks no
Deprecations no
Tests pass yes
Fixed tickets -
License MIT

Still need to write some README file on those plugins to explain what they do...

florianpreusner commented 6 years ago

Nice! 👌

Neirda24 commented 6 years ago

@florianpreusner : done

gregurco commented 6 years ago

@Neirda24 let me check the code. I need 1-2 days. I will open issues with suggestions if I will find something to fix/improve. Are you ok with that?

Neirda24 commented 6 years ago

@gregurco : Sure no problem

gregurco commented 6 years ago

@Neirda24 let's suspend this PR for a while, because this plugin system doesn't work in symfony 4. The problem is described in #182. I'm trying to find the way to do the same system in SF4 but for now I can write that 100% it will be changed. I hope changes will be needed only in main bundle but there is a chance that plugins should be adapted too :slightly_frowning_face:

Neirda24 commented 6 years ago

@gregurco : no problem. but then why don't we patch the current version of the main bundle to remove sf4 compatibility ? Because whatever changes you have to make, it will cause Breaking changes. So These plugins along with the others must also says that they are compatible with this version of the main bundle only. So it is mergeable don't you think ? I use them in symfony 3.3 so far but we plan to move to sf4. So I will have to upgrade these to make it work at some point.

gregurco commented 6 years ago

@Neirda24 so, I don't think it's good idea to break support of SF4 in current version of current bundle, because in general it works with SF4 and the plugin system doesn't create fatal errors or some breaks. Plugin system simply doesn't have possibility to be used in SF4, because there user hasn't control on initialization of bundles (possibility to pass something to constructor of bundle). I think that it's better to remove sf4 compatibility in plugins (but it's not obligatory, while there are no users with problems). But you are right, when we will find the solution we will release new major version, if it will break compatibility.

gregurco commented 6 years ago

@Neirda24 I opened issues with suggestions in your repositories :slightly_smiling_face:

gregurco commented 6 years ago

@Neirda24 I merge this PR. I didn't manage to find solution for SF4 but I'm trying. Thank you for contribution :+1:

Toflar commented 6 years ago

Here's what I did in my Kernel::registerBundles():

         $contents = require $this->getProjectDir().'/config/bundles.php';
         foreach ($contents as $class => $envs) {
             if (isset($envs['all']) || isset($envs[$this->environment])) {
-                yield new $class();
+                if ($class === EightPointsGuzzleBundle::class) {
+                    yield new $class([
+                        new GuzzleBundleCachePlugin(),
+                    ]);
+                } else {
+                    yield new $class();
+                }
             }
         }
gregurco commented 6 years ago

@Toflar yep, I thought about that but I don't think it's good idea to ask users to modify src/Kernel.php file and to define plugins in this file. Yep, it can be as temporary solution but not as final one :slightly_frowning_face:

Toflar commented 6 years ago

The Kernel belongs to your local app, it's designed to be modified. I don't see the problem. You also have to adjust the container etc. there? :)

gregurco commented 6 years ago

@Toflar hm, yep, it's in local app but it's kernel, something very-very basic :) do you know any example where symfony doc propose to modify this file?

Toflar commented 6 years ago

How else would you e.g. register interfaces for autoconfiguration etc.

https://symfony.com/doc/current/configuration/front_controllers_and_kernel.html#the-kernel-class

It's perfectly fine to adjust it.

gregurco commented 6 years ago

@Toflar hm, ok, I'm reading this documentation and I will think about modification of kernel as real solution. I will write here my ideas to discuss it with you :slightly_smiling_face: