Closed (fixed)
Project:
Drupal core
Version:
11.x-dev
Component:
system.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
24 Oct 2025 at 03:11 UTC
Updated:
5 Mar 2026 at 19:00 UTC
Jump to comment: Most recent
Problem/Motivation
system_requirements was converted to the new system by copy and paste, let's start cleaning that up.
While this is a big MR I don't see how it can be smaller. We get significant gains in the install time requirements, -1200 loc
I copied the system requirements over then deleted the relevant conditions from each copy.
Reviewing the removed code should be pretty straightforward in SystemRequirements.php since it should only be related to runtime or udpate.
The SystemRequirementsHooks could have a diff from the systemrequirements and check it's only install time logic removed.
Steps to reproduce
Proposed resolution
Copy the current checkRequirements to the Update and Runtime hook class.
Remove install phase from it.
Remove update and runtime phase from the install class
This allows us to simplify the install time requirements significantly (-1000 loc)
We also simplify update and runtime a bit, we can then review how to further separate concerns in several follow ups
Approach 2
Copy SystemRequirements as is to the SystemRequirementsHooks (no change in install time)
A follow up where we tackle removing one unnecessary chunk at a time. ( suspect we'll need at LEAST 10 follow ups just to get to this point here)
Remaining tasks
Review
Determine if we need to pivot to approach 2
Create follow ups either way since we still have a lot of clean up to do.
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3554134
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
nicxvan commentedComment #4
nicxvan commentedComment #5
nicxvan commentedComment #6
nicxvan commentedComment #7
dcam commentedI know you said to not put much time into it, but I did anyway.
I checked the diff of the two files against each other to make sure changes were copied correctly. And they look OK to me with the exception of the one thing I made a comment on.
Following that, I went through the original version of
SystemRequirementsand hunted for all the phase conditions. I verified that all the runtime and update phase conditions went into the new file and only the install phase conditions stayed aside from the things that belonged to both.Aside from the comments on the MR this is looking good to me. I think we're OK to proceed with this as the way forward instead of option 2.
Comment #8
dcam commentedSorry about some of the comments I made in my last review. You already know that I got confused about which phase I was looking at. I'm pretty sure I also misled you about a potential change to combine conditions, which is why I've set the status to Needs Work. Please double-check what I've suggested tonight. After all this back-and-forth I'm second-guessing myself.
Comment #10
catchWill be interesting to see how much of a dent this makes in the cyclomatic complexity tracking on https://dbuytaert.github.io/drupal-core-metrics/
Comment #13
nicxvan commentedAssigning this to dcam since we chatted about this on slack.
This is ready for review again.
Comment #15
dcam commentedMy review is finished. I didn't find anything to comment on in the review of the new Hook class. I verified that everything not marked for the install phase was copied over.
I did manual tests of the install (
install.php), runtime (/admin/reports/status), and update (update.php) requirements. They all worked correctly. I didn't try to check the install and upgrade requirements super thoroughly. I just verified that they continued to work after the change, which amounted to them being automatically passed as usual. I did before and after comparisons of the status report page. It reported exactly the same information.I think that once the one incorrect block of code in
SystemRequirementsis deleted, then this can be RTBC. Marking as Needs Work for that one change.Comment #16
dcam commentedComment #17
nicxvan commentedComment #18
nicxvan commentedOk I cleaned up that one clause and created the follow up: #3574156: Deprecate and move constants in SystemRequirements
Comment #19
dcam commentedMy feedback was addressed. The tests are green. This looks good to me.
Comment #22
catchThere's some degree of risk here because a lot of this is very old code with very limited if any test coverage. However per the metrics dashboard it has the highest cyclometric complexity of any method in core by a long way, so as we cut it down, we'll make it more maintainable.
Is there another issue open to try to split the update and runtime requirements into helper methods / isolate phase-specific sections?
Committed/pushed to main and 11.x, thanks!
Comment #25
nicxvan commented#3574266: Separate php version requirements for SystemHookRequirements is the first followup but after seeing php version checking I'll probably make that a meta issue.
Comment #26
nicxvan commentedFor posterity RE #10.
SystemRequirements::check requirements before this merge was number 1 on the complexity table:
Cyclomatic complexity 222, Maintainability index of 0 and 84 anti patterns.
After merge:
SystemRequirements::check requirements before this merge became number 11 on the complexity table:
Cyclomatic complexity 56, Maintainability index of 22 and 17 anti patterns.
New number 1 is SystemRequirementsHooks::checkRequirements (where most of the code moved)
Cyclomatic complexity 181, Maintainability index of 0 and 82 anti patterns.
The good news is now we can start chipping away at the real complexities now.