Add a ThemeConfig service for setting and getting default and admin themes

This patch depends on #2409811: Kernel tests should explicitly install themes

re-rolling patch

I just realized why this patch is so small.

A majority of these calls are split over multiple lines:

    $this->config('system.theme')
      ->set('default', $theme)
      ->save();

So a grep like this would find them:
grep -nrA1 "'system.theme'" * | grep "set('default'"

There are a few cases in which the admin theme is being set along side the default theme:

$this->config('system.theme')
      ->set('default', 'bartik')
      ->set('admin', 'seven')
      ->save();

I want to propose we add a new API change in which we add two new API functions to the theme handler. See attached patch file. The patch will allow the above code to be rewritten as:

 \Drupal::service('theme_handler')->setDefault('bartik');
 \Drupal::service('theme_handler')->setAdmin('seven');

I much prefer a dedicated method. Setting the admin theme is a completely separate operation, despite often being done at the same time in tests.

Using PhpStorm / manual searching I found config('system.theme') saving in:

core/modules/block/src/Tests/BlockHiddenRegionTest.php line 59
core/modules/block/src/Tests/BlockTest.php line 207
core/modules/block/src/Tests/NewDefaultThemeBlocksTest.php line 49
core/modules/block_content/src/Tests/BlockContentTypeTest.php line 155
core/modules/ckeditor/src/Tests/CKEditorTest.php line 267
core/modules/color/src/Tests/ColorTest.php line 108 and 160
core/modules/comment/src/Tests/CommentLinksTest.php line 50
core/modules/migrate_drupal/src/Tests/d6/MigrateBlockTest.php line 63
core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6Test.php line 156

and so on.

But it looks like the fix is larger than that. We should probably fix all the getting as well.

I agree with @cosmicdreams, we should also fix the getters as well. The attached patch fixes all the theme setters. I'll start working on the theme getters patch. Here is the grep command I'm using:

grep -rn "config('system.theme')" *

Which outputs these files:

core/includes/theme.maintenance.inc:50:        $config = \Drupal::config('system.theme');
core/modules/block/block.module:42:    $demo_theme = $route_match->getParameter('theme') ?: \Drupal::config('system.theme')->get('default');
core/modules/block/block.module:76:    if (\Drupal::config('system.theme')->get('default') == $theme) {
core/modules/block/block.module:96:  $theme = $theme ? $theme : \Drupal::config('system.theme')->get('default');
core/modules/block/block.module:151:    $default_theme = \Drupal::config('system.theme')->get('default');
core/modules/block/src/BlockForm.php:114:      $theme = $this->config('system.theme')->get('default');
core/modules/block/src/Controller/BlockListController.php:30:    $theme = $theme ?: $this->config('system.theme')->get('default');
core/modules/block/src/Tests/BlockLanguageTest.php:53:    $default_theme = $this->config('system.theme')->get('default');
core/modules/block/src/Tests/BlockLanguageTest.php:131:    $default_theme = $this->config('system.theme')->get('default');
core/modules/block/src/Tests/BlockTest.php:31:    $default_theme = $this->config('system.theme')->get('default');
core/modules/block/src/Tests/BlockTest.php:69:    $default_theme = $this->config('system.theme')->get('default');
core/modules/block/src/Tests/BlockTest.php:105:    $default_theme = $this->config('system.theme')->get('default');
core/modules/block/src/Tests/BlockTest.php:137:    $block['theme'] = $this->config('system.theme')->get('default');
core/modules/block/src/Tests/BlockTest.php:193:    $theme_settings = $this->config('system.theme');
core/modules/block/src/Tests/BlockTest.php:239:    $default_theme = $this->config('system.theme')->get('default');
core/modules/block/src/Tests/BlockTest.php:278:    $block += array('theme' => $this->config('system.theme')->get('default'));
core/modules/block/src/Tests/BlockTestBase.php:69:    $blocks = $block_storage->loadByProperties(array('theme' => $this->config('system.theme')->get('default')));
core/modules/block/src/Tests/BlockTitleXSSTest.php:41:    $default_theme = $this->config('system.theme')->get('default');
core/modules/block/src/Tests/NewDefaultThemeBlocksTest.php:30:    $default_theme = $this->config('system.theme')->get('default');
core/modules/block/src/Tests/Views/DisplayBlockTest.php:187:    $default_theme = $this->config('system.theme')->get('default');
core/modules/block_content/src/BlockContentForm.php:190:          $theme = $this->config('system.theme')->get('default');
core/modules/block_content/src/Tests/BlockContentCreationTest.php:213:    $url = 'admin/structure/block/add/block_content:' . $block->uuid() . '/' . $this->config('system.theme')->get('default');
core/modules/block_content/src/Tests/BlockContentCreationTest.php:267:    $url = 'admin/structure/block/add/block_content:' . $block->uuid() . '/' . $this->config('system.theme')->get('default');
core/modules/block_content/src/Tests/BlockContentFieldTest.php:94:    $url = 'admin/structure/block/add/block_content:' . $block->uuid() . '/' . $this->config('system.theme')->get('default');
core/modules/block_content/src/Tests/BlockContentTypeTest.php:152:    $theme_settings = $this->config('system.theme');
core/modules/ckeditor/ckeditor.module:83:    $theme = \Drupal::config('system.theme')->get('default');
core/modules/config/src/Tests/ConfigImportUITest.php:94:    $system_theme = $this->config('system.theme')->get();
core/modules/config/src/Tests/ConfigImportUITest.php:179:    $system_theme = $this->config('system.theme')->get();
core/modules/menu_ui/src/Tests/MenuTest.php:221:    $this->drupalGet('admin/structure/block/list/' . $this->config('system.theme')->get('default'));
core/modules/menu_ui/src/Tests/MenuTest.php:511:    $default_theme = $this->config('system.theme')->get('default');
core/modules/quickedit/quickedit.module:76:    $theme = Drupal::config('system.theme')->get('admin');
core/modules/simpletest/src/WebTestBase.php:390:      'theme' => $this->config('system.theme')->get('default'),
core/modules/system/src/Controller/SystemController.php:189:    $config = $this->config('system.theme');
core/modules/system/src/Controller/ThemeController.php:80:    $config = $this->config('system.theme');
core/modules/system/src/Form/ThemeAdminForm.php:46:      '#default_value' => $this->config('system.theme')->get('admin'),
core/modules/system/src/Tests/Menu/BreadcrumbTest.php:44:      'theme' => $this->config('system.theme')->get('admin'),
core/modules/system/src/Tests/System/ThemeTest.php:245:    $this->assertEqual($this->config('system.theme')->get('default'), 'bartik');
core/modules/system/tests/modules/menu_test/src/Theme/TestThemeNegotiator.php:35:      return \Drupal::config('system.theme')->get('admin');
core/modules/views/src/Tests/Plugin/BlockDependenciesTest.php:95:      'theme' => $this->config('system.theme')->get('default'),
core/modules/views/src/Tests/Wizard/BasicTest.php:135:    $this->drupalGet('admin/structure/block/list/' . $this->config('system.theme')->get('default'));
core/modules/views/src/Tests/Wizard/ItemsPerPageTest.php:73:    $this->drupalGet('admin/structure/block/list/' . $this->config('system.theme')->get('default'));
core/modules/views_ui/src/Tests/OverrideDisplaysTest.php:52:    $this->drupalGet('admin/structure/block/list/' . $this->config('system.theme')->get('default'));
core/modules/views_ui/src/Tests/OverrideDisplaysTest.php:111:    $this->drupalGet('admin/structure/block/list/' . $this->config('system.theme')->get('default'));

It seems those patches were missing the actual changes to ThemeHandler itself

Combined patches #7 and #12. Interdiff is patch #7

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateBlockTest.php
@@ -58,10 +58,9 @@ protected function setUp() {
+    \Drupal::service('theme_handler')->install(['bartik', 'seven']);
+    \Drupal::service('theme_handler')->setDefault('bartik');
+    \Drupal::service('theme_handler')->setAdmin('seven');

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6Test.php
@@ -153,10 +153,9 @@ class MigrateDrupal6Test extends MigrateFullDrupalTestBase {
+    \Drupal::service('theme_handler')->install(['bartik', 'seven']);
+    \Drupal::service('theme_handler')->setDefault('bartik');
+    \Drupal::service('theme_handler')->setAdmin('seven');

+++ b/core/modules/system/src/Tests/Batch/PageTest.php
@@ -29,11 +29,9 @@ class PageTest extends WebTestBase {
+    \Drupal::service('theme_handler')->install(['seven', 'bartik']);
+    \Drupal::service('theme_handler')->setDefault('bartik');
+    \Drupal::service('theme_handler')->setAdmin('seven');

+++ b/core/modules/system/src/Tests/Extension/ThemeHandlerTest.php
@@ -173,8 +173,8 @@ function testUninstallDefault() {
+    \Drupal::service('theme_handler')->install([$name, $other_name]);
+    \Drupal::service('theme_handler')->setAdmin($name);

+++ b/core/modules/system/src/Tests/Menu/MenuRouterTest.php
@@ -243,12 +243,9 @@ public function testThemeIntegration() {
+    \Drupal::service('theme_handler')->install([$this->default_theme, $this->admin_theme]);
+    \Drupal::service('theme_handler')->setDefault($this->default_theme);
+    \Drupal::service('theme_handler')->setAdmin($this->admin_theme);

+++ b/core/modules/taxonomy/src/Tests/ThemeTest.php
@@ -20,10 +20,8 @@ protected function setUp() {
+    \Drupal::service('theme_handler')->setDefault('bartik');
+    \Drupal::service('theme_handler')->setAdmin('seven');

+++ b/core/modules/tour/src/Tests/TourTestBasic.php
@@ -49,11 +49,9 @@ protected function setUp() {
+    \Drupal::service('theme_handler')->install(['bartik', 'seven']);
+    \Drupal::service('theme_handler')->setDefault('bartik');
+    \Drupal::service('theme_handler')->setAdmin('seven');

Optimize code to avoid multiple calls to \Drupal::service('theme_handler')

@almaudoh Oh cool, these can be chained? An excellent novice task.

They cannot be chained, install() doesn't return $this. @humansky and I discussed this in person, we should have mentioned it on issue.

setAdmin() and setDefault() can be chained. However #19 is about something like:

$theme_handler = \Drupal::service('theme_handler');
$theme_handler->install(...);
$theme_handler->setDefault(...)
  ->setAdmin(...);

Now working on the three test fails which have to do with some really old code trying to set 'admin_theme' = '0' in ThemeTest and ConfigTranslationUITest.

Edit: it would be nice if install could be chained as well. Better DX.

Changed the 'admin_theme' = '0' to a more explicit 'admin_theme' = '_default_' which setAdmin() recognizes as the default value. Don't know if this breaks anything...

1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -198,7 +198,8 @@ public function getAdmin() {
   -    if (!isset($list[$name])) {
   +    // Special '_default_' key for the default theme used as the admin theme.
   +    if (!isset($list[$name]) && '_default_' !== $name) {
   
   +++ b/core/modules/system/src/Form/ThemeAdminForm.php
   @@ -40,7 +40,8 @@ public function buildForm(array $form, FormStateInterface $form_state, array $th
   -      '#options' => array(0 => $this->t('Default theme')) + $theme_options,
   +      // Special '_default_' key for the default theme.
   +      '#options' => array('_default_' => $this->t('Default theme')) + $theme_options,
   
   +++ b/core/modules/system/src/Tests/System/ThemeTest.php
   @@ -222,7 +222,7 @@ function testAdministrationTheme() {
   -      'admin_theme' => '0',
   +      'admin_theme' => '_default_',
   

   This is completely out of scope, AFAICS.

2. +++ b/core/modules/system/src/Form/ThemeAdminForm.php
   @@ -25,7 +25,7 @@ public function getFormID() {
   -    return ['system.theme'];
   +    return [];
   

   Hmm, this change worries me. Might have to ping @alexpott directly for his opinion.

@tim.plunkett: 1) the change is needed to allow the tests pass. We may have to raise another issue to look into that pre-existing bug. 2) This change is not really needed for the patch but that piece of code would be redundant.

I don't think this issue is as straightforward as originally thought.

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -189,6 +189,28 @@ public function setDefault($name) {
+  public function getAdmin() {
+    return $this->configFactory->get('system.theme')->get('admin');
+  }

This should account for when the admin theme set to 0, which is supposed to use the default theme. See related issue for an example of what occurs when this isn't checked.

Re-rolled

That is an API change now, so this is certainly at least 8.1.x work.

Setting back to CNW per #28.

@dawehner, I'm not sure how this is technically a "change". If anything, it's adding in the missing pieces of ThemeManager that should have been added before 8.0.0 was released. It doesn't prevent anyone from still using config.

+++ b/core/lib/Drupal/Core/Extension/ThemeHandlerInterface.php
@@ -174,6 +174,24 @@ public function getDefault();
+  public function getAdmin();
...
+  public function setAdmin($theme);

These constitute a BC break.

The definition of "BC break" being used in this issue is very far reaching.

You cannot break something that didn't exist before (the methods, not the class/interface). If this patch were changing existing signature methods,then sure... that would constitute a BC break... but it's not, it's just a simple addition.

Which is why I didn't change the version... yet.

However, I seriously don't understand how fixing a prior feature (and incomplete I might add) (#2228093: Modernize theme initialization) also makes this task a "feature". The theme handler/manager/service stuff is great and all, but it wasn't fully baked and left a lot of stuff slide. The fact that we have errors like #2587119: Form sets system.theme:admin to '0' breaking Quick Edit and making no sense is just an example of this. If anything, this is more of a follow-up to add in the missing pieces that were simply overlooked.

So, if the powers that be want to redefine what a "BC break" is and call this a "feature", then I guess we can leave the version as 8.1.x... otherwise adding in something so simple to help fix issues like the above seems like a better win in my book.

@markcarver
If in contrib I write a class called MyBetterThemeHandler:

class MyBetterThemeHandler implements ThemeHandlerInterface {

  // Implements all methods defined in 8.0.0

}

And then this patch gets committed, my class will be broken and cause fatal errors.
Adding methods to the interface is a BC break.

Ok, I see the BC break now. I seriously doubt though that single soul has implemented this interface other than core...

Regardless, would an acceptable compromise be to comment out the interface additions with a @todo that they should be added back in 8.1.x or just create a 8.1.x follow up issue for the interface additions?

This way we can add the missing and needed methods?

Moving to the theme system where it belongs to.
A bit more of a provocative question: Should those methods really be on the ThemeHandler? It feels like setDefault() was placed there in order to have a dumping ground for methods related to themes. A simple ThemeConfig or so could have setDefault/and all the various other ones, which then would no longer be a direct BC break.

Should those methods really be on the ThemeHandler?

@dawehner, I agree ThemeHandler is not the right place for these methods.

Thanks @cilefen for tagging!

I agree that #41 sounds like a safer (and more architecturally sound) approach.

Either solution should go into the next/development minor branch and is not eligible for a patch release, since it includes API additions and (in the case of the current patch) BC breaks for an interface and base class. References: https://www.drupal.org/core/d8-allowed-changes#minor and https://www.drupal.org/core/d8-bc-policy

1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandlerInterface.php
   @@ -160,6 +160,9 @@ public function getName($theme);
      public function getDefault();
   
   @@ -170,6 +173,9 @@ public function getDefault();
      public function setDefault($theme);
   

   The implementations in ThemeHandler should be changed to delegate to the ThemeConfig service

2. +++ b/core/lib/Drupal/Core/Theme/ThemeConfig.php
   @@ -0,0 +1,87 @@
   +  public function getDefault() {
   ...
   +  public function setDefault($name) {
   ...
   +  public function getAdmin() {
   ...
   +  public function setAdmin($name) {
   

   I suggest we should name these (get|set)DefaultTheme and (get|set)AdminTheme so it is self documenting.

That is an API change. Is that allowed?

It shouldn't be an API change.

Done #49 and also renamed the class to ThemeConfiguration and ThemeConfigurationInterface. If that's too verbose, then feel free to revert.

Mh, so ThemeHandler should be certainly changed, IMHO.

Here is a general question, when we would do that (call to ThemeConfig from ThemeHandler), we would have a circular dependency. I think its fine to have circular dependencies in BC layers ...

@cilefen
Is this really the issue you wanted to link to?

@cilefen
Well no, actually this issue is about doing it right on the architecture side of things. #2635784: Deprecate ThemeHandler::setDefault() in favour of configuration and replace usages just ensures that we use the old api consistent.

Beside of the tests and the change record this looks really good!

It sure did. trying again

1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -500,4 +494,14 @@ public function hasUi($name) {
   +    return \Drupal::service('theme_config');
   

   IMO this should document that it CANNOT be injected due to a circular dependency, and maybe refer to the fact that it should go away once getDefault/setDefault are removed.

2. +++ b/core/lib/Drupal/Core/Theme/ThemeConfiguration.php
   @@ -0,0 +1,87 @@
   +  }
   +}
   

   Missing blank line

3. +++ b/core/tests/Drupal/Tests/Core/Theme/ThemeConfigurationTest.php
   @@ -0,0 +1,191 @@
   +    $this->cache = $this->getMock('Drupal\Core\Cache\CacheBackendInterface');
   ...
   +    $config = $this->prophesize(Config::class);
   

   This test is already using prophecy, might as well use it consistently.

   Also can use the ::class constant

4. +++ b/core/tests/Drupal/Tests/Core/Theme/ThemeConfigurationTest.php
   @@ -0,0 +1,191 @@
   +  }
   +}
   

   Missing blank line

1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandlerInterface.php
   @@ -160,6 +160,9 @@ public function getName($theme);
   +   *
   +   * @deprecated in Drupal 8.1.0, will be removed before Drupal 9.0.0.
   +   *   Use \Drupal\Core\Theme\ThemeConfiguration::getDefaultTheme().
       */
   
   @@ -170,6 +173,9 @@ public function getDefault();
   +   *
   +   * @deprecated in Drupal 8.1.0, will be removed before Drupal 9.0.0.
   +   *   Use \Drupal\Core\Theme\ThemeConfiguration::setDefaultTheme().
       */
   

   At that time we cannot target 8.1 anymore, so this needs to be moved to 8.2

2. +++ b/core/lib/Drupal/Core/Theme/ThemeConfigurationInterface.php
   @@ -0,0 +1,51 @@
   +/**
   + * Manages theme configuration.
   + */
   +interface ThemeConfigurationInterface {
   

   This documentation is IMHO a bit weak, maybe a bit more description of what is going on. This interface here is one example of something I just don't get why we need an interface. Why would you swap out that behaviour ever.

Addressed both 67.1 and 67.2

Cool, thank you!

So afaics ThemeHandler::setDefault() is only used in tests and ::getDefault() only has 2 usages - one outside tests. So there must be plenty of places where we are just reading and writing to config directly. I think we should just deprecate the methods and tell people to use config. Because adding a ThemeConfig service implies we could swap out the service and make it store the default theme somewhere other than config - that's not going to work.

If I'm wrong then we need a better justification of adding the new service in the issue summary.

Setting back to needs work for #70 since at the very least the issue summary needs an update.

What @alexpott said makes sense to me, I would love to see that justification.

This one still relevant?