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

Allow modules to live outside of app/code directory #1206

Merged
merged 8 commits into from Jun 19, 2015

Conversation

Projects
None yet
@joshdifabio
Contributor

joshdifabio commented Apr 20, 2015

Partially resolves #1013.

This PR aims to enable Magento modules to reside in the vendor directory, or indeed anywhere else on disk, without the use of any Composer plugins and without copying or linking any module files.

How does it work?

This PR adds the concept of a module registry to the Magento\Framework\Module package. This module registry provides Magento with the locations of any modules which do not reside in the standard modules directory (currently app/code). This PR does not remove the existing module bootstrapping functionality and aims to be backwards compatible with Magento 2's existing module implementation.

Added in the PR is a ModuleRegistryInterface which exposes two non-static methods which allow Magento to locate modules. Also provided is a concrete Registrar implementation which implements the ModuleRegistryInterface while also implementing a static registerModule() method which third party modules can use to register themselves with Magento. Magento itself does not depend on the static registerModule() method, which will help with automated testing of those Magento classes which depend on ModuleRegistry.

Modules will be installed using Composer but without the use of any Composer plugins. No code will be copied from vendor, or wherever else the modules are located, into the Magento application directory.

What would a module look like with this approach?

To use this functionality, a module will need to call the static Registrar::registerModule() method in order to notify the Magento application of its presence and location.

bootstrap.php

<?php
Magento\Framework\Module\Registrar::registerModule('FooVendor_BarModule', __DIR__);

composer.json

{
    "name": "foo-vendor/bar-module",
    "autoload": {
        "psr-4": { "FooVendor\\BarModule\\": "" },
        "files": [ "bootstrap.php" ]
    }
}

When Composer's vendor/autoload.php is included, autoload_real.php first processes the various namespaces, class paths and include paths defined in all included composer packages and registers the autoloader. Next, autoload_real.php includes all files defined in autoload.files. This means that, in the above example, bootstrap.php will be included by Composer and will have access to any classes which are known to the Composer autoloader.

Why is this good?

Copying files around using Composer plugins adds a lot of complexity and I believe Magento already know most of the related difficulties. It also makes the standard process of downloading and testing a package very difficult:

$ git clone git@github.com:foo_vendor/bar_module.git
$ cd bar_module
$ composer install
$ php vendor/bin/phpunit

However, this proposal enables the above process to work; even if the module is located in the root while the application code is installed to the vendor directory, the bootstrap.php file will be included by Composer and will register the module with Magento. This would mean that we could finally, easily, create modules and build our code on Travis, Scrutinizer, or any of the other brilliant, free code coverage and static analysis tools.

This weekend (2015-04-24 – 2015-04-26) I'll aim to finish this PR.

{
$modulesDir = $this->filesystem->getDirectoryRead(DirectoryList::MODULES);
foreach ($modulesDir->search('*/*/etc/module.xml') as $filePath) {
yield $modulesDir->readFile($filePath);

This comment has been minimized.

@sshymko

sshymko Apr 23, 2015

Wow, that's the first usage of generators in Magento 2 :)

@sshymko

sshymko Apr 23, 2015

Wow, that's the first usage of generators in Magento 2 :)

@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Apr 24, 2015

Contributor

I've added an in-depth explanation to the OP.

Contributor

joshdifabio commented Apr 24, 2015

I've added an in-depth explanation to the OP.

@otoolec

This comment has been minimized.

Show comment
Hide comment
@otoolec

otoolec Apr 24, 2015

Contributor

@joshdifabio using the composer autoload.files looks like a neat way to allow modules to self register with Magento without requiring a master module registry file or limiting discovery to a set of known locations.

I've been wondering what the best way to solve this problem is as well and have documented my thoughts here. Feel free to also look at the other code changes that were needed as they may help you out this weekend!

I'm curious to know how others about the importance of some features:

  • Module can reside anywhere
  • Module can reside in vendor or app/code
  • Module doesn’t need to provide new configuration
  • Module can define own internal folder structure (‘src’, ‘tests’, ‘doc’, ‘config’)
Contributor

otoolec commented Apr 24, 2015

@joshdifabio using the composer autoload.files looks like a neat way to allow modules to self register with Magento without requiring a master module registry file or limiting discovery to a set of known locations.

I've been wondering what the best way to solve this problem is as well and have documented my thoughts here. Feel free to also look at the other code changes that were needed as they may help you out this weekend!

I'm curious to know how others about the importance of some features:

  • Module can reside anywhere
  • Module can reside in vendor or app/code
  • Module doesn’t need to provide new configuration
  • Module can define own internal folder structure (‘src’, ‘tests’, ‘doc’, ‘config’)
@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Apr 27, 2015

Contributor

@otoolec Your research will be immensely helpful, thank you. Regarding the four questions you've raised, I'll respond with my own thoughts soon.

Unfortunately, I only had time to update the unit tests at the weekend, so the unit build is now passing but the PR is still WIP.

Contributor

joshdifabio commented Apr 27, 2015

@otoolec Your research will be immensely helpful, thank you. Regarding the four questions you've raised, I'll respond with my own thoughts soon.

Unfortunately, I only had time to update the unit tests at the weekend, so the unit build is now passing but the PR is still WIP.

@fooman

This comment has been minimized.

Show comment
Hide comment
@fooman

fooman Apr 27, 2015

Contributor

@joshdifabio Thanks for tackling this - much appreciated!

Having something like this work

$ git clone git@github.com:foo_vendor/bar_module.git
$ cd bar_module
$ composer install
$ php vendor/bin/phpunit

would be awesome. It currently feels like we are almost there with Magento but we can't yet take full advantage of composer.

@otoolec in reply to your questions

Module can reside anywhere

I don't think full flexibility is needed and might even make things more complicated (a code review for example).

Module can reside in vendor or app/code

I think this would be a good compromise - acknowledging that Magento won't be able to port over all modules in time while allowing extensions to use vendor (possibly with a long term view that Magento core modules will also be shifted there)

Module doesn’t need to provide new configuration

I think it is still early for M2 extension development so if this is tackled soon the impact would not be that big. It is still in beta after all.

Module can define own internal folder structure (‘src’, ‘tests’, ‘doc’, ‘config’)

Using composer to install we should get this for free. I think it is a good idea to support what the rest of the php world is seemingly moving towards.

Contributor

fooman commented Apr 27, 2015

@joshdifabio Thanks for tackling this - much appreciated!

Having something like this work

$ git clone git@github.com:foo_vendor/bar_module.git
$ cd bar_module
$ composer install
$ php vendor/bin/phpunit

would be awesome. It currently feels like we are almost there with Magento but we can't yet take full advantage of composer.

@otoolec in reply to your questions

Module can reside anywhere

I don't think full flexibility is needed and might even make things more complicated (a code review for example).

Module can reside in vendor or app/code

I think this would be a good compromise - acknowledging that Magento won't be able to port over all modules in time while allowing extensions to use vendor (possibly with a long term view that Magento core modules will also be shifted there)

Module doesn’t need to provide new configuration

I think it is still early for M2 extension development so if this is tackled soon the impact would not be that big. It is still in beta after all.

Module can define own internal folder structure (‘src’, ‘tests’, ‘doc’, ‘config’)

Using composer to install we should get this for free. I think it is a good idea to support what the rest of the php world is seemingly moving towards.

@kojiromike

This comment has been minimized.

Show comment
Hide comment
@kojiromike

kojiromike Apr 27, 2015

Module can reside anywhere

There should be one-- and preferably only one --obvious way to do it.
Although that way may not be obvious at first unless you're [Marius].
–– excerpt from The Zen of Python, by Tim Peters

I think Magento should be opinionated, so I don't think a module should be able to reside anywhere. I think there should be one obvious right place for a module to reside. I think that place is vendor. ;)

Module can reside in vendor or app/code

For now, for backwards compatibility, I can totally get behind needing to keep some modules in app/code. But it should be deprecated – new modules should be in vendor.

Module doesn’t need to provide new configuration

I don't know if I understand this feature. It's hard for me to imagine how modules would work if they couldn't provide new configuration. Does this include layout?
– update: hmm. I think the bootstrap.php approach is good in that modules should not need to write to files outside their vendor directories to be installed, but I think Magento should still have a way to turn off modules that are otherwise installed in vendor.

Module can define own internal folder structure (‘src’, ‘tests’, ‘doc’, ‘config’)

I think this is in the same vein as my Zen of Python quote above in that module developers should be encouraged to adhere to a consistent structure. But technically, I don't know that it matters that much as long as the module is fully encapsulated.

kojiromike commented Apr 27, 2015

Module can reside anywhere

There should be one-- and preferably only one --obvious way to do it.
Although that way may not be obvious at first unless you're [Marius].
–– excerpt from The Zen of Python, by Tim Peters

I think Magento should be opinionated, so I don't think a module should be able to reside anywhere. I think there should be one obvious right place for a module to reside. I think that place is vendor. ;)

Module can reside in vendor or app/code

For now, for backwards compatibility, I can totally get behind needing to keep some modules in app/code. But it should be deprecated – new modules should be in vendor.

Module doesn’t need to provide new configuration

I don't know if I understand this feature. It's hard for me to imagine how modules would work if they couldn't provide new configuration. Does this include layout?
– update: hmm. I think the bootstrap.php approach is good in that modules should not need to write to files outside their vendor directories to be installed, but I think Magento should still have a way to turn off modules that are otherwise installed in vendor.

Module can define own internal folder structure (‘src’, ‘tests’, ‘doc’, ‘config’)

I think this is in the same vein as my Zen of Python quote above in that module developers should be encouraged to adhere to a consistent structure. But technically, I don't know that it matters that much as long as the module is fully encapsulated.

@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Apr 27, 2015

Contributor

@fooman, @kojiromike: Consider the following workflow which, I would say, is the standard method used to build and test a PHP package:

$ git clone git@github.com:foo_vendor/bar_module.git
$ cd bar_module
$ composer install
$ php vendor/bin/phpunit

In the above scenario, the module which we are testing does not reside in app/code, nor does it reside in vendor; it resides in the root and the dependencies (Magento core code, etc.) are installed to vendor. If modules can only reside in app/code or vendor then this workflow is no longer possible, which I feel would be a big loss to Magento developers.

If, on the other hand, we allow modules to reside anywhere on disk, implemented in this PR, then the above workflow is possible. See the OP to see how this could work.

Contributor

joshdifabio commented Apr 27, 2015

@fooman, @kojiromike: Consider the following workflow which, I would say, is the standard method used to build and test a PHP package:

$ git clone git@github.com:foo_vendor/bar_module.git
$ cd bar_module
$ composer install
$ php vendor/bin/phpunit

In the above scenario, the module which we are testing does not reside in app/code, nor does it reside in vendor; it resides in the root and the dependencies (Magento core code, etc.) are installed to vendor. If modules can only reside in app/code or vendor then this workflow is no longer possible, which I feel would be a big loss to Magento developers.

If, on the other hand, we allow modules to reside anywhere on disk, implemented in this PR, then the above workflow is possible. See the OP to see how this could work.

@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Apr 27, 2015

Contributor

@otoolec

I'm curious to know how others about the importance of some features:

  • Module can reside anywhere

I think this is crucial in order to support the most basic PHP workflow: clone, composer install, test. In this workflow, the code of the module being tested will not reside in app/code or vendor.

  • Module can reside in vendor or app/code

I think that allowing modules to reside in app/code will still be useful if/when proper Composer support is added (as proposed in this PR): For many vendors, it won't be practical to create dozens of VCS repositories per project in order to house individual modules. In many cases it will be preferred to keep project-specific modules in the project VCS repository: It would be extremely messy in such cases to commit those modules to vendor.

  • Module doesn’t need to provide new configuration

The less boilerplate the better. Developers hate boilerplate.

  • Module can define own internal folder structure (‘src’, ‘tests’, ‘doc’, ‘config’)

Do you have any use cases in mind for this?

Contributor

joshdifabio commented Apr 27, 2015

@otoolec

I'm curious to know how others about the importance of some features:

  • Module can reside anywhere

I think this is crucial in order to support the most basic PHP workflow: clone, composer install, test. In this workflow, the code of the module being tested will not reside in app/code or vendor.

  • Module can reside in vendor or app/code

I think that allowing modules to reside in app/code will still be useful if/when proper Composer support is added (as proposed in this PR): For many vendors, it won't be practical to create dozens of VCS repositories per project in order to house individual modules. In many cases it will be preferred to keep project-specific modules in the project VCS repository: It would be extremely messy in such cases to commit those modules to vendor.

  • Module doesn’t need to provide new configuration

The less boilerplate the better. Developers hate boilerplate.

  • Module can define own internal folder structure (‘src’, ‘tests’, ‘doc’, ‘config’)

Do you have any use cases in mind for this?

@alankent

This comment has been minimized.

Show comment
Hide comment
@alankent

alankent Apr 27, 2015

Interesting thread. Here is my current opinion until I change my mind! ;-)

The reason for supporting app/code (yes, want to change to app/modules - separate discussion) and vendor/ is to support two use cases. The reason for app/code is "code local to my project"; the reason for vendor is "this is downloaded by Composer".

If you want to contribute a bug fix to Magento 2, you should git clone the M2 repo and create a pull request. If you want to create a production site, you SHOULD NOT do it this way. You should use composer to download official released versions of all packages. It makes sense them residing in 'vendor'. The general rule is "don't touch what is in vendor because it is not yours".

However, it has been pointed out that if every module is in a separate repo, you can use Composer to use git to get the module and put it in vendor, plus if you make local edits (e.g. a bug fix) you can easily push it back to the repo you got it from. This is convenient for developers, but assumes you create a separate git repo per module. The current hackathon approach of copying from vendor to app/code makes it harder (and slower) for developers due to a copy of the vendor files having been made.

So I like the idea of supporting two directories, vendor and app/code. The opinionated position is 'vendor is for composer downloads, app/code is for local code'. The fact that a developer can use vendor in another way is a (very useful) side bonus.

Oh, and to be clear, if you check out M2 from the master git repo, all the modules are "local" to that project, which is why they are in app/code. But if you build a real site project, they will all be downloaded under the 'vendor' directory instead. It is important to remember you WILL NOT git clone the master M2 repo when building customer sites.

alankent commented Apr 27, 2015

Interesting thread. Here is my current opinion until I change my mind! ;-)

The reason for supporting app/code (yes, want to change to app/modules - separate discussion) and vendor/ is to support two use cases. The reason for app/code is "code local to my project"; the reason for vendor is "this is downloaded by Composer".

If you want to contribute a bug fix to Magento 2, you should git clone the M2 repo and create a pull request. If you want to create a production site, you SHOULD NOT do it this way. You should use composer to download official released versions of all packages. It makes sense them residing in 'vendor'. The general rule is "don't touch what is in vendor because it is not yours".

However, it has been pointed out that if every module is in a separate repo, you can use Composer to use git to get the module and put it in vendor, plus if you make local edits (e.g. a bug fix) you can easily push it back to the repo you got it from. This is convenient for developers, but assumes you create a separate git repo per module. The current hackathon approach of copying from vendor to app/code makes it harder (and slower) for developers due to a copy of the vendor files having been made.

So I like the idea of supporting two directories, vendor and app/code. The opinionated position is 'vendor is for composer downloads, app/code is for local code'. The fact that a developer can use vendor in another way is a (very useful) side bonus.

Oh, and to be clear, if you check out M2 from the master git repo, all the modules are "local" to that project, which is why they are in app/code. But if you build a real site project, they will all be downloaded under the 'vendor' directory instead. It is important to remember you WILL NOT git clone the master M2 repo when building customer sites.

@kojiromike

This comment has been minimized.

Show comment
Hide comment
@kojiromike

kojiromike Apr 27, 2015

@joshdifabio I agree with you, but we're talking about different scopes. As a developer, it would be helpful to be able to do

composer create-project my/module
cd my_module && vendor/bin/phpunit # or whatever

and work on that module. (I believe composer create-project mimics the workflow you described.) But that's for writing code, running static checks and phpunit, which may have a few lib dependencies (hopefully mostly just interface mocks) on parts of Magento, but don't need a fully running webstore, and those Magento parts must not overwrite your checkout directory. So to revise:

  1. You should be able to run unit tests and static checks after a simple composer install in your module.
  2. But the vendor directory should still be considered the "right" place for an installed module in a full-fledged Magento webstore.

kojiromike commented Apr 27, 2015

@joshdifabio I agree with you, but we're talking about different scopes. As a developer, it would be helpful to be able to do

composer create-project my/module
cd my_module && vendor/bin/phpunit # or whatever

and work on that module. (I believe composer create-project mimics the workflow you described.) But that's for writing code, running static checks and phpunit, which may have a few lib dependencies (hopefully mostly just interface mocks) on parts of Magento, but don't need a fully running webstore, and those Magento parts must not overwrite your checkout directory. So to revise:

  1. You should be able to run unit tests and static checks after a simple composer install in your module.
  2. But the vendor directory should still be considered the "right" place for an installed module in a full-fledged Magento webstore.
@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Apr 27, 2015

Contributor

The reason for supporting app/code (yes, want to change to app/modules - separate discussion) and vendor/ is to support two use cases. The reason for app/code is "code local to my project"; the reason for vendor is "this is downloaded by Composer".

@alankent Thanks for the reply. There is a third use case, which concerns the active development of a module which lives in its own repository, has its own unit tests and which should be built and statically analysed separately from any particular project code. Think community modules.

Consider the following workflow:

$ git clone git@github.com:foo_vendor/bar_module.git
$ cd bar_module
$ composer install
$ php vendor/bin/phpunit

This is the standard, modern way of developing PHP packages. In such cases, the package (in this case a Magento module) which is being developed and tested will not be installed to the vendor directory; instead, it will reside above vendor, while Magento etc. will be installed to vendor as dependencies of the module. This is the workflow PHP developers use to test their code on CI platforms, such as Travis.

This PR makes this workflow possible and also allows modules to live in the vendor or app/code directories, which I agree should be the standard approach in production.

  1. You should be able to run unit tests and static checks after a simple composer install in your module.
  2. But the vendor directory should still be considered the "right" place for an installed module in a full-fledged Magento webstore.

@kojiromike Agreed.

Contributor

joshdifabio commented Apr 27, 2015

The reason for supporting app/code (yes, want to change to app/modules - separate discussion) and vendor/ is to support two use cases. The reason for app/code is "code local to my project"; the reason for vendor is "this is downloaded by Composer".

@alankent Thanks for the reply. There is a third use case, which concerns the active development of a module which lives in its own repository, has its own unit tests and which should be built and statically analysed separately from any particular project code. Think community modules.

Consider the following workflow:

$ git clone git@github.com:foo_vendor/bar_module.git
$ cd bar_module
$ composer install
$ php vendor/bin/phpunit

This is the standard, modern way of developing PHP packages. In such cases, the package (in this case a Magento module) which is being developed and tested will not be installed to the vendor directory; instead, it will reside above vendor, while Magento etc. will be installed to vendor as dependencies of the module. This is the workflow PHP developers use to test their code on CI platforms, such as Travis.

This PR makes this workflow possible and also allows modules to live in the vendor or app/code directories, which I agree should be the standard approach in production.

  1. You should be able to run unit tests and static checks after a simple composer install in your module.
  2. But the vendor directory should still be considered the "right" place for an installed module in a full-fledged Magento webstore.

@kojiromike Agreed.

@alankent

This comment has been minimized.

Show comment
Hide comment
@alankent

alankent Apr 27, 2015

Gotcha.

Side bar 1: This is actually another good reason to move service contracts out of implementation modules. When testing, if you have a mock, you can reduce the package download count as you don't need any implementations (and their transient dependencies), just the service contract packages.

Side bar 2: The interesting follow on unresolved question to me is how about integration, functional, etc testing. Where do those cross module tests sit and how to invoke them? But I would rather complete the vendor directory goal first and tackle the next step forwards as a separate objective.

alankent commented Apr 27, 2015

Gotcha.

Side bar 1: This is actually another good reason to move service contracts out of implementation modules. When testing, if you have a mock, you can reduce the package download count as you don't need any implementations (and their transient dependencies), just the service contract packages.

Side bar 2: The interesting follow on unresolved question to me is how about integration, functional, etc testing. Where do those cross module tests sit and how to invoke them? But I would rather complete the vendor directory goal first and tackle the next step forwards as a separate objective.

@otoolec

This comment has been minimized.

Show comment
Hide comment
@otoolec

otoolec Apr 27, 2015

Contributor

For "Module can define own internal folder structure (‘src’, ‘tests’, ‘doc’, ‘config’)" I was thinking about making it easier to change the structure of a module while still maintaining backwards compatibility for existing modules. The downside is that each module could come up with its own structure making it harder to maintain modules, but I was thinking of this as being similar to modman where the community came up with a structure different from Magento.

For “Module doesn’t need to provide new configuration”. I meant that no changes should be necessary for any existing modules to suddenly be used from the vendor directory vs being copied over. With the proposal in this PR there would be some configuration in composer.json and in bootstrap.php.

For "Module can reside in vendor or app/code" I was thinking about installed modules, but I completely agree that part of the benefit to allowing modules to live in vendor is that unit tests be run directly from a module's repository, since it no longer tries to copy files over as if it were being installed into magento when composer install is run.

Contributor

otoolec commented Apr 27, 2015

For "Module can define own internal folder structure (‘src’, ‘tests’, ‘doc’, ‘config’)" I was thinking about making it easier to change the structure of a module while still maintaining backwards compatibility for existing modules. The downside is that each module could come up with its own structure making it harder to maintain modules, but I was thinking of this as being similar to modman where the community came up with a structure different from Magento.

For “Module doesn’t need to provide new configuration”. I meant that no changes should be necessary for any existing modules to suddenly be used from the vendor directory vs being copied over. With the proposal in this PR there would be some configuration in composer.json and in bootstrap.php.

For "Module can reside in vendor or app/code" I was thinking about installed modules, but I completely agree that part of the benefit to allowing modules to live in vendor is that unit tests be run directly from a module's repository, since it no longer tries to copy files over as if it were being installed into magento when composer install is run.

@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Apr 27, 2015

Contributor

For “Module doesn’t need to provide new configuration”. I meant that no changes should be necessary for any existing modules to suddenly be used from the vendor directory vs being copied over. With the proposal in this PR there would be some configuration in composer.json and in bootstrap.php.

@otoolec Thanks for clarifying. Any Magento 2 modules which have already been implemented will have been designed to run from app/code (either committed directly to that path in a project repo or using a Composer plugin which copies them to that location). If this PR was merged, such modules would continue to function as they did before. With that in mind, is it an issue if we implement a new way of bootstrapping modules going forward? I think we just need to look for the best long-term solution which is achievable and implement it; if we want to eliminate the dependency on Composer plugins then that will impact vendors who've already been working on modules; the key point is that the impact can be minimal.

This proposal in no way conflicts with functionality which would search vendor for additional Magento modules, but do we really want to take that approach? Would we assume that any file named module.xml signifies a Magento module? Or would such files have to be located at vendor/[vendor]/[package]/etc/module.xml? Would we first load the file and check its contents before deciding that we've located a Magento module? What if a single Composer package contains multiple modules? (This is a common pattern.) Are we going to search infinitely deep in vendor for module.xml files? Searching vendor for Magento modules seems quite complex and fragile to me. Is that what you had in mind?

Contributor

joshdifabio commented Apr 27, 2015

For “Module doesn’t need to provide new configuration”. I meant that no changes should be necessary for any existing modules to suddenly be used from the vendor directory vs being copied over. With the proposal in this PR there would be some configuration in composer.json and in bootstrap.php.

@otoolec Thanks for clarifying. Any Magento 2 modules which have already been implemented will have been designed to run from app/code (either committed directly to that path in a project repo or using a Composer plugin which copies them to that location). If this PR was merged, such modules would continue to function as they did before. With that in mind, is it an issue if we implement a new way of bootstrapping modules going forward? I think we just need to look for the best long-term solution which is achievable and implement it; if we want to eliminate the dependency on Composer plugins then that will impact vendors who've already been working on modules; the key point is that the impact can be minimal.

This proposal in no way conflicts with functionality which would search vendor for additional Magento modules, but do we really want to take that approach? Would we assume that any file named module.xml signifies a Magento module? Or would such files have to be located at vendor/[vendor]/[package]/etc/module.xml? Would we first load the file and check its contents before deciding that we've located a Magento module? What if a single Composer package contains multiple modules? (This is a common pattern.) Are we going to search infinitely deep in vendor for module.xml files? Searching vendor for Magento modules seems quite complex and fragile to me. Is that what you had in mind?

@otoolec

This comment has been minimized.

Show comment
Hide comment
@otoolec

otoolec Apr 27, 2015

Contributor

is it an issue if we implement a new way of bootstrapping modules going forward?

I was thinking about that use case because if we have a way to prevent copying existing Magento modules without requiring them to make any changes, then we can stop supporting the Composer plugin instead of having to support it until all the existing extension migrate. That doesn't make it a show stopper, but I was interested in getting feedback on if anyone else cared about that.

What if a single Composer package contains multiple modules? (This is a common pattern.)

For some reason I thought every composer package would contain only a single module with metapackages being used to group modules together. I know that this repository violates that rule by putting all of the modules together and in the replace section, but I wasn't expecting that to happen with production modules so I wasn't thinking about it, but now that you mention it, it makes sense that someone might want to do the same thing for development so we should try to support. Another plus going for your proposal! 👍

Contributor

otoolec commented Apr 27, 2015

is it an issue if we implement a new way of bootstrapping modules going forward?

I was thinking about that use case because if we have a way to prevent copying existing Magento modules without requiring them to make any changes, then we can stop supporting the Composer plugin instead of having to support it until all the existing extension migrate. That doesn't make it a show stopper, but I was interested in getting feedback on if anyone else cared about that.

What if a single Composer package contains multiple modules? (This is a common pattern.)

For some reason I thought every composer package would contain only a single module with metapackages being used to group modules together. I know that this repository violates that rule by putting all of the modules together and in the replace section, but I wasn't expecting that to happen with production modules so I wasn't thinking about it, but now that you mention it, it makes sense that someone might want to do the same thing for development so we should try to support. Another plus going for your proposal! 👍

@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Apr 27, 2015

Contributor

I was thinking about that use case because if we have a way to prevent copying existing Magento modules without requiring them to make any changes, then we can stop supporting the Composer plugin instead of having to support it until all the existing extension migrate. That doesn't make it a show stopper, but I was interested in getting feedback on if anyone else cared about that.

As far as I understand, any existing Magento 2 modules will require magento/magento-composer-installer in composer.json and so they'll need updating regardless if we want to remove the Composer plugin completely.

Contributor

joshdifabio commented Apr 27, 2015

I was thinking about that use case because if we have a way to prevent copying existing Magento modules without requiring them to make any changes, then we can stop supporting the Composer plugin instead of having to support it until all the existing extension migrate. That doesn't make it a show stopper, but I was interested in getting feedback on if anyone else cared about that.

As far as I understand, any existing Magento 2 modules will require magento/magento-composer-installer in composer.json and so they'll need updating regardless if we want to remove the Composer plugin completely.

@otoolec otoolec self-assigned this Apr 28, 2015

@barryvdh

This comment has been minimized.

Show comment
Hide comment
@barryvdh

barryvdh Apr 30, 2015

Contributor

Do plug-ins need to be enabled automatically, when required with Composer (eg. with the autoload) wouldn't is be possible to use some kind of ServiceProvider class (Like other frameworks) which would be autoloaded when requested and sets up the module paths (and perhaps other bootstrapping needed). So modules can just be temporarily disabled without changing the composer file, or enable of of multiple modules from 1 package.

Contributor

barryvdh commented Apr 30, 2015

Do plug-ins need to be enabled automatically, when required with Composer (eg. with the autoload) wouldn't is be possible to use some kind of ServiceProvider class (Like other frameworks) which would be autoloaded when requested and sets up the module paths (and perhaps other bootstrapping needed). So modules can just be temporarily disabled without changing the composer file, or enable of of multiple modules from 1 package.

@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Apr 30, 2015

Contributor

Do plug-ins need to be enabled automatically, when required with Composer (eg. with the autoload) wouldn't is be possible to use some kind of ServiceProvider class (Like other frameworks) which would be autoloaded when requested and sets up the module paths (and perhaps other bootstrapping needed). So modules can just be temporarily disabled without changing the composer file, or enable of of multiple modules from 1 package. – @barryvdh

Good question. I certainly think it should be possible to explicitly disable modules, but do we want to enforce an additional step after composer install if this isn't necessary for other reasons (e.g. deploying static assets which modules provide)?

As someone who expects to run composer install or composer update and have Composer install and bootstrap any new dependencies, it would be tedious as a project maintainer to have to take additional, manual steps afterwards. And from the perspective of a module vendor, I'd want the freedom to re-factor a module into two modules, or require other modules using composer.json, without requiring any action from project maintainers beyond a simple composer update.

How about agencies with code suites which are common to many clients; if a module is added to such a suite, do we want agency developers to have to go through many client projects, individually enabling any modules which are newly required as a result of a composer update?

My personal preference is probably quite obvious: bootstrap and enable modules by default, and allow developers to disable them explicitly. But that doesn't mean I'm not open to changing my mind! I can appreciate that there might be reasons to require a manual deployment step after installing new modules.

How do Magento see this issue? @otoolec @alankent

Contributor

joshdifabio commented Apr 30, 2015

Do plug-ins need to be enabled automatically, when required with Composer (eg. with the autoload) wouldn't is be possible to use some kind of ServiceProvider class (Like other frameworks) which would be autoloaded when requested and sets up the module paths (and perhaps other bootstrapping needed). So modules can just be temporarily disabled without changing the composer file, or enable of of multiple modules from 1 package. – @barryvdh

Good question. I certainly think it should be possible to explicitly disable modules, but do we want to enforce an additional step after composer install if this isn't necessary for other reasons (e.g. deploying static assets which modules provide)?

As someone who expects to run composer install or composer update and have Composer install and bootstrap any new dependencies, it would be tedious as a project maintainer to have to take additional, manual steps afterwards. And from the perspective of a module vendor, I'd want the freedom to re-factor a module into two modules, or require other modules using composer.json, without requiring any action from project maintainers beyond a simple composer update.

How about agencies with code suites which are common to many clients; if a module is added to such a suite, do we want agency developers to have to go through many client projects, individually enabling any modules which are newly required as a result of a composer update?

My personal preference is probably quite obvious: bootstrap and enable modules by default, and allow developers to disable them explicitly. But that doesn't mean I'm not open to changing my mind! I can appreciate that there might be reasons to require a manual deployment step after installing new modules.

How do Magento see this issue? @otoolec @alankent

@otoolec

This comment has been minimized.

Show comment
Hide comment
@otoolec

otoolec Apr 30, 2015

Contributor

Should installing a module via composer cause it to be enabled and setup automatically? This would certainly be convenient, but would there be a potential negative impact since modules alter the database when their setup scripts are run? Not sure how everyone feels about having composer update causing database writes as a side-effect.

Right now to get a sample module to show up in a clone of this repository one needs to run the following commands:

composer config repositories.magento composer http://packages.magento.com/
composer require magento/sample-module-minimal:*
php bin/magento module:enable Magento_SampleMinimal
php bin/magento setup:upgrade

It should be possible to create a new command that simplifies some of the above if desired. It could also be possible to automatically enable a module but not run setup:upgrade when the module is installed from composer, but that will most likely break Magento instead of doing anything useful.

Thoughts?

Contributor

otoolec commented Apr 30, 2015

Should installing a module via composer cause it to be enabled and setup automatically? This would certainly be convenient, but would there be a potential negative impact since modules alter the database when their setup scripts are run? Not sure how everyone feels about having composer update causing database writes as a side-effect.

Right now to get a sample module to show up in a clone of this repository one needs to run the following commands:

composer config repositories.magento composer http://packages.magento.com/
composer require magento/sample-module-minimal:*
php bin/magento module:enable Magento_SampleMinimal
php bin/magento setup:upgrade

It should be possible to create a new command that simplifies some of the above if desired. It could also be possible to automatically enable a module but not run setup:upgrade when the module is installed from composer, but that will most likely break Magento instead of doing anything useful.

Thoughts?

@kojiromike

This comment has been minimized.

Show comment
Hide comment
@kojiromike

kojiromike Apr 30, 2015

I think you're right, @otoolec, that it would be inadvisable to let a module make database changes after just a composer install.

That said, it's worth considering the use case for an integrator managing projects with many module components. Must that integrator have a script that expressly enables all the modules one by one? Writing such a script would be challenging because you'd have to figure out a way to map a package's composer name to its (one or more) Magento module names.

What I'm saying is that there probably should be a way to enable modules on composer install, but I would not want it to be the default behavior.

Secondarily, it would be really nice if there were a convenient way to map a composer package's name to its Magento modules, because I have a feeling that's going to come up again.

kojiromike commented Apr 30, 2015

I think you're right, @otoolec, that it would be inadvisable to let a module make database changes after just a composer install.

That said, it's worth considering the use case for an integrator managing projects with many module components. Must that integrator have a script that expressly enables all the modules one by one? Writing such a script would be challenging because you'd have to figure out a way to map a package's composer name to its (one or more) Magento module names.

What I'm saying is that there probably should be a way to enable modules on composer install, but I would not want it to be the default behavior.

Secondarily, it would be really nice if there were a convenient way to map a composer package's name to its Magento modules, because I have a feeling that's going to come up again.

@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Apr 30, 2015

Contributor

Should installing a module via composer cause it to be enabled and setup automatically? This would certainly be convenient, but would there be a potential negative impact since modules alter the database when their setup scripts are run? Not sure how everyone feels about having composer update causing database writes as a side-effect. – @otoolec

Regarding database upgrades; I agree, they shouldn't be executed automatically, particularly as database upgrades are per-environment and most agencies won't be running Composer on UAT or production.

Regarding the default state of modules (enabled or disabled) upon installation, I think I'd prefer them to be enabled by default, and I'll try to explain why.

Firstly, the way extension developers work at present is to distribute their modules such that they are automatically enabled when they are pulled in by Composer. Check firegento for some examples (note that all the modules specify active = true in the module configuration files). The typical approach in Magento 1, which works well, is approximately:

  1. Add a package to your project's composer.json file; Composer installs potentially many Magento modules
  2. Use magerun to see which modules have pending database upgrades
  3. Use magerun to execute those database upgrades manually

Executing database upgrades with a single terminal command is easy enough; Magento developers are used to doing that and it's easy to integrate into the deployment process. Knowing which modules need to be enabled after running Composer would be less straightforward if there were a number of changes. Developers would most likely just enable everything after each update, or they'd forget to do it. Is there value in that approach?

If I add a package requirement to composer.json then it's because I need that package; whether I'm the project maintainer or an extension maintainer.

It could also be possible to automatically enable a module but not run setup:upgrade when the module is installed from composer, but that will most likely break Magento instead of doing anything useful.

True, but developers are used to this. Also, what if I updated my module and added some new setup scripts? If the module was already installed in a project and was updated, it'd still break as it'd already be enabled but the database schema would be out of date. Developers will always need to run setup scripts after updating their code. I think we should make things as simple as possible and have a single workflow.

Contributor

joshdifabio commented Apr 30, 2015

Should installing a module via composer cause it to be enabled and setup automatically? This would certainly be convenient, but would there be a potential negative impact since modules alter the database when their setup scripts are run? Not sure how everyone feels about having composer update causing database writes as a side-effect. – @otoolec

Regarding database upgrades; I agree, they shouldn't be executed automatically, particularly as database upgrades are per-environment and most agencies won't be running Composer on UAT or production.

Regarding the default state of modules (enabled or disabled) upon installation, I think I'd prefer them to be enabled by default, and I'll try to explain why.

Firstly, the way extension developers work at present is to distribute their modules such that they are automatically enabled when they are pulled in by Composer. Check firegento for some examples (note that all the modules specify active = true in the module configuration files). The typical approach in Magento 1, which works well, is approximately:

  1. Add a package to your project's composer.json file; Composer installs potentially many Magento modules
  2. Use magerun to see which modules have pending database upgrades
  3. Use magerun to execute those database upgrades manually

Executing database upgrades with a single terminal command is easy enough; Magento developers are used to doing that and it's easy to integrate into the deployment process. Knowing which modules need to be enabled after running Composer would be less straightforward if there were a number of changes. Developers would most likely just enable everything after each update, or they'd forget to do it. Is there value in that approach?

If I add a package requirement to composer.json then it's because I need that package; whether I'm the project maintainer or an extension maintainer.

It could also be possible to automatically enable a module but not run setup:upgrade when the module is installed from composer, but that will most likely break Magento instead of doing anything useful.

True, but developers are used to this. Also, what if I updated my module and added some new setup scripts? If the module was already installed in a project and was updated, it'd still break as it'd already be enabled but the database schema would be out of date. Developers will always need to run setup scripts after updating their code. I think we should make things as simple as possible and have a single workflow.

@vrann

This comment has been minimized.

Show comment
Hide comment
@vrann

vrann Apr 30, 2015

Contributor

@joshdifabio corner case situation, though it was mentioned here before: what if you download a package consisting of multiple modules and want just half of them being enabled? Right now we have just one package like that, magento2-community-edition, but I feel this will happen more often once bundle packages will be implemented.

Another concern with the module enabled via composer plugin is that sometime you may need to distinguish these two steps. For example if we build Web UI wizard on top of the composer, most likely you would like to distinguish package download and installation, because more steps can be added in between (like, database backup)

Personally, I would prefer to do what @otoolec proposed to have a wrapper tool on top of composer which can automate package installation and module enable in one step.

Contributor

vrann commented Apr 30, 2015

@joshdifabio corner case situation, though it was mentioned here before: what if you download a package consisting of multiple modules and want just half of them being enabled? Right now we have just one package like that, magento2-community-edition, but I feel this will happen more often once bundle packages will be implemented.

Another concern with the module enabled via composer plugin is that sometime you may need to distinguish these two steps. For example if we build Web UI wizard on top of the composer, most likely you would like to distinguish package download and installation, because more steps can be added in between (like, database backup)

Personally, I would prefer to do what @otoolec proposed to have a wrapper tool on top of composer which can automate package installation and module enable in one step.

@antonkril

View changes

Show outdated Hide outdated lib/internal/Magento/Framework/Module/Dir/Resolver.php
@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio May 1, 2015

Contributor

There are a few classes in the Magento code base which will only work with modules located in app/code whereas they should use the Magento\Framework\Module package to locate module files. Regardless of the approach we take with bootstrapping/installing modules, if we wish to remove the limitation of having to copy modules to app/code we will need to re-factor those classes. Can I suggest that that work is done as separate PRs which don't attempt to address the issue of bootstrapping/installing modules as a first step? That work seems like a no-brainer, and at present a few of us are duplicating that effort across PRs which seek to address too many issues at once.

Thoughts?

Contributor

joshdifabio commented May 1, 2015

There are a few classes in the Magento code base which will only work with modules located in app/code whereas they should use the Magento\Framework\Module package to locate module files. Regardless of the approach we take with bootstrapping/installing modules, if we wish to remove the limitation of having to copy modules to app/code we will need to re-factor those classes. Can I suggest that that work is done as separate PRs which don't attempt to address the issue of bootstrapping/installing modules as a first step? That work seems like a no-brainer, and at present a few of us are duplicating that effort across PRs which seek to address too many issues at once.

Thoughts?

@otoolec

This comment has been minimized.

Show comment
Hide comment
@otoolec

otoolec May 19, 2015

Contributor

@joshdifabio I'm all for incremental improvements that get us to where we want to go. I think your pull request is a step in the right direction.

Was there anything else you wanted to add to this PR?

Contributor

otoolec commented May 19, 2015

@joshdifabio I'm all for incremental improvements that get us to where we want to go. I think your pull request is a step in the right direction.

Was there anything else you wanted to add to this PR?

magento-team pushed a commit that referenced this pull request Jun 19, 2015

magento-team pushed a commit that referenced this pull request Jun 19, 2015

@magento-team magento-team merged commit f616564 into magento:develop Jun 19, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

magento-team added a commit to magento/magento2-community-edition that referenced this pull request Jun 19, 2015

0.74.0-beta14
* Framework improvements:
    * Introduced an ability to uninstall modules which were installed via composer (bin/magento module:uninstall <moduleName>)
    * Introduced an ability to uninstall themes (bin/magento theme:uninstall <themeName>)
    * Introduced an ability to backup and rollback DB and Media via CLI (bin/magento setup:backup, options are --code, --db or --media)
    * Introduced an ability to uninstall language packages (bin/magento i18n:uninstall <languagePack>)
    * Introduced API notation for the following modules: Backend, Backup, Cron, Log, PageCache
    * Added join processors to search services, joined config for services with extension attributes
    * Renamed hidden_tax to discount_tax_compensation
    * The customer address entity table was transformed from EAV into a flat model to minimize database operations
* Fixed bugs:
    * Fixed an issue where Setup Wizard failed on readiness check when Magento was deployed by composer create-project
    * Fixed the local file path disclosure when trying to browse image cache directory
    * Fixed an issue where development errors resulted in too many redirects
    * Fixed an integration test failure in Reports ViewedTest
    * Fixed an issue where it was impossible to save existent Grouped Product with no child items
    * Fixed an issue where message "We don't have as many "conf1" as you requested" appeared
    * Fixed an issue where second product from bundle product was ordered as separate item after checkout
    * Fixed an issue where configs for payments and shippings were not encrypted
    * Fixed an issue where Table Rates shipping method did not work
    * Fixed an issue where admin could not set locale properly on Account page
    * Fixed incomplete generated results of single tenant compiler
    * Fixed an issue with full page caching where one set of prices was cached for all customers
    * Fixed incorrect urls for private content
    * Fixed an issue where it was not possible to assign a product link to another product using API
    * Fixed an issue where zipcode was not displayed as required field on Create New Order page
    * Fixed the Sample Data re-installation
    * Fixed random fails on inventory tab for test CreateSimpleProductEntityTest
* Tests:
    * Covered various modules with unit tests
    * Functional tests fixed and maintained
* GitHub issues:
    * [#1156](magento/magento2#1156) -- Moves common code to all auto-generated Interceptor classes into a trait
    * [#1206](magento/magento2#1206) -- Allow modules to live outside of app/code directory
    * [#1245](magento/magento2#1245) -- Unable to save product per website wise
    * [#1347](magento/magento2#1347) -- Fixed failing Install during integration tests (MAGETWO-38482)
    * [#1368](magento/magento2#1368) -- Fix typo in getCurrentCategoryKey

magento-team added a commit to magento/magento2-community-edition that referenced this pull request Jun 19, 2015

0.74.0-beta14
* Framework improvements:
    * Introduced an ability to uninstall modules which were installed via composer (bin/magento module:uninstall <moduleName>)
    * Introduced an ability to uninstall themes (bin/magento theme:uninstall <themeName>)
    * Introduced an ability to backup and rollback DB and Media via CLI (bin/magento setup:backup, options are --code, --db or --media)
    * Introduced an ability to uninstall language packages (bin/magento i18n:uninstall <languagePack>)
    * Introduced API notation for the following modules: Backend, Backup, Cron, Log, PageCache
    * Added join processors to search services, joined config for services with extension attributes
    * Renamed hidden_tax to discount_tax_compensation
    * The customer address entity table was transformed from EAV into a flat model to minimize database operations
* Fixed bugs:
    * Fixed an issue where Setup Wizard failed on readiness check when Magento was deployed by composer create-project
    * Fixed the local file path disclosure when trying to browse image cache directory
    * Fixed an issue where development errors resulted in too many redirects
    * Fixed an integration test failure in Reports ViewedTest
    * Fixed an issue where it was impossible to save existent Grouped Product with no child items
    * Fixed an issue where message "We don't have as many "conf1" as you requested" appeared
    * Fixed an issue where second product from bundle product was ordered as separate item after checkout
    * Fixed an issue where configs for payments and shippings were not encrypted
    * Fixed an issue where Table Rates shipping method did not work
    * Fixed an issue where admin could not set locale properly on Account page
    * Fixed incomplete generated results of single tenant compiler
    * Fixed an issue with full page caching where one set of prices was cached for all customers
    * Fixed incorrect urls for private content
    * Fixed an issue where it was not possible to assign a product link to another product using API
    * Fixed an issue where zipcode was not displayed as required field on Create New Order page
    * Fixed the Sample Data re-installation
    * Fixed random fails on inventory tab for test CreateSimpleProductEntityTest
* Tests:
    * Covered various modules with unit tests
    * Functional tests fixed and maintained
* GitHub issues:
    * [#1156](magento/magento2#1156) -- Moves common code to all auto-generated Interceptor classes into a trait
    * [#1206](magento/magento2#1206) -- Allow modules to live outside of app/code directory
    * [#1245](magento/magento2#1245) -- Unable to save product per website wise
    * [#1347](magento/magento2#1347) -- Fixed failing Install during integration tests (MAGETWO-38482)
    * [#1368](magento/magento2#1368) -- Fix typo in getCurrentCategoryKey

@joshdifabio joshdifabio deleted the joshdifabio:modules-in-vendor2 branch Jun 19, 2015

@danslo

This comment has been minimized.

Show comment
Hide comment
@danslo

danslo Jul 12, 2015

Contributor

Was this tested? It works for registering the module but as it looks now, it will never find module configuration files.

Magento\Framework\Module\Dir\Reader::getConfigurationFiles calls getRelativePath but this will never be accurate for things living in vendor directory, because $basePath is <path_to_magento>/app/code.

Contributor

danslo commented Jul 12, 2015

Was this tested? It works for registering the module but as it looks now, it will never find module configuration files.

Magento\Framework\Module\Dir\Reader::getConfigurationFiles calls getRelativePath but this will never be accurate for things living in vendor directory, because $basePath is <path_to_magento>/app/code.

@danslo

This comment has been minimized.

Show comment
Hide comment
@danslo

danslo Jul 12, 2015

Contributor

I'm also not quite sure why getModulePaths and getModulePath in Magento\Framework\Module\Registrar are non-static.

Contributor

danslo commented Jul 12, 2015

I'm also not quite sure why getModulePaths and getModulePath in Magento\Framework\Module\Registrar are non-static.

@Vinai

This comment has been minimized.

Show comment
Hide comment
@Vinai

Vinai Jul 12, 2015

Contributor

I haven't specifically checked the PR, however I have created a bunch of modules installed directly (and only) into vendor since, for example https://github.com/vinai/VinaiKopp_EavOptionSetup. Works okay.

Contributor

Vinai commented Jul 12, 2015

I haven't specifically checked the PR, however I have created a bunch of modules installed directly (and only) into vendor since, for example https://github.com/vinai/VinaiKopp_EavOptionSetup. Works okay.

@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Jul 12, 2015

Contributor

@danslo There are a number of places in the code base which still need re-factoring in order for this PR to work as intended. I believe it's with @otoolec of Magento but I'm not certain. If it's not being looked at I'm happy to nibble away at some of those issues myself?

Edit:

I'm also not quite sure why getModulePaths and getModulePath in Magento\Framework\Module\Registrar are non-static.

They aren't static because they don't have to be static. Those methods are part of ModuleRegistryInterface which doesn't contain the static registerModule() method. The clients of the ModuleRegistryInterface are Magento classes which need to be able to locate modules; by making those methods non-static it's then possible to inject other implementations into those client classes, for example mock objects to help with testing. registerModule() is static because it has to be, and its clients are modules, not the Magento classes which need to be able to locate modules. So, although all these methods are implemented in ModuleRegistrar, they implement different interfaces and have different clients.

Hope that makes some sense!

Contributor

joshdifabio commented Jul 12, 2015

@danslo There are a number of places in the code base which still need re-factoring in order for this PR to work as intended. I believe it's with @otoolec of Magento but I'm not certain. If it's not being looked at I'm happy to nibble away at some of those issues myself?

Edit:

I'm also not quite sure why getModulePaths and getModulePath in Magento\Framework\Module\Registrar are non-static.

They aren't static because they don't have to be static. Those methods are part of ModuleRegistryInterface which doesn't contain the static registerModule() method. The clients of the ModuleRegistryInterface are Magento classes which need to be able to locate modules; by making those methods non-static it's then possible to inject other implementations into those client classes, for example mock objects to help with testing. registerModule() is static because it has to be, and its clients are modules, not the Magento classes which need to be able to locate modules. So, although all these methods are implemented in ModuleRegistrar, they implement different interfaces and have different clients.

Hope that makes some sense!

@danslo

This comment has been minimized.

Show comment
Hide comment
@danslo

danslo Jul 12, 2015

Contributor

@joshdifabio Fair enough on the static discussion, I just never saw an instance of Registrar being passed anywhere, that's kind of the reason for doubt.

After seeing this PR merged I (wrongly?) assumed this was working. I can definitely confirm that di.xml configuration (and likely others) aren't loaded when not in app/code (reasons mentioned above). Vinai's module only has a module.xml, maybe that follows a different codepath.

Contributor

danslo commented Jul 12, 2015

@joshdifabio Fair enough on the static discussion, I just never saw an instance of Registrar being passed anywhere, that's kind of the reason for doubt.

After seeing this PR merged I (wrongly?) assumed this was working. I can definitely confirm that di.xml configuration (and likely others) aren't loaded when not in app/code (reasons mentioned above). Vinai's module only has a module.xml, maybe that follows a different codepath.

@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Jul 13, 2015

Contributor

...I just never saw an instance of Registrar being passed anywhere, that's kind of the reason for doubt.

It should be handled by a preference in di.xml.

After seeing this PR merged I (wrongly?) assumed this was working. I can definitely confirm that di.xml configuration (and likely others) aren't loaded when not in app/code (reasons mentioned above). Vinai's module only has a module.xml, maybe that follows a different codepath.

It was definitely a fair assumption to make! There are a number of places in the code base at which it is still assumed that all module code lives in app/code which need re-factoring, such as the one you've highlighted; this is probably why Magento aren't yet officially recommending the module registrar approach. I'm not sure how high this is on Magento's priority list, but I should have some time this week so I'll submit some PRs myself if the work isn't already done (@otoolec).

Contributor

joshdifabio commented Jul 13, 2015

...I just never saw an instance of Registrar being passed anywhere, that's kind of the reason for doubt.

It should be handled by a preference in di.xml.

After seeing this PR merged I (wrongly?) assumed this was working. I can definitely confirm that di.xml configuration (and likely others) aren't loaded when not in app/code (reasons mentioned above). Vinai's module only has a module.xml, maybe that follows a different codepath.

It was definitely a fair assumption to make! There are a number of places in the code base at which it is still assumed that all module code lives in app/code which need re-factoring, such as the one you've highlighted; this is probably why Magento aren't yet officially recommending the module registrar approach. I'm not sure how high this is on Magento's priority list, but I should have some time this week so I'll submit some PRs myself if the work isn't already done (@otoolec).

@Vinai

This comment has been minimized.

Show comment
Hide comment
@Vinai

Vinai Jul 13, 2015

Contributor

I also assumed the PR was complete!
Maybe compiling a list of places where app/code is still assumed is less work then directly changing them. If so, please post it here then I can commit some time to this, too, without us working on the same areas.

Contributor

Vinai commented Jul 13, 2015

I also assumed the PR was complete!
Maybe compiling a list of places where app/code is still assumed is less work then directly changing them. If so, please post it here then I can commit some time to this, too, without us working on the same areas.

@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Jul 13, 2015

Contributor

I'll create a new issue after work this evening with a check-list of the areas which require attention, or you can create it now if you'd prefer. We can use that to keep track of what needs doing. I'm sure someone from Magento will let us know what's already been resolved internally.

Contributor

joshdifabio commented Jul 13, 2015

I'll create a new issue after work this evening with a check-list of the areas which require attention, or you can create it now if you'd prefer. We can use that to keep track of what needs doing. I'm sure someone from Magento will let us know what's already been resolved internally.

@Vinai

This comment has been minimized.

Show comment
Hide comment
@Vinai

Vinai Jul 13, 2015

Contributor

Won't have time today unfortunately, but can't wait to help make this PR feature complete. One of the most important changes from my perspective :) 👍
thumbs-up

Contributor

Vinai commented Jul 13, 2015

Won't have time today unfortunately, but can't wait to help make this PR feature complete. One of the most important changes from my perspective :) 👍
thumbs-up

@alankent

This comment has been minimized.

Show comment
Hide comment
@alankent

alankent Jul 14, 2015

We would love this change, but its not high priority internally due to other commitments. Discipline and cut lists is how we are hitting dates reliably. So we love the work but not likely to happen for 2.0 without this wonderful community contributions continuing.

alankent commented Jul 14, 2015

We would love this change, but its not high priority internally due to other commitments. Discipline and cut lists is how we are hitting dates reliably. So we love the work but not likely to happen for 2.0 without this wonderful community contributions continuing.

@joshdifabio

This comment has been minimized.

Show comment
Hide comment
@joshdifabio

joshdifabio Jul 23, 2015

Contributor

We would love this change, but its not high priority internally due to other commitments. Discipline and cut lists is how we are hitting dates reliably.

I'm sure anyone who does this for a living can relate. Thanks for the update, keep up the good work.

Contributor

joshdifabio commented Jul 23, 2015

We would love this change, but its not high priority internally due to other commitments. Discipline and cut lists is how we are hitting dates reliably.

I'm sure anyone who does this for a living can relate. Thanks for the update, keep up the good work.

@vrann vrann added Progress: accept and removed in progress labels Aug 13, 2015

okorshenko pushed a commit to magento/magento2-community-edition that referenced this pull request Nov 5, 2015

Oleksii Korshenko
2.0.0-rc
=============
* Performance improvements:
    * Refactored observer classes to satisfy the single-responsibility requirement in order to minimize the time on observer object loading
    * Improved performance of catalog advanced search
    * Improved performance of catalog quick search
    * Various micro-optimizations of the Magento framework
    * Optimized stores initialization and data loading
    * Improved the CSS and JavaScript minification mechanism
    * Sales rules (cart promotions) performance optimization
    * Improved performance of table rendering
    * Improved sample data performance
* Payment methods improvements:
    * Implemented the eWay online payment gateway using both Client side encryption and Responsive shared page APIs.
    * Implemented PayPal best practices for PayPal Express Checkout
    * Improved the UI for the Braintree payment method
* Checkout improvements:
    * Implemented persistence of entered customer’s data on Checkout flow
    * Implemented persistence of customer Shopping Cart
    * Improved Terms and Conditions settings
    * Improved error handling mechanism on Checkout flow
    * Improved the collect totals mechanism
    * Improved the checkout credit card form design
* Product improvements:
    * Added the ability to manage the list of variations/configurations using the wizard or manually
    * Added the ability to manage the list of variations based on removing or adding a new attribute
    * Added the ability to notify the user during product information update or management
    * Added the ability to notify the user during the change of the product template set when saving a product
    * Added the ability to update product data during mass action update
    * Added the "remove attribute" button to each attribute on the second step of the configurable product attributes creation
    * Removed grouped price from product creation functionality
    * Discounts logic is based on the selected option of a configurable product
* CMS improvements:
   * Added a sticky header and controls to data grids
   * Added the support for multiple select in data grid filters
   * Added the ability to change column width in data grids
   * Added the ability of inline and bulk inline editing in data grids
* WebApi Framework improvements:
    * Added the support for store codes in API calls
    * Added the ability to update the Magento system to a particular version of Magento
    * Added the ability to enable/disable modules for Magento application
    * Added the ability to use maintenance mode
    * Introduced the common interface for Webapi payload processors
    * Moved the Search API from the Search module to the Search Framework library
* Framework improvements:
    * Refactored observer classes to implement the same interface
    * Added HHVM compatibility with the `intl` extension
    * Added the support for  PHP 7
    * Improved catalog image generation
    * Added the ability to store Magento code in the `vendor` directory
    * Added support for the URN  schema in configuration files
    * Implemented a new component for Model Windows to simplify input/modification data in pop-us
    * Implemented the gallery widget with MVP functionality in Global JS Widgets Library
    * Included the migration tool to Magento CLI
    * Added inline editing in data grids
    * Updated data grids mass actions
    * Added  the export  to data grids
    * Implemented the full text search in data grids in Admin
    * Applied finalized new data grid to Customer List
    * Removed the DesignEditor module and related code
    * Added Swagger REST API schema generation for automatic API documentation creation
    * Added the ability to generate a page that reports all REST APIs in the system
    * Added the Webapidoc module to generate on-the-fly API documentation for a particular Magento instance
    * Added the support for inline translation in Magento UI components
    * New data grid component applied on Sales data grid
    * Unified database resource connections interface
    * Implemented a mechanism for rendering escaped string
    * Added the ability to extend any Magento JS Component after it is loaded on the page and before it is executed
    * Added the @remove and @display attributes to handle block and container appearance
    * Added the ability to send the purge requests for multiple servers
    * Removed deprecated code from modules and Magento/Framework
    * Implemented the independent template hints for the storefront and the Admin panel
* Setup
    * Improved the wording in the Web Installation Wizard UI
    * Updated the extensions styles in the Web Installation Wizard
    * Added the ability to control access to the setup tool
    * Added the Install Components functionality for Web Installation Wizard
    * Updated styles
* Sample Data:
    * Improved sample data installation UX
    * Updated sample data with Product Heros, color swatches, MAP and rule based product relations
    * Improved sample data upgrade flow
    * Added the ability to log errors and set the error flag during sample data installation
* Various improvements:
    * Added integration with NewRelic
    * Added dashboard for Platinum integration partners
    * Improved downloadable products UI in the Admin panel in order to provide the same experience with all other product types
    * Implemented email templates responsiveness and localization
    * Implemented WebApi to retrieve store information, country list and currency information
    * Implemented discount coupon generation, search, and multi-actions APIs
    * Added the ability to declare filter components inside the data grid column definition
    * Added the console command `catalog:image:resize`
    * Consolidated the algorithms for populating system packages for upgrade and other tasks
    * Implemented various accessibility improvements
    * Improved UX for Tax Rule Management
    * The Luma theme became default storefront theme
    * Refactored grid store selectors for their unification
    * Refactored Magento UI Library to use the 'lib-' prefix in all library mixins
    * Refactored styles to eliminate log file errors after static files deployment is executed
    * Improved user experience on the backup pages of Component Manager
    * Increased JS and PHP code coverage with unit tests
    * Implemented the product attribute swatches functionality
* GitHub issues and requests:
    * [#1397](magento/magento2#1397) -- Allow multiple caplitalized words (like typical vendor names) in ACL resource IDs
    * [#1231](magento/magento2#1231) -- Update Cm_Cache_Backend_Redis to v1.8 and Credis_Client to v1.5
    * [#1375](magento/magento2#1375) -- Allow phrases to contain more than nine numeric placeholders
    * [#1454](magento/magento2#1454) -- Permissions not set correctly #1453
    * [#1410](magento/magento2#1410) -- Allow custom config files with virtual types only by adding generic schema locator
    * [#1416](magento/magento2#1416) -- Add abstract method execute() to \Magento\Framework\App\Action\Action
    * [#1406](magento/magento2#1406) -- Fixes issue with reading store config for store with code of 'default'
    * [#1447](magento/magento2#1447) -- Missing strings for Javascript Translations
    * [#1465](magento/magento2#1465) -- Fix typo: itno => into
    * [#1476](magento/magento2#1476) -- fix typo in dispatched event
    * [#1516](magento/magento2#1516) -- Typo in addAction function: _.findIdnex should be .findIndex
    * [#1533](magento/magento2#1533) -- Updated the broken dev doc links in the README.md
    * [#1469](magento/magento2#1469) -- Remove dependency of date renderer on global state locale
    * [#1462](magento/magento2#1462) -- Product collection - Add url rewrite from different website
    * [#1422](magento/magento2#1422) -- Improve \Magento\Framework\Api\SortOrder
    * [#1528](magento/magento2#1528) -- Really hide Pdf totals with zero amounts
    * [#1641](magento/magento2#1641) -- Update create-admin-account.phtml
    * [#1440](magento/magento2#1440) -- Update Console Tool Usage for Cache and Index Operations
    * [#1523](magento/magento2#1523) -- Permissions not set correctly
    * [#1517](magento/magento2#1517) -- add router.php for php Built-in webserver
    * [#1654](magento/magento2#1654) -- Update filter.phtml
    * [#1602](magento/magento2#1602) -- Improve product export performance
    * [#1062](magento/magento2#1062) -- Add check to see if PHP > 5.6 and always_populate_raw_post_data = -1
    * [#1496](magento/magento2#1496) -- Add "Not Specified" as a gender option when customer does not specify gender
    * [#1664](magento/magento2#1664) -- AbstractPdf::_getTotalsList - fix return comment
    * [#1502](magento/magento2#1502) -- Loosened Regex on GB postcodes
    * [#1801](magento/magento2#1801) -- Enable translation for gender
    * [#1835](magento/magento2#1835) -- Added exception to event data
    * [#1854](magento/magento2#1854) -- Added missing @method annotation for setWebsiteIds
    * [#1818](magento/magento2#1818) -- use return value instead of reference parameter
    * [#1206](magento/magento2#1206) -- Allow modules to live outside of app/code directory
    * [#1869](magento/magento2#1869) -- Typo in function name fixed (stove->store)
    * [#1792](magento/magento2#1792) -- Fix invalid @method phpdoc to prevent prophecy mocking error
    * [#1483](magento/magento2#1483) -- admin external extjs.com requests acknowledged bug CS needs update
    * [#1489](magento/magento2#1489) -- Russia is eligible PayPal merchant country but absent in Magento 2.0 "Merchant Country" dropdown
    * [#1461](magento/magento2#1461) -- Cart Items are not deleted after success checkout
    * [#1452](magento/magento2#1452) -- First two orders with sample data fail
    * [#1458](magento/magento2#1458) -- window.checkout is undefined if minicart is removed
    * [#1443](magento/magento2#1443) -- GET /V1/carts/mine/items is returning "cartId is a required field"
    * [#1442](magento/magento2#1442) -- Running 'log:clean' through cli results in an error: 'Area code not set'
    * [#1435](magento/magento2#1435) -- Fatal error: Class 'Magento\Framework\HTTP\Client_Curl' not found
    * [#1432](magento/magento2#1432) -- Doesn't work sorting in the search list.
    * [#460](magento/magento2#460) -- Optimization on Weee tax calculation
    * [#647](magento/magento2#647) -- Template path hints behavior
    * [#771](magento/magento2#771) -- Fatal error when calling execute()
    * [#896](magento/magento2#896) -- i18n generator generates dictionary with duplicated phrases
    * [#930](magento/magento2#930) -- Flushing cache fails to wipe view_preprocessed dir
    * [#933](magento/magento2#933) -- Admin fields that use WYSIWYG don't pass the js validation
    * [#939](magento/magento2#939) -- Inline Translation adds <span> within <head>
    * [#941](magento/magento2#941) -- [Question] How to get the currency code and symbol of an AbstractPrice?
    * [#1159](magento/magento2#1159) -- Warning: The email and password is visible in front-end
    * [#1167](magento/magento2#1167) -- Magento_Log: creating new record with wrong store_id every each refresh page for Adminhtml
    * [#1192](magento/magento2#1192) -- Error in monetary value Brazil
    * [#1367](magento/magento2#1367) -- String class name issue for php7
    * [#1242](magento/magento2#1242) -- eclipse pdt validator error
    * [#1279](magento/magento2#1279) -- related products not able to add to cart
    * [#1423](https://github.com/magento/magento2/issue/1423) -- Magento\Email\Model\Template\Filter Comment vs Code
    * [#1418](https://github.com/magento/magento2/issue/1418) -- Items in minicart are not cleared after successful placing an order
    * [#1408](https://github.com/magento/magento2/issue/1408) -- Error command cli setup:static-content:deploy
    * [#1396](https://github.com/magento/magento2/issue/1396) -- Products are not shown in category right after import

@mazhalai mazhalai referenced this pull request Nov 11, 2015

Closed

RC deploy via composer #2329

magento-team pushed a commit that referenced this pull request Jun 20, 2017

Merge pull request #1206 from magento-qmt/MQE-121
Story
- MQE-121 Re-run failed tests for MTF 1.0

VitaliyBoyko pushed a commit to VitaliyBoyko/magento2 that referenced this pull request Jun 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment