Fix issue with ModuleDirReader not finding files in vendor modules

[Vinai]

This PR contains:

• Complete implementation of
  \Magento\Framework\Filesystem\Driver\File::getRelativePath()
• Unit test to check complete behavior of
  \Magento\Framework\Filesystem\Driver\File::getRelativePath()
• Integration test to check if the \Magento\Framework\Module\Dir\Reader finds files in modules installed into vendor/.

The integration test does not cover the check for Controller directories and gathering action classes, because the ModuleDirReader directly operates on the file system, which is a lot more effort to test.

For more information please refer to issue #1481

[danslo]

Woah, that's fast Vinai... awesome! 👍

[joshdifabio]

You're a machine!

Thanks for doing this. I'm wondering if the solution could be simpler though, as this PR changes the way the filesystem driver works which is obviously quite a fundamental change. Assuming this change is intended to resolve the issues in \Magento\Framework\Module\Dir\Reader, I think that Reader itself is broken and I'll explain why:

TL;DR

I think Module\Dir\Reader::setModuleDir() should be removed, and then Module\Dir\Reader wouldn't need to depend on Filesystem\Driver\File::getRelativePath() at all.

Long version

Ignoring for a moment Module\Dir\Reader::setModuleDir(), which introduces mutability into the application's configuration and should possibly be removed, Module\Dir\Reader::getModuleDir() wraps Module\Dir::getDir(), which always returns absolute paths. As a result, if Module\Dir\Reader::setModuleDir() was removed then Module\Dir\Reader would not need to call Filesystem\Driver\File::getRelativePath() at all, and could indeed be simplified and the changes to the filesystem driver would no longer be needed.

Does that make sense? What do you think, @Vinai?

Edit: It looks like setModuleDir() is only used in tests as an alternative to mock objects. If we were to remove it the issues in \Magento\Framework\Module\Dir\Reader should disappear.

[Vinai]

Without checking the details, what you are saying looks sound, @joshdifabio. But I think it is a bigger architectural change then the small fix this PR contains.

I think this PR is valid even outside of the scope of this issue. The implementation of \Magento\Framework\Filesystem\Driver\File::getRelativePath() was incomplete.
The original code simply assumed that any path not starting with the given base path was a relative path.
That this small fix makes the Module\Dir\Reader work as assumed is an indication that the method actually did not work as expected by the author of the Module\Dir\Reader class.

The change you suggest is more invasive from my point of view. It makes sense, but it is a bigger change since removing the setModueDir() and getModuleDir() methods is a change to the public interface of the Module\Dir\Reader class.
I can only assume that there was a use case for the setter and getter because they where added in the first place. Removing them might have other consequences that I for one can't oversee. Even if the methods aren't called in the Magento CE codebase, they might be used by EE code.

I'm not saying the setModuleDir() and getModuleDir() should remain, but I wouldn't make such an API breaking PR without first getting feedback from the core team. If they are removed, it should be independent of the fix of the \Magento\Framework\Filesystem\Driver\File::getRelativePath() method.

[Vinai]

Pushed an update because the integration test did not clean up after itself.
Registering a module is a change in global state, which was causing later tests to fail.

[joshdifabio]

Sounds good, @Vinai.

I'll make a separate PR for \Magento\Framework\Module\Dir\Reader when I can as I think there are two or three major issues with it. The fact that it starts with an absolute path and then performs lots of operations to translate that to a relative path before translating it back to an absolute path is bad, especially when you consider that this is potentially done for every non-class file loaded from any module. But as you say, this is separate from this particular PR.

Thanks for your hard work!

[Vinai]

Rebased onto current develop. I don't know why the two integration tests keep failing in travis. If I run the tests locally they are green. o_O

[vrann]

@Vinai thanks for contribution.
I made an experiment to understand consequences of this PR:

1. Added VinaiKopp_EavOptionSetup to composer, get in installed to vendor directory and enabled
2. Added this piece code to the \Magento\Framework\Filesystem\Driver\File::getRelativePath:L767
   php if (strpos($path, $basePath) === false) { var_dump($result, $this->getAbsolutePath($basePath, $result)); }
3. Run ./bin/magento module:status

Output:

string(58) "../../vendor/vinaikopp/eavoptionsetup/src/../composer.json"
string(98) "/Projects/php/magento2ce/app/code/../../vendor/vinaikopp/eavoptionsetup/src/../composer.json"

This approach works for most of configuration files. Only concern is that it feels unnatural from the framework perspective: it declares set of constants with the base paths first, then disregards what was declared adding relative part "../../".

What if instead we refactor the constants which defines paths, so that vendor directory is supported natively without patches? Would like to clarify this before accepting pull request.

[vrann]

all new files should have copyright header

[Vinai]

Absolutely, having a more natural support for modules in vendor without the need for a patch would be better!

However - as I've already written above - regardless of the way modules from vendor are loaded, it would be good if the method \Magento\Framework\Filesystem\Driver\File::getRelativePath() would return the relative path as the method name states.

[Vinai]

Just a thought that just crossed my mind: the location of the vendor dir can be specified in the project composer.json file, so it might not always be in the Magento root dir. Ideally the vendor dir location would be retrieved from the project composer.json file.

[Vinai]

Added copyright and license comment to new files and rebased onto current develop HEAD.

[alankent]

"Ideally the vendor dir location would be retrieved from the project composer.json file." - That would be nice. Personally I would love it if the code supported multiple directories to look in, but that may be hard and I would prefer small solid progress rather than making life too difficult. Having 'vendor' work is useful progress.

Use case - I have my own modules in a Git repo somewhere else. I would like to say "code is in default locations, vendor, or a set of extra directories in my personal workspace". But 'vendor' is a great step forwards and may be enough.

Oh, my other personal goal is to get deployment specific configuration as separate as possible to site configuration. E.g. graphics for my site are part of my site; directory its deployed to, or host and port of MySQL are not part of my site. That is a deployment detail. env.php is a step in this direction. Goal is to make it easier to move a "site" from dev, to test, to prod as easily as possible - its harder with deployment config files mixed in with site files. (But I digress sorry.)

[vrann]

@Vinai regarding your comment about vendor location, we also have this file /magento2ce/app/etc/vendor_path.php with the content

return './vendor';

[Vinai]

@alankent Of course you are right - one small step after the other is how good progress is made!
@vrann Ah yes, that file... I tried to run composer install in a module repository with the dependency on "magento/module-eav": ">=0.74.0-beta14,<2.0" (eavsetup) the other day. The purpose was to find out if it was enough to be able to run the modules tests, to have the module as the project and all dependencies pulled into vendor. The result was that composer died when it tried to create that file in app/etc but of course that director didn't exist.

[joshdifabio]

@vrann \Magento\Framework\Filesystem\Driver\File::getRelativePath() doesn't currently return anything useful for paths which are not beneath the specified base path. It seems that the handling of such cases by getRelativePath() should be changed either to throw an exception or, as @Vinai has done here, return an accurate relative path. The current behaviour looks like a silent failure to me which makes it difficult to detect bugs such as the one addressed by this PR. What do you think?

[buskamuza]

@Vinai , I'd suggest to run Composer with --no-plugins option, so it will not try to run Magento Composer Installer plugin and copy the files, as well, as to create that file.

[Vinai]

After investigating why the integration tests is failing, it turns out that the module registry needs an option to unregister modules, so the global state can be reset during the tests.
Providing an unregisterModule() method seems like the cleanest option to me, all other options I can think of (e.g. passing null to registerModule() to unset a module, or filtering the list of modules in getModulePaths()) seem dirty.

[Vinai]

@buskamuza Thank you for your input, I'll definitely try that!

[joshdifabio]

After investigating why the integration tests is failing, it turns out that the module registry needs an option to unregister modules, so the global state can be reset during the tests.
Providing an unregisterModule() method seems like the cleanest option to me, all other options I can think of (e.g. passing null to registerModule() to unset a module, or filtering the list of modules in getModulePaths()) seem dirty.

The idea was that mocks would be used for testing purposes, not the static registerModule() method. That's part of the reason clients of the Registrar class depend on the non-static interface, so that they can be wired with a mock registry for testing. Does that make sense?

[joshdifabio]

I don't think this is a good idea. What happens if this method is called when the application has already been bootstrapped and the unregistered module has already been loaded in some way? I think we need to make these registry objects immutable (see #1562), and tests should use mocks if they need to make use of a module registry

[Vinai]

I'm absolutely with you that Immutability is a good thing.
However, testability trumps this in my view, and since we are working with global state here, some mutability is probably required.
It should be possible to create Integration tests that run in isolation without mocks.

I did attempt to resolve the situation without changing the registrar. At first I tried the obvious solution: to run the test in a separate process with the @runTestsInSeparateProcesses annotation. But the \Magento\TestFramework\Isolation\DeploymentConfig listener sees the registered module as a deployment configuration change and complains:

ERROR: deployment configuration is corrupted. The application state is no longer valid.
Further tests may fail. This test failure may be misleading, if you are re-running it on a corrupted application.

Next try, use the @magentoAppIsolation enabled annotation. With this annotation the listener \Magento\TestFramework\Annotation\AppIsolation tries to reset the whole application state after a test with that annotation as follows:

    /**
     * Reset application global state
     */
    protected function _resetApp()
    {
        /** @var $objectManager \Magento\TestFramework\ObjectManager */
        $objectManager = \Magento\TestFramework\Helper\Bootstrap::getObjectManager();
        $objectManager->clearCache();

        \Magento\Framework\Data\Form::setElementRenderer(null);
        \Magento\Framework\Data\Form::setFieldsetRenderer(null);
        \Magento\Framework\Data\Form::setFieldsetElementRenderer(null);
        $this->_appArea = null;
    }

This fails since it resets all global state and recreates the application, but it doesn't reset the module registry.

Thinking further, the addition of the unregisterModule() method does not conflict with your #1562. In fact, it complements it nicely.
Once the registrar is instantiated by the object manager and injected into the ModuleList\Loaderand the Module\Dir, the list of modules in immutable. However, this instance is discarded when the application is reset by $objectManager->clearCache();.
Then, when the application is initialized again, new instances of the registrar is created. This time it will use the changed list of modules.
To make it even more safe the preference could be configured as a shared instance.

A test that requires changing the module list state could do the following:

    protected function setUp()
    {
        \Magento\Framework\Module\Registrar::registerModule('Test_Module', BP . '/vendor/test/example');
        \Magento\TestFramework\Helper\Bootstrap::getInstance()->reinitialize();
    }

    protected function tearDown()
    {
        \Magento\Framework\Module\Registrar::unregisterModule('Test_Module');
        \Magento\TestFramework\Helper\Bootstrap::getInstance()->reinitialize();
        // Instead of calling `reinitialize()` after `unregister()`, the tests
        // could also use the `@magentoAppIsolation enabled` annotation.
    }

I've merged your PR #1562 locally and tried this and it works as expected.

If I still didn't manage to convince you, another thought was to make the unregisterModule method final protected. That was a test could create a subclass and modify the list of registered modules that way. The benefit would be that the method signature would not be public, and thus the public static interface of the registrar is cleaner.

[joshdifabio]

Thinking further, the addition of the unregisterModule() method does not conflict with your #1562. In fact, it complements it nicely.

I agree.

However, testability trumps this in my view, and since we are working with global state here, some mutability is probably required.

It should be possible to create Integration tests without mocks that run in isolation.

I think this comes down to personal preference. I don't think you're wrong, my preference is just different. But I think the big smell comes from the mutability of instances of Registrar (addressed in #1562) which could cause the application state to be inconsistent, and which would be exacerbated by this change. If #1562 is merged then I wouldn't have any major issues with unregisterModule().

@Vinai Thanks for going into so much detail!

[Vinai]

Thank you, too, @joshdifabio. I am really enjoying this.

[Flyingmana]

what about adding another Modul Registrar Class which does not make use of static. The static method/the adding of modules is not part of the interface. Also its easier to Test with it.

[joshdifabio]

@Flyingmana What would be the use case for the additional class?

[vrann]

@Vinai update from my side: investigating the effort of changing the architecture of the framework not to depend on app/code:

• right now looking on what will it take to switch from using the relative path to the app/code to either relative path to the Magento root or to absolute path using Registar.

Depending on the amount of effort, current PR would be either accepted as the fast way, or rejected. We discussed approach from the PR, it looks interesting, but 2 major concerns are:

• getRelativePath should return a path within directory it was invoked, not relatively to that directory. We should probably throw an exception, as it was suggested earlier by @joshdifabio if it is outside (there are some evidences that such exception was thrown before)
• The way how modules are loaded is really a very core of the framework and it should be done right. To look for a path to vendor relatively to app/code is rather a backup plan.

After that, what would need to be investigated are:

• themes fallback
• changes needed for DI compiler
• paths xsd files

[Vinai]

@vrann Thanks for looking into this. Great to hear that a change using an absolute path to module directories is the preferred way to go forward. The list of items to investigate further after the change is also very interesting.

Regarding the change of Driver\File::getRelativePath($basePath, $path), that class doesn't seem like the right place to check the directory from which it was called.
Just look at the class name and the method signature. It also doesn't seem consistent with the other methods in the Driver\File class.
I think it would be more fitting to do the check in the \Magento\Framework\Filesystem\Directory\Read::getRelativePath($path) method, as that class represents the concept of "a directory", and thus the idea of a "calling directory" makes sense. Also, it does not receive the base path as an argument, so it is clear to what the relative path is calculated.
Maybe the Driver\File class interface needs to be extended with a containsPath($basePath, $path) method for that purpose. Something like this:

// \Magento\Framework\Filesystem\Directory\Read

public function getRelativePath()
{
    if (!$this->driver->containsPath($this->path, $path)) {
        throw new Exception("Nope, sorry, try again - the path has to be contained within {$this-path}");
    }
    return $this->driver->getRelativePath($this->path, $path);
}

Probably the method should be renamed for that purpose, too. Maybe getRelativePathToSubdirectory($path). Should the method on the Driver\File class be used to do that check, it should be renamed, too.

By the way, I'm fully okay with this PR not being accepted. I'm only saying this because I would like to work with code that behaves as I would expect it to do from simply looking at class and method names. That makes working with a code base a lot easier and more pleasant.
I can't see a reason why a method getRelativePath() which takes a base path and a path, should only work if the path is contained within the base path. It makes a very big difference from a third party developer perspective.

[Vinai]

Just noticed the "needs update" label on this PR. I'm still eager to get full support for modules in vendor/ working before the Q4 release.
Is there anything that needs to be done from my/the communities side at the moment?

[vrann]

Hi, @Vinai, sorry for being slow on this. As it was mentioned before, I was looking into what does it cost to make platform changes in the way that we don't need to calculate relative paths related to app/code.

This branch is result of my investigation:
https://github.com/vrann/magento2-from-vendor

Magento was able to run, having most of its components under vendor dir. The exclusion is the magento2-base, which defines the structure of the project and have some required components.

What is left to be done there in order to merge to mainline is:

• decide what to do with the xsd files. Right now xml files are failing validation and I just skipped it. One of the solution which I can think of is to have some common directory for all xsd files, let's say var/xsd and have them published there every time file is requested by php. Also, have some publication tool to publish all xsd files. In this way we will have all of them under single directory structure so it won't be confusion where to load it from.
• fix around 10 places where there is still hard dependency on app/code/Namespace/Module
• fix unit/integration tests

After that, as a separate effort, we can split magento2-base module on more meaningful components.

Still looking into that, so if you have some feedback it will be very helpful.

[vrann]

More specifically, these are open questions to the approach:

• should every module have module_registration.php or should we support default location such as app/code. My opinion — all should have it, because potentially every module may become component and be transferred to the vendor
• is it appropriate to have all xsd being copied to one specific location? It is needed in order to statically reference them from the xml files to have automatic verification in IDE. Though, if we have them copied, a problem is that if you open some xml file while xsd is not copied yet, it will highlight syntax errors. Similar to if you open a class where some dependency is auto-generated class an it is not generated yet.
• View/Element/Template/File/Validator.php has a logic to check is template file inside app/code or app/design only. Is it a helpful rule or can be avoided?

[Vinai]

Wow, this is very cool. Thanks for the update and detailed response!
Unfortunately I'm on vacation until end of August, so I'll won't be able to take a real look until then. However I hope when I'm back it will be merged already.
That said, I would like to respond to two points you raised.

Regarding the registration callback file. I don't like the idea of every module having one, since the ones under app/code don't need it. A module should be built for one location in my view and doesn't need to work in both possible locations.
That would introduce maintenance overhead that's not needed.
Of course that means that when a module is moved from app/code to vendor it will have to be changed.

Regarding the xsd locations, I think copying them to var/xsd is a pragmatic solution. Not ideal, but it fits to how many other parts of Magento 2 work.
@akent mentioned the possibility that the interfaces might be moved into their own composer installable package. In this case these would be a good place for the xsd files, too, as technically they also define an interface the xml files are built against.

Looking forward to how this progresses,
Vinai

[alankent]

A goal of XSD files was to allow new modules to define their own config files, and hence have their own XSD files. So rather than have a single shared directory for XSD files, I would probably prefer:

• Use a URN instead of a relative path - URN would contain module name as start of path. Magento built in URN resolution code would know where the module is and so would convert something like urn:magento:xsd:Magento_Sales/xsd/foo.xsd into /my/home/magento/app/code/Magento/Sales/xsd/foo.xsd.
• Have CLI command to expand a URN to a full path. The developer can then use this with https://www.jetbrains.com/phpstorm/help/referencing-dtd-or-schema.html "Add Xsi Schema Location for External Resources".

[vrann]

@Vinai thanks!
Isn't it a main use-case of community developer -- develop module in app/code but then have it packaged and shipped as a composer package to be downloaded from connect? In my mind it may increase possibility of mistake if adding the module_registration file is a part of packaging process (vs development) -- does this makes sense.

@alankent yes, looks like that will work. I don't remember a reason why we didn't do it in this way in the first place. Some IDE doesn't support it? Should this be verified on all major IDEs or PHPStorm is enough to start with?

[vrann]

@Vinai marked as rejected because I think removal of MODULES constant will do the same. But will keep open in case if discussion is still going here.

[alankent]

@vrann We did not do it in the first place as it forces you to set up the IDE with each path to the XSD file. It works well as long as modules sit under app/code. So it got us going. With this (great) effort to allow modules in other locations, the simplistic initial approach is no longer enough. Devs will have to set up the URN to correct path name mappings, which is more work. But the benefits are worth it (in my opinion).

[Vinai]

@vrann That extension development process you describe is quite likely, true. But still, I think it would be cleaner to only require a module to contain the data valid for one location.
I see myself developing a module in it's own repository or directly in vendor to be honest, so I wouldn't even start out under app/code any more. Can't speak for others though.

@alankent @vrann For me using an URN for the xsd file location would be fine. I don't know how other IDEs handle that. One thing PHPStorm supports are URNs pointing to an external source if the URL is specified after the URN separated by a space. The resource can then be downloaded and validated against using the intention keyboard command (alt-enter by default). This would make setting up the mappings trivial.

[buskamuza]

Hi @Vinai , @joshdifabio , @alankent , @vrann !
How do you think, is support of custom vendor folder (e.g., outside of Magento root) critical or not?
Does it happen often that vendor path is custom?

[alankent]

The use case I am wondering about is a developer dealing with multiple modules, possibly in different GIT repos. E.g. a local site module, a shared one a SI is using on multiple projects (not local to project, but not public either), etc. So in my mind its no so much 'vendor' that is the issue, but rather can we support developers dealing with source code and version control management.

But your explicit question regarding the 'vendor' directory, I cannot think of a reason that this directory needs to be able to move. If anything, my question would be "can we move away from having the Magento root directory" - can we make a whole site completely install under vendor with no special setup require for the root directory. Maybe that is too hard.

For example, long term I would personally like to see one directory tree for the files developers set up (source code etc), and a separate directory tree holding things like 'var' and 'pub/static' where content is generated. But maybe there should even be three directories - source code, static computed files (still read only), and writable files (like logs). The first is in GIT. The first two can be made read-only in production. All three are required for a site to run.

[Flyingmana]

I dont comment much about this here, as it is not really related to the PR or the Issue, only a possible tool which could help solving this issues. And as i is nearly on a stable release, maybe someone wants to look more into it.
http://puli.io

[magento-cicd2]

We have automated a Magento Contributor License Agreement verifier for contributions sent to our GitHub projects.
Please see the CLA agreement in the Pull Request comments.