Improved composer's autoloader exclusion list a bit. #40107
Conversation
|
Hi @hostep. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email [email protected]. |
|
@magento run all tests |
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email [email protected]. |
5 similar comments
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email [email protected]. |
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email [email protected]. |
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email [email protected]. |
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email [email protected]. |
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email [email protected]. |
|
@magento create issue |
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email [email protected]. |
engcom-Charlie
added
the
Priority: P3
hostep
force-pushed
the
improve-autoloader-exclusion-list-a-bit
branch
from
5d4b278 to
c74c65d
Compare
|
@magento run all tests |
|
The security team has been informed about this pull request due to the presence of risky security keywords. For security vulnerability reports, please visit Adobe's vulnerability disclosure program on HackerOne or email [email protected]. |
Description (*)
This started out with noticing how dumping an optimized autoloader on Magento 2.4.8(-p1) outputted some errors from test classes in the
setup/src/directory which were using unexpected namespaces (unexpected in scope of a project, not in scope of the tests themselves), which didn't happen in Magento 2.4.7 or older.Notice that the number of optimized autoload files increased with about 6.500 files and we get 9 warnings about incorrect namespaces being used (which is fine in itself, it's just not expected to see those in project-scope as warnings).
Especially this becomes bad if you try to detect these kind of wrong usages of class names or namespaces in custom code in CI/CD systems where you can use the
--strict-psroption in thedump-autoloadcommand, to give you an exit code of 1, which should stop your CI/CD. So this is not good to have this regression in Magento 2.4.8 compared to 2.4.7I dug a bit deeper and found some more interesting things and the cause of this change.
So, Magento 2.4.8 came with the following commit: 52b936d, the interesting part of this is:
"exclude-from-classmap": [ "**/dev/**", "**/update/**", - "**/Test/**" + "*/*/Test/**/*Test" ]After a bunch of digging around, I figured out why this got changed, see this discussion on the phpunit repository: sebastianbergmann/phpunit#5570
The
exclude-from-classmapis typically being used to add some path patterns to be excluded from the generated (optimized) autoloader, like Test classes which aren't in somedevdependency package. This is so that the autoloader is faster and not bloated with classes that make no sense to be autoloaded in a production system.With Magento, most test code comes included with the modules themselves, so using
exclude-from-classmapto strip them out of the autoloader makes sense.However, since PHPUnit v10, the old pattern also stripped out classes that phpunit required to be autoloaded (yes, also on non-production systems). So the old pattern had to be updated to keep those around while still trying to reduce the autoloader size in Magento.
However, I believe the choice of the new pattern was made poorly as it doesn't match many paths (even none at all in my testing). The pattern I'm introducing in this PR seems to match a lot more files and still keeps phpunit functioning like it should.
I'm proposing instead the pattern
**/Test/Unit/**in this PR as it solves all problems. It significantly reduces the number of autoloaded classes. It keeps phpunit working like it should. And it removes those warnings when dumping an optimised autoloader.Related Pull Requests
N/A
Fixed Issues (if relevant)
N/A
Manual testing scenarios (*)
Give the following steps a try with these 3 patterns:
"**/Test/**"(from Magento 2.4.7 and earlier)"*/*/Test/**/*Test"(from Magento 2.4.8)"**/Test/Unit/**"(proposal in this PR)composer dump-autoload -oGenerated optimized autoload files containing xxx classes, it should be smaller then beforeTestdirectory the classmap autoloader of composer:grep '/Test/' vendor/composer/autoload_classmap.php | wc -l, the number should be smaller then beforecd dev/tests/unit && ../../../vendor/bin/phpunit -c phpunit.xml.dist ../../../app/code/Magento/ReleaseNotification/; cd ../../..Questions or comments
Does it make sense to exclude more test classes? I don't think it's worth it, only a couple of hundreds of classes will get removed.
Examples:
**/Test/Integration/**=> good for reduction of 285 extra classes**/Test/Mftf/**=> good for reduction of 19 extra classes**/Test/Api/**=> good for reduction of 93 extra classes**/Test/Fixture/**=> good for reduction of 99 extra classesContribution checklist (*)
Resolved issues: