Problem/Motivation

This issue was originally about providing for theme templates that would represent theme callbacks used in .inc files. However, drupal_common_theme() is only ever called once, by system_theme(). We could move drupal_common_theme() directly into system_theme(), and then all the theme functions/templates there too. All the places where these functions are used without a full bootstrap currently have to include system module beforehand.

Once they're in system module, there's incentive to factor them back out again.

Original issue by steveoliver

Drupal currently has no way of supporting templates for theme implementations defined in core/includes/* except by searching in themes/ directories drupal_find_theme_templates.

In our effort to replace Drupal's theme functions with [Twig] templates, we need a way for the theme system to find templates for theme implementations in /core/includes/.

CommentFileSizeAuthor
#16 theme.templates.16.patch2.58 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

steveoliver’s picture

Maybe this just needs to be done in _theme_build_registry ?

steveoliver’s picture

Priority: Normal » Major

Not really sure where to go from here. Bumping priority to hopefully get some more eyes on this and because it blocks our conversion of markup from theme function in core .inc files to Twig templates.

catch’s picture

When menu links move to entities, they'll be owned by the menu_links module.

This is really for everything in drupal_common_theme(), and drupal_common_theme() is only ever called once, by system_theme().

So could we not just move drupal_common_theme() directly into system_theme(), and then all the theme functions/templates there too?

Normally I'd be completely against moving stuff into system module, but it's currently impossible for a theme hook to be owned by anything other than a module so in this case I think it's least worst. All the places where these functions are used without a full bootstrap currently have to include system module beforehand anyway iirc.

Once they're in system module, there's incentive to factor them back out again.

steveoliver’s picture

Priority: Major » Normal
Status: Active » Closed (won't fix)

Sounds good, catch. Closing this.

steveoliver’s picture

Title: Support templates in core/includes/templates/ » [Don't] support templates in core/includes/templates/
c4rl’s picture

Would it be worth re-opening #362889: Move drupal_common_theme() from common.inc into theme.inc to move drupal_common_theme() into system.module? Or create a new issue?

c4rl’s picture

Title: [Don't] support templates in core/includes/templates/ » Move all theme callbacks in includes to system.module (form.inc, menu.inc, pager.inc, theme.inc, theme.maintenance.inc)
Assigned: steveoliver » Unassigned
Status: Closed (won't fix) » Active

Nevermind, I'll just change the direction of this issue, and update the summary. :)

sun’s picture

I'm pretty much completely against this current/revised proposal.

drupal_common_theme() defines a range of base theme hook implementations, which are essentially owned by the theme system itself.

A (secondary) reason for #362889: Move drupal_common_theme() from common.inc into theme.inc was to make the definitions available where the implementations are. If we'd move drupal_common_theme() into system.module, then we should move all of the implementations along with it. And that's the point where I disagree.

Instead, I'd feel much more comfortable with hard-coding a special /core/[includes/]templates path into drupal_find_theme_templates() and wherever else it is needed (but I'm fairly sure that's the only point).

In the end, our ultimate goal should be to eliminate the System module dependency from the Theme system entirely. Right now, the only dependency that exists is the system_theme() hook implementation (which calls back into theme.inc).

If possible, I'd encourage you to explore that first. Off-hand, it doesn't sound hard to me.

catch’s picture

Right now, the only dependency that exists is the system_theme() hook implementation (which calls back into theme.inc).

That's not true though. The theme system depends on the theme registry in order to execute any template at all, and that depends on hook implementations. If we move all this crap into system module itself at least it makes the dependency explicit.

c4rl’s picture

Issue tags: +theme system cleanup

drupal_common_theme() defines a range of base theme hook implementations, which are essentially owned by the theme system itself.

Right now, only drupal_common_theme() is being called by system_theme(). So what are we currently accomplishing?

Please provide a reasonable example of where drupal_common_theme() would be called independently of system.module.

sun’s picture

#1886448: Rewrite the theme registry into a proper service allowed me to introspect the current theme registry/system very recently, and I'm fairly confident that the only "dependency" on System module is the system_theme() hook implementation right now. But that's just a "call-back" into theme.inc, which could equally be hard-coded into the theme registry building (like the ./templates directory).

Fabianx’s picture

So as currently the theme system is dependent on system module, moving templates should not be a big deal, because:

* It is one step anyway to decouple the theme system and the system module anyway right now
* It would be two steps once the templates are move to system/templates/

So we would like to proceed in moving templates to system now, but any issue to decouple system module and theme system would then need to:

a) add the hardcoded dependency to the theme registry
b) move the templates to the hardcoded path

That way we can make progress in the conversion now and a proper solution can be done later - once the patch to decouple that lands.

catch’s picture

The theme system is still very dependent on system module, for example http://api.drupal.org/api/drupal/core%21modules%21system%21system.module...

c4rl’s picture

Status: Active » Postponed

For now, I will be taking care of this in #1898454: system.module - Convert PHPTemplate templates to Twig, so I will consider marking this issue duplicate, or revising this issue such to de-couple the theme system from system.module (if deemed a worthy and necessary pursuit).

sun’s picture

Assigned: Unassigned » sun
Status: Postponed » Needs review
FileSize
2.58 KB

re: #14: Yes, _system_rebuild_theme_data() still performs a few theme system specific tasks, but overall, those are not related to this specific issue here. Also, I'd rather count that function to the extension system than the theme system.

AFAICS, attached patch is all that is needed for this issue.

sun’s picture

Title: Move all theme callbacks in includes to system.module (form.inc, menu.inc, pager.inc, theme.inc, theme.maintenance.inc) » Move base theme system templates into /core/templates

Clarifying issue title.

sun’s picture

Well, I expected a lot, but a green light is even better. :)

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

I like that. Works for me and was also Twig's original idea.

c4rl’s picture

Okay, in this case we'll leave the Twig conversions to their respective issues, so we'll ignore what I said in #15.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/theme.incundefined
@@ -639,6 +639,8 @@ function _theme_build_registry($theme, $base_theme, $theme_engine) {
+    _theme_process_registry($cache, 'theme', 'module', 'system', 'core');
+

@@ -3112,9 +3114,11 @@ function template_preprocess_region(&$variables) {
+ * Implements hook_theme().
+ *
+ * Registers base system theme functions and templates.
  */
-function drupal_common_theme() {
+function theme_theme() {
   return array(

This seems a bit bizarre. Theme is not actually a module. What is _theme_process_registry() doing here? We could use some docs here, at the very least.

jenlampton’s picture

I for one vote for leaving drupal_common_theme in place.

Drupal doing the same old strange things it's always done will be better than making it do something new that's also strange, cause strange + change is harder to learn, people already know the old strange

webchick’s picture

Seems like the thing that makes the most sense here is just expanding system_theme() and trying to remove theme.inc special-casing. If we want to de-couple system module from the theme system, it seems like that's a separate effort, and shouldn't affect this issue, which is needed to unblock Twig conversions.

steveoliver’s picture

+++ b/core/includes/theme.inc
@@ -639,6 +639,8 @@ function _theme_build_registry($theme, $base_theme, $theme_engine) {
   else {
+    _theme_process_registry($cache, 'theme', 'module', 'system', 'core');
+
     foreach (module_implements('theme') as $module) {
       _theme_process_registry($cache, $module, 'module', $module, drupal_get_path('module', $module));

Wouldn't we also have to do the same thing here for menu.inc and the other includes that don't yet but need to use templates?

webchick’s picture

Another thing @jenlampton pointed out is having a core/templates folder is going to be confusing for themers. They get trained to look in there, *not* to look under a module directory, which is what they'll need to do for every single other template file in both core and contrib. Learnability-- :(

sun’s picture

Status: Needs work » Needs review

#21:
The second argument to _theme_process_registry() is clearly documented and does not need any further documentation. Did you read the docs?

Also noteworthy: #1886448: Rewrite the theme registry into a proper service

#22:
If anything is strange, then it is to find the most basic HTML templates in a module called "system". How on earth do you know that they are located in there without ten years of Drupal experience?

I still need to bend my brain, each time I'm looking for one of the default templates that happen to be contained in there. Sense? Zero.

#24:
Nope, the theme system itself just provides some utterly basic theme hooks/functions and templates.

You're right in that some of those are essentially mapping to other core services of the system. However, we already have a dozen of tasks filed against core that ultimately want to remove those bindings, and instead, turn those implementations into generic theme components.

Therefore, those most basic implementations are currently provided by the theme system itself, and they will continue to be provided by the theme system itself. The only difference will be that they're no longer be related or attached to a specific other subsystem.

#25:
See my response to #22. If you think that looking up templates in a magic System module makes any sense, then you happen to know too much about Drupal. Bad habits.

As a result, I'm inclined to move this back to RTBC.

c4rl’s picture

Status: Needs review » Needs work

#26

The second argument to _theme_process_registry() is clearly documented and does not need any further documentation. Did you read the docs?

It is "bizarre" because it is making a hook out of theme_theme(), which seems strange that there would exist a module-less hook.

If you think that looking up templates in a magic System module makes any sense, then you happen to know too much about Drupal. Bad habits.

How does it, in your view, make any more sense to propose theme_theme()? This is creating a hook out of thin air, and is an exception to how our hook system actually works with respect to modules and modules_implements().

I think we can all agree that the long-term solution here is something in which the theme system is decoupled from any specific module. I appreciate sun's persistence on that point. However, for progress in the short-term, myself, Fabianx (in #12), catch (in #3 and #9), and webchick (in #23) all agree moving these to system.module in order to unblock the Twig conversion progress is a more explicit, though temporary, solution.

Is this a perfect solution? No and I acknowledge that. I would advise we err on the side of avoiding exception at present to unblock Twig, then revisit refactoring the decoupling effort later.

sun’s picture

Status: Needs work » Needs review

re: #27:

Let me clear up some facts:

Reality is, hook_theme() is not a hook. It is a callback. That is, because the only similarity that it happens to share with module hooks is the magic function name pattern. This magic callback is invoked for a range of extension types that the vast majority of us are not aware of:

MODULE_theme()
BASE_THEME_ENGINE_theme()
THEME_ENGINE_theme()
BASE_THEME_theme()
THEME_theme()

So the question of #21 inherently boils down to this:

Hold on. What you pass as argument there is a string identifier that is not the name of any registered extension known to Drupal!?!!

Exactly. In case you missed it, that's how the theme registry works for eternity already. Base theme engines and theme engines are extensions that are unknown to Drupal otherwise. These are not hooks, but dynamically composed callback function names.

Consequently:

  1. The second argument denotes the prefix to use for the magic callback function and nothing else.
  2. The theme system itself occupies the 'theme_' namespace in a dozen of other ways already, which makes it impossible for any module, theme engine, or theme to exist in that namespace.
  3. It's therefore only logical for the theme system - as long as it relies on magic procedural callback function names - to use its own namespace prefix in order to register some basic/fundamental theme function/template implementations, which the theme system provides on its own.
  4. As this patch proves, those implementations are truly provided by the theme system itself, and they do not have any dependencies on the artificial "host" of System module. The one and only reason for why they were previously registered in there is that everything in previous versions of Drupal was highly entangled and intertwined, and thus we did not think of the possibility of a theme system/registry that provides base implementations of its own.

That logic is dead simple. Please come up with flaws in that logic.

Furthermore, the magic function arguments are not comparable to any other module hook that exists. I'm trying to clean up this insane madness (among other things) in #1886448: Rewrite the theme registry into a proper service, but this fundamental fact will remain:

EXTENSION_theme() is NOT a hook. It's a magically named callback, and that's it.

The only reason for why it is currently documented as hook_theme() is that this piece of documentation is very very old. Architecturally, it belongs into theme.api.php instead of system.api.php. But as you might know, that file only exists since 2010.

c4rl’s picture

Okay, the explanation in #28 helps me understand the motivations of the patch in #16 better.

we did not think of the possibility of a theme system/registry that provides base implementations of its own.

Very true. And thus, the source of objections in #27. We get used to things. For me, without #28, it is difficult to grok what was achieved in #16 from a big picture perspective.

Unless #28 doesn't resolve objections for others in this issue, I'm ok with RTBC for the sake of forward momentum; I suspect perhaps larger refactoring will take place in a re-roll of #1886448: Rewrite the theme registry into a proper service, the specifics of which I need to catch-up on.

webchick’s picture

I'd still like to see a few comments that captures the essence of what's being argued in #28. Yes, I can (and did) read documentation (please, don't insult me). That documentation doesn't explain why those things are in theme_theme() and not system_theme(), despite the actual elements that they're theming being defined in system_element_info(). I'm willing to bet that this is going to be utterly confusing for people down the line.

sun’s picture

That documentation doesn't explain why those things are in theme_theme() and not system_theme()

Why did you expect them in System module in the first place? Where's the relation?

As we've clearly proven here, there is no relation to System module.

despite the actual elements that they're theming being defined in system_element_info().

I've the impression that you're reverse-engineering some architectural things in the wrong order. To recap:

  1. Element #types and theme hooks are decoupled from each other, by design.
  2. Element types represent a range of higher-level, abstract "components", which do not (necessarily) map 1:1 to theme functions. An element #type may call into multiple different theme functions for itself and for its sub-elements, and it may even stack multiple theme functions onto each other, in order to generate the expected output for such an abstract concept. Element types are exclusively designed for module developers only. A single #type may call into a dozen of theme hooks to generate its output.
  3. #theme hooks, on the other hand, are exclusively designed for themers. Each of them maps 1:1 to a specific theme component (or right now, at least to some specific piece of HTML markup).
  4. There is no hard binding between the two. Any kind of #type can (re-)use each and every theme hook that exists.
  5. Consequently, hook_element_info() is irrelevant and off-topic for this issue.

Please bear with me... That's a 5-minute summary of our current architectural design that's not copied from anywhere, so give or take some details. 5) is the effective result either way though.

c4rl’s picture

I may reroll #16 with a few more comments.

Why did you expect them in System module in the first place? Where's the relation?

Let's be careful here. The word "expect" connotes a sense of history. Our experiences form our expectations. So, some of us "expect" them in the system module because system_theme() has historically been the explicit fulfillment of theme functions in includes, given how drupal_common_theme() was called.

Thus asking:

"Why did you (webchick) expect them in System?"

is different than asking:

"Why would one expect them in System?"

The former directs the question toward a particular individual, who's history and experiences will form what they "expect." The latter assumes no particular individual, nor history, thus focusing on the principle of the matter here. Perhaps the latter is what you meant.

Despite it being "clearly proven here" (here === today) what others in this issue "expect" is predicated on their own experience over many years (my own being that theme callbacks are defined in hook_theme() and hook_theme() appears in modules). I don't mean to bikeshed here, I just want to make sure we can understand principle vs experience.

Given this history, the need for separation between the theme layer and the system module wasn't 100% obvious from a *practical* perspective. However, I do understand the desire to decouple the theme layer from modules, from a * theoretical* perspective. Precedent is a dull architect. I plan to follow-up in the theme registry service issue assist with progress.

thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community

#16 is nice :)

Totally agree with #28. Templates are related to the theme system and Drupal extensions, not tied to modules specifically. core/templates seems like a fair enough place for these things.

The whole patch changes about 4 lines, what extra comments do we want exactly?
We don't normally write up historical breadcrumbs in our documentation so I'm not sure what's supposed to be added here.

I'm cool with the RTBC mentioned in #29. Patch still applies cleanly.

alexpott’s picture

#16: theme.templates.16.patch queued for re-testing.

star-szr’s picture

I think this should be postponed until after Twig conversion, otherwise we'll have to do a ton of rerolling.

thedavidmeister’s picture

Status: Reviewed & tested by the community » Postponed

Just had a quick talk to Cottser in IRC about this. The issue isn't the templates that Sun is moving, it's the new templates being added during the Twig conversion. Apparently there are quite a lot so rather than commit this now it would be easier to wait until the conversion happens and then move everything new that's going to appear in one go than re-roll an existing patch already waiting for review for every one or two of those templates.

I'm going to postpone this, I've added it as an "after conversion" issue here #1757550: [Meta] Convert core theme functions to Twig templates - I hope that works for everyone :)

thedavidmeister’s picture

Issue summary: View changes

Revised summary

sun’s picture

Assigned: sun » joelpittet
Issue summary: View changes
Status: Postponed » Active

After converting tons of theme functions and templates, this makes even more sense today.

@joelpittet: Would you like to pick this up? :-)

joelpittet’s picture

Whoa this is almost turning 1 year old. I read through half of this to get a gist. Will likely need to read through it a few more times in full so I don't say something super stupid(fair warning this will happen regardless).

joelpittet’s picture

Assigned: joelpittet » Unassigned

Sorry for holding on to this issue. I'll unassign and someone else can take this one.

IMO:

  • I do very much like that *most* of those templates are moved to core/templates, less folders to dig through for common templates that don't have their own module to belong to.
  • Finding template files to override is no more helpful in core/templates as it is in core/modules/system/templates. But grouping them in some kind of logical way would help this a bit
  • I'm not fond of the registry search exceptions for "where to look", especially "theme_theme" in the patch
  • If it would help I'd much rather have a module called 'common' then to leave them in 'system'

System module has become a kind of dumping ground for any template that doesn't have a logical module.
From the info file

'Handles general site configuration for administrators.'

So to try to adhere to that description of that module here is the templates we should move.

LEAVE IN SYSTEM
Admin, site config templates.

/modules/system/templates/admin-block-content.html.twig
/modules/system/templates/admin-block.html.twig
/modules/system/templates/admin-page.html.twig
/modules/system/templates/status-report.html.twig
/modules/system/templates/system-admin-index.html.twig
/modules/system/templates/system-themes-page.html.twig
/modules/system/templates/confirm-form.html.twig

REMOVE FROM SYSTEM

/modules/system/templates/block--system-branding-block.html.twig
/modules/system/templates/breadcrumb.html.twig
/modules/system/templates/checkboxes.html.twig
/modules/system/templates/container.html.twig
/modules/system/templates/datetime.html.twig
/modules/system/templates/details.html.twig
/modules/system/templates/dropbutton-wrapper.html.twig
/modules/system/templates/feed-icon.html.twig
/modules/system/templates/field-multiple-value-form.html.twig
/modules/system/templates/field.html.twig
/modules/system/templates/fieldset.html.twig
/modules/system/templates/form-element.html.twig
/modules/system/templates/form.html.twig
/modules/system/templates/html.html.twig
/modules/system/templates/image.html.twig
/modules/system/templates/indentation.html.twig
/modules/system/templates/install-page.html.twig
/modules/system/templates/item-list.html.twig
/modules/system/templates/links.html.twig
/modules/system/templates/maintenance-page.html.twig
/modules/system/templates/page.html.twig
/modules/system/templates/pager.html.twig
/modules/system/templates/progress-bar.html.twig
/modules/system/templates/radios.html.twig
/modules/system/templates/region.html.twig
/modules/system/templates/select.html.twig
/modules/system/templates/status-messages.html.twig
/modules/system/templates/table.html.twig
/modules/system/templates/tablesort-indicator.html.twig
/modules/system/templates/textarea.html.twig
/modules/system/templates/vertical-tabs.html.twig
star-szr’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Active » Postponed

I'm not sure if we consider template locations a BC break or not. Technically you could do {% extends 'core/modules/system/templates/html.html.twig' %} but you shouldn't, {% extends 'html.html.twig' %} or {% extends '@system/html.html.twig' %} are better alternatives.

Based on what we think about that, potentially we could still move templates in a later version of 8.x.x so bumping for now.

dawehner’s picture

I'd totally love to have a smaller system module.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.