Posted by Xano on January 22, 2009 at 11:46pm
15 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | theme system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | API clean-up, Framework Initiative |
Issue Summary
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.
Comments
#1
by 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
#2
#3
@dereine, see #340723: Make modules only require .info.yml files and #345118: Performance: Split .module files by moving hooks
#4
Just for my own records.
#5
The last submitted patch failed testing.
#6
#7
The last submitted patch failed testing.
#8
#9
The last submitted patch failed testing.
#10
#11
The last submitted patch failed testing.
#12
#13
#14
The last submitted patch failed testing.
#15
+1
This is not only a nice clean-up.
It should also speed up bootstrap for cached pages, because we load much less code.
#16
Rerole
#17
Something went wrong. Tha patch only adds drupal_common_theme() to System.module.
#18
Oh and i wondered why my editor asked me to save the file :)
#19
+++ modules/system/system.module@@ -133,7 +133,206 @@ function system_help($path, $arg) {
* Implement hook_theme().
*/
function system_theme() {
Can we extend the PHPDoc with a description (below summary) to clarify that System module is registering theme functions on behalf of include files?
+++ modules/system/system.module@@ -133,7 +133,206 @@ function system_help($path, $arg) {
+ ),
'system_themes_form' => array(
Can we squeeze an inline comment in between here?
// System module.
I'm on crack. Are you, too?
#20
I'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.
#21
+++ includes/common.inc@@ -5355,213 +5355,6 @@ function element_children(&$elements, $sort = FALSE) {
- // theme.inc
...
- // from theme.maintenance.inc
...
- // from pager.inc
...
- // from locale.inc
...
- // from menu.inc
...
- // from form.inc
+++ modules/system/system.module
@@ -133,7 +133,206 @@ function system_help($path, $arg) {
+ // theme.inc
...
+ // from theme.maintenance.inc
...
+ // from pager.inc
...
+ // from locale.inc
...
+ // from menu.inc
...
+ // from form.inc
...
+ ),
'system_themes_form' => array(
We 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.
#22
That's a good approach. +1.
#23
Needs work per #21.
#24
As long as the theme funcions referred to are still inside includes/ I don't agree on moving drupal_common_theme() to system.module.
#25
Moving 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.
#26
Moving out of common.inc is in line with #1224666: [meta] Unofficial Drupal 8 Framework initiative, but into System module is not, I think.
#27
subscribe
#28
Rerolled 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.
#29
The last submitted patch, 362889-drupal_common_theme-to-system.module.patch, failed testing.
#30
ahem, 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.
#31
Actually, regardless of what we do, I don't think there's going to be any kind of performance impact here, so removing the tag.
#32
For now, I'd recommend this. :)
#33
Because we don't have hooks for sub-systems (includes/*.inc) this patch is enough
#34
Lst patch missed a added color element
+ 'color' => array(+ 'render element' => 'element',
+ ),
#35
The last submitted patch, 362889-drupal_common_theme-34.patch, failed testing.
#36
#34: 362889-drupal_common_theme-34.patch queued for re-testing.
#37
The last submitted patch, 362889-drupal_common_theme-34.patch, failed testing.
#38
Right format
#39
#38: 362889-drupal_common_theme-37.patch queued for re-testing.
#40
Ready to go if bot comes back green.
#41
The last submitted patch, 362889-drupal_common_theme-37.patch, failed testing.
#42
Quick re-roll, anyone? :)
#43
Re-roll
+++ b/core/includes/theme.inc@@ -659,7 +659,7 @@ function _theme_build_registry($theme, $base_theme, $theme_engine) {
- * declare this theme as their base theme.
+ * declare this theme as their base theme.
No more needed
#44
I manually applied this patch to confirm that the moved function contains the exact same content.
#45
#43: 362889-drupal_common_theme-43.patch queued for re-testing.
#46
Thanks! Committed/pushed to 8.x.
#47
Automatically closed -- issue fixed for 2 weeks with no activity.
#48
Related issue that may affect this outcome #1905584: Move base theme system templates into /core/templates