Properly deprecate theme functions for Drupal 10

Discussed with @catch and @alexpott on Slack since this has some security concerns which creates some urgency around this. The main problem is that theme functions are lacking auto escaping. This makes the developer experience potentially confusing since developers will have to remember when to worry about escaping.

To estimate the disruption, we looked deeper into the the usages of theme functions. A lot of the usages are in modules without any Drupal 8 releases. Some of the modules have releases but theme functions aren't actually called anywhere.

If we want to move forward with this prior to Drupal 9, we should add a proper deprecation in a future release of Drupal 8.8.x so that people can have as much time to remove usages of theme functions as possible. I'm tagging this for release manager review so they can make a final decision on this.

I did spot checks on a few modules from that grep.

For example xmlsitemap defines a theme function for an admin page, but it's dead code and never used.

Search API also defines a theme function for an admin page, but it is still used.

Several other modules don't have 8.x releases yet (i.e. abandoned 8.x branches with no release on the project page and no recent commits).

So if we deprecate in 8.8.x properly for 9.x we'll force (my guess) 5-10 maintained modules with some usage to do extra work to be 9.x compatible. This is not huge but it is more than nothing given 8.8.0 is already out.

If we don't deprecate for 9.x, I think we should still deprecate in 8.9.x for Drupal 10 to encourage people to actually finish porting from Drupal 7 / security harden, and because it's actually been deprecated for years now just without our full deprecation framework implemented for it.

There could be some themes customizing those theme functions too. It's likely that if a module ships with a theme function, that the override is also a theme function. It seems like that the Search API theme function isn't something that people would override often, but it might be a good idea to look into some of the other modules to try to ensure there aren't any instances that would be commonly overridden by people.

What are the security concerns? Or is there a private issue where we can or should be discussing?

Let's start working on a patch for this now; we can make the decision about whether to backport a deprecation to 8.8.x for 9.0.x removal vs. backporting for 10.0.x removal once we see what the impacts of the deprecation would be. After chatting with @catch it sounds like the deprecation might be a little involved, since we're not deprecating just a function or method; we're deprecating the ability to create a kind of API.

Meanwhile we should also detect them directly in Upgrade Status I think, because I think a lot of themes and theme functions don't have test coverage in contrib. Let's file an issue for that too.

Second part of #10 sounds like a good plan to me.

I'm beginning a review of the patch in #21 and noticed the issue summary is a little out of sync regarding version numbers. Tagging to "needs issue summary update" (would typically just update it myself but there's just enough potential to add something incorrect that I'd prefer someone who has been active in the issue )

@bnjmnm so it's inconsistent because we're a bit undecided. But I think it makes sense to write a patch for 9.0.x removal and the version numbers are the easiest thing to change if we end up deciding to deprecate for 10.0.x instead.

@bnjmnm so it's inconsistent because we're a bit undecided. But I think it makes sense to write a patch for 9.0.x removal and the version numbers are the easiest thing to change if we end up deciding to deprecate for 10.0.x instead.

Makes sense regarding the issue summary, removing the tag since the need to update isn't certain until this exits undecided status.

I plan to do a full review but providing a partial one for now as I may not be able to resume until tomorrow.

1. +++ b/core/modules/system/tests/modules/theme_deprecated_test/src/ThemeTestController.php
   @@ -0,0 +1,38 @@
   + * Controller routines for deprecated theme test routes.
   

   +++ b/core/modules/system/tests/src/Functional/Theme/DeprecatedThemeSuggestionsAlterTest.php
   @@ -0,0 +1,73 @@
   + * Tests deprecated theme suggestion alter hooks.
   

   These could be misinterpreted as the routes/hooks themselves being deprecated. Could these comments be rephrased? (may be additional instances of this in the patch)

2. +++ b/core/modules/system/tests/modules/theme_deprecated_suggestions_test/theme_deprecated_suggestions_test.info.yml
   @@ -0,0 +1,5 @@
   +name: 'Theme suggestions test'
   +type: module
   +description: 'Support module for testing theme suggestions.'
   +package: Testing
   +version: VERSION
   

   Could this be updated to reflect that it's specifically testing suggestions that make use of deprecated theme functions?

3. Although it should be pretty evident that theme_deprecated_test and test_deprecated_theme will require removal when theme functions are removed, including a @todo to (I think?) #3097889: Remove deprecated theme functions will make this more transparent.

4. +++ b/core/modules/system/tests/modules/theme_deprecated_suggestions_test/theme_deprecated_suggestions_test.module
   @@ -0,0 +1,32 @@
   +    'variables' => [],
   

   'variables' isn't needed.

5. +++ b/core/modules/system/tests/themes/test_theme/templates/theme-test--suggestion.html.twig
   index a5af83bc87..109ff44d22 100644
   --- a/core/modules/system/tests/themes/test_theme/test_theme.theme
   
   --- a/core/modules/system/tests/themes/test_theme/test_theme.theme
   +++ b/core/modules/system/tests/themes/test_theme/test_theme.theme
   

   There's a theme function test_theme_theme_test_function_suggestions__module_override still in this file that should be moved, or if it can't there should be a comment explaining why and a @todo to remove when theme functions are removed.

6. +++ b/core/modules/system/tests/modules/common_test/common_test.module
   @@ -123,25 +123,10 @@ function common_test_theme() {
        'common_test_empty' => [
          'variables' => ['foo' => 'foo'],
   -      'function' => 'theme_common_test_empty',
        ],
   

   common_test_empty and the accompanying template can be removed entirely. I had my suspicions but didn't want to suggest until I confirmed this by creating a test patch and drupalci was fine with it.

Here's the rest of the review. Looks like pretty minor stuff.
Tried to avoid too much scrutiny on code that was clearly just moved and not original to this patch - my goal there was to confirm the right things were properly moved to the right place as opposed to relitigating tests that will be removed shortly(?). #26.4 was an exception to this.

1. +++ b/core/modules/system/tests/src/Functional/Theme/ThemeTest.php
   @@ -143,8 +143,12 @@ public function testTemplateOverride() {
      /**
       * Ensures a theme template can override a theme function.
   +   *
   +   * @group legacy
       */
      public function testFunctionOverride() {
   +    \Drupal::service('module_installer')->install(['theme_deprecated_test']);
   +
   

   Although it should be pretty clear what is going on when theme_deprecated_test is removed, adding a @todo to the issue where that will occur will make things clearer.
   Would be helpful to do this in the unit and kernel RegistryTest as well.

2. Several other tests with @group legacy were renamed (testName)LegacyTest. That naming approach may help mitigate some of the confusion with the use of "deprecated" I mentioned earlier.

#28 is looking good! One additional thing came to mind - there should be test coverage for the deprecation itself . A test similar to \Drupal\Tests\Core\Asset\LibraryDiscoveryTest::testAssetLibraryDeprecation would provide the necessary coverage.

I also answered my own question in #30; elsewhere we mark deprecations as "in drupal:8.0.0" even if the actual deprecation was added much later

Yes it should be the version that the functionality was deprecated rather than the version we added the deprecation notice usually - as if we'd had a fully-worked-out deprecation process at the time.

All my feedback from #31 and #32 is nicely addressed. I did an additional round of manual test runs where I intentionally changed and removed things to confirm the tests work and that no unnecessary code is present and it was in good shape.

The patch itself seems RTBC worthy at this point but it seems like it would be best to hold off until this gets release manager review/policy specifics. If I'm incorrect on that rationale let me know and I'll change the status.

So IMO removing support for this in 9.x is still a good idea - theme functions can't use Twig autoescape, which was why we deprecated them in 8.x in the first place.

We have a change record for this from 2015: https://www.drupal.org/node/2575445

Tagging with Drupal 9.0.0-beta1 requirement so this can be visible in a search vs just via the parent issue.

The deprecation message looks good. I also checked that no test coverage is lost by the change. Besides that, I applied only the change for Drupal\Core\Theme\Registry and ran Drupal\KernelTests\Core\Theme\RegistryTest to ensure the deprecation error is thrown.

It also looks like @catch is feeling positive about this change so I'm moving this to RTBC to wait for a final decision from the release managers.

Just noting based on chat discussion that @catch is awaiting feedback from @xjm to make sure both release managers are on board with the target version of the deprecation especially.

Based on the extent of http://grep.xnddx.ru/search?text=function%20theme_&filename= I'm not totally comfortable deprecating this for removal before 9.0.0. I think it should have been done before 8.8 for it to be a 9.x deprecation.

If we're concerned about the security impact of this (and I am), we could deprecate for 10.0.x, but backport it to 8.9.x or even 8.8.x as an exception to the general "9.1.x" policy, and potentially follow the approach we used for string formatting placeholders and trigger_error() with no @ in 9.0.x.

@catch proposed simply changing the deprecation to 10.0.x here, backporting it to at least 8.9.x, and filing a followup to remove the @ from the trigger_error() in 9.0.x only.

Retitling for clarity.

Just did a find and replace for s/9.0.0/10.0.0/g

Removing tag.

This looks great. Yay for proper deprecation testing and @trigger_error!

Leaving open against 8.8.x as we might commit it there once 8.8.3 is out.

Committed and pushed 4a70ba3774 to 9.0.x and f8870768f5 to 8.9.x. Thanks!

I am afraid this has broken testing on PHPUnit CLI for groups:

$ $DRUPAL_PATH/vendor/bin/phpunit $TEST_ARGS

PHP Fatal error:  Cannot redeclare Drupal\Core\Theme\get_defined_functions() (previously declared in /home/travis/drupal8/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php:503) in /home/travis/drupal8/core/tests/Drupal/Tests/Core/Theme/RegistryLegacyTest.php on line 178

Issue here: #3118477: RegistryTest, RegistryLegacyTest both define the same class, use mock instead

We should probably postpone the backport here on #3118477: RegistryTest, RegistryLegacyTest both define the same class, use mock instead, but the fatal error is only an issue if running tests in a very specific way (i.e. not on DrupalCI, and not running them individually, which are the more common), so no reason to roll this back against 8.9.x/9.0.x IMO. Moving to 'postponed'

@catch and I discussed this again and agreed not to backport the issue to 8.8.x. New deprecations are minor-only as a rule, and this one in particular could be pretty disruptive. It already caused some confusion that was reported in Drupal Slack just from what's already been committed (which led to #3120954: Add function name to the deprecation message about theme functions being filed).

With the 8.9.x deprecation as well as #3117330: Trigger errors in deprecated theme functions, we're still ensuring that contrib and custom modules are going to find out about this soon.

Tagging for a release note and re-opening until we have one.