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/.
Comment | File | Size | Author |
---|---|---|---|
#16 | theme.templates.16.patch | 2.58 KB | sun |
Comments
Comment #1
steveoliver CreditAttribution: steveoliver commentedMaybe this just needs to be done in _theme_build_registry ?
Comment #2
steveoliver CreditAttribution: steveoliver commentedNot 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.
Comment #3
catchWhen 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.
Comment #4
steveoliver CreditAttribution: steveoliver commentedSounds good, catch. Closing this.
Comment #5
steveoliver CreditAttribution: steveoliver commentedComment #6
c4rl CreditAttribution: c4rl commentedWould 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?
Comment #7
c4rl CreditAttribution: c4rl commentedNevermind, I'll just change the direction of this issue, and update the summary. :)
Comment #8
sunI'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 intodrupal_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.
Comment #9
catchThat'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.
Comment #10
c4rl CreditAttribution: c4rl commentedRight 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.
Comment #11
sun#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).Comment #12
Fabianx CreditAttribution: Fabianx commentedSo 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.
Comment #14
catchThe theme system is still very dependent on system module, for example http://api.drupal.org/api/drupal/core%21modules%21system%21system.module...
Comment #15
c4rl CreditAttribution: c4rl commentedFor 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).
Comment #16
sunre: #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.
Comment #17
sunClarifying issue title.
Comment #18
sunWell, I expected a lot, but a green light is even better. :)
Comment #19
Fabianx CreditAttribution: Fabianx commentedI like that. Works for me and was also Twig's original idea.
Comment #20
c4rl CreditAttribution: c4rl commentedOkay, in this case we'll leave the Twig conversions to their respective issues, so we'll ignore what I said in #15.
Comment #21
webchickThis 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.
Comment #22
jenlamptonI 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
Comment #23
webchickSeems 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.
Comment #24
steveoliver CreditAttribution: steveoliver commentedWouldn'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?
Comment #25
webchickAnother 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-- :(
Comment #26
sun#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.
Comment #27
c4rl CreditAttribution: c4rl commented#26
It is "bizarre" because it is making a hook out of theme_theme(), which seems strange that there would exist a module-less hook.
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.
Comment #28
sunre: #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:
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:
'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.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.Comment #29
c4rl CreditAttribution: c4rl commentedOkay, the explanation in #28 helps me understand the motivations of the patch in #16 better.
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.
Comment #30
webchickI'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.
Comment #31
sunWhy 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.
I've the impression that you're reverse-engineering some architectural things in the wrong order. To recap:
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.
Comment #32
c4rl CreditAttribution: c4rl commentedI may reroll #16 with a few more comments.
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:
is different than asking:
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.
Comment #33
thedavidmeister CreditAttribution: thedavidmeister commented#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.
Comment #34
alexpott#16: theme.templates.16.patch queued for re-testing.
Comment #35
star-szrI think this should be postponed until after Twig conversion, otherwise we'll have to do a ton of rerolling.
Comment #36
thedavidmeister CreditAttribution: thedavidmeister commentedJust 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 :)
Comment #36.0
thedavidmeister CreditAttribution: thedavidmeister commentedRevised summary
Comment #37
sunAfter converting tons of theme functions and templates, this makes even more sense today.
@joelpittet: Would you like to pick this up? :-)
Comment #38
joelpittetWhoa 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).
Comment #39
joelpittetSorry for holding on to this issue. I'll unassign and someone else can take this one.
IMO:
System module has become a kind of dumping ground for any template that doesn't have a logical module.
From the info file
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.
REMOVE FROM SYSTEM
Comment #40
star-szrI'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.
Comment #41
dawehnerI'd totally love to have a smaller system module.