Replace DrupalKernel::loadLegacyIncludes with composer autoload.files

Created on 21 April 2025, 3 months ago

Problem/Motivation

The DrupalKernel has a lot of responsibilities. One of which is loading some legacy function files which are in the process of being replaced. However, the kernel shouldn't be loading files as that's what the Composer autoloader is for.

Steps to reproduce

Proposed resolution

Move the files to core's composer.json under autoload.files. Remove them from loadLegacyIncludes and deprecate that method.

Files autoload rules are included whenever vendor/autoload.php is included, right after the autoloader is registered. The order of inclusion depends on package dependencies so that if package A depends on B, files in package B will be included first to ensure package B is fully initialized and ready to be used when files from package A are included.

This ensures the files are always loaded. The function is currently called in preHandle which is both manually called in tests and the KernelPreHandle middleware, so the end result of when functions are available is the same.

All code contained in the files are functions and thus the inclusion of the files do nothing on their own besides making the functions callable. There's no side-effects to including them. The files contain use statement, but used classes are not autoloaded until runtime code actually uses the class in an executed statement.

Remaining tasks

User interface changes

Introduced terminology

API changes

  • DrupalKernelInterface::loadLegacyIncludes is deprecated and scheduled for removal in 12.0
  • DrupalKernel::loadLegacyIncludes is deprecated and scheduled for removal in 12.0

Data model changes

Release notes snippet

DrupalKernelInterface::loadLegacyIncludes has been deprecated and scheduled for removal in 12.0, there is no replacement. The files it included will now be loaded by the Composer autoloader when vendor/autoload.php is first included.

DrupalKernel::preHandle no longer calls DrupalKernel::loadLegacyIncludes. If you relied on this behaviour to load your own include files, move them to the Composer autoloader instead.

πŸ“Œ Task
Status

Active

Version

11.1 πŸ”₯

Component

bootstrap system

Created by

πŸ‡³πŸ‡±Netherlands kingdutch

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @kingdutch
  • πŸ‡³πŸ‡±Netherlands kingdutch
  • Pipeline finished with Failed
    3 months ago
    Total: 181s
    #478388
  • Pipeline finished with Failed
    3 months ago
    Total: 148s
    #478403
  • Pipeline finished with Success
    3 months ago
    Total: 412s
    #478520
  • πŸ‡«πŸ‡·France andypost

    Needs deprecation test and looks RTBC

  • πŸ‡ΊπŸ‡Έ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.

  • Pipeline finished with Success
    3 months ago
    Total: 435s
    #479225
  • πŸ‡³πŸ‡±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.

Production build 0.71.5 2024