Replace list_themes() with a service.

Plugins using list_themes() as well in their constructor to set up common things like YAML based discovery are not unit testable due to this and require custom wrappers to avoid using list_themes() direct. That is pretty painful. In short +1.

Looks good from skimming over it. I would have expected ThemeHandler handler to live in \Drupal\Core\Extension, however, is there a specific reason for not doing that?

1. +++ b/core/includes/theme.inc
   @@ -766,29 +725,7 @@ function list_themes($refresh = FALSE) {
    function drupal_find_base_themes($themes, $key, $used_keys = array()) {
   

   The $used_keys argument doesn't seem to be needed anymore.

2. +++ b/core/lib/Drupal/Core/Theme/ThemeHandler.php
   @@ -0,0 +1,461 @@
   +   * e.g., to prefix all values of the 'stylesheets' or 'scripts' properties with
   
   +++ b/core/lib/Drupal/Core/Theme/ThemeHandlerInterface.php
   @@ -0,0 +1,107 @@
   +   * Retrieved from the database, if available and the site is not in maintenance
   ...
   +   *     and the complete filepath as value. Not set if no scripts are defined in
   ...
   +   *     the themes' machine names, and the values are the themes' human-readable
   +   *     names. This element is not set if there are no themes on the system that
   ...
   +   *   Returns an array of all of the theme's ancestors; the first element's value
   

   80 chars.

3. +++ b/core/lib/Drupal/Core/Theme/ThemeHandler.php
   @@ -0,0 +1,461 @@
   +  protected function themeInfoPrefixPath($info, $path) {
   

   The docblock of this method is missing typehints for params and return

4. +++ b/core/lib/Drupal/Core/Theme/ThemeHandler.php
   @@ -0,0 +1,461 @@
   +  public function getBaseThemes(array $themes, $theme) {
   +    return $this->doGetBaseThemes($themes, $theme);
   +  }
   +
   +  protected function doGetBaseThemes(array $themes, $theme, $used_themes = array()) {
   

   Why do we need the helper function here?

5. +++ b/core/lib/Drupal/Core/Theme/ThemeHandler.php
   @@ -0,0 +1,461 @@
   +  protected function getSystemListingInfo() {
   ...
   +  protected function resetSystem() {
   ...
   +  protected function systemListReset() {
   ...
   +  protected function clearCssCache() {
   ...
   +  protected function themeRegistryRebuild() {
   ...
   +  protected function systemThemeList() {
   

   Missing docblocks.

6. +++ b/core/lib/Drupal/Core/Theme/ThemeHandlerInterface.php
   @@ -0,0 +1,107 @@
   + * Manages the list of available themes as well as enable/disable them.
   + */
   +interface ThemeHandlerInterface {
   

   I think the interface should have a different description than the class.

The interdiff looks good but I think you uploaded an old version of the patch because it contains

function drupal_find_base_themes($themes, $key, $used_keys = array()) {

and inside it only the $key parameter is actually used..

1. +++ b/core/modules/system/lib/Drupal/system/Tests/Batch/PageTest.php
   @@ -50,6 +50,7 @@ function testBatchProgressPageTheme() {
   +    debug(batch_test_stack());
   

   :)

2. +++ b/core/lib/Drupal/Core/Theme/ThemeHandler.php
   @@ -0,0 +1,485 @@
   +   * e.g., to prefix all values of the 'stylesheets' or 'scripts' properties with
   
   +++ b/core/lib/Drupal/Core/Theme/ThemeHandlerInterface.php
   @@ -0,0 +1,108 @@
   +   *     the themes' machine names, and the values are the themes' human-readable
   +   *     names. This element is not set if there are no themes on the system that
   ...
   +   *   Returns an array of all of the theme's ancestors; the first element's value
   

   Since the patch needs a reroll anyway for that debug(), I found a few lines that need to be rewrapped.

1. +++ b/core/tests/Drupal/Tests/Core/Theme/ThemeHandlerTest.php
   @@ -0,0 +1,421 @@
   +    $this->themeHandler->systemList['bartik'] = (object) array(
   ...
   +    $this->assertEquals($this->themeHandler->systemList['bartik']->info['stylesheets'], $list_info['bartik']->stylesheets);
   +    $this->assertEquals($this->themeHandler->systemList['bartik']->scripts, $list_info['bartik']->scripts);
   ...
   +    $this->themeHandler->systemList['seven'] = (object) array(
   ...
   +    $this->assertEquals($this->themeHandler->systemList['seven']->info['stylesheets'], $list_info['seven']->stylesheets);
   ...
   +    return $this->systemList;
   

   what exactly this property is? i cant figure it out.it is used only in the test. what is the usecase for this:S

2. +++ b/core/tests/Drupal/Tests/Core/Theme/ThemeHandlerTest.php
   @@ -0,0 +1,421 @@
   +  public function testGetBaseThemesWithoutBaseTheme() {
   ...
   +  public function testGetBaseThemesWithNonExistingBaseTheme() {
   ...
   +  public function testGetBaseThemesWithOneBaseTheme() {
   ...
   +  public function testGetBaseThemesWithMultipleBaseThemes() {
   

   how about turning those test methods into one with the help of a data provider?

aaah..ok..i got it now, thanks.

+++ b/core/lib/Drupal/Core/Theme/ThemeHandler.php
@@ -0,0 +1,485 @@
+  protected $list = array();

dont kill me, but i just saw this:/ it needs docs

otherwise its ready to go imo

1. +++ b/core/includes/theme.inc
   @@ -695,56 +695,15 @@ function _theme_build_registry($theme, $base_theme, $theme_engine) {
    function list_themes($refresh = FALSE) {
   -  $list = &drupal_static(__FUNCTION__, array());
   +  /** @var \Drupal\Core\Theme\ThemeHandler $theme_handler */
   +  $theme_handler = \Drupal::service('theme_handler');
    
   

   This needs an @deprecated.

2. +++ b/core/includes/theme.inc
   @@ -757,38 +716,13 @@ function list_themes($refresh = FALSE) {
   +function drupal_find_base_themes($themes, $key) {
   +  return \Drupal::service('theme_handler')->getBaseThemes($themes, $key);
    }
   

   This needs a @deprecated.

3. +++ b/core/includes/theme.inc
   @@ -1478,36 +1412,7 @@ function theme_settings_convert_to_config(array $theme_settings, Config $config)
     *   An array of theme names.
     */
    function theme_enable($theme_list) {
   

   @deprecated.

4. +++ b/core/includes/theme.inc
   @@ -1517,34 +1422,7 @@ function theme_enable($theme_list) {
     *   An array of theme names.
     */
    function theme_disable($theme_list) {
   

   @deprecated. And any others that just wrap the service.

5. +++ b/core/lib/Drupal/Core/Theme/ThemeHandler.php
   @@ -0,0 +1,485 @@
   +      // Extract from the database only when it is available.
   +      // Also check that the site is not in the middle of an install or update.
   +      if (!defined('MAINTENANCE_MODE')) {
   

   This isn't a new bug, but that data is no longer in the database. It's in YAML/CMI.

6. +++ b/core/lib/Drupal/Core/Theme/ThemeHandler.php
   @@ -0,0 +1,485 @@
   +  protected function configInstallDefaultConfig($theme) {
   +    config_install_default_config('theme', $theme);
   +  }
   

   This is seriously not injectable??? (Ibid for the next several methods.)

Addressed #24 and #25.

Yes Crell, stuff isn't always injectable. You always seem surprised by this, but no issues are opened...

26: theme-2109287-26.patch queued for re-testing.

I am surprised when it's a new system in D8. New systems in D8 that aren't injectable but are just a bunch of functions make Crell sad. ;-(

@Crell: Drupal 8 development was open long enough before the DIC / injectability craze kick in :)

BTW this issue would help remove some stub code from config_translation as well :) In fact the lack of unit testability of the theme integration of config translation ignited this issue! Yay :) Thanks all!

Judging by the other "normals" in the RTBC queue, this is certainly major.

Rerolled after #1886448: Rewrite the theme registry into a proper service

Also after discussion with @dawehner, decided to move this to Drupal\Core\Extension, next to ModuleHandler. It makes more sense that way, and the result can even be seen by the reduced use statements.

1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -0,0 +1,485 @@
   +
   +  protected function doGetBaseThemes(array $themes, $theme, $used_themes = array()) {
   ...
   +
   +  protected function getSystemListingInfo() {
   

   Missing docblocks

2. +++ b/core/modules/system/system.module
   @@ -2454,115 +2454,7 @@ function system_rebuild_module_data() {
    function _system_rebuild_theme_data() {
   ...
   +  return \Drupal::service('theme_handler')->rebuildThemeData();
   

   Shouldn't we add @deprecated in docblock for this function?

I think the patch in #35 isn't the correct one. None of the docblock changes seem to have made it in doGetBaseThemes(), getSystemListingInfo() or _system_rebuild_theme_data().

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -0,0 +1,497 @@
+
+  protected function getSystemListingInfo() {

Still missing docblock.

Back to rtbc.

Great work, @dawehner!

A straight conversion is definitely a good first step — although I'm a bit concerned about DX due to the difference/disparity to ModuleHandler, so after committing this, I think we should proceed with #2024083: [META] Improve the extension system (modules, profiles, themes).

As a result of both, we should be able to finally resolve the epic #1067408: Themes do not have an installation status, which is still critically required to make CMI/staging work properly for themes :-)

Lastly, there are still a whole bunch of procedural functions being invoked here, for which we should create follow-up issues (not blocking a commit here).

Sorry, my last comment was meant to reference #1067408: Themes do not have an installation status as CMI blocker. (corrected)

Reroll and pulling in a change from #1851018: Improve breakpoint configuration implementation., relevant interdiff attached.

Another reroll, just a context change in core.services.yml.

Fixing test failure due to some new CSS added in #1986074: Buttons style update.

Some very minor code style issues:

1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -0,0 +1,496 @@
   +    // Find theme engines
   

   Needs full stop.

2. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -0,0 +1,496 @@
   +    // Read info files for each theme
   

   Needs full stop.

3. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -0,0 +1,496 @@
   +    // Now that we've established all our master themes, go back and fill in data
   +    // for subthemes.
   

   Exceeeds 80 chars.

4. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -0,0 +1,496 @@
   +   * e.g., to prefix all values of the 'stylesheets' or 'scripts' properties with
   

   Exceeds 80 chars

5. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -0,0 +1,496 @@
   +   *   an empty array.
   

   Newline before @return

6. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -0,0 +1,496 @@
   +   * @param string $theme
   

   Missing param comment

7. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -0,0 +1,496 @@
   +   * @return array
   

   Missing return comment

8. +++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
   @@ -0,0 +1,423 @@
   +        )
   

   Missing comma

9. +++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
   @@ -0,0 +1,423 @@
   +   * Provides test data for testGetBaseThemes
   

   Needs full stop.

10. +++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
    @@ -0,0 +1,423 @@
    +   * @return array
    

    Missing return comment

11. +++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
    @@ -0,0 +1,423 @@
    +    $data[] = array($themes, 'test_1', array('test_2' => 'test_2', 'test_3' => 'test_3'));
    

    Long arrays should be broken into individual lines.

Fixes for #54

1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -0,0 +1,500 @@
   +    'node_user_picture',
   

   Not the fault of this patch at all, but with these configurable for entity view modes these really ought to go.

2. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -0,0 +1,500 @@
   +    $this->clearCssCache();
   

   Does the CSS cache need to be cleared when a theme is enabled? It's not going to change the contents of any files, only the set of files, which results in new aggregates anyway.

   I'd also expect that to happen at the end of the process rather than the beginning, if it's actually necessary - because the cache could be regenerated before the rest of the work is done.

3. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -0,0 +1,500 @@
   +    // listInfo() calls system_info which has a lot of side effects that have to
   

   system_info()?

4. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -0,0 +1,500 @@
   +    // @todo It feels wrong to have the requirement to clear the local tasks
   

   It does feel wrong but I can't think of a way around this at the moment.

5. +++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
   @@ -0,0 +1,428 @@
   +if (!defined('DRUPAL_EXTENSION_NAME_MAX_LENGTH')) {
   

   Is this now used anywhere outside the module and theme handler? I suppose in system module somewhere still.

Opened #2149631: Don't clear the CSS cache when installing themes.

Let's leave the others and handle them later on.

Looks like catch's comments have been addressed, so back to RTBC

Committed/pushed to 8.x, thanks!

Needs a change notice for the new service and the deprecation.

Suggested change notice:

list_themes(), theme_enable() and theme_disable() have been replaced with a new service

Drupal 8 has introduced a new 'theme_handler' service for managing themes. The procedural functions from Drupal 7 have been deprecated.

Drupal 7:

$themes = list_themes(); // Get a list of available themes.
theme_enable($theme_list); // Enable themes.
theme_disable($theme_list); // Disable themes.
drupal_find_base_themes($theme, $key); // Find base themes for a theme.

Drupal 8:

$theme_handler = \Drupal::service('theme_handler');

$themes = $theme_handler->listInfo();  // Get a list of available themes.
$theme_handler->enable($theme_list); // Enable themes.
$theme_handler->disable($theme_list); // Disable themes.
$theme_handler->getBaseThemes($themes, $key); // Find base themes for a theme.

Change notice here: https://drupal.org/node/2150863

Should we have an issue to change the existing uses? Eg. there is significant code in config translation to be removed working around this not being a service :)

@Gabor it makes a lot of sense! Filed #2151469: Clean-up usage of deprecated list_themes() and _system_rebuild_theme_data() in favor of theme_handler service

Crossposting the issue to remove @todos pointing to this issue: #2188991: Use ThemeHandler instead of list_themes() in unit tests