Skip to content

[EventDispatcher] Add an interface for the ContainerAwareEventDispatcher #11870

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
* @author Bernhard Schussek <bschussek@gmail.com>
* @author Jordan Alliot <jordan.alliot@gmail.com>
*/
class ContainerAwareEventDispatcher extends EventDispatcher
class ContainerAwareEventDispatcher extends EventDispatcher implements ContainerAwareEventDispatcherInterface
{
/**
* The container from where services are loaded
Expand Down Expand Up @@ -52,16 +52,7 @@ public function __construct(ContainerInterface $container)
}

/**
* Adds a service as event listener
*
* @param string $eventName Event for which the listener is added
* @param array $callback The service ID of the listener service & the method
* name that has to be called
* @param int $priority The higher this value, the earlier an event listener
* will be triggered in the chain.
* Defaults to 0.
*
* @throws \InvalidArgumentException
* {@inheritdoc}
*/
public function addListenerService($eventName, $callback, $priority = 0)
{
Expand Down Expand Up @@ -132,10 +123,7 @@ public function getListeners($eventName = null)
}

/**
* Adds a service as event subscriber
*
* @param string $serviceId The service ID of the subscriber service
* @param string $class The service's class name (which must implement EventSubscriberInterface)
* {@inheritdoc}
*/
public function addSubscriberService($serviceId, $class)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\EventDispatcher;

/**
* A ContainerAwareEventDispatcher is able to load the listeners
* and subscribers from the dependency injection container.
*
* @author Tristan Darricau <tristan@darricau.eu>
*/
interface ContainerAwareEventDispatcherInterface extends EventDispatcherInterface
{
/**
* Adds a service as event listener
*
* @param string $eventName Event for which the listener is added
* @param array $callback The service ID of the listener service & the method
* name that has to be called
* @param int $priority The higher this value, the earlier an event listener
* will be triggered in the chain.
* Defaults to 0.
*
* @throws \InvalidArgumentException
*/
public function addListenerService($eventName, $callback, $priority = 0);

/**
* Adds a service as event subscriber
*
* @param string $serviceId The service ID of the subscriber service
* @param string $class The service's class name (which must implement EventSubscriberInterface)
*/
public function addSubscriberService($serviceId, $class);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\EventDispatcher\Debug;

use Symfony\Component\Stopwatch\Stopwatch;
use Psr\Log\LoggerInterface;

use Symfony\Component\EventDispatcher\ContainerAwareEventDispatcherInterface;

/**
* Collects some data about event listeners.
*
* This event dispatcher delegates the dispatching to another one.
*
* @author Tristan Darricau <tristan@darricau.eu>
*/
class TraceableContainerAwareEventDispatcher extends TraceableEventDispatcher implements ContainerAwareEventDispatcherInterface
{
/**
* Constructor.
*
* @param ContainerAwareEventDispatcherInterface $dispatcher An EventDispatcherInterface instance
* @param Stopwatch $stopwatch A Stopwatch instance
* @param LoggerInterface $logger A LoggerInterface instance
*/
public function __construct(ContainerAwareEventDispatcherInterface $dispatcher, Stopwatch $stopwatch, LoggerInterface $logger = null)
{
parent::__construct($dispatcher, $stopwatch, $logger);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really necessary to implement this constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, a StopwatchInterface would solve BC causes in the future ;)

didn't see the comment for some reason

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But a such interface does not exist and I'm not sure that it's in the scope of this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I stand by my question, is it really necessary to implement then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal here is not to prevent BC but to prevent the use of this class with an instance of EventDispatcherInterface instead of an instance of ContainerAwareEventDispatcherInterface (or we could throw a Fatal error late and "randomly" at the runtime)

}

/**
* {@inheritdoc}
*/
public function addListenerService($eventName, $callback, $priority = 0)
{
$this->dispatcher->addListenerService($eventName, $callback, $priority);
}

/**
* {@inheritdoc}
*/
public function addSubscriberService($serviceId, $class)
{
$this->dispatcher->addSubscriberService($serviceId, $class);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ class TraceableEventDispatcher implements TraceableEventDispatcherInterface
{
protected $logger;
protected $stopwatch;
protected $dispatcher;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct me if I'm wrong, but I think private to protected needs a very good use case in order to pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to implement addListenerService and the other methods added by the "ContainerAware part" we need to access to the real dispatcher and there is only to way to do that:

  1. Make it protected instead of private
  2. Add a getters for the dispatcher.

And I think that to have a consistent api it's better to make it protected (also, the logger and the stopwatch object are protected and not private, considering that I don't think that it's more consistent to have the dispatcher procted as well).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they remain protected due to BC. Personally I'm against protected because it's pretty close to making it public, thus killing encapsulation. As far as I know, @api is for methods, that's what you talk to, even when extending.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to talk about a consistent implementation*
But yeah, I agree with you about protected vs private. Here the best visility should be package but it does not exist PHP :) so private or protected...


private $called;
private $dispatcher;

/**
* Constructor.
Expand Down