Tag pre-existing guzzle clients about csaguzzlebundle HOT 4 CLOSED

PR-s are always welcome! ;)

Actually, the only reason I used the factory was because I actually forgot about some advanced features of the DIC component (configurators, you name it!). 😐

Why a configurator (or a factory)? Simply because there's no direct way to run $client->getEmitter()->attach($subscriber); on the client instance. So we need to delegate this to another service.

Basically, all we have to do is to add a new compiler pass which uses a specific tag (we could use csa_guzzle.client, it's fine by me), in order to automatically configure clients which are defined using the semantic configuration, or which use the tag.

This would also help removing the factory altogether in the long term (Maybe for version 2.0 of the bundle as this would break BC). In the mean time, we could use the configurator in the factory to configure the client's subscribers, and trigger a deprecation warning.

Regarding the tag, something like the following would register a list of subscribers:

<service id="my_client" class="GuzzleHttp\Client">
    <tag name="csa_guzzle.client" subscribers="debug,stopwatch,logger" />
</service>

Without the subscribers attribute, the default subscribers would be injected by the configurator. This would make things the easiest for migrating back and forth to other bundles to CsaGuzzleBundle.

from csaguzzlebundle.

@csarrazi yep, I agree that a configurator is the place tool. At first the "foo,bar,baz" syntax for threw me off a bit (vs having 3 tags each specifying a subscriber). But thinking through this I don't think there's much of a difference. And simplifying migration is a very good point!

from csaguzzlebundle.

As you mentioned, I actually considered using multiple tags. However, it felt less natural as the base logic is around registering a client, which will need to be configured by the configurator (boy does sentence that sound redundant!). So I opted for a comma-separated list as we cannot have two of the same attributes in a tag. :)

from csaguzzlebundle.

@csarrazi before I begin I wanted to ask for a sanity check. I don't think a single configurator will do. There are a few problems:

1. What if the service definition already declares a configurator? In that case the "old" one can't simply be replaced. It has to be wrapped instead and thus potentially making each configurator unique to a specific client;
2. Different clients will have different subscribers and I'm not seeing any way to declare extra arguments for the configurator;

This leaves us probably with a rather inelegant solution. A compiler pass goes over all tagged clients. For each client it creates a private configurator service. Now, the list of client-specific subscriber services get passed as constructor args. The configurator service is then injected to the current client.

BC can be mantained if the same pass adds all subscribers to the factory, i.e. keeping the current implementation.

What are Your thoughts?

from csaguzzlebundle.