The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Problem/Motivation
All core modules except Node have been converted, let's mark them as skippable.
This excludes test modules.
Would be great to do a before/after comparison of drush install to see if/how much memory and potentially time we save.
Steps to reproduce
N/A
Proposed resolution
Add the following container parameter
parameters:
module_name.skip_procedural_hook_scan: true
Call out any unique modules:
- Node
- SDC (stub)
- Comment missed preprocess
- Media library missed preprocess
- Views missed preprocess
Core modules
- comment_preprocess_field
- media_library_preprocess_views_view__media_library
- views_preprocess_node
- views_preprocess_comment
Test modules
- twig_theme_test_preprocess_status_messages
Remaining tasks
N/A
User interface changes
N/A
Introduced terminology
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
| Comment | File | Size | Author |
|---|
Issue fork drupal-3494908
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:
- 3494908-pp-1-mark-several
changes, plain diff MR !10614 - 3494908-skip_scanning changes, plain diff MR !12706
- 11.x compare
Comments
Comment #2
nicxvan commentedComment #4
nicxvan commentedComment #5
nicxvan commentedComment #6
dwwGlad to see the rest of this committed. Thanks for opening this as a new issue.
Comment #7
smustgrave commentedWith last nights commits appears to need a rebase.
if you are another contributor eager to jump in, please allow the original poster @nicxvan at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
Comment #8
nicxvan commentedYeah I had left this in needs work because I knew this needed the test fixes in the other issue.
Comment #9
nicxvan commentedComment #10
dwwIt's still
61 files +788 −683. Can/should we split this up further to be easier to review? Maybe media* and system their own issues, everything else in here? I think that'd get us closer to +/- 500 here.Comment #11
nicxvan commentedI'll split media and library and then system to their own
Comment #12
nicxvan commentedComment #13
nicxvan commentedComment #14
nicxvan commentedComment #15
nicxvan commentedOk I split system and the media modules out, those two issues are ready for review too.
Comment #16
nicxvan commentedComment #17
dwwComment #18
smustgrave commentedCan the IS be updated with exactly what's being achieved here? Example the child issues marked what modules are being converted so it was super clear.
Thanks.
Comment #19
nicxvan commentedI added a list of the modules that I updated to the IS. Did you want more detail such as which changes are for each module?
Comment #20
nicxvan commentedComment #21
ambot112 commentedThanks for your progress and effort @nicxvan for MR !10614. Looking forward for its release.
Comment #22
smustgrave commented- no that's probably not needed, just needed a quick glance on what was done "several modules" was kinda vague.
Will try and find time this week to take a look, much larger then the others so takes a little longer.
Comment #23
nicxvan commentedLet's postpone this for a bit on #3495943: Handle module preprocess functions as OOP hooks
Comment #24
larowlanBlocker is in
Comment #25
nicxvan commentedLet's convert this to a meta, We have to re-evaluate some modules after #3495943: Handle module preprocess functions as OOP hooks.
We can also do this in groups and stages, we pushed this through faster originally due to the massive memory leak.
Comment #26
nicxvan commentedOk I did a full audit of core modules, I checked whether the services parameter was set, if there were hooks that hadn't been converted, preprocess, template_preprocess, .inc, files and helpers.
I think this round is focused on hooks, preprocess and template preprocess in most cases. I think we only tackle .inc if they are preprocess hooks in them.
I think we try to balance how many are in a given issue but we address organization and what we can in each.
Comment #27
nicxvan commentedComment #28
nicxvan commentedIt ends up being a lot of changes even for mostly converted modules.
I think we may need to restrict this to an issue per module, I think we also want to isolate theme, theme suggestion and preprocess functions that do not inject services from ones that do.
I'm mostly through bigpipe and block on the issue linked.
Could use a high level review.
Comment #29
berdirWe seem to have multiple overlapping (meta) issues now. The block issue is now essentially #3494908: Set skip procedural scanning for all modules in core and also does DI, regroups hooks and so on. And there's the template_preprocess meta issue.
I'm unsure what the best way to split these is. The way it's done for block now kind of means we could just inline the actual last step that this aims to do (add the flag to services.yml once ready) into the cleanup meta and close this. I think the performance gains for those last few remaining modules is minor to tiny and it's no longer a priority from a performance perspective.
That said, the cleanup is fairly low prio and I expect those issues to land only slowly as they are hard to review and get committed. IMHO it's more useful to make sure that we've converted all legacy hooks and also preprocess stuff, with the goal of being able to explicitly deprecate legacy hooks and preprocess stuff in 11.3 for D13. For preprocess, we could expand the scope of #3504381: [meta] Convert Template Preprocess hooks to OOP equivalent to not only cover template_preprocess but all preprocess in modules. And I'd probably approach the hooks similar to #3265945: Deprecate plugins using annotations and plugin types not supporting attributes. Figure out how exactly we want to deprecate legacy hooks, that will tell us which bits we missed, and then we can possibly have a single issue to convert those.
Comment #30
nicxvan commentedYeah, as I was doing it I knew the scope was too large but I wanted to check for sure.
I think this one reverts back to the minimum needed to mark a module as converted worth two exceptions.
1 hook requirements are handled in the issues dedicated to those.
2 for smaller module's we include converting template preprocess.
Bigger modules like node, views, and system will need dedicated hook, proprietary and template issues.
Comment #31
nicxvan commentedTests just started, but this should be our target I think: https://git.drupalcode.org/project/drupal/-/merge_requests/12022/diffs
If there is a module like block_content that only has template_preprocess we can take care of it in the template preprocess issue.
Comment #32
nicxvan commentedSince all of core (except node) is converted I'll retarget this just to set the container parameter.
Comment #35
nicxvan commentedComment #36
nicxvan commentedComment #37
nicxvan commentedComment #39
nicxvan commentedComment #40
nicxvan commentedComment #41
nicxvan commentedThis is ready for review!
We missed 4 preprocess functions, I will create follow ups to convert them.
Comment #42
nicxvan commentedComment #43
nicxvan commentedComment #44
nicxvan commentedComment #45
nicxvan commentedComment #46
needs-review-queue-bot commentedThe 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.
Comment #47
nicxvan commentedRebased
Comment #48
berdirThere are 17 #[ProceduralHookScanStop] usages in those modules still. It's a fairly trivial search & replace for the modules that are switched here, I think it might be easier to include it here instead of a new issue, I think that's closely related and not scope creep?
If I saw that correctly, it's just two modules for this issue, locale and update. there's 2 in media as well which has a separate issue and one in a test module that we'll keep.
Comment #49
nicxvan commentedMakes sense!
I've removed them here, let me do the same in the media module issue.
Comment #50
nicxvan commentedI removed the media module preprocess here too.
Comment #51
berdirI was confused by comment #50 at first, I guess you meant that you removed the stop attribute, not preprocess.
I think we're good now here. This will help to speed up discovery a bit and reduce container size. And it will ensure that we won't be adding new hooks to those modules, because they will no longer work (Silently, without any notice, I'd prefer an explicit deprecation, but one step at a time).
Comment #52
catchThis looks good but needs a rebase.
Comment #53
nicxvan commentedI think this is good actually, I think that merge error is a bug.
I pulled it down and rebased and it shows clean. I also checked the actual MR and there are no conflicts shown.
If I am missing something please let me know.
Comment #55
catchAll fine now, guess I could have tried
/rebasemyself.Committed/pushed to 11.x, thanks!
Comment #58
cilefen commentedThis caused #3539377: After core commit 11.x-dev#7e27c7f can no longer save field definitions in Webform.
Comment #59
nicxvan commentedThanks!
This is because we set system to skip, but system has system_hook_info which needs to still execute.
#3540033: Do not skip scanning for procedural hooks in the system module