-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Added loadModule.post event to loadModule(). #2821
Conversation
…latile conditions with loadModule.post.
Added unit test for loadModule.post event. I didn't see any other Event test within the ModuleManager -- I hope I put it in the right spot. Unit test would be necessary: if loadModule.post does not recognize the triggering module as "loaded", an infinite loop could occur in the code should that module get asked for again (think dependencies of dependents circle referencing original module). |
How do I handle this case? The build failed on "ZendTest\Crypt\RsaTest::testEncryptionWithOwnKeys" which is absolutely of no relation to my changes. Test worked for two other build versions. Is there a way to get the final test to run again? |
@guice don't worry about it; we look at failures closely to determine if they were introduced by the changeset, or timing-based failures (crypt and cache sometimes create those). |
Ah. Sweet. Thanks! |
Actually, I'm not sure that this is really needed. The ModuleManager event object contains the module just loaded when the loadModule event is triggered; you can retrieve it simply by calling: $module = $event->getModule(); within a listener. The only thing that cannot be done is verify that the module is now in the array of loaded modules; but the fact that you've been passed it should be guarantee enough. Based on the unit test you provided, I'm not certain what use case is desired that's not answered by simply listening on the loadModule event. Can you clarify, please? |
And this is the verification that I need: to be able to verify if a module is loaded, to prevent sub-dependencies from trying to reload a module again. The situation I ran into: I have a site, running with several modules (about a dozen). We're migrating to this structure, so at this time, several modules are dependent on other each. In our specific case, we have an Auth module, Albums, and say Remote. Auth will require Albums and Remote. The .post trigger was designed once Auth is loaded, it would load the dependencies, too; so it would load Albums, and Remote. The issue came into play when, say Albums, also lists Auth as a dependent. So now you're looking at this: Auth -> Albums -> Auth... If Auth was not viewed loaded, you end up with: Auth -> Albums -> Auth -> Albums -> Auth (infinite). .post is called after a module is "flagged" as loaded, so this loop won't occur; Auth -> Albums -> Auth (already loaded Done.); Auth -> Remote. This is the attach code, shared among all modules. public function init(\Zend\ModuleManager\ModuleManager $moduleManager) {
$events = $moduleManager->getEventManager();
// Once a module is called to load: the .post queue can be cleared
$events->clearListeners('loadModule.post');
$events->attach('loadModule.post', array($this, 'loadDependencies'));
} loadDependencies, for a simple example runs: foreach ($config['depends']['module'] as $module) {
$moduleManager->loadModule($module['name']);
} The dependencies and shared code are setup this way because you can come in to Albums first, and that will have to load Auth, and Auth would, in turn, attempt to load Albums and Remote. |
It sounds like the simpler fix would be to swap the order of lines 151 and 152. These currently read: $this->getEventManager()->trigger(ModuleEvent::EVENT_LOAD_MODULE, $this, $event);
$this->loadedModules[$moduleName] = $module; Swapping them would mean that the module would show up in the list of loaded modules from within a triggered event. Additionally, it appears to be backwards compatible -- I tried it just now and ran tests, and all looks good. @EvanDotPro -- does the above sound like a reasonable solution? If so, I can make that change, or @guice can change his PR to do this instead of adding the new event. @guice does this seem like a reasonable approach to you? |
@weierophinney That's reasonable to me. I wasn't sure if that order had any real relevance (e.g. the specific .pre and .post events for loadModules). |
@@ -88,7 +89,7 @@ public function getModule() | |||
/** | |||
* Set module object to compose in this event | |||
* | |||
* @param object $module | |||
* @param object $module |
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.
Why this large intendation?
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.
This was done by php-cs-fixer. My best guess is that it's aligning the variable up with the "next argument" due to the next line: @throws Exception\InvalidArgumentException.
Looking through the code changes, php-cs-fixer seems to line up all the argument lists.
Closed, as more appropriate fix implemented in #3030. |
I added a loadModule.post event that is trigged at the end of loadModule() within Zend\ModuleManager\ModuleManager.
While I found loadModules.post useful, there was no way to do the same when loading just one module manually. loadModule.post event is triggered right after loadFinished is set to true.
I required a post event due to loading module dependencies: a module would dynamically load, and after it loads, the post even will read its dependencies, and load those modules. loadModule event was called before a module officially "loads", so when a dependent, of a dependent, recursively called a module that was suppose to be loaded, "loadModule" event did not see the module loaded, and went through an infinite loop. "loadModule.post" will now recognize a module was already loaded, and bail-out before attempting to call itself again.