Updated: Comment #N

Problem/Motivation

#2102125: Big Local Task Conversion

Proposed resolution

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#57 theme_handler-2109287.patch47.92 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,315 pass(es).
[ View ]
#57 interdiff.txt725 bytesdawehner
#55 interdiff.txt4.1 KBkim.pepper
#55 2109287-theme-service-55.patch47.92 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 59,020 pass(es).
[ View ]
#51 2109287-51.patch47.73 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 59,114 pass(es).
[ View ]
#51 interdiff.txt857 bytesCottser
#33 interdiff.txt6.52 KBtim.plunkett
#33 theme-2109287-33.patch47.3 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,000 pass(es).
[ View ]

Comments

Gábor Hojtsy’s picture

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.

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new25.54 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Let's see how much the world crashes.

Status:Needs review» Needs work

The last submitted patch, theme-2109287-2.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
Issue tags:+Twig
StatusFileSize
new29.6 KB
FAILED: [[SimpleTest]]: [MySQL] 57,022 pass(es), 213 fail(s), and 67 exception(s).
[ View ]

Ha, this time with the actual interface.

Status:Needs review» Needs work

The last submitted patch, theme-2109287-4.patch, failed testing.

tstoeckler’s picture

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?

dawehner’s picture

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?

No, there is no reason at all. If we decide to not move the theme handler to the extension directory we might should rename it to module, as this is what left. But I agree with you that the extension directory might be the better place.

dawehner’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new15.65 KB
new41.16 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Started with a bunch of unit tests while waiting on some of the other ones.

Status:Needs review» Needs work

The last submitted patch, 8: theme-2109287-8.patch, failed testing.

dawehner’s picture

Issue tags:+phpunit
StatusFileSize
new42.49 KB
FAILED: [[SimpleTest]]: [MySQL] 58,892 pass(es), 128 fail(s), and 19 exception(s).
[ View ]

Some fixes and more unit tests.

dawehner’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 10: theme-2109287.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new46.93 KB
FAILED: [[SimpleTest]]: [MySQL] 59,922 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new7.44 KB

Some more test fixes.

Status:Needs review» Needs work

The last submitted patch, 13: theme-2109287-13.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new47.32 KB
PASSED: [[SimpleTest]]: [MySQL] 59,987 pass(es).
[ View ]
new1.28 KB

Fix the remaining tests.

amateescu’s picture

  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.

dawehner’s picture

StatusFileSize
new5.78 KB
new42.49 KB
FAILED: [[SimpleTest]]: [MySQL] 58,372 pass(es), 128 fail(s), and 19 exception(s).
[ View ]

Thank you for the review!

Why do we need the helper function here?

I don't want to have the accumulator as part of the interface nor public method.

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

Improved the doc of the default implementation.

amateescu’s picture

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..

Status:Needs review» Needs work

The last submitted patch, 17: theme-2109287.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new48.29 KB
PASSED: [[SimpleTest]]: [MySQL] 59,806 pass(es).
[ View ]

Good catch, here is a proper patch.

amateescu’s picture

  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.

ParisLiakos’s picture

  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?

dawehner’s picture

StatusFileSize
new47.5 KB
PASSED: [[SimpleTest]]: [MySQL] 60,385 pass(es).
[ View ]
new5.88 KB

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

We want to be able to not call system_list() so we wrap that method and override the behavior in our test code.
This technique is basically the same as mocking some of the dependencies.

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

Good point!

ParisLiakos’s picture

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

Crell’s picture

  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.)

tim.plunkett’s picture

StatusFileSize
new48.05 KB
PASSED: [[SimpleTest]]: [MySQL] 60,262 pass(es).
[ View ]
new2.89 KB

Addressed #24 and #25.

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

Status:Needs review» Needs work

The last submitted patch, 26: theme-2109287-26.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review

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

Crell’s picture

Status:Needs review» Reviewed & tested by the community

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. ;-(

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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!

tim.plunkett’s picture

Priority:Normal» Major

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

tim.plunkett’s picture

StatusFileSize
new47.3 KB
PASSED: [[SimpleTest]]: [MySQL] 58,000 pass(es).
[ View ]
new6.52 KB

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.

markcarver’s picture

Status:Reviewed & tested by the community» Needs work
  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?

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.43 KB
new47.5 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch theme-2109287_2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thank you for the reroll and the review!

Status:Needs review» Needs work

The last submitted patch, 35: theme-2109287.patch, failed testing.

markcarver’s picture

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().

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new47.79 KB
PASSED: [[SimpleTest]]: [MySQL] 57,997 pass(es).
[ View ]

Right, here is one.

markcarver’s picture

Status:Needs review» Needs work
+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -0,0 +1,497 @@
+
+  protected function getSystemListingInfo() {

Still missing docblock.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new686 bytes
new47.93 KB
PASSED: [[SimpleTest]]: [MySQL] 57,958 pass(es).
[ View ]

Ups.

amateescu’s picture

Status:Needs review» Reviewed & tested by the community

Back to rtbc.

sun’s picture

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).

sun’s picture

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

dawehner’s picture

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) .

I totally agree, there are a lot of things to solve everywhere. As you see in the patch there is still great coupling between system module and the theme system which is for itself horrible to understand.

Cottser’s picture

StatusFileSize
new1.05 KB
new47.46 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2109287-45.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

dawehner’s picture

I like the interdiff! Thank you for your patch!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 45: 2109287-45.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

45: 2109287-45.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 45: 2109287-45.patch, failed testing.

Cottser’s picture

Status:Needs work» Needs review
StatusFileSize
new47.52 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

Cottser’s picture

StatusFileSize
new857 bytes
new47.73 KB
PASSED: [[SimpleTest]]: [MySQL] 59,114 pass(es).
[ View ]

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

The last submitted patch, 50: 2109287-50.patch, failed testing.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC

kim.pepper’s picture

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.

kim.pepper’s picture

StatusFileSize
new47.92 KB
PASSED: [[SimpleTest]]: [MySQL] 59,020 pass(es).
[ View ]
new4.1 KB

Fixes for #54

catch’s picture

Status:Reviewed & tested by the community» Needs work
  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.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new725 bytes
new47.92 KB
PASSED: [[SimpleTest]]: [MySQL] 59,315 pass(es).
[ View ]

Thank you for the review!

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.

I tried to keep the exact same flow as before, which also had drupal_clear_css_cache() at the beginning. Should we really try to figure out the best way here?

system_info()?

Sure.

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

Other places are: file.install, menu_link.install, and quite often in user.module.

Status:Needs review» Needs work

The last submitted patch, 57: theme_handler-2109287.patch, failed testing.

catch’s picture

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

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

dawehner’s picture

Status:Needs work» Needs review

57: theme_handler-2109287.patch queued for re-testing.

kim.pepper’s picture

Status:Needs review» Reviewed & tested by the community

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

catch’s picture

Title:Replace list_themes() with a service.» Change notice: Replace list_themes() with a service.
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

Committed/pushed to 8.x, thanks!

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

kim.pepper’s picture

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:

<?php
$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:

<?php
$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.
?>
kim.pepper’s picture

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

kim.pepper’s picture

Status:Active» Fixed
kim.pepper’s picture

Title:Change notice: Replace list_themes() with a service.» Replace list_themes() with a service.
Issue tags:-Needs change record
Gábor Hojtsy’s picture

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 :)

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tim.plunkett’s picture

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