- Issue created by @kingdutch
- Merge request !11902Issue #3520300: Replace DrupalKernel::loadLegacyIncludes with composer autoload.files β (Open) created by kingdutch
- πΊπΈUnited States smustgrave
Also probably needs a CR and links updated.
- πΊπΈUnited States nicxvan
Will this affect unit tests?
If I understand correctly the legacy files would not be loaded before this change, but after this change all unit tests will load all legacy includes.Not sure if this is a problem or acceptable, but it likely is a change.
- π³π±Netherlands kingdutch
Will this affect unit tests?
If I understand correctly the legacy files would not be loaded before this change, but after this change all unit tests will load all legacy includes.Not sure if this is a problem or acceptable, but it likely is a change.
You understand correctly. There were two tests that still relied on those functions and redefined them themselves, those have been changed in the diff. For the other tests if they ignore the functions then nothing happens. I guess the only risk is that people start using them in more places? However, there's nothing stopping someone from doing that in a place that's covered by a test that's not a unit test, so I'd argue that's acceptable.
I'll write up a change record and update the deprecation links later this week :) I'm not sure how I would go about writing a deprecation test. Any pointers to an example would be welcome.
- π³πΏNew Zealand quietone
Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies β .
- πΊπΈUnited States nicxvan
Left one other question.
You don't need to test deprecations directly unless there is complex BC logic.
The question is what happens if someone is calling that function directly and the files get loaded in composer as well.
- π¨π¦Canada Charlie ChX Negyesi πCanada
My worry about unit tests would be performance but I am so often wrong in what is useless micro optimization and what is legit worry.
- πΊπΈUnited States nicxvan
Great question!
This is somewhat anecdotal, the test run here was 6 minutes.
I checked 4 other issues they all ranged from 8-15 minutes.
- π³π±Netherlands kingdutch
You don't need to test deprecations directly unless there is complex BC logic.
Then I think in this case a test isn't needed since all it does is trigger a deprecation? There's no logic involved.
The question is what happens if someone is calling that function directly and the files get loaded in composer as well.
The requires have been removed from the function, so nothing happens. If someone has their own implementation that copy pasted the files then PHP will not require the files again because it was using require_once so it was a no-op.
My worry about unit tests would be performance but I am so often wrong in what is useless micro optimization and what is legit worry.
Fair concern. I think without profiling directly I can only reason towards "it's unnoticeable/no impact":
The list of files that was automatically loaded (without loading any classes; just preloaded files) was 40 items long and has now increased to 45. That on its own is a significant increase. However, if we look at one of the unit tests that was changed in this PR,
Unit/LanguageNegotiationUrlTest.php
, then that already loads 100-200 classes and files just for the test and this does not include what PHPUnit itself might use.If we make the math simple and we say a simple unit test in Drupal requires 100 files loaded to run, then we're adding a 5% increase in file load (assuming all files are equal). Then if the test did nothing but load files we'd have a 5% decrease in test performance. However, file loading is usually a small part of a test so the 5% is really then an upper limit and is much likely far below 1%.
The reason I focus on file loading is that the files themselves only contain function definitions and don't run any code, so they don't actually execute anything.
I hope that alleviates your worry.
I've written a change record and addressed the review feedback (except for the deprecation test). The Needs Review Queue Bot β tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.