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

Module in vendor #1

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

Module in vendor #1

wants to merge 6 commits into from

Conversation

otoolec
Copy link
Owner

@otoolec otoolec commented Apr 24, 2015

Purpose

This is a simple POC to show how we can have modules live in the vendor directory instead of needing to copy them to app/code.

Install

  1. Delete modules Backend and Customer
  2. Run dev/tools/Magento/Tools/Module/createBasicModuleClass.php
  3. Run composer install to see how code can be run from the vendor directory.

Caveats

  • The POC can easily be re-written to NOT require a breaking change from a RC perspective. See Design Thoughts for details.
  • The POC focuses only on modules for now and doesn’t do anything for other component types such as themes.
  • The POC doesn’t address tools such as our DI compiler, which only scans the ‘app’ directory

Design Thoughts

Each module has a Module class that must be named by convention <namespace>\<module>\Module so that it can be loaded from the fully qualified module name namespace_module. The Module class can be asked for various directories of a module, which allows the module to be placed anywhere as long as the class can be autoloaded and as long as the Module can be discovered.

Discovering modules is currently done by scanning both app/code and vendor directories for a module.xml file.

A possible alternative implementation could be to record the location of the module.xml file during the discovery phase, then use that to identify module location instead of a Module class. Benefits of that approach would be that no code changes are required by developers to get their module to be discoverable.

The Module class approach provides an extra layer of abstraction that would allow modules to re-arrange their folder structure. This could for example allow all production source files to be moved into a src folder, all test files moved into a test folder and config files to remain in the etc folder. The module composer.json could then add PSR-4 autoload directives that prevent test classes from being loaded during production installs.

Approach Can reside anywhere Avoids copying files No config/code changes needed Abstracts internal structure
autoload.files
Module Interface
module.xml
Module Interface + registry file

Further Work

If we go with the Module class approach then the lib/internal/Magento/Framework/Module/ModuleList/Loader.php class and the Setup application should be refactored so that only the Setup application knows about the vendor directory.

If we go with the module.xml file approach then lib/internal/Magento/Framework/Module/ModuleList/Loader.php should be refactored to return the directory in which the module.xml file was found and Setup application should be refactored to store the module location in the config.php file.

If we go with the Module class and registry file approach then the magento/magento-composer-installer will need to be refactored to write installed module names into a registry file. The lib/internal/Magento/Framework/Module/ModuleList/Loader.php class needs to be refactored to read the registry file for a list of installed modules, and use the lib/internal/Magento/Framework/Module/Dir/Reader.php file to read the module.xml file.

The scripts and tools under dev will need to be refactored to use ModuleListInterface and
Magento\Framework\Module\Dir.

Notes

All module file access must be funneled through a class like lib/internal/Magento/Framework/Module/Dir/Reader.php

This class can iterate over the known modules (can filter based on active if desired) and their directory structure, regardless of where they are.

Example of interfaces/classes that will help with this effort:
lib/internal/Magento/Framework/Module/Dir/Reader.php
lib/internal/Magento/Framework/Module/Dir.php
lib/internal/Magento/Framework/Config/FileResolverInterface.php
lib/internal/Magento/Framework/Config/ReaderInterface.php
lib/internal/Magento/Framework/View/Design/FileResolution/Fallback/ResolverInterface.php

Needs to be refactored:
lib/internal/Magento/Framework/Module/Dir/Reader.php
app/code/Magento/Email/Model/Template/Config/FileIterator.php
lib/internal/Magento/Framework/Module/Dir/ReverseResolver.php
lib/internal/Magento/Framework/Module/ModuleList/Loader.php
lib/internal/Magento/Framework/View/File/Collector/Base.php
lib/internal/Magento/Framework/View/Design/Fallback/RulePool.php
lib/internal/Magento/Framework/View/Element/Template.php

@joshdifabio
Copy link

I've also considered creating a ModuleInterface but I don't think I see the value in it. The interface could never be modified, because doing so would break all existing implementations, and if it only contained a single getDir() method then what would its value be? Essentially, anything which consumed ModuleInterface would only be interested in the directory path of a module, in which case that consumer would be coupled to this single-method interface when it would be better to simply pass it the module directory path directly.

It might be that ModuleInterface should contain further methods, but then I think we'd be deviating from the Magento 2 approach of configuration-by-XML.

Might it be worth adding a method to Magento\Framework\App\Bootstrap which is called after the app is created, e.g. afterCreateApplication(callable $observer), where observers would receive the application instance? Modules could call this method early by providing a bootstrap.php and making using of Composer's autoload.files directive.

@@ -660,6 +660,9 @@ public function fileUnlock($resource)
*/
public function getAbsolutePath($basePath, $path, $scheme = null)
{
if (strlen($path) > 0 && $this->fixSeparator($path)[0] === '/') {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behaviour of getAbsolutePath() surprised me and I actually think the change you've made here should be part of Magento 2 regardless of this particular PR. Is there any reason not to submit this as its own PR?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I need to review the changes I made here and see if there aren't others I should provide as a standalone fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants