architecture icon indicating copy to clipboard operation
architecture copied to clipboard

Simplified module declaration (More details and clarification)

Open antonkril opened this issue 6 years ago • 8 comments

Simplified Module Declaration

Problem

To create a Magento module you need to create 3 files:

  • composer.json - to declare composer package name ('magento/module-catalog'), description, version, dependencies, etc
  • module.xml - to declare Magento module name ('Magento_Catalog') and configuration loading dependencies
  • registration.php - to let Magento know where the module resides, so Magento can load its configuration files

This implementation has a following issues:

  • more things to learn and remember (module.xml, registration.php, and how are they different from registration.php)
  • more boilerplate code
  • module sorting mechanism is slow
  • need to maintain infrastructure to read/parse/process module.xml
  • need to know about ComponentRegistrar is and different types of components (ComponentRegistrar::MODULE, ComponentRegistrar::THEME, ...), even though you already defined the type of your module in composer.json
  • registration.php of every module/theme/translation is included by composer autoloader on every request.

Solution

  • Eliminate module.xml.
    • move "Magento name" (Magento_Catalog) to "extra" section of composer.json. Make it optional with default composer_name => magento_name mapping mechanism.
    • move "sequence" section to "extra" section of composer.json
  • Eliminate registration.php
    • create and register a composer hook that after composer install & composer update will:
      • find all installed packages with "type": "magento2-\*" in composer.json
      • find all packages by pattern app/code/*/*, app/design/*/*/*, and app/i18n/*/* with "type": "magento2-*" in their composer.json files
      • sort all packages using topological sorting of dependencies
      • generate magento_components.php that will register the sorted list of components in ComponentRegistrar
      • remove magento built-in module sorting mechanism. Rely on component list being sorted in composer hook

NOTE: This proposal DOES NOT eliminate app/code. The changes described above should preserve the ability to load modules from app/code

Open Questions

As described by Vinai Kopp, performance of composer dependency resolution impacts developer experience.

registration.php file allows to quickly install a module without composer. Moving to Composer will remove this optimization.

The solution to this problem should be provided before registration.php can be removed. Possible options:

  • make registration.php optional instead of getting rid of it
  • add a command to Composer that will to re-read composer.json files of Magento modules without full dependency resolution

POC

POC that partially implements the proposal is available in https://github.com/magento-architects/magento2/tree/split-framework (see \Magento\Framework\Component\ComponentInstaller::install)

antonkril avatar May 16 '19 22:05 antonkril

CLA assistant check
All committers have signed the CLA.

magento-cicd2 avatar May 16 '19 22:05 magento-cicd2

First of all, thank you @antonkril!

composer or bin/magento command

I'm sure you considered keeping the logic to create the magento_components.php file outside of composer, so it can be triggered by both a composer command as well as a bin/magento command? To me bin/magento would be a more natural place for a custom command in the Magento context rather than creating a custom composer command.

developer tooling

Another issue that isn't considered by the proposal is developer tooling.
Currently we have auto-completion for declaring module load sequence nodes in the module.xml file, thanks to XSD and also the PHPStorm plugin.
In the registration.php file we have auto-completion simply because it's PHP.

When moving the sequence information and the mapping of the Magento name into the composer.json file, that tooling support is lost. That forces developers to either memorize the syntax or resort to copy&paste.
It would be great to include a paragraph in the proposal that developer toolig has to be created so the experience is at least on par with what currently exists, so developers don't have to learn more rather than less.

duplication of knowledge in files

There is one more thing to consider. The proposal states that every module has to have three files, a composer.json, a module.xml, and a registration.php file. The duplication is between the composer.json file and the other two files. However, modules in app/code often do not have a composer.json file (in practice this works well, unless modules have external, non-magento package dependencies, which is pretty rare.).
This means the duplication isn't a real issue for most project code.

regarding issues with current implementation

A few comments regardign the issues with the current implementation listed in the proposal:

  • more things to learn and remember (module.xml, registration.php, and how are they different from registration.php)

Thanks to IDE support there is very little to remember and learn.
The same amount of learning would be required when adding that information to the composer.json file - it's just a different place for the same information.

more boilerplate code

For project modules there are mostly two files instead of one, which isn't a big difference.

module sorting mechanism is slow

This is an implementation detail that can be solved without changing anything in the module structure

need to maintain infrastructure to read/parse/process module.xml

This would be replaced by infrastructure to parse and process the extra information in the composer.json file. The infrastructure is being moved around, not removed.

need to know about ComponentRegistrar is and different types of components (ComponentRegistrar::MODULE, ComponentRegistrar::THEME, ...), even though you already defined the type of your module in composer.json

The duplication of type information issue is assuming all modules have a composer.json file. This is only an issue for extensions installed via composer. Since the type of a package is highly unlikely to change however this isn't a practical problem. I don't think a module would ever be changed into a theme or language pack.

Developers still have to learn about different types of magento2-* components, regardless of where the type is specified. The ComponentRegistrar is pretty much the most simple class in Magento 2. It is very useful to know about for development and also for debugging, not only for module registration. I think the need to have an understanding of the ComponentRegistrar would not go away through this proposal.

registration.php of every module/theme/translation is included by composer autoloader on every request.

That is indeed a real issue. Unfortunately it's tricky to change in a backward compatible way. Having a single magento_components.php file is a good idea. The magento_components.php then could register all modules with the ComponentRegistrar.

The problem is that moving all information into composer.json is a backward incompatible change that breaks the current tooling.

suggestion to extend proposal

Regarding making registration.php optional ("open questions" in proposal):

  • Keep composer.json optional for modules in app/code to not place an additional burdon on developers working on project code.

  • Make registration.php optional for modules installed by composer to avoid the above overhead issue.

  • Keep the module.xml file, do not move that information into composer.json to keep backward compatibility and current tooling intact.

  • Implement code that builds the magento_components.php file. Two cases need to be handled:

    • Modules installed via composer: Add a install hook that adds the package to the magento_components.php file (using the Magento Name to directory mapping as described by proposal) unless the package declares a registration.php file.
    • Modules in app/code:
      • Create a bin/magento command to scan app/code for modules, themes and language packs with a registration.php files and merge them into the magento_components.php file.
      • Change NonComposerComponentRegistration.php to only check directories that are not yet registered with the ComponentRegistrar.

For modules in app/code, this is simply a change of an implementation detail, when the registration.php is evaluated. The registration.php would no longer be included on every request.

For composer installed packages, the registration.php still would be included on every request as long as it's defined as part of the composer.json autoload section. As soon as a composer installed module removes the registration.php file, it will be listed in magento_components.php. I don't think it's possible to stop the inclusion of the registration.php file by the composer autoloader without some serious composer hackery. The good thing is this would keep backward compatibilty intact.

Benefits:

  • Keep current tooling intact.
  • Remove loading of registration.php on every request.
  • Good developer experience for both extension vendor developers as well as merchant developers.
    • Merchant developers do not need to think more about composer.
    • No need to add a registration.php for extension developers.

I'm sure you have thought through the possible solutions more than I have and it's very possible I'm missing some obvious problem. If so, please explain. I'm sure there is a good solution that makes everyone happy.

Vinai avatar May 17 '19 07:05 Vinai

I would welcome the removal of registration.php. I remember it wasn't there when M2 was first released. At the the time it felt like it was added haphazardly to fix some problem (I don't know which).

Any change to composer.json and module.xml would make make Magento more complicated, because there would be shops to maintain before and after this change. This will not make my life as a "merchant developer" easier.

patrick-bigbridge avatar May 17 '19 13:05 patrick-bigbridge

@patrick-bigbridge The registration.php was added before the Magento 2 developer beta. It was added to allow modules to be installed in vendor/ via composer packages without the symlinking or copying by the old magento-composer-installer into app/code.

Vinai avatar May 17 '19 16:05 Vinai

If we are expanding / changing the way module registration works I think we should also look at this implementation here. It is used by the Web Setup Wizard and essentially hard codes the location of the composer.json to be in the same path as registration.php.

If per Vinai's proposal composer.json explicitly becomes optional I would suggest the following enhancement to the above implementation.

The package information should be read from the actual file that composer reads - which would be different for modules with a folder structure common to other php projects:

composer.json
src/registration.php
tests/

Maybe where no composer.json exists at any level we can display some info to indicate this module is store specific.

fooman avatar May 20 '19 07:05 fooman

Good idea @fooman! Moving a bit closer to allowing modules to follow a Maven standard directory layout would be nice. This is something I've heard many developers who are new to Magento wonder about.

Vinai avatar May 20 '19 08:05 Vinai

Would this be feasible? It's kind of a hybrid of Anton and Vinai's proposals.

  • Stop using registration.php.
  • Use composer.json for module discovery, replacing registration.php. Scan for these files regularly in default/developer mode but store as part of compilation in generated magento_components.php in production mode.
  • Keep module.xml for sequence declaration (as a supplement to Composer dependencies) and module name declaration, but make optional. (If not present module name will be inferred from composer.json convention.)
  • Provide a migration mechanism (CLI command?) to generate composer.json based on registration.php for existing modules to ease upgrade for existing stores.

Reasoning:

  • The registration.php file adds little of benefit right now and is just pure boilerplate. Good to remove if we can.
  • Having to maintain a composer.json file under app/code is a change for merchant developers, but it is an industry standard tool that lowers the barrier to entry for PHP developers familiar with other leading projects like Laravel & Symfony. It cuts down on Magento-specific obscurities.
  • We retain module.xml for what is actually Magento-specific, keeping the benefit of XSD validation rather than adding to freeform extra section of composer.json.

The result would be that a module can be added in developer mode with just a single file (albeit composer.json instead of registration.php), keeping dev effort low, but using an industry standard that will be intuitive to non-Magento developers. This is accomplished in conjunction with improving production performance.

scottsb avatar May 29 '19 23:05 scottsb

@antonkril Great update, thanks for capturing our discussion!

I don't know if you want to include it, but we also talked about how the module name is determined if the module.xml and registration.php are not present. What we ended up with where the rules:

  • if only a composer.json is present, determine the module name from the package name, e.g. magento/module-hello-world becomes Magento_HelloWorld.
  • if an etc/module.xml is present, take the name from there

This will keeps backward compatibility intact and allows for flexibility in case a company doesn't want to follow the Magento standard for package naming.

Regarding the creation and updating of vendor/magento_components.php, in addition to the composer hook, we discussed this also to be hooked into bin/magento module:enable and disable and setup:upgrade. This would makes things a little more convenient and I don't see a reason to keep the step separate.

Maybe you can also add a paragraph why there still is a need for module.xml besides backward compatibility. The scenario we discussed was:

  • Two independent extensions are installed.
  • They declare conflicting configuration (e.g. a preference or type argument in di.xml).
  • The configuration load order needs to be specified by a project developer to resolve the conflict.
  • Declaring a dependency in of one of the extensions composer.json files to enforce load order isn't a good approach because the file is "owned" by the extension vendor, and changes will be lost during upgrades.
  • The project developer can enforce load order for modules that don't depend on each other by specifying a sequence in a module.xml file.

We talked about adding that comfiguration with a new extra composer.json section, however, because the functionality already exists for module.xml, and it will be a rare use case, and it keeps backward compatibility intact, we agreed that module.xml would a better place for that configuration.

Vinai avatar Aug 13 '19 10:08 Vinai