Extract the install/uninstall functionality to a ThemeInstaller

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -190,180 +172,14 @@ public function setDefault($name) {
+    \Drupal::service('theme_installer')->install($theme_list, $install_dependencies);
...
+    \Drupal::service('theme_installer')->uninstall($theme_list);

Can we use setter injection to avoid circular dependency and remove \Drupal use? Just like we did in #2448847: [regression] Themes unable to implement hook_theme_registry_alter().

The patch looks great to me, it should improve the performance a bit because the object that we're using more often, the theme handler, is quite a bit lighter in dependencies.

Also rerolled the patch, added a docblock to the new interface and removed the methods from ThemeHandlerInterface (that's why also ThemeHandler implements ThemeInstallerInterface now, right?)

Just a small question before RTBC-ing this:

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -190,180 +172,14 @@ public function setDefault($name) {
+    \Drupal::service('theme_installer')->install($theme_list, $install_dependencies);
...
+    \Drupal::service('theme_installer')->uninstall($theme_list);

Do you think we should put a comment above this line to say that not doing any kind of dependency injection here is because it would cause a circular dependency and it would also remove all the benefits of this patch.

Thanks for the comment, it should be helpful if anyone stumbles upon this code. Looks great now :)

Should anyone care to comment on #10?

This makes sense - it brings us inline with ModuleHandler and we're not breaking BC. It would be nice to know the impact of doing this. But I do appreciate that it part of disentangling the dependency knots in the container.

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -190,180 +172,18 @@ public function setDefault($name) {
    * {@inheritdoc}
    */
   public function install(array $theme_list, $install_dependencies = TRUE) {
...
   /**
    * {@inheritdoc}
    */
   public function uninstall(array $theme_list) {

+++ b/core/lib/Drupal/Core/Extension/ThemeHandlerInterface.php
@@ -8,45 +8,11 @@
   /**
-   * Installs a given list of themes.
-   *
-   * @param array $theme_list
-   *   An array of theme names.
-   * @param bool $install_dependencies
-   *   (optional) If TRUE, dependencies will automatically be installed in the
-   *   correct order. This incurs a significant performance cost, so use FALSE
-   *   if you know $theme_list is already complete and in the correct order.
-   *
-   * @return bool
-   *   Whether any of the given themes have been installed.
-   *
-   * @throws \Drupal\Core\Extension\ExtensionNameLengthException
-   *   Thrown when the theme name is to long
-   */
-  public function install(array $theme_list, $install_dependencies = TRUE);
-
-  /**
-   * Uninstalls a given list of themes.
-   *
-   * Uninstalling a theme removes all related configuration (like blocks) and
-   * invokes the 'themes_uninstalled' hook.
-   *
-   * @param array $theme_list
-   *   The themes to uninstall.
-   *
-   * @throws \InvalidArgumentException
-   *   Thrown when you uninstall an not installed theme.
-   *
-   * @see hook_themes_uninstalled()
-   */
-  public function uninstall(array $theme_list);

I don't think we should remove them from the interface I think we should add an @deprecated D9 statement and an @see to the new service.

1. Theres a tonne of use statements at the top of ThemeHandler that can be removed.

2. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
   @@ -0,0 +1,306 @@
   +
   +class ThemeInstaller implements ThemeInstallerInterface {
   

   No class doc.

Still see parts of ThemeHandler that look rough, but nothing in scope for this issue. This patch is a solid code improvement. Looks great!

No changes, just git apply -3

Meant to put it back

Not instantiating ConfigInstaller at general runtime just seems sensible and a performance improvement. Committed 6e8eb8e and pushed to 8.0.x. Thanks!

This caused a regression: #2531878: 'Theme not found' when installing any theme but the theme installs

Filed follow-up to clean-up usage #3017231: Properly deprecate ThemeHandlerInterface install() and uninstall() also replace usage

+++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
@@ -0,0 +1,312 @@
+    // @todo It feels wrong to have the requirement to clear the local tasks
+    //   cache here.
+    Cache::invalidateTags(array('local_task'));

Filed follow-up for todo #3350933: Fix @todo in \Drupal\Core\Extension\ThemeInstaller::resetSystem()