8p / EightPointsGuzzleBundle

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

Create specific events per client #283

Closed rpkamp closed 4 years ago

rpkamp commented 4 years ago

For the PRE_TRANSACTION and POST_TRANSACTION events I'd like to propose two events should be fired for each request/response, instead of one.

  1. A generic event that fires regardless of which client is active, and
  2. A client-specific event that can be handled by listeners/subscribers subscribing specifically to that client

The generic event can be used by listeners who want to do stuff like logging, tracing, profiling, auditing, etc. The specific events can be used by listeners/subscribers that want to do something specific for a specific client (like auth headers etc).

Basically:

$this->eventDispatcher->dispatch(
    GuzzleEvents::PRE_TRANSACTION,
    $preTransactionEvent
);
$this->eventDispatcher->dispatch(
    GuzzleEvents::PRE_TRANSACTION . '.' . $this->serviceName,
    $preTransactionEvent
);

So suppose I have a client called payment and a client called crm. I can:

  1. Listen to eight_points_guzzle.pre_transaction to get all events for both (and future) clients
  2. Listen to eight_points_guzzle.pre_transaction.payment for all events from the payment client only
  3. Listen to eight_points_guzzle.pre_transaction.crm for all events from the crm client only

This would have several advantages:

  1. Listeners don't need to filter for the correct client anymore (if ($event->getServiceName() !== $this->serviceName) { return; }), they can subscribe to a specific client and not be bothered with events from other clients (which indeed means that \EightPoints\Bundle\GuzzleBundle\Events\GuzzleEventListenerInterface can be deprecated)
  2. It's easy to implement subscribers because they can use the same events without needing any specific interfaces
  3. Once \EightPoints\Bundle\GuzzleBundle\Events\GuzzleEventListenerInterface is removed, \EightPoints\Bundle\GuzzleBundle\DependencyInjection\Compiler\EventHandlerCompilerPass can be removed as well, as it doesn't serve a purpose anymore.

Thoughts? :slightly_smiling_face:

gregurco commented 4 years ago

I think the idea is good. You are right, that in this case we can remove a lot of code and to reduce the complexity. If @florianpreusner is ok with change, then we can: 1) do one more commit with deprecation on v7 and one more minor version 2) to refactor events in master and this change will be in v8 3) describe guide how to upgrade events from v7 to v8 in UPGRADE.md

@florianpreusner what do you think? do you see any bottlenecks?

florianpreusner commented 4 years ago

A very nice idea! I don't see any disadvantages. 👍 👍 👍

gregurco commented 4 years ago

@rpkamp please let us know, if you have desire to integrate these change in v8?

rpkamp commented 4 years ago

Yes, I'd be happy to do that :)

But I'll first send a PR for the deprecation in 7.x :smile:

gregurco commented 4 years ago

@rpkamp Ok, greate! If you want, I can do that. Just write.

rpkamp commented 4 years ago

No, I'm happy to do it :slightly_smiling_face:

rpkamp commented 4 years ago

This done now (PRs https://github.com/8p/EightPointsGuzzleBundle/pull/287 and https://github.com/8p/EightPointsGuzzleBundle/pull/284), I'll close this issue :)