Improvements to Maho's autoloaders #40

justinbeaty · 2024-10-04T16:56:39Z

It started with me trying to enable cache under System > Cache Management, and it not working. I tracked the problem down to this line, but the problem only occurs if you're running via git clone, as I am doing for development.

maho/app/code/core/Mage/Core/Model/Config.php

Line 350 in 737536e

$files = array_reverse($files);

The reason is that because of the array_reverse it loads app/etc/local.xml before app/etc/config.xml and that's setting <active>0</active> under the db connection node. I re-wrote those functions in the two Config.php files to not use array_reverse so files are loaded in alphabetical order like before, and I can enable cache now.

To make the code a bit cleaner, I defined two new globals, MAHO_IS_STARTER_KIT which is true if you required mahocommerce/maho with composer, and MAHO_ROOT_SOURCE_DIR which always points to the root of maho's code, either in vendor or where you cloned it.

I was also debugging something else, and on a hunch I tried changing the order of include_paths and the autoloaders. I'm pretty sure I solved OpenMage/magento-lts#3462. You can override Zend classes in any of app/code/core/, app/code/local/, app/code/community/, lib/, and any of those directories in 3rd party modules.

One thing that threw me for a loop though was the behavior of opcache. If you were to override Zend/Log/Formatter/Abstract.php, it wouldn't load because opcache has already cached the file with require_once 'Zend/Log/Formatter/Abstract.php'. There is an opcache setting to disable that, and I put that in for developer mode, but on a production system you may need to restart httpd or php-fpm.

Also I wanted to say how much I appreciate your idea of the whole autoloading system. It's amazing. Makes the whole project cleaner and more robust. I need to be offline for a bit, but take some time to review and ask any questions.

app/Mage.php

fballiano

sorry @justinbeaty, isn't there a simpler way to solve the problem you had cause this is gonna be very difficult to test

app/code/core/Mage/Core/functions.php

lib/Varien/Autoload.php

app/Mage.php

fballiano · 2024-10-04T19:37:47Z

Git install is used only by us and it is forbidden to store projects, we shouldnt even consider it as a possibility ans it’s never coveres in the doc and never will 😂 Il giorno ven 4 ott 2024 alle 20:35 Justin Beaty ***@***.***> ha scritto:

…

***@***.**** commented on this pull request. ------------------------------ In app/code/core/Mage/Core/functions.php <#40 (comment)>: > + + $path = realpath($version['install_path']); + if ($path !== BP) { + $packages[] = $package; + $packageDirectories[] = $path; + } With a git install, composer lists mahocommerce/maho as an installed package, but it's installed path is just where you cloned the repo. The check filters that out. I'm not sure it's strictly needed, but anytime you use the results of that function, it seems you're expecting the path to be in vendor. I will investigate. — Reply to this email directly, view it on GitHub <#40 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG6DL5SS4OIB3CCB6NOQILZZ3UWJAVCNFSM6AAAAABPMIAQGKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNBZGAYDAOJVGY> . You are receiving this because you commented.Message ID: ***@***.***>

justinbeaty · 2024-10-04T19:45:33Z

Git install is used only by us and it is forbidden to store projects, we shouldnt even consider it as a possibility ans it’s never coveres in the doc and never will 😂

I do agree, but it's not great that things break on a git install, because it makes testing harder since things work differently between the two install types.

sorry @justinbeaty, isn't there a simpler way to solve the problem you had cause this is gonna be very difficult to test

I could drop the Zend override stuff from this PR if it will make it easier to test.

fballiano · 2024-10-04T19:49:29Z

I could drop the Zend override stuff from this PR if it will make it easier to test.

I think people should use a composer patch if they need to patch zf1, overrides are always risky when there is an upgrade of the base file. I think this whole zf1 override thing is overrated.

justinbeaty · 2024-10-04T20:01:19Z

One other thought just now, it might be possible to drop the varien autoloader all together, but still allow Zend overrides. Then we get the fast Composer autoloader if available, and fallback to the slow PSR-0 type.

Although I do agree it's not great practice to allow overrides.

justinbeaty · 2024-10-05T00:03:55Z

@fballiano Check it out, no more Varien auto loader! There's only one caveat. Before you could do stuff like:

$this->_frontController = new mage_core_controller_varien_front();

Where it was case-insensitive, not you cannot, it must be Mage_Core_Controller_Varien_Front.

But I don't think that's a deal breaker, and it doesn't seem like that's always an issue, I think maybe only the first call to the class must be right.

Still working on the PR.

fballiano · 2024-10-05T00:06:44Z

@fballiano Check it out, no more Varien auto loader!

wow! very interesting!!

There's only one caveat. Before you could do stuff like:
$this->_frontController = new mage_core_controller_varien_front();
Where it was case-insensitive, not you cannot, it must be Mage_Core_Controller_Varien_Front.

mmm I kinda don't think that's a problem, since everybody uses an IDE (I hope) :-D

justinbeaty · 2024-10-05T00:07:36Z

Also, question:

Do you think it's okay to change the output of mahoGetComposerInstallationData() to something like this?

[
    'mahocommerce/maho' => [
        'type' => 'maho-source',
        'path' => '...',
    ],
    'my/module' => [
        'type' => 'magento-module',
        'path' => '...',
    ],
]

There might be a few keys to add to each module for an optimization idea I have.

justinbeaty · 2024-10-05T00:09:08Z

mmm I kinda don't think that's a problem, since everybody uses an IDE (I hope) :-D

I think I've seen some ancient modules that named something like My_Module_Block_MySuperAwesomeBlock, but didn't match the filesystem. But I can't recall, and it's worth not supporting that.

fballiano · 2024-10-05T00:10:53Z

Do you think it's okay to change the output of mahoGetComposerInstallationData() to something like this?
[
    'mahocommerce/maho' => [
        'type' => 'maho-source',
        'path' => '...',
    ],
    'my/module' => [
        'type' => 'magento-module',
        'path' => '...',
    ],
]

only difference is the key of the array right? I don't see any problem with that at this stage of the project.

I think I've seen some ancient modules that named something like My_Module_Block_MySuperAwesomeBlock, but didn't match the filesystem. But I can't recall, and it's worth not supporting that.

exactly, it's the kind of code that should die anyway

justinbeaty · 2024-10-05T00:13:41Z

only difference is the key of the array right? I don't see any problem with that at this stage of the project.

Right now you return two arrays:

['mahocommerce/maho', 'my/module']
['path1', 'path2']

And yeah, I don't think anyone has relied on it yet. 😄

fballiano · 2024-10-05T00:22:55Z

right, change it if you think it's better, no problem

…2.0, so make sure we have it Ref: https://getcomposer.org/doc/07-runtime.md#installed-versions

justinbeaty · 2024-10-05T03:13:50Z

It is actually faster to check which dirs exist first in included modules instead of adding all possible options into the path. So I added a check in mahoGetComposerInstallationData().

See this test: https://pastebin.com/z3T7cQ2j, test 1 is the old way, test 2 is new, ignore test 3 for now. While we will not be loading 3,581 classes on a page load, the numbers are still much better.

Test 1 took 0.7837700843811
Test 2 took 0.2346019744873
Test 3 took 1.171895980835

Edit, I logged how many lookups are done on a normal page load. Product pages are about 350, so with 10 modules installed, it's 18ms faster.

app/Mage.php

…opic-autoloader

fballiano · 2024-10-10T17:26:01Z

HAHAHHA I like the title of your last commit

justinbeaty · 2024-10-10T17:26:16Z

Wait, maybe we should have left the _canUseLocalModules variable and method? In case someone overrides the class?

fballiano · 2024-10-10T17:26:32Z

let me see

fballiano · 2024-10-10T17:28:18Z

mmm method and variable are "protected" and somebody should have overridden/extended app/code/core/Mage/Core/Model/Config.php which should be quite unusual, would you want to leave them in a disabled state and deprecate them?

justinbeaty · 2024-10-10T17:30:15Z

mmm method and variable are "protected" and somebody should have overridden/extended app/code/core/Mage/Core/Model/Config.php which should be quite unusual, would you want to leave them in a disabled state and deprecate them?

Not sure. One the other hand, if someone did extend Mage_Core_Model_Config, and for some reasons checked if local modules were disabled, they might be better off knowing that the function is removed by getting an error.

But we could also log an exception if someone calls it....

justinbeaty · 2024-10-10T17:33:00Z

We could also check to see if the user has it enabled, and log an exception there too. But I'm almost in favor of just having it completely removed.

fballiano · 2024-10-10T17:33:24Z

nah, the thing is that if we log an exception not a lot of people check the exception log... if we throw an exception at that point it's the same as the code breaking so... I'd say let's go if you don't want to think about it for a bit

fballiano · 2024-10-10T17:35:20Z

let's think about it for a couple of hours and then merge, it's a minor thing anyway I think and again I think in production nobody will want to have all local modules disabled, otherwise remove them altogether at that point

…opic-autoloader

justinbeaty · 2024-10-10T17:44:29Z

Okay, let me get back to testing. I'm pretty sure no more changes unless a problem. Will explain some of previous changes soon.

justinbeaty · 2024-10-10T18:47:01Z

Okay, everything is passing with my test modules. Let me overview some changes. I know you understand most of this anyway, but just for completeness sake.

Building our paths with `\MahoAutoload::generatePaths()`

This is basically the same code we had to build the include paths that would have been used by Varien_Autoload. One fix/improvement is that instead of:
mod_a/app/code/local, mod_a/app/code/community, mod_a/app/code/core, mod_b/app/code/local, mod_b/app/code/community, mod_b/app/code/core

We do all local, all community, all core, which seems more correct:
mod_a/app/code/local, mod_b/app/code/local, mod_a/app/code/community, mod_b/app/code/community, mod_a/app/code/core, mod_b/app/code/core

Generating PSR-0 prefixes with `\MahoAutoload::generatePsr0()`

Using the paths from above, we let composer know these paths contain PSR-0 classes. Since Varien_Autoload would have scanned those same paths in the same order and found the same files, there should be no breaking change. However, it does save composer from having to look in every module's path for Mage_ classes if we know there are none there.

Composer `use-include-path`

Because we have generated all of the PSR-0 prefixes, we have no need to let composer look in the include paths for files. If it wasn't found in our PSR-0 scan, it wouldn't have been found by Composer. We do still use set_include_path() in Mage.php so that modules can include or require files that aren't PSR-0 compatible.

Controller Classmaps

I believe the only type of file in Magento core that doesn't use PSR-0 classes are the controllers (but not Controller which do). So, we also scan those files and add them directly to Composer's classmap definition.

This has two benefits: (1) Varien_Router is faster because we don't have to loop all our paths to find the class. (2) We don't have to put require statements in controller classes that extend other controllers. Composer will find the class you're extending.

Regarding point 2, it doesn't break if you do have require statements in controller files, they're just unnecessary so I removed them. The only place I could see something breaking is if a module includes a controller with a relative path like include './AbstractController.php', and another module overrides the abstract class. Don't know if I explained that well, but it's so niche I wouldn't worry about it, and I'm pretty sure it would have been broken without this PR anyway.

Optimized autoloader

Enable with either composer dump -o, or composer config optimize-autoloader true. A warning message is displayed in the admin panel if set to true and in developer mode. One nice thing is that it will tell you which classes conflict, and which one it will be using:

It should definitely speed things up in production, and I see no reason not to use it.

Zend overrides

There are now no caveats in overriding Zend, or any other class in lib. Both myzend/override and zend/override work. You also don't have to explicitly define PSR-0 classes in your composer.json or delete the vendor folder. Just put the file in your module's or starter kit's lib folder and it will work.

Breaking changes

Only two I can think of:

Class names are case sensitive now, i.e. mage_core_model_abstract doesn't work anymore.
The removal of disable_local_modules support.

I think that explains everything. Will take a lunch break now, but feel free to do a final review if needed.

justinbeaty · 2024-10-10T21:42:29Z

Sweet! 🚀🚀🚀

fballiano · 2024-10-10T21:42:36Z

@justinbeaty merged!! thank you so much for such amazing work! I'll create a page on the website with the info from your last commit.

I've published the new version to https://demo.mahocommerce.com and check how freaking fast it is (there's not even an FPC module nor redis yet!)!

justinbeaty · 2024-10-10T21:46:07Z

@justinbeaty merged!! thank you so much for such amazing work! I'll create a page on the website with the info from your last commit.

I've published the new version to https://demo.mahocommerce.com and check how freaking fast it is (there's not even an FPC module nor redis yet!)!

My absolute pleasure. When I get around to it I am going to try and spin up my store on Maho locally. I have a totally janky process for building the codebase that tries to work around some of the shortcomings of the openmage composer plugins.

fballiano · 2024-10-10T22:03:17Z

boom https://mahocommerce.com/autoloaders/

justinbeaty · 2024-10-10T22:07:32Z

boom https://mahocommerce.com/autoloaders/

Nice! 😃

And good luck again to you for the conference. Looks like you'll be speaking in about 10 hours from now.

fballiano · 2024-10-10T22:18:33Z

Thank mate, I should definitely go to sleep now ehehehe I'm reviewing the slides right now :-D

justinbeaty added 7 commits October 3, 2024 19:52

autoloader stuff

ac1b16c

allow override of zend

213330a

fix

3505777

fix

3a56fa6

fix

904c60a

fixes

87006f8

tweak

f930560

fballiano reviewed Oct 4, 2024

View reviewed changes

app/Mage.php Outdated Show resolved Hide resolved

justinbeaty added 2 commits October 4, 2024 10:12

phpstan, will rename constant later

1bfbeef

phpstan typo

409a457

fballiano reviewed Oct 4, 2024

View reviewed changes

app/code/core/Mage/Core/functions.php Outdated Show resolved Hide resolved

lib/Varien/Autoload.php Outdated Show resolved Hide resolved

app/Mage.php Outdated Show resolved Hide resolved

Remove Varien Autoloader

ec6ba30

justinbeaty added 2 commits October 4, 2024 17:26

Composer\InstalledVersions is only available in composer-runtime-api …

4bae9b5

…2.0, so make sure we have it Ref: https://getcomposer.org/doc/07-runtime.md#installed-versions

Improve autoloader speed about 2.5x

b8fc991

fballiano added 3 commits October 5, 2024 09:11

Merge branch 'main' into topic-autoloader

e2cd0a4

phpcs

8c7ece7

renamed constants

bd4c3ce

fballiano reviewed Oct 5, 2024

View reviewed changes

app/Mage.php Outdated Show resolved Hide resolved

fballiano and others added 2 commits October 10, 2024 18:52

Minor renaming of a variable

960d346

better disable_local_module support

c19910b

justinbeaty mentioned this pull request Oct 10, 2024

Improvements to Maho's autoloaders MahoCommerce/maho-composer-plugin#1

Merged

justinbeaty added 5 commits October 10, 2024 10:05

remove mahoGetComposerInstallationData

98a21e9

Merge branch 'topic-autoloader' of github.com:justinbeaty/maho into t…

93c43a4

…opic-autoloader

cleanup

3d0a67f

remove mahocommerce/maho conditional

34f23fd

Nuke disable_local_modules support

b1bff6a

Merge branch 'main' into topic-autoloader

cc90f99

justinbeaty added 2 commits October 10, 2024 10:37

update composer.json/lock

8e19920

Merge branch 'topic-autoloader' of github.com:justinbeaty/maho into t…

3c32074

…opic-autoloader

just typehints

2b71d68

fballiano changed the title ~~Autoloader improvements~~ Improvements to Maho's autoloaders Oct 10, 2024

fballiano merged commit 1576c67 into MahoCommerce:main Oct 10, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements to Maho's autoloaders #40

Improvements to Maho's autoloaders #40

justinbeaty commented Oct 4, 2024

fballiano left a comment

fballiano commented Oct 4, 2024 via email

justinbeaty commented Oct 4, 2024

fballiano commented Oct 4, 2024

justinbeaty commented Oct 4, 2024

justinbeaty commented Oct 5, 2024

fballiano commented Oct 5, 2024

justinbeaty commented Oct 5, 2024

justinbeaty commented Oct 5, 2024

fballiano commented Oct 5, 2024

justinbeaty commented Oct 5, 2024

fballiano commented Oct 5, 2024

justinbeaty commented Oct 5, 2024 •

edited

Loading

fballiano commented Oct 10, 2024

justinbeaty commented Oct 10, 2024

fballiano commented Oct 10, 2024

fballiano commented Oct 10, 2024

justinbeaty commented Oct 10, 2024 •

edited

Loading

justinbeaty commented Oct 10, 2024

fballiano commented Oct 10, 2024

fballiano commented Oct 10, 2024

justinbeaty commented Oct 10, 2024

justinbeaty commented Oct 10, 2024 •

edited

Loading

justinbeaty commented Oct 10, 2024

fballiano commented Oct 10, 2024

justinbeaty commented Oct 10, 2024

fballiano commented Oct 10, 2024

justinbeaty commented Oct 10, 2024

fballiano commented Oct 10, 2024

Improvements to Maho's autoloaders #40

Improvements to Maho's autoloaders #40

Conversation

justinbeaty commented Oct 4, 2024

fballiano left a comment

Choose a reason for hiding this comment

fballiano commented Oct 4, 2024 via email

justinbeaty commented Oct 4, 2024

fballiano commented Oct 4, 2024

justinbeaty commented Oct 4, 2024

justinbeaty commented Oct 5, 2024

fballiano commented Oct 5, 2024

justinbeaty commented Oct 5, 2024

justinbeaty commented Oct 5, 2024

fballiano commented Oct 5, 2024

justinbeaty commented Oct 5, 2024

fballiano commented Oct 5, 2024

justinbeaty commented Oct 5, 2024 • edited Loading

fballiano commented Oct 10, 2024

justinbeaty commented Oct 10, 2024

fballiano commented Oct 10, 2024

fballiano commented Oct 10, 2024

justinbeaty commented Oct 10, 2024 • edited Loading

justinbeaty commented Oct 10, 2024

fballiano commented Oct 10, 2024

fballiano commented Oct 10, 2024

justinbeaty commented Oct 10, 2024

justinbeaty commented Oct 10, 2024 • edited Loading

Building our paths with \MahoAutoload::generatePaths()

Generating PSR-0 prefixes with \MahoAutoload::generatePsr0()

Composer use-include-path

Controller Classmaps

Optimized autoloader

Zend overrides

Breaking changes

justinbeaty commented Oct 10, 2024

fballiano commented Oct 10, 2024

justinbeaty commented Oct 10, 2024

fballiano commented Oct 10, 2024

justinbeaty commented Oct 10, 2024

fballiano commented Oct 10, 2024

justinbeaty commented Oct 5, 2024 •

edited

Loading

justinbeaty commented Oct 10, 2024 •

edited

Loading

justinbeaty commented Oct 10, 2024 •

edited

Loading

Building our paths with `\MahoAutoload::generatePaths()`

Generating PSR-0 prefixes with `\MahoAutoload::generatePsr0()`

Composer `use-include-path`