Move drupal_common_theme() from common.inc into theme.inc

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

@dereine, see #340723: Make modules and installation profiles only require .info.yml files and #345118: Performance: Split .module files by moving hooks

+1

This is not only a nice clean-up.

It should also speed up bootstrap for cached pages, because we load much less code.

Rerole

Oh and i wondered why my editor asked me to save the file :)

+++ 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?

+++ 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.

Needs work per #21.

As long as the theme funcions referred to are still inside includes/ I don't agree on moving drupal_common_theme() to system.module.

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.

Moving out of common.inc is in line with #1224666: [meta] Unofficial Drupal 8 Framework initiative, but into System module is not, I think.

subscribe

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.

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.

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.

For now, I'd recommend this. :)

Because we don't have hooks for sub-systems (includes/*.inc) this patch is enough

Lst patch missed a added color element

+    'color' => array(
+      'render element' => 'element',
+    ),

#34: 362889-drupal_common_theme-34.patch queued for re-testing.

Right format

#38: 362889-drupal_common_theme-37.patch queued for re-testing.

Ready to go if bot comes back green.

Quick re-roll, anyone? :)

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

I manually applied this patch to confirm that the moved function contains the exact same content.

#43: 362889-drupal_common_theme-43.patch queued for re-testing.

Thanks! Committed/pushed to 8.x.

Related issue that may affect this outcome #1905584: Move base theme system templates into /core/templates

Updated issue summary.