-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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); | ||
} | ||
|
||
/** | ||
* {@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 |
---|---|---|
|
@@ -28,9 +28,9 @@ class TraceableEventDispatcher implements TraceableEventDispatcherInterface | |
{ | ||
protected $logger; | ||
protected $stopwatch; | ||
protected $dispatcher; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to implement
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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to talk about a consistent implementation* |
||
|
||
private $called; | ||
private $dispatcher; | ||
|
||
/** | ||
* Constructor. | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#11870 (comment)
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ofContainerAwareEventDispatcherInterface
(or we could throw a Fatal error late and "randomly" at the runtime)