Deprecate theme functions for removal before Drupal 9 (docs only)

I'm not sure whether we can do anything harder than a deprecation error. If we do something harder later, we should ideally break it now already, given how the promise of SEMV works.

So this is a case where the BC layer is not just a wrapper, but a whole layer of code and concepts that are actively harmful.

In general I'm not a fan of marking things @deprecated while core still uses them, but we should clarify what our default position is on that.

I do think we should trigger_error() or assert() in a minor release after theme functions aren't used in core - i.e. that's something we could do in the 8.1.x branch once it's open.

At a later date we could even disable discovery via a container parameter and require intervention to re-enable it again. That would be a one-line (or ten line perhaps) change for modules that still need it which could be OK in a minor version for something already deprecated for months or years.

The biggest impact this would have is on contrib module developers Outright removal of the ability to use theme functions would be a pretty hard break at this point, but ultimately that's the direction Drupal needs to go. As a themer/front-end developer, I'm very much in favor of easier access to markup, ideally in templates wherever possible. So if we're able to put up some big warning sign to contrib to head in that direction, I think that's good. As for a hard deprecation in a minor release? I know joelpittet has at times downloaded all contrib modules to check how many times a certain thing appears in code, and others have probably done so as well. Maybe we could do a search to see how often contrib is using theme functions in d8. If there are only a few instances, reach out to those module's maintainers. If theme functions are rampant, that's a harder problem. So the sooner we nip this in the bud, the better.

I don't quite understand what we are deprecating. The individual theme functions core is still using (that doesn't sound like something that needs deprecating,) or the ability to implement a theme function?

The latter.

We can also do 5a/b which would allow people to prevent theme functions being discovered on specific installs as security hardening.

Could still then flip the default later.

theme_functions module :P

The latter.

Woah, Nelly.

I guess #10 sounds like plan. We'd definitely need some broad communication about this. I'm not about 5. though. Is that necessary, or do we consider this important enough to break sites?

I don't think we should break existing sites, not really what I meant, so here's a more fleshed out version of #5.

In a minor release, we add a toggle for theme function discovery, or better provide an alternate service with a subclass that doesn't do it.

Otherwise everyone else just gets the E_DEPRECATED in theme registry rebuilding.

Option #1:

- default to no discovery
- add a module that enables discovery again
- for every existing install, we add a hook_update_N() to enable that module.

new installs wouldn't get it though.

So a contrib module or theme could break, a distribution could break, but a site couldn't - unless they disabled the module manually.

Option #2:

- default discovery to on
- have a module that switches it off
- enable that module in minimal/standard profile

Contrib modules and themes could break, sites and distributions would be fine.

Option #3:

- default discovery to on
- have a module that switches it off
- don't enable it by default anywhere

Sites can opt-in to the security hardening and lose the E_DEPRECATED notice.

None of these break sites, they involve different levels of breakage for contrib modules and themes. Allow people to stop executing that code is both a nice security hardening and small (once the theme registry 90% speed up patch lands) performance improvement so even if we only do it as an addition rather than a change, it's worth looking at ways to do it.

#1

new installs wouldn't get it though.

New sites aren't always built with new modules though.

#2

distributions would be fine.

Assuming they update accordingly to turn it off if they need to, particularly in cases where they don't provide their own copy of core now.

#3 sounds reasonable and the least risky to me. It is a nice security enhancement, and leaving it optional means limited chance of breaks while giving anyone who cares the ability to do it. Does it need to be a module, or can we turn off the discovery with a setting?

My huge +1 for #1. Contribs can add the module as dependency. In order to make that work properly with #2 & #3 modules would need to write some special logic. #1 makes it easy to see which modules are requiring deprecated code. It would be also very clear message for everyone: "DO NOT USE THEME FUNCTIONS!".

In any case the trigger_error/assert would hopefully mostly do the trick as it will make their tests go red :P

Well, trigger_error() would allow people to still use it, you can workaround that. asserts() would be too harsh and well semantically wrong. trigger_error() talks about deprecation, which matches perfectly here.

The problem with E_USER_DEPRECATED is you have to explicitly lower the error reporting level to stop getting it which is not really desirable.

Or stop using theme functions.

The problem with E_USER_DEPRECATED is you have to explicitly lower the error reporting level to stop getting it which is not really desirable.

If you are stupid you are stupid, there is no way around that. assert() is IMHO a tool for a total different usecase.

distributions ship with specific versions of core

Not necessarily. It depends on the use case. There are many different ways of building out a distribution. The things you can download from drupal.org are not the only things we have to account for. Many orgs a have a lot of different internal products built in different ways, and deploy sites in their own ways.

I agree with catch that throwing an error is a good way to keep things working but be annoying enough to incentivize people change.

There's only ever one version of core that's supported - the latest patch release. It's possible we do 8.0.6 and 8.1.0 the same day, then there'd be two for a month.

So distributions with old core versions are in a pretty bad state.

It would be great IMHO to have a follow up which adds the E_USER_DEPRECATED thing.

I think we should focus on docs here, doing that seems pretty non-controversial.

Then we can reuse #2575081: [policy, no patch] Use E_USER_DEPRECATED in Drupal 8 minor releases for the assert()/trigger_error()/whatever. Cool?

+++ b/core/lib/Drupal/Core/Render/theme.api.php
@@ -68,12 +68,12 @@
+ * functions are deprecated in Drupal 8.x and will be removed before Drupal 9.0.

@@ -98,7 +98,8 @@
+ * vulnerabilities. Theme functions are deprecated in Drupal 8.x and will be
+ * removed before Drupal 9.0. Use Twig templates instead.

@@ -1072,9 +1072,10 @@ function hook_page_bottom(array &$page_bottom) {
+ *   - function: (deprecated in Drupal 8.x, will be removed in Drupal 9.x) If

Shouldn't these be 8.0.x (or 8.x.x) and 9.0.0?

Yes docs here and actual E_DEPRECATED/assert in a follow-up is good.

Thanks, doing some retitling and tagging.

Adding 8.0.x and 9.0.x

+++ b/core/lib/Drupal/Core/Render/theme.api.php
@@ -68,12 +68,12 @@
+ * functions are deprecated in Drupal 8.0.x and will be removed before Drupal 9.0.x.

@@ -1072,9 +1072,10 @@ function hook_page_bottom(array &$page_bottom) {
+ *   - function: (deprecated in Drupal 8.0.x, will be removed in Drupal 9.0.x) If

Now these two lines need to be rewrapped to 80 chars :)

Yes please.

Done.

This looks great. Still needs change record. Would you mind trying your hand at one @snehi?

@joelpittet i did not get you.

Oh so this issue needs a change record to indicate that we are deprecating theme functions. Just needs a draft started. Here are the instructions: https://www.drupal.org/contributor-tasks/draft-change-record

Well it probably needs to be NW for the chang record anyway. You win this one testbot!

I've linked some existing change records - the default to template one, and theme_render_and_autoescape() (which I retitled), since both of these were deprecation in-practice, and we just waited to document that in core until core was done.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x, thanks!