Improve module manager to accept instance #3814

Closed
wants to merge 3 commits into
from

Projects

None yet

5 participants

@blanchonvincent
Contributor

Hi,

For the performance and to make learning easier, we can accept modules instances :

Before :

<?php
return array(
    'modules' => array(
        'Application',
        'Cron',
        'Administration',
        'Blog',
    ),
    'module_paths' => array(// my paths),
);
?>

Now, i do :

<?php
return array(
    'modules' => array(
        'Application' => new Application\Module(),
        'Cron' => new Cron\Module(),
        'Administration' => new Administration\Module(),
        'Blog' => new Blog\Module(),
    ),
);
?>

There is no BC, more natural, and perf little better.

@EvanDotPro EvanDotPro was assigned Feb 19, 2013
@ralphschindler
Member

I like the explicitness, but how does your PHP know where these objects are located on disk? The inclusion of this particular application.ini file is before any kind of autoload munging. This would seem to imply that module/ is in your include_path and you have a special autoloader for it?

@blanchonvincent
Contributor

@ralphschindler if i use init_autoloader.php like the skeleton (with an autoloader : classmap autoloader, standard autoloader, etc.), there is no problems

@weierophinney weierophinney commented on an outdated diff Feb 19, 2013
library/Zend/ModuleManager/ModuleManager.php
{
+ $moduleName = $module;
+ if(is_object($module)) {
@weierophinney
weierophinney Feb 19, 2013 Member

Add space between statement and conditional.

@weierophinney weierophinney and 1 other commented on an outdated diff Feb 19, 2013
library/Zend/ModuleManager/ModuleManager.php
{
+ $moduleName = $module;
+ if(is_object($module)) {
+ $moduleName = substr(get_class($module), 0, strpos(get_class($module), '\\'));
@weierophinney
weierophinney Feb 19, 2013 Member

This may not be correct -- since module names are typically namespaces, and submodules are possible, the first namespace segment may not be right. As an example, if I had a submodule named "Phly\RestTools", with the class "Phly\RestTools\Module", the actual module name the above line would detect is "Phly", which is incorrect.

I'm thinking you may need to do key/value pairs of module name => module instance to make this work.

@blanchonvincent
blanchonvincent Feb 20, 2013 Contributor

@weierophinney it's was my first idea, and i think you are right, it's better, thx

@weierophinney weierophinney commented on an outdated diff Feb 19, 2013
library/Zend/ModuleManager/ModuleManager.php
-
- $this->loadFinished = false;
-
- $result = $this->getEventManager()->trigger(ModuleEvent::EVENT_LOAD_MODULE_RESOLVE, $this, $event, function ($r) {
- return (is_object($r));
- });
-
- $module = $result->last();
-
- if (!is_object($module)) {
- throw new Exception\RuntimeException(sprintf(
- 'Module (%s) could not be initialized.',
- $moduleName
- ));
+
+ if(!is_object($module)) {
@weierophinney
weierophinney Feb 19, 2013 Member

CS: need a space between operator and conditional.

@weierophinney
Member

As noted above, I think a few additional tests and use cases need to be considered.

@blanchonvincent
Contributor

@weierophinney I updated the PR (test with type & submodule) Do you think in other use cases ?

@Freeaqingme Freeaqingme and 1 other commented on an outdated diff Mar 6, 2013
library/Zend/ModuleManager/ModuleManager.php
$event = ($this->loadFinished === false) ? clone $this->getEvent() : $this->getEvent();
$event->setModuleName($moduleName);
+
+ if (!is_object($module)) {
@Freeaqingme
Freeaqingme Mar 6, 2013 Member

To keep this method clean (and small), perhaps you could move the contents of this if-block to their own method (e.g. loadModuleByName())?

@blanchonvincent
blanchonvincent Mar 8, 2013 Contributor

Ok look cool, I updated my PR

@Freeaqingme
Member

I like the PR from a functional perspective. I'll let it to @weierophinney or @EvanDotPro to do the final review however.

Please note, as it primarily introduces new functionality, I think it should be merged into develop only(?).

@weierophinney weierophinney added a commit that referenced this pull request Mar 11, 2013
@weierophinney weierophinney [#3814] CS fixes
- trailing whitespace
4ab3275
@weierophinney weierophinney added a commit that referenced this pull request Mar 11, 2013
@weierophinney weierophinney Merge branch 'feature/3814' into develop
Close #3814
203ee6b
@weierophinney
Member

Merged to develop for release with 2.2.0.

@weierophinney weierophinney added a commit to zendframework/zend-modulemanager that referenced this pull request May 15, 2015
@weierophinney weierophinney [zendframework/zendframework#3814] CS fixes
- trailing whitespace
6f36e9f
@weierophinney weierophinney added a commit to zendframework/zend-modulemanager that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/3814' into develop 9b92152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment