Injection Inception, Change #1: Adding EventDispatcher to DI #7954
Conversation
diosmosis
added
c: Platform
|
Awesome, really easy to review :D
|
|
|
||
| $diConfig['observers.global'] = \DI\add($globalObservers); | ||
|
|
||
| StaticContainer::addDefinitions($diConfig); |
There was a problem hiding this comment.
Nitpicking: just to make it look more "familiar" for those that aren't with DI config, I would write it like that:
StaticContainer::addDefinitions(array(
'observers.global' => \DI\add($globalObservers),
));There was a problem hiding this comment.
$diConfig is set to provideContainerConfig(), so this would override the entire array (StaticContainer::addDefinitions does not use array_merge()).
|
I'm 👍 for merge when tests are OK. By the way we need to keep in mind that |
It's needed mainly because the proxy scripts don't create the environment themselves, the real entry points do. So we can't change it to our own environment, and must define the event handlers before the environment is created. Since I added the concept of EnvironmentManipulators in another PR, though, maybe I can re-use that for this. I'm no longer sure which should get merged first now, though :)
It doesn't do an array_merge(), did you intend it to when making the change? I can include that change here, since most test classes now create Environment instances directly, instead of using StaticContainer methods. (so extra definitions would not pile up if array_merge() was used) EDIT: Also should note that we pass an array of event handlers in proxy/piwik.php to TestingEnvironment::addHooks(), which we add items to in the aforementioned addHooks(), so nothing should get overwritten. |
diosmosis
force-pushed
the
test_env_di_1
branch
3 times, most recently
from
45c6415
to
c0640d5
Compare
…llObservers method. This method is only used to 'reset' the EventDispatcher, and is no longer necessary thanks to DI.
…ia the observers.global DI config entry.
…observers, since the container is not created at this point.
…d of calling Piwik::addAction, since the latter needs the DI container (which is not setup yet).
…required since the environment can be modified during tests, and these modifications must be removed after (eg, event observers can be registered). Since Plugin\Manager is in DI, we must also reload plugins. Integration/Plugin/ManagerTest.php was modified; order of plugins changed because Fixture::loadPlugins() will use TestingEnvironment to gather plugins, which sorts plugins by name.
…d since the Plugin\Manager is provided through DI.
Adding EventDispatcher to DI, allow defining event handlers in DI so they can be defined before the container is created, make sure to recreate the test environment for integration tests in setUp().
As the title states, this PR puts the EventDispatcher singleton into the DI container.
It includes the addition of a new config entry
'observers.global'that allows defining observers before the container is created. This is necessary for test environment setup, since we register event observers before the actual entry scripts create the container. It can also be useful to users who want to augment their Piwik, without actually creating a plugin (they can define observers in config/config.php).Tests may not pass yet, travis is still having a hard time.