Skip to content
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

Adding logic to check module dependencies at module loading time #3443

Closed

Conversation

Projects
None yet
4 participants
@Ocramius
Copy link
Member

commented Jan 15, 2013

Provides implementation for #3427

@Ocramius

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2013

If this one is okay (from the logic perspective/coding), please let me know and don't merge until I wrote the docs for it (I noticed it's the only way to get me to write docs =_= )

@marc-mabe

This comment has been minimized.

Copy link
Member

commented Jan 16, 2013

What if two modules depend on each other?

@Ocramius

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2013

@marc-mabe makes no real sense since modules have a loading order. The feature can be turned off at any time btw. It's just a safety check before letting the spaceship starts the engines.

@marc-mabe

This comment has been minimized.

Copy link
Member

commented Jan 16, 2013

@Ocramius As it's only a check does it make sense to check it after loading.
Circular dependencies could make sense on closed environments like company internal modules.

@Ocramius

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2013

@marc-mabe loading (including init triggers) may depend on other modules anyway. That's why I do it so early. Even tempted to handle it before autoloading eventually.

@marc-mabe

This comment has been minimized.

Copy link
Member

commented Jan 16, 2013

@Ocramius Is the loading process not known the list of modules for loading - isn't it?
So the loading order could be changed automatically to load depended modules first?
To support circular dependencies the module itself need to say which dependencies are required for initialization and which are required after the loading process finished.

Does this make sense ?

@Ocramius

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2013

@marc-mabe no, my point is exactly to AVOID any kind of magic here. It's just a check. If you know what you're doing, you can turn it off and handle dependencies as you wish.

This is just a very important thing for people who may (example) install ZfcUserDoctrineORM and don't enable ZfcUser, and in turn forget to enable ZfcBase. This is a self-documenting exception that helps beginners in getting started, nothing more. No changing of anything :)

And yes, it would just help discovering circular dependencies. Doesn't mean it has to solve 'em :)

Ocramius added a commit to Ocramius/zf2-documentation that referenced this pull request Jan 16, 2013

Documenting feature proposed at zendframework/zendframework#3443
Adding description of how the ModuleDependencyIndicatorInterface works
and how the listener associated to it behaves
@marc-mabe

This comment has been minimized.

Copy link
Member

commented Jan 16, 2013

@Ocramius I'm on you. Because it's a development feature and can be disabled - circular dependencies aren't a must-have and too much magic isn't good.

@Ocramius

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2013

AFAIK, this is complete

@prolic

View changes

library/Zend/ModuleManager/Exception/MissingDependencyModuleException.php Outdated
* @package Zend_ModuleManager
* @subpackage Exception
*/
class MissingDependencyModuleException extends \RuntimeException implements ExceptionInterface

This comment has been minimized.

Copy link
@prolic

prolic Jan 17, 2013

Contributor

This one should extend Zend\ModuleManager\Exception\RuntimeException, not the base exception class.

*
* @return array
*/
public function getDependencyModules();

This comment has been minimized.

Copy link
@prolic

prolic Jan 17, 2013

Contributor

Perhabs rename the method to getDependendModules() ? It would be more clear, what you get.

This comment has been minimized.

Copy link
@Ocramius

Ocramius Jan 18, 2013

Author Member

No, that would be the modules that depend on this one.

dependency is correct

@prolic

View changes

library/Zend/ModuleManager/Listener/ModuleDependencyCheckerListener.php Outdated

namespace Zend\ModuleManager\Listener;

use Zend\ModuleManager\Exception\MissingDependencyModuleException;

This comment has been minimized.

Copy link
@prolic

prolic Jan 17, 2013

Contributor

Import Zend\ModuleManager\Exception only, and refer to the exceptions with:
throw new Exception\MissingDependencyModuleException()

@prolic

View changes

library/Zend/ModuleManager/Listener/ModuleDependencyCheckerListener.php Outdated
/**
* @param \Zend\ModuleManager\ModuleEvent $e
*
* @throws \Zend\ModuleManager\Exception\MissingDependencyModuleException

This comment has been minimized.

Copy link
@prolic

prolic Jan 17, 2013

Contributor

see above: this one would be:
@throws Exception\MissingDependencyModuleException
which is much shorter, if you want to read this class.

@prolic

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2013

Implementation looks straightforward. I like it. Good work!

@Ocramius

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2013

Done, rebased :shipit:

@weierophinney

This comment has been minimized.

Copy link
Member

commented Jan 21, 2013

Looks good. The only issue I have is similar to what @prolic says: getDependencyModules doesn't read well, and is hard to understand. May I suggest getModuleDependencies()? If you agree, I can even make such a change at merge time.

@weierophinney

This comment has been minimized.

Copy link
Member

commented Jan 21, 2013

Merged -- see a087928

@Ocramius Ocramius deleted the Ocramius:feature/module-dependencies branch Jan 21, 2013

Maks3w added a commit to zendframework/zf2-documentation that referenced this pull request Jan 21, 2013

weierophinney added a commit to zendframework/zend-modulemanager that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-modulemanager that referenced this pull request May 15, 2015

Merge branch 'feature/3443' into develop
Close zendframework/zendframework#3443

Modified original to rename getDependencyModules to getModuleDependencies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.