Split Mage_All.xml into own module XML files

[tmewes]

Description (*)

Mage_All.xml contains 48 individual core modules. Until I split it for this PR, I wasn't even aware of how many modules were actually in there. The name of this module XML suggests that it really does contain all Mage modules, which unfortunately does not correspond to reality. Aside from this inconsistency, the module names are also unsorted in this file.

In the company I'm working for, not all Mage modules are used by far. Currently the way of simply deleting all module files of the unused modules and adding them to the .gitignore works great. Unfortunately, this way causes a lot of local core hacks (and a precise cognitive focus on every OpenMage update) in Mage_All.xml by commenting out the unused modules, which can be avoided if the Mage_All.xml is divided into its components. By merging this PR each module would have its own module XML, as it's already the case with all Mage modules outside of Mage_All.xml.

Manual testing scenarios (*)

No special manual test scenarios should be necessary.

Questions or comments

The adjustments have already been successfully tested on my local machine as well as in the test environment of my team. :)

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[tobihille]

Great work, thank you.
Thought about doing it myself but I was too lazy tbh 😅

[tmotyl]

Have you checked if modules are loaded in the same order as before this change? Beware that not all dependencies are being declared in the xml.

[fballiano]

Loading order can be very tricky and has to be checked, anyway the best way to disable a module in the Mage_All.xml is in your local.xml, adding something like

    <modules>
        <Mage_Install>
            <active>false</active>
        </Mage_Install>
    </modules>

in your <config> node

[Sdfendor]

@tmotyl Would it be a good idea to find out this order (with all file still present) and then making this order explicit within the xml depends directives in the single modules after separating? So the order would be clear and no historic guess work because the all file is present for so long.

[kiatng]

@fballiano method is how I always disable unused modules. Another way is disable it in the backend system configuration. These methods work for me. IMHO, commenting out the modules in Mage_All.xml is unnecessary, I do not see the need for this PR.

[addison74]

Indeed, the order in which these XML files are taken is important in operation. If we separate them into individual files, then we have to analyze how they are joined in one by OM. In addition, what is the ultimate goal? For the ease of finding a certain module and disabling it, to count how many modules the OM has? If it is for disabling, there are two methods, the first one (hard) which assumes what @fballiano showed, which completely disables the module and second one (soft) in the Backend which @kiatng showed, which only disables the output, but the module is loaded by OM. The hard/soft disable option depends on the environment.

[Sdfendor]

If kept than this "The name of this module XML suggests that it really does contain all Mage modules, which unfortunately does not correspond to reality" should be at least considered and all of the core modules should be in the all file not a quite large but arbitrary selection of them.

[addison74]

I agree that the name chosen for Mage_All is not appropriate at all (not mage all). They are not all because there are about 15 more in that directory. are these orphans or renegades from the All category?

If we look in the file, the modules are not listed in alphabetical order. Some of them depend only on Mage_Core, others on one or more modules. Maybe not splitting the file into individual files would be beneficial, but grouping them by dependencies. This is for the sake of arranging something by code.

[tmewes]

First of all: Thank you very much for your commitment and your numerous feedback! I really appreciate that. :) And I'm very happy to go into all the points.

I think the best thing start with is the topic of deactivating modules: In the past, we also went the described way of 'hard' deactivating modules in the local.xml, because the soft variant (as correctly described) only deactivates the module output, but the module is loaded. But then we changed to a more 'radical' path, which brings us in the direction of the 'ultimate goal' of PR. :)

As mentioned in the description of the PR, in my opinion the way to completely remove unused modules from the project is the best (for the projects I'm working on - no offense to all other ways you solve this!) and works great so far. It streamlines the project, which e.g. shortens the PHPStorm index times and automatically significantly speeds up the results of the global PHPStorm search and improves the quality of the results (since results from unused modules cannot appear at all). The XML module is simply part of a module and removing it at the same time immediately ensures for all developers of the project that nothing of this module is loaded and used (since it is not even present in the project). In addition, unused files should not simply lie around in production (just like actually on staging and dev instances). This also brings us directly to the idea of improving order, clarity and unification, which is another goal of this PR.

This PR would get rid of the questions "Why is there a Mage_All.xml in which some but not all Mage modules are integrated?" and "Why are they unsorted in there?", which will definitely will come up again and I've already asked this to myself several times. It would provide a clear structure for the core modules in OpenMage: A module always has its own module XML.

You are absolutely right about the loading order, thank you very much for your hints! No changes were noticed during testing after the split, but I don't want any shop to have any problems as a result. The loading sequence must therefore be ensured in any case. That's why I took a closer look at where the loading order of the (Mage) modules came from and found what I was looking for.
app/code/core/Mage/Core/Model/Config.php:915 is a good line for a breakpoint to get the final result of the loading order.

So I checked out the current main branch (1.9.4.x), took a screenshot of the list, checked out the branch of this PR and took a screenshot of the list again to compare the results:

As you can see, the order is different as you feared. What I found out right away is that the order in the Mage_All.xml is not only unsorted alphabetically, but the order does not even correspond to the loading order. If so, Mage_Customer should be loaded after Mage_Directory. However, Mage_Dataflow is loaded next instead.

The entire loading logic of the module XML files takes place in the above mentioned file with the breakpoint. A distinction was made between 'base' (= Mage_All.xml), 'mage' (= all other Mage modules) and 'custom' (= all other modules such as Cm_RedisSession). These are gathered one after the other and then merged using array_merge(). I initially stuck to this structure because that way I would only have had to concentrate on the 'base' part. But after more careful consideration, I changed my mind because I don't see any added value in this division into 'base' and 'mage', since both are 'mage'. Only the order matters.

Coming back to the idea of overview/clarity: It is currently nowhere visible in which order the modules are loaded without either tracing all depends nodes in all Mage modules or explicitely debug it like I did here.

In order to make sure that the loading order remains the same and to make it comprehensible, I recorded the modules in an array in the order in which they are loaded in the current main branch and was thus able to simplify the evaluating code even more. Please take a look at the code update. And here is the final comparison:

This implementation has the advantage that nothing changes in the loading order, we have now recorded the explicit loading order and can quickly understand it at any time if necessary. I have tried to get the best out of it for everyone and take away all fears, regardless of whether a direct benefit is seen for your projects or not.

I look forward to your feedback. I really enjoyed doing more research here. I got to know corners of the core here that I had never been to before. :) Thank you for your commitment and your constructive feedback in this project. You guys rock!

[fballiano]

I have to say that I don't really love the hardcoded list of modules, wouldn't it be possible to have a "loading order" parameter in the module definition xml? probably it would be slower so I don't know...

[addison74]

I think that this PR is useful because it makes order in the Magento modules and especially because this order can be modified in one file. Now the module loading mechanism is clearer, I knew about its existence but I never asked myself how the magic is done behind it. Obviously, if in the future another solution is found for their loading position, we will analyze it then. If there are no other objections in the next period, I will approve the modification.

[tmewes]

@fballiano May I ask what your force push is for? I have never experienced that with any other PR here. :)

[fballiano]

@fballiano May I ask what your force push is for? I have never experienced that with any other PR here. :)

It is just to update the branch with the current version of 1.9.4.x since some PRs got merged after you created yours 😉 there’s a github web interface button just for that

[kiatng]

I have to say that I don't really love the hardcoded list of modules, wouldn't it be possible to have a "loading order" parameter in the module definition xml? probably it would be slower so I don't know...

That was exactly my initial thought. I didn't like it hard-coded; we loose flexibility and it makes maintenance harder. When Magento was in the development phase, the xml was a neat way to manage the modules.

But now, the core modules are fixed; will there be many new Mage modules to be added? Probably not. So why not hard-code them, as @addison74 observes, the loading sequence is no longer opaque, which is a bonus.

[kiatng]

With the loading sequence shown to be the same before the split, would be nice if someone can verify that OM can install without error, I can think of foreign key constraints from MySQL.

[tmewes]

That was exactly my initial thought. I didn't like it hard-coded; we loose flexibility and it makes maintenance harder. When Magento was in the development phase, the xml was a neat way to manage the modules.

But now, the core modules are fixed; will there be many new Mage modules to be added? Probably not. So why not hard-code them, as @addison74 observes, the loading sequence is no longer opaque, which is a bonus.

Yes, that's exactly how I see it. I also originally also come from the direction of "don't hard-code anything that can be avoided somehow", but what would we do it for in this case? We would only have generic code for generic code's sake at the expense of clarity. There will most likely not be any Mage modules added at all, and if they do (or if one is deleted) we can update the list easily. I'll keep an eye on this.

With the loading sequence shown to be the same before the split, would be nice if someone can verify that OM can install without error, I can think of foreign key constraints from MySQL.

Sure, no problem:

Frontend and Adminhtml are running and there were no errors during the installation process on my machine.

[kiatng]

@fballiano:

Maybe the only thing I'd evaluate is to keep the Mage_All.xml file but empty, let me explain, for people that just unzip the release on top of their projects, the previous Mage_All.xml file will still be there and maybe cause problems?

I do not think it will cause any issue, but perhaps @tmewes can do a quick test in his setup, just add the original Mage_All.xml along with all the splits. I predict that the loading sequence remains the same.

[fballiano]

@fballiano:

Maybe the only thing I'd evaluate is to keep the Mage_All.xml file but empty, let me explain, for people that just unzip the release on top of their projects, the previous Mage_All.xml file will still be there and maybe cause problems?

I do not think it will cause any issue, but perhaps @tmewes can do a quick test in his setup, just add the original Mage_All.xml along with all the splits. I predict that the loading sequence remains the same.

The problem I see is when somebody modified the Mage_All.xml, so the behaviour becomes a bit unpredictable, while having an empty Mage_All.xml we'd make sure that any old configuration wouldn't be loaded.

I also don't like the idea of having an empty file there but... I don't know...

[kiatng]

The problem I see is when somebody modified the Mage_All.xml, so the behaviour becomes a bit unpredictable, while having an empty Mage_All.xml we'd make sure that any old configuration wouldn't be loaded.

Well, do-not-modify-core-files is the thing since the beginning. Leaving an empty Mage_All.xml would replace any modification there. IMHO, empty Mage_All.xml is unnecessary.

[fballiano]

Well, do-not-modify-core-files is the thing since the beginning. Leaving an empty Mage_All.xml would replace any modification there. IMHO, empty Mage_All.xml is unnecessary.

but this is a configuration file not really the core, the original author literally said this PR is because he wants to delete some of the files from his projects :-)

anyway, it's fine to me without the empty mage_all but somebody will have glitch

[tmewes]

I do not think it will cause any issue, but perhaps @tmewes can do a quick test in his setup, just add the original Mage_All.xml along with all the splits. I predict that the loading sequence remains the same.

Of course, no problem. :) I can confirm that the loading sequence remains the same with the original Mage_All.xml along with all the splits.

@fballiano is right if someone modified this core file. But I agree with @kiatng's opinion:

do-not-modify-core-files is the thing since the beginning

So either we remove Mage_All.xml directly or we leave it e.g. for the next one or two releases empty with a comment like this and remove it afterwards, so everyone who is really using OpenMage and modified this file manually after each update will get informed and we are safer than safe.:

<!-- This file can be safely removed. It's just here to prevent problems when updating OpenMage by unzipping the latest release. All contents of Mage_All.xml are already split into own module XML files since OpenMage v19.4.18. -->

I would agree with the variant desired by all of you here, although I have already made my own point of view clear here by directly deleting it.

I am very pleased that this PR is now nearing completion. It should only be the first clean-up action and the basis for further major improvements regarding this topic - then of course in further discussions. Thank you for your numerous feedback!

[addison74]

It is preferable to leave Mage_All.xml for a while but with other content than to delete it. Whoever is curious can read the information message in the file. On this message I would work a little more on its final content, I would even inform the curious that in future versions the file will be deleted.

There are still open ideas to do in other PRs:

• the grouping of mandatory modules on which the running of OpenMage depends in a file called Mage_Require.xml (I like the name proposed by @kiatng).

• if there is a better option to control the order in which the modules are loaded. who knows, maybe an optimization is necessary and another option can be proposed.

• deleting the Mage_All.xml file from the repository.

[addison74]

In each XML file could we order the dependencies in ascending order?

Before

            <depends>
                <Mage_Customer />
                <Mage_Adminhtml />
                <Mage_Wishlist />
                <Mage_Sendfriend />
            </depends>

After

            <depends>
                <Mage_Adminhtml />
                <Mage_Customer />
                <Mage_Sendfriend />
                <Mage_Wishlist />
            </depends>

Of course in other PR but it is relevant to discuss here were a lot of opinions were expressed.

[addison74]

I would go further with the discussion because we have mandatory modules for running Magneto, but we also have modules that are not mandatory but on which others depend. for example. Above this module depends on the Wishlist module which can be disabled without creating running issues. Most likely we will have to group the modules on 3 levels: level 1 (mandatory, required), level 2 (modules from level 3 that depend on any of these must be mentioned in a comment, this way we prevent accidental disabling), level 3 (depend on level 1 and 2).

Of course in other PR but it is relevant to discuss here were a lot of opinions were expressed.

[tobihille]

Since the modules are now sorted by code the dependencies sorted by alphabet is really nice to see :-)

[tmewes]

In each XML file could we order the dependencies in ascending order?

Sure, done and tested. :) The loading order remains the same because it is set in \Mage_Core_Model_Config::MAGE_MODULES, which means that we can also sort alphabetically here directly.

I also added the empty Mage_All.xml with revised documentation. I hope that's detailed enough and you like it.
Since I don't want to add an invalid XML file, I added the XML head and an empty config node in addition to the comment.

This should really clarify everything in this PR as far as the actual PR is concerned, so that it can hopefully be merged and subsequent discussions can take place. :)

[addison74]

I appreciate that you ordered the dependencies of a module alphabetically.

I will analyze the contents of the Mage_All file, there are still changes to be made. I will present how I would see the message included in the file, in addition an warning that the file might be deleted in the future. I would not give more details in the content.

[fballiano]

To me this is fine, maybe we can improve something in a next PR, this to me can be merged now

[addison74]

In this case I approve the PR and for the other issues I will open discussions. Following the discussions, new PRs can be created.

[github-actions]

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 65011c9. ± Comparison against base commit 0ef045b.