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

Rewrite autoloader #2734

Closed
wants to merge 1 commit into from
Closed

Rewrite autoloader #2734

wants to merge 1 commit into from

Conversation

luigifab
Copy link
Contributor

@luigifab luigifab commented Nov 17, 2022

Description

This PR fix #1780 and replace #1817 and #2300.
It's created from @woutersamaey, @Asfolny, @Flyingmana, and me ideas.

There is a tiny breaking change:
for example with Ext4mage/Orders2csvpro, there are:
<source_model>ext4mage_orders2csvpro_model_source_creditmemofiles</source_model>
This does not work any more, because ucwords is removed in this PR.

I checked with Blackfire, this is a performance regression (see comment bellow).
But from ~5 ms to ~10 ms.

I didn't tested with #1969.

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

@github-actions github-actions bot added Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Mage.php Relates to app/Mage.php labels Nov 17, 2022
@tmotyl
Copy link
Contributor

tmotyl commented Nov 17, 2022

Please add more info and context around this change.

  • why?
  • what is changed
  • Why is it breaking
  • Why is it slower?

@github-actions github-actions bot removed the Mage.php Relates to app/Mage.php label Nov 17, 2022
@@ -13,10 +13,3 @@
);
}
}

spl_autoload_register(function($className) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file should get removed by #2411

@luigifab
Copy link
Contributor Author

Original version is:
return @include str_replace(' ', DIRECTORY_SEPARATOR, ucwords(str_replace('_', ' ', $class))) . '.php';

The simplified version used in this PR (all tests bellow with Blackfire with it):
$path = str_replace(['_', '\\', ' '], DIRECTORY_SEPARATOR, $class) . '.php';

First, with return @include $path;
image

Second, with stream_resolve_include_path (link)
image

Third, with foreach get_include_path (link)
image

The difference between stream_resolve_include_path (12.3 - 10.0 - 8.81 ms) and foreach get_include_path (10.4 - 12.1 - 10.8 ms) seems to be negligible.

@luigifab
Copy link
Contributor Author

luigifab commented Nov 20, 2022

Today I realize that perhaps the replace of the space is not necessary? Not tested.
For the removal of ucwords, perhaps I can keep it, I didn't tested, or I can remove it only on 20.0.

@fballiano
Copy link
Contributor

lib/phpseclib is no longer with us :-)

@fballiano
Copy link
Contributor

this has a conflict now, I'll move it to draft

@fballiano fballiano marked this pull request as draft December 19, 2022 16:27
@Flyingmana
Copy link
Contributor

What speaks against first finishing/merging #2300 to fix the bug, and then do the performance improvement separately?

@luigifab
Copy link
Contributor Author

luigifab commented Jan 1, 2023

Closed for #2300, and also because #1780 is now fixed.

@luigifab luigifab closed this Jan 1, 2023
@luigifab luigifab deleted the autoload branch January 1, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

include(): Failed opening 'Phpseclib\Crypt\Blowfish.php'
4 participants