Extension System, Part III: ThemeExtensionList and ThemeEngineExtensionList

In general this looks pretty good already. Didn't had a deep look into it yet, but I totally agree that the theme extension list part should be extracted
from the theme handler. Great work!

So, do we still need the system_list cache?

I guess no?

Should we move all the drupal_get_profile() calls to wrapper function?

1. +++ b/core/includes/module.inc
   @@ -22,43 +23,19 @@
     */
    function system_list($type) {
   -  $lists = &drupal_static(__FUNCTION__);
   ...
     */
    function system_list_reset() {
   -  drupal_static_reset('system_list');
   

   Nice.

   We should deprecate those functions and mark for removal in 9.x

2. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -16,21 +16,6 @@
   -   */
   -  protected $defaultFeatures = array(
   -    'favicon',
   -    'logo',
   -    'node_user_picture',
   -    'comment_user_picture',
   -    'comment_user_verification',
   -  );
   

   Wondering if the changes in here could be considered a BC break in some way...

3. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -184,16 +169,11 @@ public function uninstall(array $theme_list) {
   -      }
   +      $themes = $this->getThemeListing()->listExtensions();
          foreach ($themes as $theme) {
   -        $this->addTheme($theme);
   +        if ($theme->status == TRUE) {
   +          $this->addTheme($theme);
   +        }
   

   Should we have a way to get enabled themes only? a method for this?

   The extension list is different for themes than for modules, as it also knows about enabled or not.

4. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -246,186 +216,27 @@ public function reset() {
   +  protected function getThemeListing() {
   +    if (!isset($this->themeList)) {
   +      $this->themeList = \Drupal::service('theme_listing');
        }
   -    return $this->extensionDiscovery;
   +    return $this->themeList;
      }
   

   is this a fallback when the extension isn't injected yet?

5. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
   @@ -179,17 +179,12 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
   -      // @todo Remove system_list().
   -      $this->systemListReset();
   +      // Reset theme listing.
   +      \Drupal::service('theme_listing')->reset();
    
   

   can we inject this?

Let's postpone it for now

Thank you for your reroll @almaudoh!

1. +++ b/core/includes/module.inc
   @@ -22,43 +23,19 @@
   +  return array_filter(\Drupal::service($type . '_listing')->listExtensions(), function(Extension $extension) {
   +    return $extension->status == TRUE;
   +  });
   

   What about just do a return $extension->status ?

2. +++ b/core/lib/Drupal/Core/Extension/ThemeEngineExtensionList.php
   @@ -0,0 +1,18 @@
   +
   +class ThemeEngineExtensionList extends ExtensionList {
   
   +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
   @@ -0,0 +1,230 @@
   +
   +class ThemeExtensionList extends ExtensionList {
   

   Let's still mark this internal? Let's also add some documentation :)

3. +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
   @@ -0,0 +1,230 @@
   +  protected function doListExtensions() {
   

   This methods could be made easier to rad by splitting it up into two bits: $themes = readThemeInfoFiles($themes) and $themes = $this->fillInSubThemes($themes). Maybe we could do that in another issue

4. +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
   @@ -0,0 +1,230 @@
   +    // @todo Move into a generic ExtensionHandler base class.
   +    // @see https://www.drupal.org/node/2208429
   

   I fear we would need to create a new follow up, given that this is extension part 3 :)

5. +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
   @@ -0,0 +1,230 @@
   +  public function getBaseThemes(array $themes, $theme) {
   

   I see, this is public due to BC reasons. We should clearly communicate that with another @internal statement + an explanation.

6. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
   @@ -178,17 +178,12 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
    
   -      // Update the current theme data accordingly.
   -      $current_theme_data = $this->state->get('system.theme.data', []);
   -      $current_theme_data[$key] = $theme_data[$key];
   -      $this->state->set('system.theme.data', $current_theme_data);
   -
   ...
    
   -      // @todo Remove system_list().
   -      $this->systemListReset();
   +      // Reset theme listing.
   +      \Drupal::service('theme_listing')->reset();
   

   👍 Doing less magic on theme installation is an obvious improvement

7. +++ b/core/tests/Drupal/Tests/Core/Extension/ThemeHandlerTest.php
   @@ -153,158 +131,6 @@ public function testThemeLibrariesEmpty() {
   -  /**
   -   * Tests rebuild the theme data with theme parents.
   -   */
   -  public function testRebuildThemeDataWithThemeParents() {
   -    $this->extensionDiscovery->expects($this->at(0))
   -      ->method('scan')
   

   We need to add back test coverage for those ...

#2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList is in.

Change parent to the right META issue - this is really a sibling, not a child, of #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList

Re-roll.

I haven't done a detailed read of the patch, I just peeked in, but I saw something right away at the top:

1. --- a/core/core.services.yml
   +++ b/core/core.services.yml
   @@ -511,6 +511,12 @@ services:
      extension.list.profile:
        class: Drupal\Core\Extension\ProfileExtensionList
        arguments: ['@app.root', 'profile', '@cache.default', '@info_parser', '@module_handler', '@state', '%install_profile%']
   +  theme_listing:
   +    class: Drupal\Core\Extension\ThemeExtensionList
   +    arguments: ['@app.root', 'theme', '@cache.default', '@info_parser', '@module_handler', '@config.factory', '@theme_engine_listing']
   +  theme_engine_listing:
   +    class: Drupal\Core\Extension\ThemeEngineExtensionList
   +    arguments: ['@app.root', 'theme_engine', '@cache.default', '@info_parser', '@module_handler']
   

   Shouldn't these services be named 'extension.list.theme' and 'extension.list.theme_engine' to match the extension list services for modules and profiles?

2. +++ b/core/includes/bootstrap.inc
   @@ -226,45 +226,24 @@ function drupal_get_filename($type, $name, $filename = NULL) {
   +  $service_id = 'extension.list.' . $type;
   +  /** @var \Drupal\Core\Extension\ExtensionList $extension_list */
   +  $extension_list = \Drupal::service($service_id);
   

   This code will depend on that!

This fixes the review from #27 - hopefully, this one will test a little better

Ah, apparently what is breaking the tests is:

Drupal\Tests\action\Functional\ActionListTest::testEmptyActionList
PHPUnit_Framework_Exception: Fatal error: Class Drupal\Core\Extension\ThemeEngineExtensionList contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Drupal\Core\Extension\ExtensionList::getInstalledExtensionNames)

A minor improvement and a correction ($theme->status is normally 0 or 1 so we don't want === TRUE).

Fixed the coding standards errors.

But the Functional test failures are beyond me, on this occasion.

2. I'm just wondering why these two would be marked @internal while the ModuleExtensionList is not. Should we make all three internal.

Oh yeah totally, till we are sure these services are stable.

Assigning to myself to fix the failure

• I fixed some minor comments
• I applied https://www.drupal.org/files/issues/2939904-4.patch as this seems to solve a lot of those problems.

@almaudoh Ah interesting. Do you mind on your next patch iteration make a comment why we are doing something this specific way? Thank you!

It's green! Nice work!

1. Do we need profiling here?

2. +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
   @@ -0,0 +1,272 @@
   +    // @todo Move into a generic ExtensionHandler base class.
   +    // @see https://www.drupal.org/node/2208429
   

   #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList is fixed. Can we remove this now?

3. +++ b/core/lib/Drupal/Core/Extension/ThemeInstaller.php
   @@ -178,17 +178,12 @@ public function install(array $theme_list, $install_dependencies = TRUE) {
   +      \Drupal::service('extension.list.theme')->reset();
   
   @@ -279,7 +269,7 @@ protected function resetSystem() {
   +    \Drupal::service('extension.list.theme')->reset();
   

   Can we inject this if not then can we add a comment here?

4. +++ b/core/includes/module.inc
   @@ -22,43 +23,19 @@
    function system_list($type) {
   ...
    function system_list_reset() {
   

Are we going to deprecate these? If yes then can we have a change notice?

We should also add information about ThemeExtensionList and ThemeEngineExtensionList in _system_rebuild_module_data() and co. are replaced by services to give you all available modules.

Re #41:
I'm really not a huge fan of flagging these as @internal.

However, if these are only being temporarily flagged as such for "stability" reasons, then a comment and @todo linking to an issue that removes this state once the criteria deeming them as "stable" has been met should also be added.

Otherwise, we risk leaving them this way and I know for a fact that I have plans on extending some of these classes in the future since they're now proper services that can easily be extended or replaced entirely by contrib.

Please see #54.

I would even suggest that all these issues for extension listings get backported to 8.5.x so we can actually start using them and then before 8.6.0 is released, these @internal tags are removed because we'll have fixed any major bugs that may have crept up.

I opened up #2940481: Road to stabilize ExtensionList services. To be honest I'm a huge fan of adding an interface on the longrun, but let's discuss that over there.

1. +++ b/core/includes/module.inc
   @@ -22,43 +23,19 @@
   +  return array_filter(\Drupal::service('extension.list.' . $type)->getList(), function (Extension $extension) {
   +    return $extension->status;
   +  });
   

   This feels like the extension list should already support returning a filtered list based on status.

   Perhaps a parameter could be added to ->getList(), e.g. $enabled = NULL?

   • NULL - return all regardless of status
   • TRUE - return all enabled
   • FALSE - return all disabled

   This can't be the only place where having native support for this kind of filter would be necessary.

2. +++ b/core/lib/Drupal/Core/Extension/ThemeEngineExtensionList.php
   @@ -0,0 +1,34 @@
   +    return ['twig'];
   

   Again, this shouldn't be a static list. It should actually return the installed theme engine names.

3. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -179,16 +164,11 @@ public function uninstall(array $theme_list) {
   +      $themes = $this->getThemeExtensionList()->getList();
          foreach ($themes as $theme) {
   -        $this->addTheme($theme);
   +        if ($theme->status) {
   +          $this->addTheme($theme);
   +        }
          }
   

   Ah, here is another place that could benefit from an already filtered list as mentioned above.

   It could probably even be reduced to a one-liner for that matter:

   array_map([$this, 'addTheme'], $this->getThemeExtensionList()->getList(TRUE));
   

1. +++ b/core/includes/module.inc
   @@ -5,6 +5,7 @@
    
   +use Drupal\Core\Extension\Extension;
    use Drupal\Core\Extension\ExtensionDiscovery;
   

   Why do you need this use statement?

2. +++ b/core/includes/module.inc
   @@ -22,43 +23,17 @@
   -  foreach ($lists['filepaths'] as $item) {
   -    system_register($item['type'], $item['name'], $item['filepath']);
   -  }
   

   What happened with the system_register call?

3. +++ b/core/includes/module.inc
   @@ -22,43 +23,17 @@
    /**
     * Resets all system_list() caches.
     */
    function system_list_reset() {
   -  drupal_static_reset('system_list');
   +  \Drupal::service('extension.list.profile')->reset();
      \Drupal::service('extension.list.module')->reset();
   -  \Drupal::cache('bootstrap')->delete('system_list');
   +  \Drupal::service('extension.list.theme_engine')->reset();
   +  \Drupal::service('extension.list.theme')->reset();
    }
    
   

   This reads quite nice now.

4. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
   @@ -10,6 +10,12 @@
   + * @internal
   + *   This class is not yet stable and therefore there are no guarantees that the
   + *   internal implementations including constructor signature and protected
   + *   properties / methods will not change over time. This will be reviewed after
   + *   https://www.drupal.org/project/drupal/issues/2940481
     */
   

   👍 This makes this easy to understand what is going on.

5. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
   @@ -257,6 +263,26 @@ public function get($extension_name) {
   +   * Returns a list of installed extensions.
   +   *
   +   * @return \Drupal\Core\Extension\Extension[]
   +   *   A list of installed extension objects, keyed by machine name.
   +   */
   +  public function getInstalled() {
   +    return array_intersect_key($this->getList(), array_flip($this->getInstalledExtensionNames()));
   +  }
   +
   +  /**
   +   * Returns a list of uninstalled extensions.
   +   *
   +   * @return \Drupal\Core\Extension\Extension[]
   +   *   A list of uninstalled extension objects, keyed by machine name.
   +   */
   +  public function getUninstalled() {
   +    return array_diff_key($this->getList(), array_flip($this->getInstalledExtensionNames()));
   +  }
   

   What means installed/uninstalled in the context of profiles/theme engines?

6. +++ b/core/lib/Drupal/Core/Extension/ThemeEngineExtensionList.php
   @@ -0,0 +1,34 @@
   +   * {@inheritdoc}
   +   */
   +  protected function getInstalledExtensionNames() {
   +    return ['twig'];
   +  }
   

   Can't you have multiple theme engines? There is probably not a single person using one, but this would break this, wouldn't it?

7. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -122,16 +107,16 @@ class ThemeHandler implements ThemeHandlerInterface {
       *   The info parser to parse the theme.info.yml files.
   -   * @param \Drupal\Core\Extension\ExtensionDiscovery $extension_discovery
   +   * @param \Drupal\Core\Extension\ThemeExtensionList $theme_list
       *   (optional) A extension discovery instance (for unit tests).
       */
   ...
   +    $this->themeList = $theme_list;
   
   @@ -249,186 +208,27 @@ public function reset() {
   -      $this->extensionDiscovery = new ExtensionDiscovery($this->root);
   +  protected function getThemeExtensionList() {
   +    if (!isset($this->themeList)) {
   +      $this->themeList = \Drupal::service('extension.list.theme');
        }
   -    return $this->extensionDiscovery;
   +    return $this->themeList;
      }
   

   ❓Why do we need both?

8. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -249,186 +208,27 @@ public function reset() {
       * {@inheritdoc}
       */
      public function rebuildThemeData() {
   

   Should we mark the interface method deprecated?

9. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleExtensionListTest.php
   @@ -35,4 +35,49 @@ public function testGetlist() {
   +  /**
   +   * @covers ::getInstalled
   +   * @covers ::getUninstalled
   +   */
   +  public function testInstalledUninstalled() {
   

   👍 Nice!

The other thing system_register() does is to register the theme in the PSR4 classloader. I don't think this is needed either since all modules and themes are already registered on installation.

Where are they registered on installation? I cannot find this in any of the *Installer classes.

I'm very worried/cautious about this change. Themes like https://www.drupal.org/project/bootstrap rely heavily on PSR autoloading and, from what I have always been able to determine, this was where theme namespaces were registered. There's even a "fix" for this in the base theme:
https://drupal-bootstrap.org/api/bootstrap/autoload-fix.php/8#source.16

It purposefully invokes \Drupal::service('theme_handler')->listInfo(); to trigger this registration because, during certain circumstances (e.g. AJAX requests), theme namespaces haven't yet been registered (regardless if they were installed).

From past conversations (#2002606: Allow themes to provide services.yml and other IRC/slack convos), I've been told this is primarily due to which theme is "active".

That being said, this has always registered all "installed" themes... not just the "active" ones... so.... idk.

I'm almost tempted to say that theme namespaces are a very vital aspect of this issue and need to be flushed out more first. AFAIK, there are no [extensive] theme namespace tests, so... the current "passing" may be a false positive as it's likely to break contrib.

I think maybe you're confusing "registered on installation" with DrupalKernel::compileContainer. This automatically registers module namespaces, because they can provide services (not themes):
http://cgit.drupalcode.org/drupal/tree/core/lib/Drupal/Core/DrupalKernel...

Should probably be postponed on this, but leaving status to CNW for now. If you agree, you can change.

Sure, I'm OK with doing that as a follow-up.

FTR, I think just reinstating drupal_classloader_register() will be enough.

I think this should not be a follow up. Instead we should do two things:

• Have a test which adds a theme with namespaces
• Ensure this test doesn't fail

Maybe this should be done before this issue, but I think we should avoid temporary regressions.

I'm seeing the theme namespace issue as really out of scope

It's not, but I suppose considering that there has been very little documented "support" for this, I could see how you would say that.

We could add the test and make sure it doesn't fail in this issue

I think that's what @dawehner was suggesting.

That being said, #1538478: Register lib/ directories of themes as PSR-0 roots for Drupal\$theme namespace supposedly already added such a test:
http://cgit.drupalcode.org/drupal/tree/core/modules/system/src/Tests/The...

Which instantiates a new ThemeClass() object.

I think, however, this is still a false positive since it's not actually testing against core's container namespaces using the extension registration/autoloading, but rather it may just be successfully autoloading due to the nature of directly calling the class in the test itself (i.e. using the test namespacing/autoloader, not core's).

A real test would be to load a page where the theme itself can instantiate one of its own classes from within the *.theme file, like Drupal Bootstrap does. This ensures that the container properly registers the classes within the scope of the request.

+++ b/core/modules/system/src/Tests/Theme/ThemeTest.php
--- a/core/modules/system/tests/modules/theme_test/src/ThemeTestController.php
+++ b/core/modules/system/tests/modules/theme_test/src/ThemeTestController.php

+++ b/core/modules/system/tests/modules/theme_test/src/ThemeTestController.php
@@ -159,4 +160,19 @@ public function preprocessSuggestions() {
+      new ThemeClass();

This still isn't exactly what I meant.

Themes cannot provide controllers/routes. In addition to the theme_test.test_theme_class route in the module, I suggest the content be rendered by the test theme itself. Then ThemeClass should be invoked inside the .theme file in a preprocess hook or something similar.

This would be a "true" baseline test for this IMO because \Drupal::service('theme_handler')->listInfo() is what used to register installed theme namespaces and they wouldn't necessarily be available in just a normal module callback.

+++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
@@ -179,6 +184,24 @@ protected function fillInSubThemeData(array &$themes, array $sub_themes) {
+  protected function registerThemeNamespaces(array $themes) {

I'd say let's mark this as @internal and/or @deprecated since we know this is likely going away when the other issue is fixed.

Other than that... I'd say this looks good!

It's also great to know that the existing test was definitely a false positive :D

I actually thought about doing that for a moment, then remembered that the whole class is already marked @internal

No, the class is only temporarily marked as @internal until #2940481: Road to stabilize ExtensionList services; at which point that label will be removed.

Ideally, the registerThemeNamespaces() method will be removed entirely by #2941757: Extension System, Part IV: Properly register all installed extensions/namespaces during container generation. However, in the off chance that it doesn't, I think this should still be marked as such now (since we already have plans for this) with @internal and/or @deprecated. If it doesn't, it can then just stay as an empty method that does nothing (since the other issue would take care of this at the container level).

1. +++ b/core/lib/Drupal/Core/Extension/ThemeEngineExtensionList.php
   @@ -0,0 +1,101 @@
   +    $this->themeEngine = $this->configFactory->get('core.extension')->get('theme_engine') ?: 'twig';
   

   There is no need for a config entry like this. There is no "default" engine per se. The theme engine used is determined by the theme itself, which automatically defaults to "twig" if not defined.

2. +++ b/core/lib/Drupal/Core/Extension/ThemeEngineExtensionList.php
   @@ -0,0 +1,101 @@
   +    return [$this->getThemeEngine()];
   

   This still isn't right. It's still only returning a single theme engine.

   There has always been the ability to have multiple theme engines installed (https://www.drupal.org/project/project_theme_engine).

   The one that is actually used is determined by the active theme during rendering.

   E.g. the admin theme may use a different engine than the default theme.

   edit: I've even seen some cases where certain AJAX requests were rendered using a different theme engine.

   One can still even use PHPTemplate (although I don't know why they would): https://cgit.drupalcode.org/phptemplate/tree/phptemplate.engine

   This should return a list of all available theme engines (since they don't actually have an "install" state).

3. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
   @@ -249,186 +208,27 @@ public function reset() {
   -    $this->state->set('system.theme_engine.files', $files_theme_engine);
   

   ThemeHandler::rebuildThemeData is where theme engines where originally stored. Since they don't have an "install" state, they're all "available" and were just returned as needed.

There is no need for a config entry like this. There is no "default" engine per se. The theme engine used is determined by the theme itself, which automatically defaults to "twig" if not defined.

I agree with @markcarver, we should not introduce a new concept as part of this issue. I also believe that installing a theme extension doesn't make necessarily even sense, just as pointed out like @markcarver

So the question is, what does the concept of an "installed" theme engine really mean?

For ThemeEngineExtensionList, it just means that it should return all discovered theme engines. They have no "install" state.

we should not introduce a new concept as part of this issue

That's actually what #1685492: Convert theme engines into services is attempting to solve. Since that issue is likely the "end goal" with theme engines, it would ultimately make ThemeEngineExtensionList moot since they would no longer be an "extension" as any module would be able to define a theme engine service.

For now, and in anticipation of that issue, I think we should keep ThemeEngineExtensionList simply for furture BC reasons.

Glad to see this green. Thanks, for all the work @almaudoh. Hopefully, all the feedback has been addressed. Let's get to some profiling.

I'd do the profiling, but not currently set up to do so (nor very familiar with how to, if I'm being honest).

Profiled patch added in #100
tldr: ~2% performance decrease for everything except the memory.

Cold cache:

Time+553 µs (+0.02%)
2.71 s → 2.71 s

I/O Wait+55.5 ms (+3.61%)
1.54 s → 1.6 s

CPU Time-55 ms (-4.7%)
1.17 s → 1.12 s

Memory-109 kB (-0.488%)
22.2 MB → 22.1 MB
SQL Queries+56.9 ms / +13 rq

Blackfire:
- without patch and aggregation:
https://blackfire.io/profiles/c6bfa8c9-8834-41d2-a766-57fc96e646d2/graph
- with patch and without aggregation:
https://blackfire.io/profiles/957ba241-e66b-48d7-a6a9-acf25582763b/graph
- compare:
https://blackfire.io/profiles/compare/d4bea6a7-3035-4ca5-a562-23d568f779...

Warm cache:

Time+4.41 ms (+1.61%)
274 ms → 279 ms

I/O Wait+1.97 ms (+1.98%)
99.5 ms → 101 ms

CPU Time+2.44 ms (+1.39%)
175 ms → 177 ms

Memory-26 kB (-0.229%)
11.5 MB → 11.5 MB

Blackfire:
- without patch and with aggregation:
https://blackfire.io/profiles/607b82b5-f77e-4962-8f00-589cccba95d2/graph
- with patch and aggregation:
https://blackfire.io/profiles/5ecae1a8-f70b-440d-996f-37c5b86e671f/graph
- compare:
https://blackfire.io/profiles/compare/f9b21477-65a5-4fb2-8159-776bcbc650...

Thanks, this doesn't look too bad to me. Let's get some opinions from core committers.

@zviryatko Nice work!
I was looking at https://blackfire.io/profiles/compare/f9b21477-65a5-4fb2-8159-776bcbc650...() and I'm wondering. Have the original request been really warm? state set shouldn't happen on normal requests. Any idea why they are there?

@dawehner, check \Drupal\update\UpdateManager::projectStorage() it removes state for listed routes.

Update: added list of routes from mentioned function:

    $route_names = [
      'update.theme_update',
      'system.modules_list',
      'system.theme_install',
      'update.module_update',
      'update.module_install',
      'update.status',
      'update.report_update',
      'update.report_install',
      'update.settings',
      'system.status',
      'update.manual_status',
      'update.confirmation_page',
      'system.themes_page',
    ];
    if (in_array(\Drupal::routeMatch()->getRouteName(), $route_names)) {
      $this->keyValueStore->delete($key);
    }

Went ahead and re-queued the last patch (just to double check).

Other than that, I'd say that this is RTBC.

Ok. I'll do the honors...

Re-roll, but should still probably RTBC

Hm... seems I was behind 1 commit from HEAD somehow, whoops.

I've not done a deep dive review yet just a couple of quick thoughts that jump out.

+++ b/core/lib/Drupal/Core/Extension/ThemeHandlerInterface.php
@@ -119,6 +119,11 @@ public function reset();
+   * @deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0.
+   *   Use \Drupal::service('extension.list.theme')->reset() (if you actually
+   *   need to rebuild) and \Drupal::service('extension.list.theme')->getList()
+   *   instead.
    */
   public function rebuildThemeData();

@@ -136,6 +141,9 @@ public function rebuildThemeData();
+   * @deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0.
+   *   Use \Drupal::service('extension.list.theme')->getBaseThemes() instead.
    */
   public function getBaseThemes(array $themes, $theme);

As per the policy we should not add deprecations unless we also do an @trigger_error() to the code. In this case I would suggest filing followups to cope with each deprecation separately. See https://www.drupal.org/core/deprecation

There also needs to be a change record to detail the introduce of the new extension lists - ThemeExtensionList and ThemeEngineExtensionList

In this case I would suggest filing followups to cope with each deprecation separately.

Agreed. I think it can be a single issue though as it should a relatively small patch. I'll fix this shortly.

There also needs to be a change record to detail the introduce of the new extension lists

There is: https://www.drupal.org/node/2709919

I went ahead and created the following issues as well:

#2972152: Deprecate ThemeHandlerInterface::rebuildThemeData and remove usages from core
#2972153: Deprecate ThemeHandlerInterface::getBaseThemes and remove usages from core

I went ahead and updated the CR as well: https://www.drupal.org/node/2709919

Back to RTBC.

1. +++ b/core/includes/module.inc
   @@ -22,43 +22,17 @@
    function system_list($type) {
   ...
   +  return \Drupal::service('extension.list.' . $type)->getInstalled();
   

   Should we catch when $type yields a 'service not found exception, i.e. is of a type we don't support and trigger and error?

   At present it would cause an uncaught exception.

2. +++ b/core/includes/module.inc
   @@ -22,43 +22,17 @@
   +  \Drupal::service('extension.list.profile')->reset();
      \Drupal::service('extension.list.module')->reset();
   ...
   +  \Drupal::service('extension.list.theme_engine')->reset();
   +  \Drupal::service('extension.list.theme')->reset();
   

   Starting to feel like these might need to be tagged services with a collector to clear them all in one go? That would be the path to deprecating system_list_reset right? Follow up?

3. +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
   @@ -0,0 +1,300 @@
   +      if (isset($engines[$engine])) {
   

   what happens if the engine doesn't exist?

4. +++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
   @@ -0,0 +1,300 @@
   +   * @internal
   ...
   +  protected function registerThemeNamespaces(array $themes) {
   

   Would that be a case to declare this as private and prevent subclassing or calling from children?

5. +++ b/core/modules/system/tests/themes/test_theme/test_theme.theme
   @@ -154,3 +156,16 @@ function test_theme_preprocess_theme_test_preprocess_suggestions__kitten__flamin
   +    new ThemeClass();
   ...
   +  catch (\Exception $exception) {
   

   Are we sure this would throw an Exception? In PHP 7 it would be an \Error right? In PHP 5 it would be a fatal, not an exception right?

   Should we instead be using class_exists?

6. +++ b/core/tests/Drupal/Tests/Core/Extension/ThemeExtensionListTest.php
   @@ -0,0 +1,263 @@
   +  define('DRUPAL_MINIMUM_PHP', '5.3.10');
   

   I know this is test only, but shouldn't this be 5.5.9?

1. Probably. Same should probably happen in drupal_get_filename() as well.
2. Yes, follow-up.
3. It defaults to twig, as per the comment above it: // Defaults to 'twig' (see self::defaults above).
4. Considering it's already marked as @internal and will be removed in #2941757: Extension System, Part IV: Properly register all installed extensions/namespaces during container generation, yes, that makes sense.
5. I really don't think it matters, but I'll change it. The test checks if $variables['message'] = 'Loading ThemeClass was successful.', which only happens after the class is constructed. If it errors, it doesn't get that value and the test would fail, yes?
6. Yep

Given that we're changing low-level extension system stuff in this issue we should also do some profiling to make sure we're not removing some static that the installer or the cold cache rebuild relies on to be performant.

#103 only does the cold cache. In the previous issue we found profiling the install also important.

Also looking at https://blackfire.io/profiles/compare/d4bea6a7-3035-4ca5-a562-23d568f779...() I'm not sure we've profiled what we think we're profiling. Since I would expect some of the new functions to be listed. That said this might be blackfire because on the free version it does not list all methods called. I miss XHProf.

make sure we're not removing some static that the installer or the cold cache rebuild relies on to be performant

The only statics we're actually removing are in system_list() and drupal_get_filename(). Now that they're using services and once all the extensions are found, it's cached.

We could keep the statics in both of these functions for performance/BC reasons if you want? This could help mitigate any currently unforeseen performance impacts until we actually remove usages throughout core, when #2940190: [meta] Deprecations of old functions in Extension system and sub-issues are dealt with.

That being said, I seriously doubt any of this will have any negative impact moving forward.

Also, we could just XHProf PHP 5.6, since PHP 7 is likely to not really be an issue here.

In the previous issue we found profiling the install also important.

What previous issue? #2208429: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList didn't mention profiling around the installation.

Re the previous issue and the installer see #2208429-243: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList and onwards.

Well, considering that you already optimized ExtensionList to take into account of this scenario, which this extends from, I don't think there's much more we can do here (aside from what I mentioned in #123).

Adding this tag merely for historical purposes (as this issue is ultimately the first step in this journey).

1. +++ b/core/includes/bootstrap.inc
   @@ -235,45 +232,30 @@ function drupal_get_filename($type, $name, $filename = NULL) {
   +    trigger_error(new FormattableMarkup('Unknown type specified: "@type". Must be one of: "core", "profile", "module", "theme", or "theme_engine".', [
   +      '@type' => $type,
   +    ]), E_USER_WARNING);
   +  }
   +  catch (\InvalidArgumentException $e) {
   +    // Catch the exception. This will result in triggering an error.
   +    // If the filename is still unknown, create a user-level error message.
   +    trigger_error(new FormattableMarkup('The following @type is missing from the file system: @name', [
   +      '@type' => $type,
   +      '@name' => $name
   +    ]), E_USER_WARNING);
   

   +1 for making the errors way more helpful!

2. +++ b/core/lib/Drupal/Core/Extension/ThemeEngineExtensionList.php
   @@ -0,0 +1,36 @@
   +  protected $defaults = [
   +    'dependencies' => [],
   +    'description' => '',
   +    'package' => 'Other',
   +    'version' => NULL,
   +    'php' => DRUPAL_MINIMUM_PHP,
   +  ];
   

   As a follow up we could explore to move these to the base class. These seemed to be shared between implementations

Starting to feel like these might need to be tagged services with a collector to clear them all in one go? That would be the path to deprecating system_list_reset right? Follow up?

Sure, there we go: #2973439: Make extension list services tagged services

@alexpott, could you respond to #125 please? I don't think this needs more profiling for the install scenario you mentioned.

So here's a comparison of installing minimal via drush with and without the patch. https://blackfire.io/profiles/compare/1de227a7-af08-4780-a6b3-93a9ea8ca8...

Interestingly it seems we have additional container builds that we did not have before. And we're calling drupal_get_filename more too. And doing more queries. We need to work out why.

+++ b/core/includes/bootstrap.inc
@@ -235,45 +232,30 @@ function drupal_get_filename($type, $name, $filename = NULL) {
+    trigger_error(new FormattableMarkup('Unknown type specified: "@type". Must be one of: "core", "profile", "module", "theme", or "theme_engine".', [
...
+    trigger_error(new FormattableMarkup('The following @type is missing from the file system: @name', [

+++ b/core/includes/module.inc
@@ -22,43 +24,27 @@
+    trigger_error(new FormattableMarkup('Unknown type specified: "@type". Must be one of: "core", "profile", "module", "theme", or "theme_engine".', [

We actually shouldn't be using FormattableMarkup in errors. We should be using sprintf() or just combining the strings. All error messages are escaped anyways. Yep this was already an issue but we're changing it here so let's fix it.

It got the profile by doing the following:

1. Setting up https://blackfire.io/ as per their docs
2. Doing composer require blackfire/php-sdk so I can add instrumentation to our code.
3. Adding
   

     $blackfire = new \Blackfire\Client();
     $config = new \Blackfire\Profile\Configuration();
     $probe = $blackfire->createProbe($config);
   

   to the top of the install_drupal() function

4. Adding $profile = $blackfire->endProbe($probe); to the end of the function
5. Using drush to install the minimal profile.

The problem grows are bit with a standard install... https://blackfire.io/profiles/compare/ad7e32c1-41a0-4ffb-81f4-e144f0a968...()

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -179,17 +164,7 @@ public function uninstall(array $theme_list) {
-      $themes = $this->systemThemeList();
-      // @todo Ensure that systemThemeList() does not contain an empty list
-      //   during the batch installer, see https://www.drupal.org/node/2322619.
-      if (empty($themes)) {
-        $this->refreshInfo();
-        $this->list = $this->list ?: [];
-        $themes = \Drupal::state()->get('system.theme.data', []);
-      }
-      foreach ($themes as $theme) {
-        $this->addTheme($theme);
-      }
+      array_map([$this, 'addTheme'], $this->getThemeExtensionList()->reset()->getInstalled());

@@ -442,23 +242,6 @@ public function getName($theme) {
-  /**
-   * Wraps system_list().
-   *
-   * @return array
-   *   A list of themes keyed by name.
-   */
-  protected function systemThemeList() {
-    return system_list('theme');
-  }

The reset() here looks really wrong. We were not doing a system list reset here before. How come this was added?

So here's a start on fixing the problem with the increased number of queries and activity during install plus it fixes a couple of coding standards minor things.

We use the same technique as the module list to do static caching during the install process and I've also fixed #132.

Here are the comparisons:
Head to #133 - https://blackfire.io/profiles/compare/11be1ed0-74ea-4227-835e-1d900defcd... - still 52 more queries.
#120 to #133 - https://blackfire.io/profiles/compare/748235c8-3021-4756-a962-2888ab0cbf... - we've lost 139 queries - so we're heading the right way :)

#133 is going to fail badly. One thing that sticks out now looking at this issue more carefully is what's the point of the ThemeHandler?

One reason this change is having an effect on the install is because we do:

  // Prepare for themed output. We need to run this at the beginning of the
  // page request to avoid a different theme accidentally getting set. (We also
  // need to run it even in the case of command-line installations, to prevent
  // any code in the installer that happens to initialize the theme system from
  // accessing the database before it is set up yet.)
  drupal_maintenance_theme();

in install_begin_request() - we're alway initialising the theme layer during the installer to work around things :(

+++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
@@ -216,32 +191,16 @@ public function addTheme(Extension $theme) {
-    $extension_config = $this->configFactory->get('core.extension');
-    $installed = $extension_config->get('theme');
-    // Only refresh the info if a theme has been installed. Modules are
-    // installed before themes by the installer and this method is called during
-    // module installation.
-    if (empty($installed) && empty($this->list)) {
-      return;
-    }

Removing this isn't great - that was added in #2872611: Optimise \Drupal\Core\Extension\ThemeHandler::refreshInfo() for the early installer. We need to ensure we maintain this.

+++ b/core/lib/Drupal/Core/Extension/ThemeExtensionList.php
@@ -0,0 +1,300 @@
+  /**
+   * The list of installed themes.
+   *
+   * @var string[]
+   */
+  protected $installedThemes = [];
...
+    // Cache the installed themes to avoid multiple calls to the config system.
+    if (!isset($this->installedThemes)) {
+      $this->installedThemes = $this->configFactory->get('core.extension')->get('theme') ?: [];
+    }

Checking isset() and initialising to an empty array is flawed. And the reason for all the failures in the previous patch.

https://blackfire.io/profiles/compare/455a3ab8-52f1-4c70-9bb7-ed9f142a29... - closing in only 38 more queries than HEAD. Still lots more container work.

Okay with this patch we're now at less queries than HEAD! https://blackfire.io/profiles/compare/381a44ab-efe2-4dd1-b868-3627879385...

Fixing the last error. This change makes no difference to the callgraph of minimal install.

Oh wow! I was just expecting a yes/no :D Glad you took that on though because profiling isn't really my strong suit (especially without XHprof).

I just couldn't see how this could have been further optimized, I was wrong. My apologies.

I've reviewed the interdiffs and gave the overall patch a good once over again. Only thing that really jumpped out at me was:

+++ b/core/lib/Drupal/Core/Installer/ExtensionListTrait.php
@@ -2,12 +2,10 @@
+ * @todo

Setting to CNW to fix this, but other than that I think this could be RTBC! Woo hoo!

@alexpott++

@vaplas we need further samples for that :) so much variation on bot runs. I've kicked off some more test runs.

For me there is still quite a bit of work to do here. One of the big things is should the ThemeHandler be the theme extension list service. I'm not sure that the decoupling here is worth it.

Looking at the existing ThemeHandler:

• install() deprecated
• uninstall() deprecated
• listInfo - this is a listing function
• addTheme - this adds a theme to the list
• refreshInfo - resetting the list
• reset - resetting the list
• rebuildThemeData - rebuilding data used for the list
• getBaseThemes - gets a special list
• getName - gets info from the list
• getDefault - gets the default theme
• setDefault - sets the default theme
• themeExists - checks if a theme is in the list
• getTheme - gets a theme from the list
• hasUi - checks if the theme should be listed in the UI.

So there does seem to be some differentiation in terms of getDefault / setDefault / hasUi - but everything else is really on the extension list. Having the ThemeHandler and the theme extension list would mean that we can only have one static list cache and not both \Drupal\Core\Extension\ThemeHandler::$list and \Drupal\Core\Extension\ExtensionList::$extensions which is wasteful and too many layers of caching. Also I would argue that things should just read from config to get and set the default theme so those methods and the config factory dependency shouldn't really be on the ThemeHandler.

Before I start work on merging here's a patch the properly guts themehandler and calls the theme list were possible.

So #145 is doing a bit less container work than #138 but is doing more YAML parsing which is odd. https://blackfire.io/profiles/compare/7abcdc68-2a88-4abd-a4bf-6ec75d9ee7...

I agree that ThemeHandler is basically useless now, just like ThemeManager will be once #2869859: [PP-1] Refactor theme hooks/registry into plugin managers happens.

Both ThemeHandler and ThemeManager will ultimately become artifacts because when they were created it was out of immediate necessity to migrate procedural code into OO code. It really didn't have a lot of architectural thought behind it (like much of the theme system's "OO conversion").

Gutting/deprecating ThemeHandler should probably be a separate follow-up issue IMO.

I'd like to see if we could possibly move the default getter/setter and hasUI methods elsewhere and get rid of the entire class.

At the very least, I think we should probably do it as a follow-up just to make sure we don't step on any toes.

Fixing the tests and #150 - nice spot @almaudoh. We still need to understand the extra info parsing this patch causes.

I talked to @dawehner yesterday and he pointed out that sun's original plan had extension discovery and handling as separate things. The current patch makes extension discovery aware of installation status so it's mixing responsibilities. I've rolled a patch locally that combines ThemeHandler and ThemeExtensionList which completes the mixing of responsibilities. Thinking some more I don't think this is a good way to go. I think the ThemeHandler should handle installed themes etc and the ThemeExtensionList should handle the listing.

So in light of #152 we already have ModuleHandler and ModuleExtensionList well and truly entangled. And the extension listing stuff already has the concept of installation status :( ho hum but let's try not to make it worse in this issue.

1. +++ b/core/includes/module.inc
   @@ -22,43 +24,27 @@
    function system_list($type) {
   

   In HEAD this function only works for themes it doesn't work for modules or anything. With this patch we change it to work for every type again. That's not a good idea. We also break it for the filepaths type. Not that that is documented. Imo we shouldn't change what this method can return.

2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
   @@ -256,6 +262,26 @@ public function get($extension_name) {
   +  /**
   +   * Returns a list of installed extensions.
   +   *
   +   * @return \Drupal\Core\Extension\Extension[]
   +   *   A list of installed extension objects, keyed by machine name.
   +   */
   +  public function getInstalled() {
   +    return array_intersect_key($this->getList(), array_flip($this->getInstalledExtensionNames()));
   +  }
   +
   +  /**
   +   * Returns a list of uninstalled extensions.
   +   *
   +   * @return \Drupal\Core\Extension\Extension[]
   +   *   A list of uninstalled extension objects, keyed by machine name.
   +   */
   +  public function getUninstalled() {
   +    return array_diff_key($this->getList(), array_flip($this->getInstalledExtensionNames()));
   +  }
   

   Let's not add these methods we don't need to. system_list() is dead code and we can do the logic that's need in ThemeHandler.

3. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
   @@ -314,6 +340,9 @@ protected function doList() {
   +      // Add extension-type-specific information.
   +      $this->alterExtensionInfo($extension);
   +
          // Invoke hook_system_info_alter() to give installed modules a chance to
          // modify the data in the .info.yml files if necessary.
          $this->moduleHandler->alter('system_info', $extension->info, $extension, $this->type);
   
   @@ -540,4 +569,12 @@ public function getPath($extension_name) {
   +  protected function alterExtensionInfo(Extension $extension) {}
   

   I'm not sure we should call this alter... and then trigger a proper alter next in the code. That's confusing.

Here's a start on #153.

Here's a proper deprecation of system_list(). I think because this only lists themes we need to do this here - also we need to ensure we don't change it's behaviour.

I'm confused, you said in #113 that [triggering] deprecations should be done in a follow-up issue (so we can remove code that uses it and not trigger errors). That's why I created the follow-up issues.

Fixing the test.

Re #158 - the only use of system_list() in HEAD is to list themes. This patch makes fundamental changes to it and in earlier version unnecessarily expanded system_lists()'s capabilities. In reviewing all the code it makes it obvious that with this patch core has no reason to call system_list() anymore. Therefore this patch should take care to deprecate it. Especially as this patch extended the public API of the extension listing system to support system_list() - which doesn't make sense to me. It hard to explain to someone the difference between ExtensionList::getInstalled() and ExtensionList::getAllInstalledInfo() but now we don't have to because ExtensionList::getInstalled() is no longer being added.

@vaplas does this patch make InstallUninstallTest worse?

Some more improvements. We now have less queries and less info parsing. Obviously we still have more container stuff because we have another service - https://blackfire.io/profiles/compare/b19f2764-50c2-4241-9168-0746436d22...