Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplified module declaration #63

Merged
merged 3 commits into from May 15, 2019

Conversation

@antonkril
Copy link
Contributor

commented Jan 10, 2019

The document describes changes to module registration and management implemented in scope of "Lighter application" POC for Service Isolation project.

Clarification: the proposal does not discuss the removal of app/code folder or ability to keep project-specific code in the project folder.

Simplified module declaration
The document describes changes to module registration and management implemented in scope of "Lighter application" POC for Service Isolation project.
* Register composer installation hook that will perform magento-specific module management.
* Eliminate `registration.php` files.
* Composer installation hook will generate a 'magento_components.php' file that will contain all magento components and their installation paths. Only this file will be included on every request.
* Move module sorting behavior to composer installation hook. Use third-party topological sorting library. Write sorted modules to `magento_components.php`. This will move sorting behavior from run time to build time.

This comment has been minimized.

Copy link
@paliarush

paliarush Jan 10, 2019

Contributor

Just a note, currently modules are not sorted in run time either, the sequence is calculated and written to config.php during deployment (and during modules installation/upgrade)

This comment has been minimized.

Copy link
@antonkril

antonkril Jan 10, 2019

Author Contributor

Good point. Edited the comment to emphasize that the expensive operation will be moved from deploy stage to build stage. Btw, using config.php instead of magento_components.php is worth exploring.

antonkril added some commits Jan 10, 2019

@Vinai

This comment has been minimized.

Copy link

commented Jan 10, 2019

Even though the registration.php file could be avoided by the approach described in this PR, developer experience would suffer because composer is very slow with a large project such as Magento.
Creating a registration.php file manually is a lot quicker, and since it's mostly done by code generation in the IDE or other tools, it's not a real burden at all.
On the other hand, installing a package with composer even with a path repository allows for at least a coffee break. It would be really very annoying to be forced to do that for every new module during development.
Please continue to support non-composer modules indefinitely.

@davidverholen

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

@Vinai I partly agree. I think a similar approach that is currently in place for registering non composer components could still be used, but instead read all information from composer.json, which also can mostly be generated by ide / tooling.

To avoid having to composer update every time you add a new module, it would have to figure out the module sequence in each request so I guess this really should only work in developer mode.

@Vinai

This comment has been minimized.

Copy link

commented Jan 11, 2019

@davidverholen

but instead read all information from composer.json

That would be fine for me, but it would probably be slower to build the DAG data compared to the current approach with registration.php.

To avoid having to composer update every time you add a new module, it would have to figure out the module sequence in each request

I don't see a problem of having the module load sequence pre-generated for non-composer modules like it is now. No reason to do that work on every request. Especially if a slower method than registration.php would be used.

this really should only work in developer mode.

Maybe could would be a better word. It could also work in production mode, even if it wouldn't be used due to pre-generation, without harm (like class generation or deploy static content on the fly if the flag is set).

All I'm trying to say is a fast and light developer experience is crucial to me and many others, and the introduction of artificial constraints is more annoying than helpful in many cases (like making things only work in developer mode when they could also be used in production as a fallback).

@davidverholen

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

I don't see a problem of having the module load sequence pre-generated for non-composer modules like it is now. No reason to do that work on every request

Right, I was thinking not wanting to composer update every time. But the process could of course still be triggered by commands like setup:upgrade and by composer

Maybe could would be a better word

if the sequence loading mechanism does not differ I also see no reason why it should not work in production mode

@Vinai so the remaining question then is, should this approach still support loading modules from app/code, right? I really like this folder but from an architectural point of view I totally understand why it should maybe not be supported. (Or maybe this one only in developer mode? ;) )

@Vinai

This comment has been minimized.

Copy link

commented Jan 11, 2019

the remaining question then is, should this approach still support loading modules from app/code, right?

As long as non-composer modules can be used I'm basically fine. It would be preferable to keep them outside of vendor/, as that is ephemeral. It could be app/code/, src/, non-composer-modules/, that doesn't really matter to me.

That said, from a backward compatibility perspective it would make a lot of sense to keep app/code/ instead of removing it and introducing some new folder.

@jissereitsma

This comment has been minimized.

Copy link

commented Jan 11, 2019

What about the composer hook generating the registration.php file as it is now? This way, modules can optionally ship witht the registration file, while nothing needs to change for existing modules.

@speedupmate

This comment has been minimized.

Copy link

commented Jan 11, 2019

As long as non-composer modules can be used I'm basically fine.

Why we even have a term "non-composer module" ? If composer.json is all you need to describe your module to Magento2 then we would not need to discuss this . For a developer to define a magento2-module generating one composer.json should be enough. Where do you place it inside a system is a developer or devops type preference

That said, from a backward compatibility perspective it would make a lot of sense to keep app/code/ instead of removing it and introducing some new folder.

From devops perspective having something nested inside "core" code has always been a struggle with Magento (1 and 2). Especially in terms of defining validation or separate code and asset deployments.

First level folder like /vendor or /extensions yields more sane approach on managing code. You can always symlink the path or paths to app/code for backwards compatibility or describe this as option for getting backwards compatibility.

I've been a supporter and heavy user of "Maven Standard Directory Layout" myself and currently Magento2 does not even allow this in composer packages (at least those you need to serve trough marketplace), that is pretty much standard that world seems to use. To get to this point (Maven like structure src/ tests/ etc) on your extension projects you either need two composer.json files with different psr-4 paths or extra work on composer.json generation , placement and psr-4 mapping on package generation time. workarounds do not speed up the development process nor make it easier to start with.

And as @jissereitsma suggested to generate those meta units from composer.json to keep the similar speed (the need that @Vinai referred) up the system makes sense. At first glance you can generate the registration.php type of content out from psr-4 node and to some tmp folder separately and outside from extensions. One composer.json file to contain the information should be enough and would clear up lot of unknown behind needed boilerplate for developers be it extension generation or packaging (with differences in IRL composer usage and marketplace needs) or asset management for devops.

@andrewhowdencom

This comment has been minimized.

Copy link

commented Jan 11, 2019

More generally, this seems like a reduction in the absolute complexity possible for module declaration, and I am thus in favour of it.

However, @Vinai's points are valid -- if this will negatively impact on developer experience implementers will reject this pattern in the same way compiling the frontend is currently a haphazard, time consuming endeavour.

I would probably not push for generation, even to retain BC. We're not reducing complexity then, just shifting and automating it.

My mental model this problem is something like "service discovery"; implementing a particular pattern of modules such that they can be discovered and pulled in to the application. Magento is tricker than straight forward service discovery, as it must do dependency resolution on those modules. This is an inherently inefficient process, which is why we're talking about doing it at "build time" and not at "run time" -- similar to how composer generates that autoload.php file that stores all the paths for statically included autoloaded files; including registration.php. Composer thus seems like a sensible point to do this reconciliation.

To further this proposal I would like to poke at why composer takes so long with new module resolution. Is it inherently expensive to calculate the DAG? Can we improve upstream composer itself? Is there a process to declare a module that means we don't have to fully rebuild the DAG?

/2c

@Vinai

This comment has been minimized.

Copy link

commented Jan 11, 2019

👍 for the "Maven Standard Directory Layout", however that isn't an option as "the Magento way" any more I believe. It could have been adopted early during the Magento 2 lifecycle, but now standards have been created implicitly and explicitly.
However, it would be possible to relax the current standards, so a Maven layout would be valid, in addition to the current standard. Relaxing requirements is a backward compatible process, so no problem should be expected.

@melnikovi

This comment has been minimized.

Copy link
Member

commented May 15, 2019

I worked on deploying admin UI separately and used composer to install only necessary modules. It's not as fast as adding module manually, but it's not bad. Considering that during development most time being spent on implementing functionality and not installing or changing load order, I think it's ok. We are going to monitor this issue and change the approach if needed. Based on the number of people that liked the proposal, I'm going to accept it. @Vinai please reach out to me and @antonkril if you think that it's really big problem for developer experience and we will discuss it further.

@melnikovi melnikovi merged commit 94145b0 into master May 15, 2019

1 check was pending

licence/cla Contributor License Agreement is not signed yet.
Details
@Vinai

This comment has been minimized.

Copy link

commented May 15, 2019

@melnikovi The number of likes, seriously? 6 up, 1 down, and 3 likes for my comment on why I think it's not a good idea.
Numbers like those are next to nothing and should not be an indicator why a PR is merged or not.
Also, most people add a thumbs-up reaction without even thinking about consequences.

I still think it is a mistake to bind the Magento platform that much to composer and semver.
And regarding developer experience, even on the brand new MBP I'm workin on now, waiting for composer still is longer than I like (plus, most developers don't get the option to use 5000€ MBPs).

But I also realize that I'm on a pretty lonely position regarding composer and semver. It does feel a bit like Magento shooting itself in the foot without realizing it though (by forcing more complexity on developers).

@melnikovi

This comment has been minimized.

Copy link
Member

commented May 15, 2019

I should have phrased it differently probably. Likes are not deciding factor of why we except proposals. I did some experimentation and don't think that this will be an issue. Could you describe use cases when it will be a problem? Can you join meeting tomorrow to discuss your concerns? I can demo installing modules from composer.

@hostep

This comment has been minimized.

Copy link

commented May 15, 2019

I have a remark regarding removing the module.xml file.

Right now you can define modules as "dependencies" in the sequence section of the module.xml file. But those modules don't necessarily have to be installed. Magento won't complain about missing modules. This sequence section is more to determine the sort order of modules. Sometimes you would want a specific module to be loaded later then another specific module, but that last module doesn't necessarily have to be installed as it's not a hard dependency of the first module. If it is installed then fine, but if it isn't, then also fine and in each case the sort order is calculated correctly.

Whether this is smart way of writing modules is another question, but currently you can do such a thing in Magento.

If you start using composer.json for sort order calculations, I think there is no equivalent to say "put me later in the sort order of that particular module, whether it is installed or not". There is no way to define optional dependencies in composer.json I think, unless you want to use the suggest field for that?

I've seen some strange cases in the past where the only way to fix something, was to put a module in the sequence of another module, but it wouldn't always make sense to include that module as a hard depenceny in the composer.json file, since you aren't relying on anything from that module. It's only to change the sort order.
I can't really come up with a concrete example at the moment unfortunately.

Not sure if this is a valid remark though, just had to think about this while reading through the suggested changes.

@antonkril

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@hostep this is a valid remark. Configuration loading dependencies are useful if you have "soft" dependencies. While I think that "soft" dependencies are a bad practice, we still have them, so module.xml sequence feature should be moved to composer.json. Will add that.

@Vinai I agree that registration.php should not be killed before we have a good solution to the performance problem you described.

I would like to hear more about why you think relying on composer for module management is a bad thing. My understanding is that if we solve the performance problem, it will reduce complexity:

  • less framework code to maintain
  • less things for a new Magento developer to learn
  • less boilerplate code
@Vinai

This comment has been minimized.

Copy link

commented May 16, 2019

I'm writing a blog post to put all the things that are wrong in my eyes with composer and semver into writing. Then I can at least point people to that.
Unfortunately the architecture meetings are hard to make for me @melnikovi.

@Vinai

This comment has been minimized.

Copy link

commented May 16, 2019

Here is a write up where I explain the reasons:
https://gist.github.com/Vinai/a94f2500cc5694a258620bbd30692b87

The TL;DR:
In summary, it's valuable to be able to use non-composer packages in Magento.

@antonkril argues:

My understanding is that if we solve the performance problem, it will reduce complexity:

  • less framework code to maintain
  • less things for a new Magento developer to learn
  • less boilerplate code

I disagree. It is easier to use module.xml and registration.php than to apply Magento's module dependency rules and semver.
For an extension vendor and some SIs it may be worth the cost, but for merchant developer teams it most certainly isn't.

Taking away non-composer modules will increase development cost for merchants and make it harder to hire Magento developers.

For a more in-depth explanation please read the post above.

@antonkril

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

Copying response here:
Thanks for the detailed post, Vinai. It describes real problems we rarely hear about.

I don't think the proposal you referenced argues with any point you brought up as it does not:

ban app/code. I agree that project code should be developed in project folder, not separate composer packages.
force you to specify SemVer dependencies in custom code. I'm not a big fan of SemVer.

The proposal is about minor file duplication removal. Banning app/code would definitely require a more detailed and motivated proposal. I think the part that causes confusion is the point about relying on Composer for dependency management. It needs clarification.

You can avoid most of the dependency resolution problems you described by specifying only major versions or stars in dependencies if you don't need the notification feature of Composer. But I agree that SemVer is not a good notification mechanism.

@fooman

This comment has been minimized.

Copy link

commented May 16, 2019

@hostep the times I have come across the issue where specifying extra sequence nodes fixed an issue it was always due to some extension not including enough sequence information in the first place (mostly for db instructions like it was in the past for some EE modules). Due to the non-deterministic sort order it took a while for these issues to surface.

The benefit which sequence in module.xml could provide which I believe at this stage could not be covered by reading the requires of the composer.json is for sort order purposes would be for distributed extensions and conditional sorting (only if other extension is present sort yourself after this as Composer does not do conditional requires).

I currently have no example of this in my extensions so it would be good to get some further input on how widespread this is. My gut feeling here is that if such a soft dependency fixes something there is either an issue in one of the extensions or both extensions genuinely clash in what they are trying to achieve which needs to be solved separately.

@Vinai

This comment has been minimized.

Copy link

commented May 16, 2019

@antonkril I just re-read the design proposa, and it indeed reads to me as if once implemented, non-composer modules would no longer be possible.

@hostep @fooman In my experience there are two situations when Module load order problems happen:

  1. XML merging (di.xml, layout XML, ...)
    Modules loaded later can change values specified by modules loaded earlier. If two modules specify the same node value, the modules are in conflict. The "winner" can be selected by adding a sequence node so it is loaded after the "looser".

  2. DB Setup
    Setup scripts and patches can only refer to DB objects that where created by setup scripts run earlier. With the new schema and data patches this can (and should) be solved by declaring dependencies in the appropriate PHP method of the patch class, but with setup scripts this had to be solved by specifying the module load order with sequence nodes.

There might be more situations in which the module load order can cause conflicts, but at least for me these where the common cases.

@ProcessEight

This comment has been minimized.

Copy link

commented May 16, 2019

Basically, I agree with @Vinai that removing app/code folder would be a backwards step.

I also am a 'merchant developer', who works on one install of Magento and therefore does not feel the benefits of Composer. To me it's just another layer of boilerplate I need to add, but will only lightly benefit from (at least not as much as a module vendor).

One thing that annoys me about web development is how many tools I need to learn competently before I can write a line of code. I want to program! I want to solve problems! I don't want to mess around with running multiple different tools before I can write a single line of meaningful code. The JavaScript community seems to be going through a phase where multiple tools need to be installed before you even start writing - package managers, transpilers, etc. The community even makes fun of itself because of this proliferation of tools. My point being that, rather than composer being an aid to reduce technical complexity, I think conversely it increases it. It creates a barrier to entry. It will make my job harder - and it's hard enough as it is.

Allowing non-composer modules in app/code has the following advantages:

  • Easy to create new modules with just two files
  • Easy to hack together modules to test things
  • Easy to create tools which can programmatically create modules
  • Only one command needed to enable a module

One of the key selling points of Magento is it's flexibility - if you remove app/code, then you going against that philosophy by making it less flexible.

I don't really care if composer is used to track module dependencies or if that is handled by Magento, I just don't want to be forced to do it when it's not helpful to my workflow.

Final point: I don't think it's a good idea to so closely align Magento with a single tool either; What if this time next year everyone has switched to a shiny new PHP dependency-management tool? The history of web development/software engineering is littered with tools which were de facto standards, then were replaced by something else which became a new de facto standard. When I started programming, SVN was the most popular VCS. Now it's git.

@ihor-sviziev

This comment has been minimized.

Copy link

commented May 16, 2019

+1 for @Vinai
Just try to migrate magento/magento2 fully to composer and just you’ll see that it’s not so easy to work with it. Try to imagine that each PR merge will require updating composer version, review dependencies, etc.
simplicity - I think this is the main reason why currently all magento modules located in one repo, without composer, it’s just not needed there

@antonkril

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

This PR was merged so had to create new: #162

It clarifies that this proposal DOES NOT suggest removal of app\code, and describes how the ability to load modules from app\code can be preserved.

@orlangur orlangur referenced this pull request May 24, 2019

Closed

Improve ComponentRegistrar with new register methods #23008

2 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.