Clean-up usage of deprecated list_themes() and _system_rebuild_theme_data() in favor of theme_handler service

Working on this

Just to make sure, we're on the right track, we're talking about a *lot* of this type of change:

 $ git diff
diff --git a/core/includes/config.inc b/core/includes/config.inc
index 8e9ce5e..3742ec1 100644
--- a/core/includes/config.inc
+++ b/core/includes/config.inc
@@ -53,7 +53,7 @@ function ($value) use ($name) {
       }
     );
     $enabled_extensions = array_keys(\Drupal::moduleHandler()->getModuleList());
-    $enabled_extensions += array_keys(array_filter(list_themes(), function ($theme) {return $theme->status;}));
+    $enabled_extensions += array_keys(array_filter(\Drupal::service('theme_handler')->listInfo(), function ($theme) {return $theme->status;}));

     $other_module_config = array_filter($other_module_config, function ($config_name) use ($enabled_extensions) {
       $provider = Unicode::substr($config_name, 0, strpos($config_name, '.'));

Should we have one massive patch for this? Or handle modules individually? And as Gábor mentions, individual modules may have a lot more things that need to be changed like config_translation which has code just to get around this.

Here is a patch replacing all the depreciated procedural calls with calls to the service. Someone with a bit more understanding of things can look at what can be simplified because of this.

Wow, quite a massive patch, nice work!
There's a detailed review below, which basically boils down to a few things:

• The changes in the test files are fine.
• In almost all of the other classes the theme handler should be injected. I've given specific instructions in all cases, but they're not very elaborate so don't hesitate to ask if you're having problems with this!!!
• When referencing the functions in documentation ThemeHandlerInterface should be referenced instead of \Drupal::service('theme_handler'). In @see (and other PhpDoc statements) the full namespace should be specified as well, i.e. \Drupal\Core\Extension\ThemeHandlerInterface
• At some places we can save a tiny bit of performance by saving $theme_handler into a local variable instead of calling \Drupal::service('theme_handler') multiple times

If this is overwhelming (which I could definitely understand!!!) you could also avoid all the injection stuff for now and limit this issue to the changes in the test files and then let the rest be handled in a separate issue. That's your call. :-)

Follow-up issues that should be created:

• Inject all the things in DisplayPluginBase (something like this might already exist)
• Make ThemeHandlerInterface::enable()/disable()/reset() chainable.

1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -385,7 +385,7 @@ protected function build() {
   -   *   The loaded $theme object as returned from list_themes().
   +   *   The loaded $theme object as returned from \Drupal::service('theme_handler')->listInfo().
   

   This does not wrap at 80 characters, but more importantly this should say "ThemeHandlerInterface::listInfo()" instead of \Drupal::service...

2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -576,12 +576,12 @@ protected function getPath($module) {
      /**
   -   * Wraps list_themes().
   +   * Wraps \Drupal::service('theme_handler')->listInfo().
       *
       * @return array
       */
      protected function listThemes() {
   -    return list_themes();
   +    return \Drupal::service('theme_handler')->listInfo();
      }
   

   Instead of this change, the theme handler should be injected (see __construct() in this class and the core.services.yml file)
   and any usage of listThemes() in this class should be replaced by $this->themeHandler->listInfo() directly.

3. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.php
   @@ -102,7 +102,7 @@ public function addForm(CustomBlockTypeInterface $custom_block_type, Request $re
   -    if (($theme = $request->query->get('theme')) && in_array($theme, array_keys(list_themes()))) {
   +    if (($theme = $request->query->get('theme')) && in_array($theme, array_keys(\Drupal::service('theme_handler')->listInfo()))) {
   

   The theme handler should be injected instead (see the __construct() and create() methods in this class).

4. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
   @@ -236,7 +236,7 @@ public function form(array $form, array &$form_state) {
   -      foreach (list_themes() as $theme_name => $theme_info) {
   +      foreach (\Drupal::service('theme_handler')->listInfo() as $theme_name => $theme_info) {
   

   The theme handler should be injected here. (__contstruct() and create()).

5. +++ b/core/modules/block/lib/Drupal/block/Controller/BlockController.php
   @@ -26,7 +26,7 @@ class BlockController extends ControllerBase {
   -    $themes = list_themes();
   +    $themes = \Drupal::service('theme_handler')->listInfo();
   

   BlockController doesn't use ContainerInjectionInterface yet, so I think it's fine to leave this as is.

   For extra credit, though, you could inject this as well. (Look at how CustomBlockController does it for guidance.)

6. +++ b/core/modules/block/lib/Drupal/block/Plugin/Derivative/ThemeLocalTask.php
   @@ -47,7 +47,7 @@ public static function create(ContainerInterface $container, $base_plugin_id) {
   -    foreach (list_themes() as $theme_name => $theme) {
   +    foreach (\Drupal::service('theme_handler')->listInfo() as $theme_name => $theme) {
   

   This should be injected (__construct() and create()).

7. +++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigMapperManager.php
   @@ -89,13 +89,20 @@ public function __construct(CacheBackendInterface $cache_backend, LanguageManage
       * @return array
       *   An associative array of the currently available themes. The keys are the
   -   *   themes' machine names and the values are objects. See list_themes() for
   +   *   themes' machine names and the values are objects. See ThemeHandler::listInfo() for
       *   documentation on those objects.
       *
       * @todo Remove this once https://drupal.org/node/2109287 is fixed in core.
       */
      protected function getThemeList($refresh = FALSE) {
   -    return list_themes($refresh);
   +    $theme_handler = \Drupal::service('theme_handler');
   +
   +    if ($refresh) {
   +      $theme_handler->reset();
   +      system_list_reset();
   +    }
   +
   +    return $theme_handler->listInfo();
      }
   

   Per the todo, this function should be removed, and instead a) the theme handler should be injected (see the config_translation.services.yml) and all usages of getThemeList() should be replaced with direct calls to the theme handler. Note: ConfigMapperManagerTest will have to be updated for the constructor change.

8. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Controller/ConfigTranslationBlockListController.php
   @@ -29,7 +29,8 @@ class ConfigTranslationBlockListController extends ConfigTranslationEntityListCo
   -    $this->themes = list_themes();
   +    $this->themes = \Drupal::service('theme_handler')->listInfo
   +    ();
   

   Weird wrapping here.

   More importantly, this should be injected as well. Here you have to actually use createInstance() (instead of the usual create()). You can look at how EntityListController does this.

9. +++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
   @@ -63,7 +63,7 @@ public function disable(Request $request) {
   -      $themes = list_themes();
   +      $themes = \Drupal::service('theme_handler')->listInfo();
   
   @@ -72,7 +72,7 @@ public function disable(Request $request) {
   -          theme_disable(array($theme));
   +          \Drupal::service('theme_handler')->disable(array($theme));
   
   @@ -104,11 +104,16 @@ public function enable(Request $request) {
   -      $themes = list_themes(TRUE);
   +      $theme_handler = \Drupal::service('theme_handler');
   +
   +      $theme_handler->reset();
   +      system_list_reset();
   +
   +      $themes = $theme_handler->listInfo();
   ...
   -        theme_enable(array($theme));
   +        \Drupal::service('theme_handler')->enable(array($theme));
   

   Here the theme handler should be injected (__construct() / create()).

   Also, the system_list_reset() should not be necessary, $this->themeHandler->reset() already takes care of that.

10. +++ b/core/modules/system/lib/Drupal/system/Form/ThemeSettingsForm.php
    @@ -71,7 +71,7 @@ public function getFormId() {
    -    $themes = list_themes();
    +    $themes = \Drupal::service('theme_handler')->listInfo();
    
    @@ -82,7 +82,7 @@ public function buildForm(array $form, array &$form_state, $theme = '') {
    -      $themes = list_themes();
    +      $themes = \Drupal::service('theme_handler')->listInfo();
    

    This should be injected with the usual __construct() / create().

11. +++ b/core/modules/system/lib/Drupal/system/Plugin/Derivative/ThemeLocalTask.php
    @@ -18,7 +18,7 @@ class ThemeLocalTask extends DerivativeBase {
    -    foreach (list_themes() as $theme_name => $theme) {
    +    foreach (\Drupal::service('theme_handler')->listInfo() as $theme_name => $theme) {
    

    Since this is a plugin (derivative) I don't think we can get by without injecting the theme handler. Look at other local task derivatives on how they do that, e.g. FieldUiLocalTask.

12. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/EntityFilteringThemeTest.php
    @@ -81,8 +81,8 @@ function setUp() {
    -    $this->themes = array_keys(list_themes());
    -    theme_enable($this->themes);
    +    $this->themes = array_keys(\Drupal::service('theme_handler')->listInfo());
    +    \Drupal::service('theme_handler')->enable($this->themes);
    

    We could avoid getting the service twice by doing $theme_handler = \Drupal::service('theme_handler') once and then using the local variable.

13. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
    @@ -188,13 +188,13 @@ function testFunctionOverride() {
    -    // Check if drupal_theme_access() retrieves enabled themes properly from list_themes().
    ...
    +    // Check if drupal_theme_access() retrieves enabled themes properly from listInfo().
    

    This does not wrap properly. Also, instead of just saying listInfo(), we should probably be specific and say ThemeHandlerInterface::listInfo().

14. +++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTest.php
    @@ -188,13 +188,13 @@ function testFunctionOverride() {
    -    // Check if list_themes() returns disabled themes.
    +    // Check if \Drupal::service('theme_handler')->listInfo() returns disabled themes.
    

    Again, this doesn't wrap at 80 characters and should say ThemeHandlerInterface::listInfo() instead of \Drupal::....

15. +++ b/core/modules/system/system.admin.inc
    @@ -192,13 +192,13 @@ function system_theme_default() {
    -    $themes = list_themes();
    +    $themes = \Drupal::service('theme_handler')->listInfo();
    ...
    -       theme_enable(array($theme));
    +       \Drupal::service('theme_handler')->enable(array($theme));
    

    We could store $theme_handler in a local variable here to avoid calling \Drupal::service() twice.

16. +++ b/core/modules/system/system.admin.inc
    @@ -206,9 +206,9 @@ function system_theme_default() {
    -      // theme_enable(). However, modules must know the current default theme in
    +      // \Drupal::service('theme_handler')->enable(). However, modules must know the current default theme in
    ...
    -      // implementations, and doing the variable_set() before the theme_enable()
    +      // implementations, and doing the variable_set() before the \Drupal::service('theme_handler')->enable()
    

    Again, this should say ThemeHandlerInterface::enable() and wrap at 80 characters.

17. +++ b/core/modules/system/system.install
    @@ -546,7 +546,7 @@ function system_requirements($phase) {
    -  // Enable and set the default theme. Can't use theme_enable() this early in
    +  // Enable and set the default theme. Can't use \Drupal::service('theme_handler')->enable() this early in
    

    This should say ThemeHandlerInterface::enable() and wrap at 80 characters.

18. +++ b/core/modules/system/theme.api.php
    @@ -224,7 +224,7 @@ function hook_theme_suggestions_HOOK_alter(array &$suggestions, array $variables
    - * @see theme_enable()
    + * @see \Drupal::service('theme_handler')->enable()
    

    Same as above, but here we need the full namespace: \Drupal\Core\Extension\ThemeHandlerInterface::enable()

19. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.php
    @@ -1765,7 +1765,7 @@ public function buildOptionsForm(&$form, &$form_state) {
    -          $themes = list_themes();
    +          $themes = \Drupal::service('theme_handler')->listInfo();
    
    @@ -1826,7 +1826,7 @@ public function buildOptionsForm(&$form, &$form_state) {
    -        foreach (list_themes() as $key => $theme) {
    +        foreach (\Drupal::service('theme_handler')->listInfo() as $key => $theme) {
    

    This should *not* be injected as there's currently a lot of uninjected stuff in DisplayPluginBase and that should be a separate issue. In theory we could store $theme_handler in a local variable here, but that will make it slightly harder when actually injecting stuff. So, I don't really care, but I would leave it as is.

20. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Form/BasicSettingsForm.php
    @@ -29,7 +29,7 @@ public function buildForm(array $form, array &$form_state) {
    -    foreach (list_themes() as $name => $theme) {
    +    foreach (\Drupal::service('theme_handler')->listInfo() as $name => $theme) {
    

    Here I personally find injection less important, but for extra credit you could use ContainerInjectionInterface as explained above.

21. +++ b/core/profiles/standard/standard.install
    @@ -18,8 +18,8 @@ function standard_install() {
    -  theme_enable(array($default_theme));
    -  theme_disable(array('stark'));
    +  \Drupal::service('theme_handler')->enable(array($default_theme));
    +  \Drupal::service('theme_handler')->disable(array('stark'));
    
    @@ -79,7 +79,7 @@ function standard_install() {
    -  theme_enable(array('seven'));
    +  \Drupal::service('theme_handler')->enable(array('seven'));
    

    Here we can store the theme handler in a local variable to save some cycles.

    In theory we should really make enable() and disable() chainable but that's a different issue.

Also Re #2: Let's open a follow-up issue to create a listEnabledInfo() (or similar) method on the theme handler that would encapsulate that code.

I will look at the comments and update the patch. This was my project at today's code sprint in Boston. I will need a bit of study/instruction on the "injection" comments.

One question, would it be better to split the patch into a patch per core module being updated, so perhaps some can be updated quicker, or should I keep it all as one big patch for the full issue?

Here is a patch with much of the fixes done. I haven't actually implemented full dependency injection into the classes, but have added the class member to hold the handler, and am initilizing it in the constructor. This sets things up for making the changes to implement the injection.

There is also one test that failed last time, apparently due to the change breaking an ad-hoc mocking of list_themes, which will need to be changed into a real mock.

I am tempted to break this into two patches, one for what is basically completed now, and that which still needs work, so that the issue has a big bite taken out of it for now.

The last patch passed Simple test for me locally, but now this needs a re-roll.

I'll look into re-rolling the patch this week.

I will try to reroll the patch since it's been there for few weeks. I am working on this through core mentoring hours.

I reroll the patch and uploaded the latest patch.

I reroll the patch again, the patch in #15 has some test functions which are removed from the latest code. Please review the latest patch attached to this comment.

17: deprecated-2151469-17.patch queued for re-testing.

17: deprecated-2151469-17.patch queued for re-testing.

WIP.

Am hoping for at least a test run.

another quick try.

I don't know why the test run failed. None of the actual tests failed. We just had a random error. I'm trying just the changes on the block module.

Does anyone knows why all tests pass, but I still get this error in Simpletest: PHP Fatal error: Call to a member function get() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal.php on line 138.

Next, Ill try a patch for a different module, not block.

This includes the changes for only some of the modules, just to see if it passes simpletest.

just the color module changes?

Redo of #33. That patch had unintended changes in it. If this passes, the problem is most likely in the block module code changes.

This doesn't currently have a patch to review.

I think the basics of this could be handled by #2208607: Write script to automate replacement of deprecated functions where possible, and then a followup patch here for documentation and dependency injection.

Summary of the last few patches: I couldn't figure out which change is causing the test to break. A patch with the changes that contains just the block modules changes failed Simpletest. Another patch with the changes for 6 other modules passed.

So, I still don't know why the changes in block module are not passing. Here is another test to see if it has anything to do with the protected themehandler variable.

protected themehandler was misued. Removal of that code passeed in #38.

Here the complete reroll: patch from #28 + removal of the protected $themehandler local variable (as passed in patch from #38).

So this is a patch with all of the $themehandler local variables removed.

We've got a better patch, so removing the link to the automated script issue.

Recommend implementing dependency injection in a followup: #2221801: Use Dependency Injection for theme handler. as suggested by tstoeckler in #5.

Maybe someone that understand the task from comment #6 can create an issue for that too.

Adding two related issues that are significantly changing the affected code.

Some of the suggested conversions in this issue/patch are probably trivial, but it would probably be a good idea to carefully review all of them to see whether it makes sense to convert them here.

42: 2151469-42.patch queued for re-testing.

1. +++ b/core/modules/block/src/BlockForm.php
   @@ -90,7 +90,7 @@ public function form(array $form, FormStateInterface $form_state) {
          $theme_options = array();
   -      foreach (list_themes() as $theme_name => $theme_info) {
   +      foreach (\Drupal::service('theme_handler')->listInfo() as $theme_name => $theme_info) {
            if (!empty($theme_info->status)) {
   

   Inject service on the class instead.

2. +++ b/core/modules/block_content/src/Controller/BlockContentController.php
   @@ -90,7 +90,7 @@ public function addForm(BlockContentTypeInterface $block_content_type, Request $
   -    if (($theme = $request->query->get('theme')) && in_array($theme, array_keys(list_themes()))) {
   +    if (($theme = $request->query->get('theme')) && in_array($theme, array_keys(\Drupal::service('theme_handler')->listInfo()))) {
   

   Inject service...

3. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -69,7 +69,7 @@ public function getFormId() {
   +    $themes = \Drupal::service('theme_handler')->listInfo();
   

   Inject service...

4. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -80,7 +80,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
   -      $themes = list_themes();
   +      $themes = \Drupal::service('theme_handler')->listInfo();
          $features = $themes[$theme]->info['features'];
   

   Inject service...

5. +++ b/core/modules/views_ui/src/Form/BasicSettingsForm.php
   @@ -30,7 +30,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    foreach (\Drupal::service('theme_handler')->listInfo() as $name => $theme) {
   

   Inject service...

6. +++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
   @@ -195,17 +195,22 @@ function testFunctionOverride() {
   +    // Check if ThemeHandlerIntface::listInfo() returns disabled themes.
   

   Typo

Created straight reroll.

Addressed all changes as suggested by rpayanm. Patch attached!

Note to self: Review patches before submitting them...

Updated BlockFormTest + replaced ThemeHandler with ThemeHandlerInterface in __construct.

Removed the remaining list_themes() references + also found and removed a reference to _system_rebuild_theme_data().

+++ b/core/includes/theme.inc
@@ -351,8 +351,10 @@ function theme_get_setting($setting_name, $theme = NULL) {
+    if ($theme && isset($themes[$theme])) {

When would !$theme occur, this looks like a logic change, not just a cleanup.

Otherwise looks good.

Good spot. That must have been a reroll mistake - the line was changed the other way round a few weeks ago. They are effectively identical anyway.

This patch is #61 without that line changed.

rerolled...

good reroll, back to rtbc

Tests pass again, so RTBC

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -180,7 +180,7 @@ protected function init($theme_name = NULL) {
-      $themes = $this->listThemes();
+      $themes = \Drupal::service('theme_handler')->listInfo();

+++ b/core/lib/Drupal/Core/Theme/ThemeAccessCheck.php
@@ -39,7 +39,7 @@ public function access($theme) {
-    $themes = list_themes();
+    $themes = \Drupal::service('theme_handler')->listInfo();

can't we inject the service here?

oppss sorry :)

ummmm.

Test...

fighting against the Tests...

Yeah :D
Interdiff #71 - #80

@rpayanm, nice work. Thank you! What about the one in ThemeAccessCheck? Here are a couple minor coding standards/docs things that can be fixed up:

1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -149,13 +157,16 @@ class Registry implements DestructableInterface {
   +   * @param $theme_handler
   +   *   The theme handler.
   

   I think this @param should have the data type.

2. +++ b/core/modules/block_content/src/Controller/BlockContentController.php
   @@ -48,10 +57,13 @@ public static function create(ContainerInterface $container) {
       * @param \Drupal\Core\Entity\EntityStorageInterface $block_content_type_storage
       *   The custom block type storage.
   +   * @param \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler
   +   * The theme handler.
   

   The description for this param needs to be indented by 2 spaces.

Next, ThemeAccessCheck...

Trying with ThemeAccessCheck...

#85 is #66 plus the injection that alexpott asked for, so RTBC.

+++ b/core/lib/Drupal/Core/Theme/ThemeAccessCheck.php
@@ -16,6 +17,22 @@
   /**
+   * The theme handler.
+   *
+   * @var \Drupal\Core\Extension\ThemeHandlerInterface
+   */
+  protected $themeHandler;
+
+  /**
+   * Constructs a \Drupal\Core\Theme\Registry object.
+   *
+   * @param \Drupal\Core\Extension\ThemeHandlerInterface $theme_handler
+   *   The theme handler.
+   */
+  public function __construct(ThemeHandlerInterface $theme_handler) {
+    $this->themeHandler = $theme_handler;
+  }

@@ -39,7 +56,7 @@ public function access($theme) {
   public function checkAccess($theme) {
-    $themes = list_themes();
+    $themes = \Drupal::service('theme_handler')->listInfo();
     return !empty($themes[$theme]->status);

Now we've injected it we need to actually use it :) $this->themeHandler instead of \Drupal::service(...

Addressed feedback of alexpott in 87.

Go testbot!

Go testbot!

This issue is a prioritized change (removing deprecated function use) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 88a6bbc and pushed to 8.0.x. Thanks!

drupal_theme_access() is still is used.

bash-3.2$ grep drupal_theme_access -nr *
core/includes/theme.inc:88:function drupal_theme_access($theme) {
core/modules/system/src/Tests/Theme/ThemeTest.php:211:    // Check if drupal_theme_access() retrieves enabled themes properly from
core/modules/system/src/Tests/Theme/ThemeTest.php:213:    $this->assertTrue(drupal_theme_access('test_theme'), 'Installed theme detected');
core/modules/system/src/Tests/Theme/ThemeTest.php:215:    $this->assertTrue(drupal_theme_access('test_theme'), 'Enabled theme detected');
core/modules/system/system.module:321:  return \Drupal::currentUser()->hasPermission('administer themes') && drupal_theme_access($theme);
bash-3.2$ 

Created https://www.drupal.org/node/2151469 to get rid of this.