Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
- Theme registry clutter in common.inc.
Goal
- Move drupal_common_theme() out of common.inc.
Details
- drupal_common_theme() registers theme functions and templates on behalf of:
- HTML, page, region rendering
- general HTML: link, link list, item list, image, table, html_tag
- subsystems: menu tree, breadcrumb, pager, tablesort, progress bar, form elements, render elements
- common UI elements: messages, "mark", more help link, feed icon
- modules: username, indentation
- installer: maintenance_page, install_page, update_page, task list, authorize message/report
- Some of them have corresponding entries in hook_element_info() of system.module.
- All of them require the theme system to be initialized, but after decoupling the installer from system.module, most of them need to work in a maintenance theme context (without DB, without module system).
Possible solutions
- Move drupal_common_theme() into system.module. Doesn't really solve anything.
- Create a new element.module or element.inc include file/subsystem. Move it in there as element_theme(). Allows to move further element_* functionality in there.
---- OP ----
Since hooks are pretty much exclusive to modules, it makes sense to put them in there as well. Dereine and I discussed that moving drupal_common_theme()
to system.module would be a logical step. I will try to find out if this is possible.
Comment | File | Size | Author |
---|---|---|---|
#43 | 362889-drupal_common_theme-43.patch | 15.31 KB | andypost |
#38 | 362889-drupal_common_theme-37.patch | 15.81 KB | andypost |
#34 | 362889-drupal_common_theme-34.patch | 16.41 KB | andypost |
#32 | drupal8.theme-common.32.patch | 15.17 KB | sun |
#28 | 362889-drupal_common_theme-to-system.module.patch | 14.08 KB | cweagans |
Comments
Comment #1
dawehnerby the way, i wondered once, what can be done know with registry in core.
for example: create a .theme.inc file foreach module and place there at least the hook_theme implementation
This will load drupal less code
Comment #2
XanoComment #3
swentel CreditAttribution: swentel commented@dereine, see #340723: Make modules and installation profiles only require .info.yml files and #345118: Performance: Split .module files by moving hooks
Comment #4
XanoJust for my own records.
Comment #6
XanoComment #8
davyvdb CreditAttribution: davyvdb commentedComment #10
davyvdb CreditAttribution: davyvdb commentedComment #12
XanoComment #13
XanoComment #15
sun+1
This is not only a nice clean-up.
It should also speed up bootstrap for cached pages, because we load much less code.
Comment #16
dawehnerRerole
Comment #17
XanoSomething went wrong. Tha patch only adds drupal_common_theme() to System.module.
Comment #18
dawehnerOh and i wondered why my editor asked me to save the file :)
Comment #19
sunCan we extend the PHPDoc with a description (below summary) to clarify that System module is registering theme functions on behalf of include files?
Can we squeeze an inline comment in between here?
// System module.
I'm on crack. Are you, too?
Comment #20
XanoI'm not sure if extra information in the PHPDoc block is needed. System.module contains more information belonging to includes files (which I like to see moved to System.module in D8 anyway), so IMO there is no need to describe this.
Comment #21
sunWe have such inline comments already, and they were really good helpers for people in the past. This is the largest hook_theme() implementation, and it would be horrible if people would just add their stuff anywhere.
So please, just add
// system.module
before the actual theme functions of System module. While being there, we could also remove the "from " prefixes from the other inline comments.
The PHPDoc block should clarify and ensure that people who are looking at example hook implementations in core, do NOT copy this style of System module.
Comment #22
XanoThat's a good approach. +1.
Comment #23
catchNeeds work per #21.
Comment #24
casey CreditAttribution: casey commentedAs long as the theme funcions referred to are still inside includes/ I don't agree on moving drupal_common_theme() to system.module.
Comment #25
catchMoving this to Drupal 8. There's an issue somewhere else about moving all the theme functions out of theme.inc into system.module (or somewhere that's not theme.inc), so let's give this a bit more thought given nothing's actually broken here.
Comment #26
sunMoving out of common.inc is in line with #1224666: [meta] Unofficial Drupal 8 Framework initiative, but into System module is not, I think.
Comment #27
Jacinesubscribe
Comment #28
cweagansRerolled this patch against HEAD. Basically the same patch, but instead of moving the menu.inc stuff into system, it's now in menu.module.
I agree that it's probably not 100% in line with #1224666: [meta] Unofficial Drupal 8 Framework initiative, but I think a good first step is to get this out of common.inc. Once that's done, we can figure out what to do with all of the generic theme function declarations (html, page, etc.). I'm kind of thinking it might be smart to create a "theme" module, but I don't know how realistic that is.
Comment #30
sunahem, ok, this needs more discussion.
I've created a proper issue summary, and added my suggested solution to it: New element.module or element.inc subsystem.
Comment #31
sunActually, regardless of what we do, I don't think there's going to be any kind of performance impact here, so removing the tag.
Comment #32
sunFor now, I'd recommend this. :)
Comment #33
andypostBecause we don't have hooks for sub-systems (includes/*.inc) this patch is enough
Comment #34
andypostLst patch missed a added color element
Comment #36
andypost#34: 362889-drupal_common_theme-34.patch queued for re-testing.
Comment #38
andypostRight format
Comment #39
sun#38: 362889-drupal_common_theme-37.patch queued for re-testing.
Comment #40
sunReady to go if bot comes back green.
Comment #42
sunQuick re-roll, anyone? :)
Comment #43
andypostRe-roll
No more needed
Comment #44
sunI manually applied this patch to confirm that the moved function contains the exact same content.
Comment #45
catch#43: 362889-drupal_common_theme-43.patch queued for re-testing.
Comment #46
catchThanks! Committed/pushed to 8.x.
Comment #48
c4rl CreditAttribution: c4rl commentedRelated issue that may affect this outcome #1905584: Move base theme system templates into /core/templates
Comment #48.0
c4rl CreditAttribution: c4rl commentedUpdated issue summary.