Problem/Motivation

Part of #2186491: [meta] D8 Extension System: Discovery/Listing/Info to untangle the Drupal 8 extension (module, profile, theme, theme_engine) system.

Proposed resolution

  1. Provide a common way to list extensions (enabled and not enabled ones)

    → Central data source for ExtensionList (ModuleExtensionList, ThemeExtensionList, ...)

  2. → Provide common methods for e.g. resolving dependencies

    → Use it directly for profiles and theme engines.

This issue deals with the listing of available profiles/modules only, theme and theme_engine will be dealt with in #2659940: Extension System, Part III: ThemeExtensionList.

Review instructions

Note: There is a change in DrupalKernel around sessions, read more about on #2208429-98: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList

Remaining tasks

* Switch from extensionExists($name) to extensionExists($machine_name);

Final reviews
Commit

User interface changes

None

API changes

New ExtensionList, ModuleList and ProfileList classes. New module_listing and profile_listing services to access information on extensions.

Data model changes

Module info is now stored in the default cache and no longer in state.

Previous Report

  1. The "list" of extensions defines the list of enabled/installed extensions.
  2. There is a list for each extension type: profile, module, theme, theme_engine.

The installation profile

  1. is defined as $settings['install_profile'] in settings.php (but only its name)
  2. is added to the list of enabled modules with a weight of 1000 in drupal_rebuild_module_data()
  3. is contained in the %container.modules% parameter on the service container

There is a second "parent" installation profile when a test is executed, which

  1. is defined as $config['simpletest.settings']['parent_profile'] in config/settings.php (only its name)
  2. is NOT added to the list of enabled modules
  3. is NOT contained in the %container.modules% parameter of the service container
  4. is only used by ExtensionDiscovery to additionally find extensions in the parent installation profile, from which tests were executed.
  • The list of enabled modules

    1. is maintained by ModuleHandler::install() and ModuleHandler::uninstall()
    2. is stored in the core.extension configuration object
    3. is contained in the %container.modules% parameter on the service container
    4. can be manipulated at runtime (without touching configuration) via ModuleHandler::setModuleList()
    5. is consumed by system_rebuild_module_data() to enhance module info with a $status property reflecting the installation status

    Aside from that and the system_rebuild_module_data() aspect, management and maintenance of the module list is pretty clean already.

  • The list of enabled themes

    1. is maintained by ThemeHandler::install() and ThemeHandler::uninstall()
    2. is stored in the core.extension configuration object
    3. is consumed, enhanced with theme info, and cached by system_list()
    4. is consumed by system_rebuild_theme_data() to enhance theme info with a $status property reflecting the status (whereas there is no installation status yet)

    Except of the core.extension:theme configuration, this list factually does not really exist on its own right now — it is deeply intertwined with theme info currently.

    The list is also not stored in an efficient way like the %container.modules% parameter on the service container; i.e., the system always has to consult the configuration system to retrieve it.

  • The list of theme engines

    1. does not exist at all.

    ThemeHandler::rebuildThemeData() blatantly performs an ExtensionDiscovery to simply retrieve all available engines.

  • Proposed solution

    1. Provide a common way to list extensions (enabled and not enabled ones)

      → Central data source for ExtensionList (ModuleExtensionList, ThemeExtensionList, ...)

    2. → Provide common methods for e.g. resolving dependencies

      → Use it directly for profiles and theme engines.

    3. Try to untangle at least some of the theme list/data/info mess.

    This issue deals with the listing of available profiles/modules only, theme and theme_engine will be dealt with in #2659940: Extension System, Part III: ThemeExtensionList.

    Files: 

    Comments

    sun’s picture

    sun’s picture

    catch’s picture

    If we're doing this we should also separate out install/uninstall etc. from list retrieval and hook invocation. Most of the ModuleHandler dependencies are undeclared, I'm not sure it'd be possible to actually declare them without circular references, and even if it is they make more sense split up.

    sun’s picture

    Yeah, splitting out the install/enable/disable/uninstall functionality into a dedicated ExtensionInstaller class would be a good idea.

    With that goal in mind, I already "prepared" ThemeHandler in #1067408: Themes do not have an installation status by copy/pasting the code and logic from ModuleHandler (even though the code contains some nonsensical logic, which felt silly to copy as-is, but did so nevertheless for the sake of making them uniform).

    donquixote’s picture

    I was trying to get an idea how ModuleHandler works, and there was this comment sending me here.. so here are some thoughts.

    A good idea seems to be to split "available extensions" (with discovery) from "enabled extensions". The former can be rather low-level, not depending on enabled modules. The "available extensions" would be without any hook_system_info_alter() applied. Instead, there would be an additional level on top of available modules and enabled modules, "altered available module info". This may limit what can be done with hook_system_info_alter(), but also make it more predictable.

    I would suggest to split up ModuleHandler even more than that.
    Currently what I imagine is to split it into really small pieces like ModuleImplements, ModuleImplementsDiscovery, ModuleImplementsAlter, HookSystem, etc, but I might change my mind about that.

    One obvious benefit of small pieces is that each piece will be individually unit-testable, and that pieces can be replaced by mock objects. E.g. you could create a HookSystem with a fake (mock) ModuleImplementsDiscovery, or with a fake module list, so you can test it without setting up a Drupal installation, or without actually creating example implementations in test modules.

    Slicing it into pieces could also make it easier to solve the circular dependencies.

    Another solution for the circular dependency can be passing stuff to the actual method that does the business of an object, instead of giving the object a permanent reference in the constructor.

    E.g.

    $discovery = new ModuleImplementsDiscovery();
    $implementations = $discovery->discoverImplementations($module_list, $hook);
    

    instead of

    $discovery = new ModuleImplementsDiscovery($module_list);
    $implementations = $discovery->discoverImplementations($hook);
    

    We could also do some observer stuff, so that ModuleImplements automatically gets a reset whenever ModuleList is reset, if these end up to be in two different classes.

    sun’s picture

    @donquixote: Discovery of available extensions has been separated into ExtensionDiscovery already.

    The primary objective of this issue to "consolidate" the base concepts of ModuleHandler + ThemeHandler into a generic ExtensionHandler, since their code is fairly similar.

    This change MAY also involve to split the install/enable/disable/uninstall functionality into a separate logical piece of an ExtensionInstaller or similar.

    However, changing internals of the module hook system in ModuleHandler is not within the scope of this issue and off-topic here, as that is too specific to modules. — Some level of theme interaction is currently mixed into that, but #2228093: Modernize theme initialization has a concrete plan and very good reasons to remove themes altogether from it.

    In other words, this issue here architecturally operates on a macro/meta level, having the goal of introducing a formal notion of a "list of active/enabled extensions" (per extension type), since that notion/logic only exists in the form of inconsistent one-off implementations currently (and does not exist at all for half of the known extension types).


    PS: Regarding module hooks, have a look at #2237831: [PP-2] Allow module services to specify hooks

    jibran’s picture

    So what's next here?

    dawehner’s picture

    dawehner’s picture

    Status: Active » Needs review
    FileSize
    12.68 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,918 pass(es), 8 fail(s), and 7 exception(s). View

    Alright, here is a first version of a extension listing service, using the module handler as example.
    Once we have that we could also get rid of the other custom code in ThemeHandler

    Status: Needs review » Needs work

    The last submitted patch, 10: 2208429-10.patch, failed testing.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    23.02 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
    13.33 KB

    Let's fix failures!

    dawehner’s picture

    Mh, that seem to get stuck in the bot, but --all works kinda locally. Anyone has a concrete suggestion?

    dawehner’s picture

    FileSize
    19.94 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,003 pass(es), 2 fail(s), and 0 exception(s). View
    8.61 KB

    Let's do a bit less, and fix the tests first.

    The last submitted patch, 12: 2208429-12.patch, failed testing.

    Status: Needs review » Needs work

    The last submitted patch, 14: 2208429-14.patch, failed testing.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    20.42 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,006 pass(es), 1 fail(s), and 0 exception(s). View
    689 bytes

    Let's see whether this is enough so far.

    Status: Needs review » Needs work

    The last submitted patch, 17: 2208429-17.patch, failed testing.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    20.57 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,509 pass(es). View
    2.72 KB

    Wow, that was tricky to figure out, but as @berdir wrote in IRC, these tests find pretty bugs.

    stefan.r’s picture

    Having worked with this in the other issue, this does look nicer already! :)

    1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,196 @@
      +  public function getName($name) {
      +    $extensions = $this->listExtensions();
      +    if (isset($extensions[$name])) {
      +      return $extensions[$name]->info['name'];
      +    }
      +    return 'n/a';
      

      Should this really have n/a as a fallback? What if the extension has accidently been removed and we can't find the info file (we do seem to account for this in drupal_get_filename), shouldn't we at least show the machine name?

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,196 @@
      +    if ($extensions = $this->state->get($this->getStateKey())) {
      +      return $extensions;
      +    }
      

      This should probably set $this->extensions?

    3. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,196 @@
      +  protected function doListExtensions() {
      +    $listing = new ExtensionDiscovery($this->root);
      +
      

      This method name could be clearer as it actually rebuilds the extension list. Also just out of interest, we do a bunch of other stuff in system_rebuild_module_data() with this data (add weight, status, schema version, add dependencies, cache file names), are there any plans of moving that out?

    dawehner’s picture

    Assigned: sun » Unassigned
    FileSize
    23.44 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
    7.74 KB

    Thank you for your review.

    Should this really have n/a as a fallback? What if the extension has accidently been removed and we can't find the info file (we do seem to account for this in drupal_get_filename), shouldn't we at least show the machine name?

    What about throwing an exception in those cases?

    This should probably set $this->extensions?

    Good point!

    This method name could be clearer as it actually rebuilds the extension list. Also just out of interest, we do a bunch of other stuff in system_rebuild_module_data() with this data (add weight, status, schema version, add dependencies, cache file names), are there any plans of moving that out?

    Well yeah, my strategy was to get the first bit converted and passing and then tackle the next one.
    Let's see whether this works already.

    Status: Needs review » Needs work

    The last submitted patch, 21: 2208429-21.patch, failed testing.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    23.43 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
    789 bytes

    Ups, that was horrible.

    Status: Needs review » Needs work

    The last submitted patch, 23: 2208429-23.patch, failed testing.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    23.53 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,357 pass(es). View
    1.39 KB

    Just fixes.

    Status: Needs review » Needs work

    The last submitted patch, 25: 2208429-25.patch, failed testing.

    dawehner queued 25: 2208429-25.patch for re-testing.

    dawehner’s picture

    Issue summary: View changes
    Status: Needs work » Needs review

    Updates the IS a bit.

    jibran’s picture

    I think it's good to go but we need a change notice and beta evaluation for this.

    stefan.r’s picture

    FileSize
    23.47 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,635 pass(es). View

    reroll

    Looks like this gets rid of the circular call @effulgentsia mentioned here? http://www.drupal.org/node/2281989#comment-9840429

    Wim Leers’s picture

    Status: Needs review » Needs work

    Looks good. But either the IS is outdated, or this still needs to be updated to handle themes/profiles/theme engines as well.

    1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,206 @@
      +   * The type of the extension, either module or theme.
      
      +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,146 @@
      +   *   The module info.
      

      s/module/'module'/
      s/theme/'theme'/

      What about install profiles and theme engines, like the IS says?

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,206 @@
      +   * entries to this raw listing.
      

      s/entries/extensions/

    3. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,206 @@
      +      // @todo Remove $type argument, obsolete with $module->getType().
      +      $this->moduleHandler->alter('system_info', $extensions[$key]->info, $extensions[$key], $this->type);
      

      Can be done now?

    4. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,146 @@
      +      // Installation profiles are hidden by default, unless explicitly specified
      

      80 cols.

    dawehner’s picture

    Well yeah, I was thinking about just doing it for modules for now and update the usages in themes and install profiles later ... feel free to disagree with that process,
    but it seemed to be the most straightforward one.

    dawehner’s picture

    We (alexpott, xjm, stefan_r, dawehner) tried hard to discuss, whether this issue can be done in 8.0.x.

    It seems to be that solving that in a sane way is 8.1.x stuff, though.
    @alexpott @xjm
    If you have changed your mind, please comment.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    24.8 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2208429-34.patch. Unable to apply patch. See the log in the details link for more information. View
    1.54 KB

    Can be done now?

    I'd like to keep the amount of changes as small as possible.

    Status: Needs review » Needs work

    The last submitted patch, 34: 2208429-34.patch, failed testing.

    jibran queued 34: 2208429-34.patch for re-testing.

    The last submitted patch, 34: 2208429-34.patch, failed testing.

    dawehner’s picture

    Issue tags: +Needs reroll

    Just tagging

    amitgoyal’s picture

    Status: Needs work » Needs review
    Issue tags: -Needs reroll
    FileSize
    24.8 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/system/system.module. View

    Reroll of #34.

    Status: Needs review » Needs work

    The last submitted patch, 39: 2208429-39.patch, failed testing.

    amitgoyal’s picture

    Status: Needs work » Needs review
    FileSize
    24.8 KB
    FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2208429-41.patch. Unable to apply patch. See the log in the details link for more information. View
    490 bytes

    As per #39, invalid PHP syntax in core/modules/system/system.module fixed.

    Status: Needs review » Needs work

    The last submitted patch, 41: 2208429-41.patch, failed testing.

    anavarre queued 41: 2208429-41.patch for re-testing.

    The last submitted patch, 41: 2208429-41.patch, failed testing.

    anavarre’s picture

    Issue tags: +Needs reroll
    dawehner’s picture

    Version: 8.0.x-dev » 8.1.x-dev

    Well, we will not work on that before 8.1 and then need a proper strategy.

    alvar0hurtad0’s picture

    Here's the reroll

    alvar0hurtad0’s picture

    Status: Needs work » Needs review

    I forgot the status (again).

    :D

    jonhattan’s picture

    Status: Needs review » Needs work
    1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,206 @@
      +    if (!isset($extensions[$name])) {
      +      return $extensions[$name];
      +    }
      

      I guess it is isset(), without !.

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,206 @@
      +    $this->cache->get($this->getCacheId());
      

      you want to actually *set* cache here.

    dawehner’s picture

    Yeah that that point we need to address with the big complain that this is adding another cache laying without getting better with the full crazy situation.
    We need to come up with a full plan for 8.1 and do small steps towards that full plan.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    26.78 KB

    Did some reroll and a general different approach::

    a) We now use cache on write
    b) But still rebuild the cache info if accessed directly.

    Status: Needs review » Needs work

    The last submitted patch, 51: 2208429-51.patch, failed testing.

    The last submitted patch, 51: 2208429-51.patch, failed testing.

    almaudoh’s picture

    Status: Needs work » Needs review
    FileSize
    4.51 KB
    26.73 KB

    The last patch was failing because of a recursive function call from ExtensionList::recalculateInfo() and ExtensionList::recalculateFilenames().

    So my understanding of this is that at the end of the refactoring the following should be the mapping of old procedural functions to new ExtensionList methods

    Old procedural function New ExtensionList method
    system_get_info() ::getInfo()
    _system_rebuild_module_data() ::reset()
    ::listExtensions()
    system_rebuild_module_data() ::reset()
    ::listExtensions()
    drupal_get_filename() ExtensionList::getFilenames()

    Here's a fix that does that...

    theme_listing has not been implemented yet even though it is used in system_get_info() so expecting some test failures.

    Status: Needs review » Needs work

    The last submitted patch, 54: extension_system_part-2208429-54.patch, failed testing.

    almaudoh’s picture

    Let's just focus on modules for now...

    dawehner’s picture

    Nice work @almaudoh

    +++ b/core/modules/system/system.module
    @@ -893,9 +893,18 @@ function system_get_info($type, $name = NULL) {
    -    /** @var \Drupal\Core\Extension\ModuleExtensionList $module_listing */
    -    $module_listing = \Drupal::service('theme_listing');
    -    return $module_listing->getInfo($name);
    +    // @todo
    +    $info = array();
    +    $list = system_list($type);
    +    foreach ($list as $shortname => $item) {
    +      if (!empty($item->status)) {
    +        $info[$shortname] = $item->info;
    +      }
    +    }
    +    if (isset($name)) {
    +      return isset($info[$name]) ? $info[$name] : array();
    +    }
    +    return $info;
    

    Ah, so this reverts the theme handling bit?

    almaudoh’s picture

    Ah, so this reverts the theme handling bit?

    Yeah. The theme handling part is a bit messy, so I wanted to get the module handling part passing tests first before digging into the theme bit.

    Status: Needs review » Needs work

    The last submitted patch, 56: extension_system_part-2208429-56.patch, failed testing.

    almaudoh’s picture

    Status: Needs work » Needs review
    FileSize
    2.81 KB
    28.44 KB

    Now drupal_get_path() is limited in the ModuleInstaller, so we replace it with the module_listing service. Also, having problems with \Drupal::state() saving to the database, so commented that out just to confirm that the thing works first.

    This is far from complete, but sending to testbot still...

    Status: Needs review » Needs work

    The last submitted patch, 60: extension_system_part-2208429-60.patch, failed testing.

    Mile23’s picture

    I think it's great to be normalizing all this stuff so it's only in one place, and meets a bunch of different needs.

    Some stuff I see...

    1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,298 @@
      +    \Drupal::state()->delete("system.{$this->type}.files");
      ...
      +      $file_names = \Drupal::state()->get("system.{$this->type}.files");
      ...
      +//    \Drupal::state()->set("system.{$this->type}.files", $file_names);
      

      If it needs to be the state service, then inject it in the constructor.

      But it should be in the cache, instead. I don't think we have a way to clear state from the UI, and having a filesystem problem and a non-clearable cache is bad news for users.

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,298 @@
      +  /**
      +   * Returns all available extensions.
      +   *
      +   * @return \Drupal\Core\Extension\Extension[]
      +   */
      +  public function listExtensions() {
      

      All available or all enabled? Maybe rename the method to listEnabledExtensions() if the latter.

    3. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,160 @@
      +    if ($profile && isset($profiles[$profile])) {
      +      // Prime the drupal_get_filename() static cache with the profile info file
      +      // location so we can use drupal_get_path() on the active profile during
      +      // the module scan.
      +      // @todo Remove as part of https://www.drupal.org/node/2186491
      +      drupal_get_filename('profile', $profile, $profiles[$profile]->getPathname());
      

      Priming this cache doesn't help you if you're normalizing on ExtensionDiscovery, only wastes time and eats up memory in a static.

    almaudoh’s picture

    Status: Needs work » Needs review
    FileSize
    577 bytes
    29 KB

    So drupal_get_path() / drupal_get_filename() are no more effective. Let's see how many test fails this fixes.

    #62:
    1. Totally agree it should be in cache, not state, but I guess the main consideration was maintaining BC, since we don't know how many contrib modules out there are using \Drupal::state() directly to get module info.
    2. All available. There's a boolean ModuleExtension::status to know if an extension is enabled.
    3. Yeah, will be removed in the next patch.

    Status: Needs review » Needs work

    The last submitted patch, 63: extension_system_part-2208429-63.patch, failed testing.

    Berdir’s picture

    1. +++ b/core/includes/bootstrap.inc
      @@ -251,6 +251,9 @@ function drupal_get_filename($type, $name, $filename = NULL) {
       function drupal_get_path($type, $name) {
      +  if ($type === 'module' || $type === 'profile') {
      +    return dirname(\Drupal::service('module_listing')->getFilenames()[$name]);
      +  }
      

      This could result in notices if $name is missing I guess? What about adding getFilename($name)?

      I guess we can't officially deprecate this function as it's still needed for themes, but maybe we can add documentation on what you should call instead for modules?

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,298 @@
      +  public function getName($name) {
      

      getName($name) reads a bit strange. We have two different names apparently. Not sure if we can do something about it, but possibly worth thinking about. Maybe start using $machine_name or so?

    3. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,298 @@
      +      // @todo Remove $type argument, obsolete with $module->getType().
      

      Is that todo from the old code?

      We can't do that before 9.x, so maybe just get rid of it?

    4. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,298 @@
      +   * @param string $name
      +   *   (optional) The name of a module or theme whose information shall be
      +   *   returned. If omitted, all records for the provided $type will be returned.
      +   *   If $name does not exist in the provided $type or is not enabled, an empty
      +   *   array will be returned.
      

      I think we should stop with that "if empty, return all" stuff. Add two methods, one that returns all, one with a required $name argument.

    5. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,298 @@
      +
      +  protected function recalculateInfo() {
      

      missing docs.

    6. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,298 @@
      +    // Store the module information in cache. This cache is cleared by
      +    // calling system_rebuild_module_data(), for example, when listing
      

      still true, is this a method now?

    7. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,298 @@
      +   * Return a list of extension folder names keyed by extension name.
      

      Returns, missing description below.

    8. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,298 @@
      +//    \Drupal::state()->set("system.{$this->type}.files", $file_names);
      

      why commented out?

    9. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,160 @@
      +      // Prime the drupal_get_filename() static cache with the profile info file
      +      // location so we can use drupal_get_path() on the active profile during
      +      // the module scan.
      +      // @todo Remove as part of https://www.drupal.org/node/2186491
      +      drupal_get_filename('profile', $profile, $profiles[$profile]->getPathname());
      +    }
      

      this calls back in here again I think, so we should update that and call this->something.

    10. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,160 @@
      +      $this->moduleDataEnsureRequired($extension, $extensions);
      

      the method reads like a converted function name. methods usually start with a verb, maybe we can just leave out the moduleData prefix?

    11. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,160 @@
      +        $dependency_name = ModuleHandler::parseDependency($dependency)['name'];
      

      it's just a static method, but it's still kind of a dependency, while ModuleHandler also depends on this. Maybe move the static method here, or make it a trait?

    12. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
      @@ -722,8 +722,7 @@ public function getModuleDirectories() {
         public function getName($module) {
      -    $info = system_get_info('module', $module);
      -    return isset($info['name']) ? $info['name'] : $module;
      +    return \Drupal::service('module_listing')->getName($module);
         }
      

      As mentioned before, we should inject this.

    13. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -171,7 +171,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
      -            $module_path = drupal_get_path('module', $name);
      +            $module_path = dirname(\Drupal::service('module_listing')->getFilenames()[$name]);
      

      yeah, this can't be a replacement. as I said, we should add getFilename($name), and looking at this, we should also add getPath($name)

    14. +++ b/core/modules/book/src/Tests/BookUninstallTest.php
      @@ -82,6 +82,10 @@ public function testBookUninstall() {
           // No nodes exist therefore the book module is not required.
           $module_data = _system_rebuild_module_data();
      +    /** @var \Drupal\Core\Extension\ModuleExtensionList $module_listing */
      +    $module_listing = \Drupal::service('module_listing');
      +    $module_listing->reset();
      +    $module_data = $module_listing->listExtensions();
      

      If this is a replacement for _system_rebuild_module_data() then we should remove the old call?

    15. +++ b/core/modules/system/system.module
      @@ -888,27 +888,12 @@ function system_check_directory($form_element, FormStateInterface $form_state) {
      -    $info = &drupal_static(__FUNCTION__);
      

      ẁe should remove the drupal_static_reset() calls for this function

    16. +++ b/core/modules/system/system.module
      @@ -888,27 +888,12 @@ function system_check_directory($form_element, FormStateInterface $form_state) {
      +    // @todo
           $info = array();
           $list = system_list($type);
           foreach ($list as $shortname => $item) {
      

      is this meant to be done in this issue? if so, then we can deprecate this and a lot of other functions for real.

    17. +++ b/core/modules/system/system.module
      @@ -930,105 +915,8 @@ function system_get_info($type, $name = NULL) {
      -function _system_rebuild_module_data_ensure_required($module, &$modules) {
      

      this is prefixed with _, but I'm not sure if we can really safely remove that?

    18. +++ b/core/modules/system/system.module
      @@ -1038,32 +926,8 @@ function _system_rebuild_module_data_ensure_required($module, &$modules) {
      -  $modules_cache = &drupal_static(__FUNCTION__);
      

      same here for the static reset thing.

    19. +++ b/core/modules/system/system.module
      @@ -1038,32 +926,8 @@ function _system_rebuild_module_data_ensure_required($module, &$modules) {
      +  $modules_cache = \Drupal::service('module_listing')->listExtensions();
         return $modules_cache;
      

      keeping $modules_cache is pretty useless, just return it.

    20. +++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
      @@ -120,6 +120,7 @@ function testDependencyResolution() {
           drupal_static_reset('system_rebuild_module_data');
      +    \Drupal::service('module_listing')->reset();
      

      yeah, like those, we should remove the old calls.

    Berdir’s picture

    Also, looking at module.inc for other functions to be replaced by this service (I think we want to remove this and other files like this completely in 9.x). Likely in follow-ups:

    * system_list() is the part that would move in a ThemeExtensionList service I guess?
    * module_load_include() has an interesting todo
    * drupal_required_modules()
    * module_config_sort() could be a static method somewhere.
    * Not sure where module_set_weight could live, module installer maybe?

    dawehner’s picture

    Thanks a lot for that review @berdir!

    almaudoh’s picture

    Status: Needs work » Needs review
    FileSize
    28.67 KB
    37.76 KB

    Lots of changes in this patch. I reverted the hack in #63 and properly addressed the underlying issues. Theme still not yet included in this though.

    1. Introduced profile_listing service ProfileExtensionList to decouple profiles from the special behavior of modules listing.
    2. Made the ExtensionDiscovery an instance property to allow ModuleExtensionList set the profile directories before scanning for modules
    3. Added ExtensionList::getFilename() and ExtensionList::getPath() to directly replace drupal_get_filename() and drupal_get_path()
    4. Removed all calls to drupal_get_(path|filename) in the listing classes, therefore allowing:
    5. drupal_get_filename() now calls out to ExtensionList::getFilename() for modules and profiles (themes will be done later)

    #65: Thanks for the review @berdir

    1. Good point! Reverted that change and implemented ExtensionList::getFilename($name). Also see point 3) & 5) above.
    2. Used $machine_name. Can still be improved.
    3. From old code. Removed.
    4. Added getAllInfo() method.
    5. Done.
    6. Removed.
    7. Fixed.
    8. Uncommented.
    9. Rewrote that section.
    10. Changed to ensureRequiredDependencies
    11. Will punt this to the next patch...
    12. Is that not a BC break? If not, then in next patch...
    13. Done
    14. Removed.
    15. Removed.
    16. It should be in this issue (unless the patch gets too big for review). Deprecations may be done in another issue when conversions of drupal_get_(path|filename) is completed.
    17. Didn't find it used anywhere else. Was moved to ModuleExtensionList::ensureRequiredDependencies
    18. Removed.
    19. Done.
    20. Done.

    Status: Needs review » Needs work

    The last submitted patch, 68: extension_system_part-2208429-68.patch, failed testing.

    almaudoh’s picture

    Status: Needs work » Needs review
    FileSize
    11 KB
    38.34 KB

    As a matter of fact, we cannot use State to store extension lists and info because State depends on the system module, and the system module needs module lists for installation -> a circular dependency. So we have no choice but to use cache.
    Fixed a couple of bugs and test fails and reverted some changes missed out in previous patch.

    Status: Needs review » Needs work

    The last submitted patch, 70: extension_system_part-2208429-70.patch, failed testing.

    Berdir’s picture

    12. That's a good question. I'm not sure myself. We usually say that the constructor isn't part of the API, so we are allowed to change it in minor releases. But especially on base classes and commonly subclassed classes it is indeed very problematic. There's a documentation page somewhere that tries to specify this.

    1. +++ b/core/includes/bootstrap.inc
      @@ -198,11 +198,17 @@ function drupal_get_filename($type, $name, $filename = NULL) {
      +    if (isset($filename)) {
      +      // Manually add the info file path of an extension.
      +      return \Drupal::service($service)->setFilename($name);
      +    }
      

      This seems to be missing an argument?

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -284,6 +328,15 @@ public function getFilenames() {
      +   * @return array
      +   *   An array of .info.yml file locations keyed by the extension name.
      

      I think we usually document this as string[] as we know the inner type.

    almaudoh’s picture

    Status: Needs work » Needs review
    FileSize
    1.49 KB
    39.3 KB

    More fixes...

    dawehner’s picture

    nice work!

    1. +++ b/core/includes/bootstrap.inc
      @@ -211,15 +217,6 @@ function drupal_get_filename($type, $name, $filename = NULL) {
      -    // If the pathname of the requested extension is not known, try to retrieve
      -    // the list of extension pathnames from various providers, checking faster
      -    // providers first.
      -    // Retrieve the current module list (derived from the service container).
      -    if ($type == 'module' && \Drupal::hasService('module_handler')) {
      -      foreach (\Drupal::moduleHandler()->getModuleList() as $module_name => $module) {
      -        $files[$type][$module_name] = $module->getPathname();
      -      }
      -    }
      

      How do we deal with the faster provider optimization here?

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,413 @@
      +    $extensions = $this->listExtensions();
      +    if (isset($extensions[$machine_name])) {
      +      return $extensions[$machine_name]->info['name'];
      +    }
      

      This method could be $this->getExtension($name)->info['name'] directly

    3. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,413 @@
      +    throw new \InvalidArgumentException(new FormattableMarkup('The @type %name does not exist.', ['@type' => $this->type, '%name' => $machine_name]));
      ...
      +    throw new \InvalidArgumentException(new FormattableMarkup('The @type %name does not exist.', ['@type' => $this->type, '%name' => $name]));
      

      Note: We no longer use FormattableMarkup for exception messages. Let's use "The {$this->type} $machine_name does not exist."

    4. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,413 @@
      +  /**
      +   * Returns information about a specified module or theme.
      +   *
      

      Should that kind of documentation in general just talk about extensions?

    5. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,413 @@
      +  public function getInfo($name) {
      ...
      +    return isset($this->extensionInfo[$name]) ? $this->extensionInfo[$name] : NULL;
      +  }
      

      Do we want to throw an exception as well, if the extension don't exist?

    6. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,413 @@
      +  /**
      +   * Sets the filename for an extension.
      +   *
      

      It would be nice to document the usecases of setting the filename. In general I think we want to document that its not encouraged to call it.

    7. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,413 @@
      +    $this->addedFileNames[$extension_name] = $filename;
      

      It would be nice to document why we have this additional property on top of the fileNames one

    8. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,413 @@
      +      trigger_error(new FormattableMarkup('The following @type is missing from the file system: @name', array('@type' => $this->type, '@name' => $extension_name)), E_USER_WARNING);
      

      same as before

    9. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,413 @@
      +  public function getPath($extension_name) {
      

      IMHO we should document that this method can also throw an exception.

    10. +++ b/core/modules/book/src/Tests/BookUninstallTest.php
      @@ -81,7 +81,7 @@ public function testBookUninstall() {
      -    $module_data = _system_rebuild_module_data();
      +    $module_data = \Drupal::service('module_listing')->reset()->listExtensions();
      

      Nice!

    11. +++ b/core/modules/system/system.module
      @@ -888,27 +888,10 @@ function system_check_directory($form_element, FormStateInterface $form_state) {
      +    // @todo
           $info = array();
           $list = system_list($type);
      

      I think it would be fine to have a dedicated follow up to deal with the theme aspect. Do you think we should create one?

    dawehner’s picture

    @almaudoh I'll work on some unit tests at the meantime.

    dawehner’s picture

    +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
    @@ -0,0 +1,413 @@
    +    if (!isset($extensions[$name])) {
    

    This is wrong, it should be if (isset($extensions)) instead

    Status: Needs review » Needs work

    The last submitted patch, 73: extension_system_part-2208429-72.patch, failed testing.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    46.41 KB
    7.97 KB

    Here is a unit test.

    Status: Needs review » Needs work

    The last submitted patch, 78: 2208429-78.patch, failed testing.

    almaudoh’s picture

    Thanks for the reviews.
    #72

    1. Fixed in #73
    2. Fixed in this patch

    #74

    1. I was thinking the module listing service would become the only source of this info. Maybe revert that hunk and check ModuleHandler first?
    2. Done
    3. Done
    4. Done
    5. Done
    6. Improved the documentation
    7. It was documented already, do we need to improve what's there?
    8. Done
    9. Done
    10. :)
    11. Create one: #2659940: Extension System, Part III: ThemeExtensionList, also updated the @todo

    Thanks for the unit test @dawehner

    Status: Needs review » Needs work

    The last submitted patch, 80: extension_system_part-2208429-80.patch, failed testing.

    dawehner’s picture

    I was thinking the module listing service would become the only source of this info. Maybe revert that hunk and check ModuleHandler first?

    It would be great to talk with catch about it, whether he believes we still need that optimization.

    The last submitted patch, 80: extension_system_part-2208429-80.patch, failed testing.

    almaudoh’s picture

    Woah!! is DrupalCI down???

    The last submitted patch, 80: extension_system_part-2208429-80.patch, failed testing.

    almaudoh’s picture

    Status: Needs work » Needs review
    FileSize
    40.17 KB

    Looks like CI is choking on the unit test. Here's a patch without the unit test just to confirm...

    Status: Needs review » Needs work

    The last submitted patch, 86: 2208429.patch, failed testing.

    catch’s picture

    Totally agree it should be in cache, not state, but I guess the main consideration was maintaining BC, since we don't know how many contrib modules out there are using \Drupal::state() directly to get module info.

    That's OK to change in a minor version. Module info happening to be in state isn't part of the public API, per https://www.drupal.org/core/d8-bc-policy

    #80/#82 - can't really answer that question without profiling. However I'd hope that #2351919: Replace uses of drupal_get_path() with __DIR__ where possible means the answer is 'no'.

    Berdir’s picture

    Issue tags: +Needs profiling

    Yes, this will most certainly require profiling anyway.

    The failing tests indicate that we're missing some cache clearing, something updated them before and doesn't anymore, it seems.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    47.08 KB

    I absolutely agree, this change is quite lowlevel so we really should be better safe than sorry.

    The @group as simply missing.

    Status: Needs review » Needs work

    The last submitted patch, 90: 2208429-90.patch, failed testing.

    The last submitted patch, 80: extension_system_part-2208429-80.patch, failed testing.

    almaudoh’s picture

    Status: Needs work » Needs review
    FileSize
    3.59 KB
    49.82 KB

    Fixed two test fails...

    Status: Needs review » Needs work

    The last submitted patch, 93: extension_system_part-2208429-93.patch, failed testing.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    52.61 KB
    2.79 KB

    What about something like this?

    stefan.r’s picture

    Title: Extension System, Part III: ExtensionList/ExtensionHandler » Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList
    Issue summary: View changes
    Issue tags: +Needs issue summary update
    almaudoh’s picture

    dawehner++. So the test fail is due to unintended initialization of accountproxy. I've spent 3 days tracking down that test fail without any success :).

    dawehner’s picture

    dawehner++. So the test fail is due to unintended initialization of accountproxy. I've spent 3 days tracking down that test fail without any success :).

    This was a classical instance where a fresh mind totally helps.
    I'm honestly not sure how this actually works in HEAD. \Drupal::getContainer() is pointing to some old container, and by that contains some old, not reloaded configuration information.

    almaudoh’s picture

    So the next question is who maintains the list of enabled / installed modules? ModuleHandler, ModuleInstaller or ModuleList. I would say it should be the ModuleList mainly to avoid circular dependencies. I've seen a similar symmetrical case with ThemeHandler and ThemeInstaller.

    almaudoh’s picture

    Issue tags: -Needs profiling

    Did some (not so) quick profiling with ab. TL;DR No signifaicant difference in performance between HEAD and patch, even though there seems to be a slight improvement on the front page with the page. Expect slight performance improvements on pages where drupal_get_(filename|path) is called multiple times.

    Five different scenarios were compared for HEAD and the patch in #12

    1. Front page:ab -c1 -n1000 http://drupal.d8
    2. Front page:ab -c10 -n1000 http://drupal.d8
    3. Front page;ab -c1 -n10000 http://drupal.d8
    4. User permissions:ab -c1 -n500 http://drupal.d8/admin/people/permissions
    5. Modules:ab -c1 -n500 http://drupal.d8/admin/modules

    Results

    HEAD Patch #95

    ab -c1 -n1000 http://drupal.d8

    Requests per second:    1485.40 [#/sec] (mean)
    Time per request:       0.673 [ms] (mean)
    Time per request:       0.673 [ms] (mean, across all concurrent requests)
    Transfer rate:          903.72 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       0
    Processing:     0    1   0.2      1       3
    Waiting:        0    1   0.2      0       3
    Total:          1    1   0.2      1       4
    
    Requests per second:    1530.62 [#/sec] (mean)
    Time per request:       0.653 [ms] (mean)
    Time per request:       0.653 [ms] (mean, across all concurrent requests)
    Transfer rate:          931.23 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       1
    Processing:     0    1   0.1      1       1
    Waiting:        0    1   0.1      0       1
    Total:          1    1   0.1      1       2
    

    ab -c10 -n1000 http://drupal.d8

    Requests per second:    3198.85 [#/sec] (mean)
    Time per request:       3.126 [ms] (mean)
    Time per request:       0.313 [ms] (mean, across all concurrent requests)
    Transfer rate:          1946.18 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.4      0       2
    Processing:     1    3   1.2      2      20
    Waiting:        1    3   1.1      2      20
    Total:          1    3   1.2      3      20
    
    Requests per second:    3372.35 [#/sec] (mean)
    Time per request:       2.965 [ms] (mean)
    Time per request:       0.297 [ms] (mean, across all concurrent requests)
    Transfer rate:          2051.73 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    1   0.6      0       4
    Processing:     1    2   0.9      2       8
    Waiting:        0    2   0.8      2       7
    Total:          1    3   1.0      3       8
    

    ab -c1 -n10000 http://drupal.d8

    Requests per second:    1605.64 [#/sec] (mean)
    Time per request:       0.623 [ms] (mean)
    Time per request:       0.623 [ms] (mean, across all concurrent requests)
    Transfer rate:          976.87 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       1
    Processing:     0    1   0.2      1       6
    Waiting:        0    1   0.2      0       6
    Total:          1    1   0.2      1       6
    
    Requests per second:    1596.17 [#/sec] (mean)
    Time per request:       0.627 [ms] (mean)
    Time per request:       0.627 [ms] (mean, across all concurrent requests)
    Transfer rate:          971.10 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       5
    Processing:     0    1   0.2      1       7
    Waiting:        0    1   0.1      0       6
    Total:          1    1   0.2      1       7
    

    ab -c1 -n500 http://drupal.d8/admin/people/permissions

    Requests per second:    1.36 [#/sec] (mean)
    Time per request:       732.813 [ms] (mean)
    Time per request:       732.813 [ms] (mean, across all concurrent requests)
    Transfer rate:          343.42 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       0
    Processing:   703  733  35.0    723    1038
    Waiting:      677  706  34.4    697    1005
    Total:        703  733  35.0    723    1038
    
    Requests per second:    1.33 [#/sec] (mean)
    Time per request:       754.185 [ms] (mean)
    Time per request:       754.185 [ms] (mean, across all concurrent requests)
    Transfer rate:          333.69 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       0
    Processing:   713  754  68.6    738    1970
    Waiting:      685  726  66.8    711    1905
    Total:        713  754  68.6    738    1970
    

    ab -c1 -n500 http://drupal.d8/admin/modules

    Requests per second:    0.73 [#/sec] (mean)
    Time per request:       1363.646 [ms] (mean)
    Time per request:       1363.646 [ms] (mean, across all concurrent requests)
    Transfer rate:          431.23 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       0
    Processing:  1288 1363 172.8   1323    3737
    Waiting:     1254 1330 172.5   1289    3704
    Total:       1288 1364 172.8   1323    3737
    
    Requests per second:    0.71 [#/sec] (mean)
    Time per request:       1403.607 [ms] (mean)
    Time per request:       1403.607 [ms] (mean, across all concurrent requests)
    Transfer rate:          418.95 [Kbytes/sec] received
    
    Connection Times (ms)
                  min  mean[+/-sd] median   max
    Connect:        0    0   0.0      0       0
    Processing:  1313 1403 166.5   1354    3866
    Waiting:     1279 1368 155.8   1320    3470
    Total:       1313 1404 166.5   1354    3866
    

    catch’s picture

    @almaudoh thanks for the load testing with ab. It'd be better to have profiling with xhprof so we can see the difference in function calls - easier to see if something unexpected was happening, and usually individual changes aren't enough for ab to give reliable data.

    almaudoh’s picture

    Issue summary: View changes
    Mile23’s picture

    #95 needed a reroll, so here it is.

    dawehner’s picture

    Here is a total unscientific profiling (frontpage, without page_cache/dynamic_page_cache, no content)

    The relative numbers are pretty 0, there seems to be no measurable difference.

    Run #56d452431030b Run #56d4524f84a32 Diff Diff%
    Number of Function Calls 27,700 27,813 113 0.4%
    Incl. Wall Time (microsec) 78,463 78,750 287 0.4%
    Incl. MemUse (bytes) 13,950,232 14,327,536 377,304 2.7%
    Incl. PeakMemUse (bytes) 14,084,848 14,462,328 377,480 2.7%

    The two runs are attached

    dawehner’s picture

    Same scenario, this time with some more stats, powered by xhprof-kit:

    
    ### FINAL REPORT
    
    === 8.1.x..8.1.x compared (56d454b883f33..56d4551bde20a):
    
    ct  : 27,653|27,653|0|0.0%
    wt  : 68,071|67,140|-931|-1.4%
    mu  : 13,942,216|13,942,232|16|0.0%
    pmu : 14,078,592|14,078,632|40|0.0%
    
    === 8.1.x..2208429 compared (56d454b883f33..56d4552d7c90e):
    
    ct  : 27,653|27,659|6|0.0%
    wt  : 68,071|67,020|-1,051|-1.5%
    mu  : 13,942,216|13,952,824|10,608|0.1%
    pmu : 14,078,592|14,089,304|10,712|0.1%
    
    === SUM: 2208429-summary..8_1_x-summary compared (56d455370d772..56d4553b35033):
    
    ct  : 2,767,579|2,767,092|-487|-0.0%
    wt  : 7,065,647|7,426,053|360,406|5.1%
    mu  : 1,399,300,984|1,396,472,816|-2,828,168|-0.2%
    pmu : 1,412,991,056|1,410,642,896|-2,348,160|-0.2%
    
    ---
    ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
    
    ### XHPROF-LIB REPORT
    
    +-------------------+------------+------------+------------+------------+------------+
    | namespace         |        min |        max |       mean |     median |       95th |
    +-------------------+------------+------------+------------+------------+------------+
    | Calls             |            |            |            |            |            |
    |                   |            |            |            |            |            |
    | 2208429           |     27,659 |     29,338 |     27,676 |     27,659 |     27,659 |
    | 8_1_x             |     27,653 |     29,445 |     27,671 |     27,653 |     27,653 |
    |                   |            |            |            |            |            |
    | Wall time         |            |            |            |            |            |
    |                   |            |            |            |            |            |
    | 2208429           |     67,020 |     89,879 |     70,656 |     69,630 |     74,454 |
    | 8_1_x             |     67,140 |     92,927 |     74,261 |     73,854 |     83,085 |
    |                   |            |            |            |            |            |
    | Memory usage      |            |            |            |            |            |
    |                   |            |            |            |            |            |
    | 2208429           | 13,952,760 | 14,362,928 | 13,993,010 | 13,952,824 | 14,265,799 |
    | 8_1_x             | 13,942,224 | 14,356,472 | 13,964,728 | 13,942,232 | 14,307,440 |
    |                   |            |            |            |            |            |
    | Peak memory usage |            |            |            |            |            |
    |                   |            |            |            |            |            |
    | 2208429           | 14,089,160 | 14,536,312 | 14,129,911 | 14,089,304 | 14,402,476 |
    | 8_1_x             | 14,078,576 | 15,018,136 | 14,106,429 | 14,078,632 | 14,443,752 |
    |                   |            |            |            |            |            |
    +-------------------+------------+------------+------------+------------+------------+
    
    dawehner’s picture

    Interesting enough the profiling in #104 had an accidental apc cache set involved.

    This is a screenshot of another run, without that: https://www.drupal.org/files/issues/Screen%20Shot%202016-02-29%20at%2015...

    almaudoh’s picture

    Soo...now that we're in beta, is there any chance this can go in? We still have some refactoring to do after this. And it will help us progress #2347783: [PP-2] Remove drupal_get_path() as public facing API for including files, fix #2442383: Add the option to cache drupal_get_filename() and some others...

    almaudoh’s picture

    Issue tags: +beta target triage
    dawehner’s picture

    No idea. I asked catch, well maybe I'll ping him again. Maybe berdir would be also a great person for more reviews.

    Version: 8.1.x-dev » 8.2.x-dev

    Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

    dawehner’s picture

    Rerolled + some minor fixes

    alexpott’s picture

    1. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
      @@ -722,8 +722,7 @@ public function getModuleDirectories() {
          * {@inheritdoc}
          */
         public function getName($module) {
      -    $info = system_get_info('module', $module);
      -    return isset($info['name']) ? $info['name'] : $module;
      +    return \Drupal::service('module_listing')->getName($module);
         }
      

      We should deprecate ModuleHandler::getName(). ModuleHandler is far to low level to be handling that.

    2. +++ b/core/modules/system/system.module
      @@ -1065,33 +955,7 @@ function _system_rebuild_module_data_ensure_required($module, &$modules) {
      +  return \Drupal::service('module_listing')->reset()->listExtensions();
      

      Why's there a reset in here?

    3. ]

    4. FILE: .../drupal8alt.dev/core/lib/Drupal/Core/Extension/ExtensionList.php
      ----------------------------------------------------------------------
      FOUND 8 ERRORS AFFECTING 8 LINES
      ----------------------------------------------------------------------
       125 | ERROR | [ ] Return comment must be on the next line
       148 | ERROR | [ ] Return comment must be on the next line
       190 | ERROR | [ ] Return comment must be on the next line
       207 | ERROR | [ ] Return comment must be on the next line
       229 | ERROR | [ ] Return comment must be on the next line
       238 | ERROR | [ ] Return comment must be on the next line
       333 | ERROR | [ ] Return comment must be on the next line
       428 | ERROR | [x] Use "elseif" in place of "else if"
      ----------------------------------------------------------------------
      PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
      ----------------------------------------------------------------------
      
    5. FILE: ...l8alt.dev/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      ----------------------------------------------------------------------
      FOUND 1 ERROR AFFECTING 1 LINE
      ----------------------------------------------------------------------
       161 | ERROR | [x] Line indented incorrectly; expected 10 spaces,
           |       |     found 9
      ----------------------------------------------------------------------
      PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
      ----------------------------------------------------------------------
      
    6. FILE: ...ore/lib/Drupal/Core/Session/AccountProxyInitializedInterface.php
      ----------------------------------------------------------------------
      FOUND 2 ERRORS AFFECTING 2 LINES
      ----------------------------------------------------------------------
       10 | ERROR | [x] Missing interface doc comment
       15 | ERROR | [ ] Return comment must be on the next line
      ----------------------------------------------------------------------
      PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
      ----------------------------------------------------------------------
      
    dawehner’s picture

    Why's there a reset in here?

    So before, _system_rebuild_module_data used to be the function which didn't had any kind of caching. As we now have encapsulated the behaviour in its own service, and still keeping the exact same behaviour, resetting helps us to keep the behaviour.

    dawehner’s picture

    Rerolled against 8.2.x coding standard changes.

    alexpott’s picture

    1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
      @@ -780,7 +781,14 @@ protected function initializeContainer() {
      +        // Ensure to not accidentally initialize the user.
      +        if ($current_user instanceof AccountProxyInitializedInterface) {
      +          $current_user_id = $current_user->accountIsInitialized() ? $current_user->id() : 0;
      +        }
      +        else {
      +          $current_user_id = $current_user->id();
      +        }
      

      I think given the interface rule we recently we agreed we should just add the method to AccountProxyInterface and include this in the CR. Less complexity and weirdness. Unless we reckon there are lots of alternate implementations.

    2. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -182,10 +182,10 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
      +        // Clear the static cache of ModuleExtensionList to pick up the
      
      @@ -437,10 +437,10 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
      +      // Clear the static cache of ModuleExtensionList to pick up the
      

      Should be the service name not the class name since services can be swapped out.

    dawehner’s picture

    Thank you alex!

    I think given the interface rule we recently we agreed we should just add the method to AccountProxyInterface and include this in the CR. Less complexity and weirdness. Unless we reckon there are lots of alternate implementations.

    A cool

    alexpott’s picture

    Issue tags: +Needs change record

    So we need a CR ;)

    dawehner’s picture

    Issue tags: -Needs change record

    Added one ...

    andypost’s picture

    +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -780,7 +780,9 @@ protected function initializeContainer() {
    -        $current_user_id = $this->container->get('current_user')->id();
    +        $current_user = $this->container->get('current_user');
    +        // Ensure to not accidentally initialize the user.
    +        $current_user_id = $current_user->accountIsInitialized() ? $current_user->id() : 0;
    

    why this change?
    "initialized" is internal state and implementation details of service \Drupal\Core\Session\AccountProxy::getAccount()

    dawehner’s picture

    Issue summary: View changes

    Let's add the explanation to the issue summary.

    Thank you for your review @andypost!

    alexpott’s picture

    Hmmm @dawehner I think @andypost has a point. Whilst the fix in #98 fixes the test fail - is it the best fix?

    andypost’s picture

    @dawehner I'm sure that Kernel should not know new interface method to be able to run!
    I'm sure that API addition makes no sense in scope of the issue
    Very probably this points to some error in clean-up or execution logic as #95 shows

    +++ b/core/modules/simpletest/src/KernelTestBase.php
    @@ -100,7 +100,7 @@ function __construct($test_id = NULL) {
    -      $this->moduleFiles = \Drupal::state()->get('system.module.files') ?: array();
    +      $this->moduleFiles = \Drupal::cache('default')->get('system.module.files') ?: array();
    
    @@ -222,7 +222,7 @@ protected function setUp() {
    -      $this->container->get('state')->set('system.module.files', $this->moduleFiles);
    +      $this->container->get('cache.default')->set('system.module.files', $this->moduleFiles);
    

    Maybe changing this order caused account service state change

    dawehner’s picture

    @dawehner I'm sure that Kernel should not know new interface method to be able to run!
    I'm sure that API addition makes no sense in scope of the issue
    Very probably this points to some error in clean-up or execution logic as #95 shows

    Well,its needed to fix the bug in #95, its not just we do it for fun. It already makes sense.

    alexpott’s picture

    +++ b/core/lib/Drupal/Core/Session/AccountProxyInterface.php
    @@ -43,4 +43,12 @@ public function getAccount();
    +  public function accountIsInitialized();
    

    If we do need this - shouldn't it be called isAccountInitialized()

    alexpott’s picture

    Or maybe even just hasAccount()?

    dawehner’s picture

    Maybe changing this order caused account service state change

    This is just internal caches, so that doesn't rebuild the container, so it doesn't matter here.

    @alexpott
    I fear you are actually right :)

    Xano’s picture

    FileSize
    12.8 KB
    52.9 KB

    This patch improves documentation, centralizes cache key definitions, marks AccountProxyInterface as internal, implements the feedback from #125, and renames the services to have a consistent prefix so they're easier to find (similar to what we do with plugin managers). I did not find anything conceptually wrong with this patch.

    Status: Needs review » Needs work

    The last submitted patch, 127: drupal_2208429_127.patch, failed testing.

    Xano’s picture

    Status: Needs work » Needs review
    FileSize
    7.51 KB
    53 KB
    almaudoh’s picture

    1. +++ b/core/core.services.yml
      @@ -475,10 +475,10 @@ services:
      +  extension.list.module:
      ...
      +  extension.list.profile:
      

      Like the new names

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -128,25 +129,43 @@ protected function getExtensionDiscovery() {
           return 'core.extension_listing.' . $this->type;
      

      We should make this match the general pattern as well: 'core.extension.list.' . $this->type

    3. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -287,18 +306,19 @@ public function getInfo($name) {
      +      $cache_key = "system.{$this->type}.info";
      

      We can use the new cache ID helper function here too.

    dawehner’s picture

    +2 to all of those changes. Nice improvements!

    Like the new names

    Totally agreed!

    donquixote’s picture

    Architecture issues:

    I think we should be using composition, instead of squeezing it all into one inheritance hierarchy.
    There could be one "layer" that just returns an array of extension objects, and nothing more.
    A cache layer in between, using the decorator pattern.
    And then another layer on top of it which provides all the other methods like extensionExists().
    And another layer on top of it with the getInfo() and getAllInfo().

    In the patch, we are treating the Extension objects as mutable. That is, we are writing information on them after construction, in doListExtensions(). This seems undesirable to me.
    Especially, the mutable object is stored in the static cache in ExtensionDiscovery::$files. So all the modifications have side effects.

    I think instead we should have one layer that reads and caches and buffers the raw discovered extensions from the filesystem, with their *.info.yml files.
    And then another layer that adds dynamic information, e.g. with hook_system_info_alter(). But in a way that does not pollute the original objects. E.g. there could be a RawExtensionObject representing the raw discovered extension as it lives in the filesystem. This can be statically cached, because it does not depend on any dynamic information. And then an Extension object referencing the RawExtensionObject. The Extension object would not be cached statically, because it depends on e.g. hook_system_info_alter().

    ---------------

    Implementation details of current approach:

    There should be an interface, e.g. ExtensionListInterface. Or multiple interfaces, if we do split up the responsibilities.
    This way, consumers are not forced to depend on a concrete implementation.

    The abstract base class should be named ExtensionListBase. Or are we no longer doing this?

    We are confusing human name and machine name.
    E.g. getName($machine_name) gives us a human name, but takes the machine name as the parameter.
    On the other hand, in extensionExists($name), "name" means machine name. This is inconsistent.

    I am not a friend of methods that sound like getters, but take a parameter. For me, $object->getFilename() means get the one filename of $object. I would personally use method names like extensionGetFilename($extension_name), if it takes a parameter, to indicate that the returned filename is relative to the parameter. Especially on a service like the extension list, this allows to distinguish between methods that apply to all extensions in the list, from those that target a specific extension (by name).
    Maybe this is just not the way we do things in Drupal core - then so be it.

    Maybe I will come up with a patch. I am still looking and thinking.

    donquixote’s picture

    Btw, ExtensionDiscovery already suffers from the same problems:
    - Lack of interface.
    - Lack of separation / composition.
    - Static cache of mutable objects. First modification already happens in scan(), after they are already added to the static cache.

    donquixote’s picture

    #129

    On ExtensionList::getInfo():

    +   * @param string $name
    +   *   The name of an extension whose information shall be returned. If
    +   *   $name does not exist or is not enabled, an empty array will be returned.
    

    "an empty array will be returned" should be "an exception will be thrown".

    (Btw it would help if we could tell by the method name whether it returns NULL, empty array, or throws an exception, if nothing is found This is not currently the way we are doing things, but it would be a good thing.)

    donquixote’s picture

    #129

      /**
       * Returns a list of extension folder names keyed by extension name.
       *
       * @return string[]
       */
      public function getFilenames() {
    

    "Returns a list of extension folder names" should be "Returns a list of extension file names".

    donquixote’s picture

    #192, ExtensionList::doListExtensions()

    +      $extension->info = $this->infoParser->parse($extension->getPathname());
    

    The $extension->info is an anonymous object property.
    Should we make it an explicit public property on class Extension? Like this:

    class Extension {
    
      [..]
    
      /**
       * Parsed contents of the *.info.yml file. Stays null until initialized.
       *
       * @var array|null
       */
      public $info;
    
    donquixote’s picture

    ModuleExtensionList::doListExtensions():

          $this->extensionDiscovery->setProfileDirectories([
            $profile => $profiles[$profile]->getPathname(),
          ]);
    

    We should avoid modifying objects long after they are created. It's a bad habit that often causes problems.
    $this->extensionDiscovery is created in the constructor of ExtensionDiscovery. Modifications to the discovery object should happen there, not be delayed until doListExtensions().
    Of course even better would be to inject the ExtensionDiscovery, instead of creating it in the constructor.

    If this is somehow not possible due to early bootstrap limitations, then the state change should be avoided altogether, and instead only happen in local scope.

    In ModuleExtensionList::doScanExtensions():

        // Find installation profiles.
        $profiles = $this->profileList->listExtensions();
    
        // Include the installation profile in modules that are loaded.
        if ($profile = drupal_get_profile()) {
    

    Some of this profile logic is repeated further down in doListExtensions(). Maybe this should happen only once?

    donquixote’s picture

    ModuleExtensionList::doListExtensions():

        $profiles = $this->profileList->listExtensions();
        $profile = drupal_get_profile();
        if ($profile && isset($profiles[$profile])) {
          // Set the profile in the ExtensionDiscovery so we can scan from the right
          // profile directory.
          $this->extensionDiscovery->setProfileDirectories([
            $profile => $profiles[$profile]->getPathname(),
          ]);
    

    It would clarify things if $profile was named $profile_name. Otherwise one would think that $profile is an array value of $profiles - so it would be an extension object, not a machine name.

    And is scanning the profile directory really specific to modules? Should this really be in ModuleExtensionList? Or rather in ExtensionListBase?
    Do we even need it at all? ExtensionDiscovery::scan() already calls ExtensionDiscovery::setProfileDirectoriesFromSettings(), which adds the profile directory for scanning. The only difference seem to be special cases for tests and during install.

    dawehner’s picture

    Issue summary: View changes
    FileSize
    53.04 KB
    2.23 KB

    @donquixote
    Thank you for your feedback.

    The goal of this issue is to NOT reinvent parts of the extension system, but rather provide a layer to list information and by that being able to get rid of a big dependency to system module. In order to do that, we don't want to change for example the mutability of the extension object, as the alter hook should not change its API layer.

    Given that we also decided to not solve every problem at once, which means that we aren't done after this issue. Given that introducing an interface would be kind of a lie for me. A interface communicates that this is the final design. For me there could be for example a Module/Theme object that wraps/decorates an existing Extension object with the additional information coming from info files.

    E.g. getName($machine_name) gives us a human name, but takes the machine name as the parameter.
    On the other hand, in extensionExists($name), "name" means machine name. This is inconsistent.

    Good point, let's switch to $machine_name, see issue summary tasks

    I am not a friend of methods that sound like getters, but take a parameter. For me, $object->getFilename() means get the one filename of $object. I would personally use method names like extensionGetFilename($extension_name), if it takes a parameter, to indicate that the returned filename is relative to the parameter. Especially on a service like the extension list, this allows to distinguish between methods that apply to all extensions in the list, from those that target a specific extension (by name).

    So you would suggest something like getExtensionFileName?

    The $extension->info is an anonymous object property.
    Should we make it an explicit public property on class Extension? Like this:

    See my explanation in this comment. This is also out of scope of this issue.

    In general I hope we don't try to solve every problem we have in this issue but rather continue with a clear path where we can iterate further in the future.

    PS: The reason extension discovery doesn't have an interface is that this layer is not meant to be swappable. It is required when you rebuild the container, there is really limited amount of swappability available at that point. Another point is that there a lot of assumptions about the way how extension discovery works (for example that /modules contain the modules), that swapping it out, is not easy/actual sane.

    donquixote’s picture

    not solve every problem at once

    Very true.
    I wanted to restrict my comments to the new classes we are introducing: ExtensionList with its subclasses. I thought it should be easy to split it up.
    Then I noticed that the mutability of the extension object, and the design of ExtensionDiscovery, is an obstacle when trying to split up ExtensionList.
    But yes, if we do anything about ExtensionDiscovery or the Extension class, it should be in a separate issue.

    In order to do that, we don't want to change for example the mutability of the extension object, as the alter hook should not change its API layer.

    While mutability in itself is already somewhat undesirable, the real problem here is something else.
    It is the "mutations" leaking into lower layers, causing side effects on other consumers of these lower layers.

    If ExtensionList finds a cached list of extensions, then ExtensionDiscovery will give other components a "raw" list of extensions, without the ->info or alterations.
    On the other hand, on a cache miss in ExtensionDiscovery, the added ->info and alterations will leak into the ExtensionDiscovery::$files static cache. So other components using the discovery will get the prepared and altered extension objects.

    To fix this, we don't have to give up on mutability in the "upper" layer. We could simply clone the objects coming from ExtensionDiscovery, to avoid leaking of alterations. If this happens in ExtensionList, it could be well within the scope of this issue. But if we want to open a separate issue for it, fine.

    introducing an interface would be kind of a lie for me. A interface communicates that this is the final design.

    PS: The reason extension discovery doesn't have an interface is that this layer is not meant to be swappable. It is required when you rebuild the container, there is really limited amount of swappability available at that point. Another point is that there a lot of assumptions about the way how extension discovery works (for example that /modules contain the modules), that swapping it out, is not easy/actual sane.

    I personally use interfaces all the time, even for "internal" stuff, or stuff that I plan to change.
    Interfaces allow to clean up the dependencies and communication between classes / objects. And classes depending on interfaces instead of implementations are generally better for testing. I prefer to have these qualities for internal objects as much as for external / publically visible objects. I rather have a bunch of interfaces that could possibly change, then a big and noodly class hierarchy. Maybe the respective interface could be marked as "@internal"?
    (To not derail this issue too much, we can continue this point in IRC or elsewhere)
    (So far I am only talking about interfaces for ExtensionList, ExtensionDiscovery, and possibly some in between. Interfaces for Extension objects are also desirable, but a different subject)

    So you would suggest something like getExtensionFileName?

    My first idea would have been ->extensionGetInfo(), ->extensionGetFilename() etc. But then we would also have extensionGetExtension(), which is weird, because now "extension" means both the object and the machine name. I think it is better if "extension" means just the object.
    So the next idea is ->nameGetInfo(), ->nameGetFilename(), ->nameGetLabel() or ->nameGetHumanName(), ->nameGetExtension().
    This may sound a bit awkward, but I think it is very clear. If the list object has a good variable name, then it will be $extension_list->nameGetInfo($machine_name), which I think immediately explains what is happening.
    A question would be if we keep extensionExists(), or if this needs to be renamed nameExists() or nameIsAvailableExtension(). I think we can keep it as extensionExists(), and live with the slight inconsistence.

    donquixote’s picture

      protected $addedFileNames;
      [..]
        $this->addedFileNames = NULL;
    

    I think the default value of this should be an empty array, not NULL.

    Fyi: I am working on a patch that splits ExtensionList into smaller components, but (hopefully) is still within the issue scope. This also fixes details like this one.

    donquixote’s picture

      /**
       * Sets the filename for an extension.
       *
       * This method is used in the Drupal bootstrapping phase, when the extension
       * system is not fully initialized, to manually set locations of modules and
       * profiles needed to complete bootstrapping.
       *
       * It is not recommended to call this method except in those rare cases.
       *
       * @param string $extension_name
       *   The name of the extension for which the filename is requested.
       * @param string $filename
       *   The filename of the extension which is to be set explicitly rather
       *   than by consulting the dynamic extension listing.
       */
      public function setFilename($extension_name, $filename) {
        $this->addedFileNames[$extension_name] = $filename;
      }
      [..]
      public function getFilename($extension_name) {
        // Ensure that $this->fileNames is primed.
        $this->getFilenames();
        if (isset($this->addedFileNames[$extension_name])) {
          return $this->addedFileNames[$extension_name];
        }
        elseif (isset($this->fileNames[$extension_name])) {
          return $this->fileNames[$extension_name];
        }
        throw new \InvalidArgumentException("The {$this->type} $extension_name does not exist.");
      }
    

    The purpose of setFilename() is to make some extensions available even before the extension scanning process is available.

    But in the new ExtensionList class, any attempt to access these "special" extensions will trigger a re-discovery, or cache lookup.
    This was not the case with drupal_get_filename(), where a re-discovery or cache lookup only happened if the file was not previously registered.

    I think this can be fixed by moving the $this->getFilenames() call further down, below the if (isset($this->addedFileNames[$extension_name])).

    dawehner’s picture

    @donquixote
    Its great to see that you agree with not fixing everything at once.

    I think the default value of this should be an empty array, not NULL.
    Fyi: I am working on a patch that splits ExtensionList into smaller components, but (hopefully) is still within the issue scope. This also fixes details like this one.

    Totally, well we need to be able to distinct between the case where there is no result, and it wasn't scanned yet.

    The purpose of setFilename() is to make some extensions available even before the extension scanning process is available.
    But in the new ExtensionList class, any attempt to access these "special" extensions will trigger a re-discovery, or cache lookup.
    This was not the case with drupal_get_filename(), where a re-discovery or cache lookup only happened if the file was not previously registered.

    Good catch

    (To not derail this issue too much, we can continue this point in IRC or elsewhere)
    (So far I am only talking about interfaces for ExtensionList, ExtensionDiscovery, and possibly some in between. Interfaces for Extension objects are also desirable, but a different subject)

    Well as said, I think we should introduce interfaces when we are convinced that this is it.

    My first idea would have been ->extensionGetInfo(), ->extensionGetFilename() etc. But then we would also have extensionGetExtension(), which is weird, because now "extension" means both the object and the machine name. I think it is better if "extension" means just the object.
    So the next idea is ->nameGetInfo(), ->nameGetFilename(), ->nameGetLabel() or ->nameGetHumanName(), ->nameGetExtension().
    This may sound a bit awkward, but I think it is very clear. If the list object has a good variable name, then it will be $extension_list->nameGetInfo($machine_name), which I think immediately explains what is happening.

    Well I would argue that you are already in the context of extensions, given that you will have the extension (module/theme/profile) list initialized. Given that extension again would be sort of a duplicated information.

    donquixote’s picture

    Attempt to break up ExtensionList into smaller components.
    This is the kind of architecture I usually work with. Or in fact I tend to slice it up even more.. but it is good enough for now.

    With the break-up, I also did some renaming, which can be debated.

    • ExtensionListInterface now only has ->listExtensions() and ->reset().
    • ExtensionInfoListInterface has ->getAllInfo() and ->reset().
    • ExtensionFilenameListInterface has ->getFilenames() and ->reset()
    • ExtensionHubInterface inherits from all three, and adds all the other methods.
      This means what was ExtensionList in previous patches is now ExtensionHub. I found this more reasonable than inventing another name for ExtensionListInterface. The list really is just a list, whereas a "hub" has a variety of methods for different purposes.
      Note that the service names are not renamed.

    Known issues:

    • The code contains some /** @noinspection ... */ docs. We should remove these later, but for now they make it easier to review the code with an IDE (ok, probably only works with PhpStorm).
    • Also contains some @todo which we might want to remove later.
    • As said, services are still named *.list, not *.hub. This is up for discussion.
    • Class names DiscoveryExtensionListBase and ModuleDiscoveryExtensionList can be debated.
    • The sliced-up architecture allows a lot more unit tests, targetting the different layers, and e.g. testing components without the cache or buffer decorator.
      For now I just made the existing unit test to work with the changed architecture.
    • The architecture is still limited by the problems with leaked mutability, that I mentioned in previous comments. Otherwise, things could be sliced up more.
    • (EDIT) Maybe some of the doc comments need updating. Mostly I just copied stuff and then made sure the type docs are ok.
      dawehner’s picture

      Sorry for this negative comment. I don't want to sound that negative.

      There is IMHO no good reason to add a ExtensionDiscoveryInterface as part of this issue. Please don't try to make it harder that needed.
      Its also out of scope of this issue to add info to the extension object. This kind of code existed exactly like that before.

      Can you please elaborate why you think its helpful to add interface for every small bit, even if stuff will never be replaced in its implementation? Its harder to follow through multiple layers than one, as long each individual method is still simple. I mean no question for something which is meant to be flexible I totally get that kind of architecture, but this is more even like a

      +class ExtensionHub implements ExtensionHubInterface {

      Its totally weird that we hardcode the type of extensions to 3 different kinds.

      PS: There are gazillion of CS problems in your patch :(

      donquixote’s picture

      There is IMHO no good reason to add a ExtensionDiscoveryInterface as part of this issue. Please don't try to make it harder that needed.
      Its also out of scope of this issue to add info to the extension object. This kind of code existed exactly like that before.

      You are right.
      I am sorry. I meant to keep these changes out of this patch, and then missed to remove them.

      PS: There are gazillion of CS problems in your patch :(

      Sorry if this is the case. I will review this later.

      Its totally weird that we hardcode the type of extensions to 3 different kinds.

      Do we? Not any more than previous patches, right? We already have ModuleExtensionList and ProfileExtensionList in the previous patches. The only thing I see being added here are the static factories like ExtensionHub::createForModules(). But I don't think this can be considered any more or less "hardcoded" than previous patches.
      Also could it be that this is the wrong code snippet?

      -----

      The rest of your comment deserves a longer reply, which goes into a separate, follow-up comment.
      (EDIT: After some IRC talk)

      dawehner’s picture

      I think I understand now why my brain has problems with this new direction: Instead of knowing 2/1 concept (ExtensionList/ModuleList) this patch now requires people for the full understanding and debugging on runtime to actually have ExtensionFilenameList, ExtensionFilenameListBuffer ExtensionFilenameListCache, ExtensionHub ExtensionInfoListBuffer ExtensionInfoListCache DiscoveryExtensionListBase ExtensionListBuffer ExtensionListCache

      almaudoh’s picture

      I agree with @dawehner that the last patch has introduced too many classes and interfaces that makes it even more difficult for a developer to understand the extension system. Additionally, the introduction of so many new interfaces bring along the risk of not being able to refactor in future (if people begin to use those interfaces - which may happen even when they are @internal) when the architecture has not been fully completed.

      I suggest we still stick to the narrow scope of this issue, which is the first step of decoupling the extension system from the system.module and also making it easier to understand. Further refactoring, based on actual use cases can be done in follow ups.

      There is the potential that this issue could be delayed further, while it is blocking some other cleanup issues e.g. #2347783: [PP-2] Remove drupal_get_path() as public facing API for including files, #2442383: Add the option to cache drupal_get_filename(), #697946: Deprecate module_load_include() for removal before Drupal 9.0.x.

      donquixote’s picture

      I think there are two types of people: Cake people, and cookie people.

      The cake person says: I prefer the cake, because the cookies are too many.
      The cookie person says: I prefer the cookies, because the cake is too big.
      (assuming the same total weight or volume for both)

      The cake person measures complexity as the number of "things".
      The cookie person measures complexity as the size of one "thing".

      The trick of the cookie person is to only look at one cookie at a time, and not be bothered with the other cookies.
      One cookie fits nicely on a small plate. One cake does not.
      On the other hand, the one cake sits more stable on the big plate, than a heap of cookies would.

      This is not just a tradeoff, but a different mindset.
      The cookie person will put one cookie on a small plate at a time, and be happy with it. The cake person will attempt to put all cookies on a big plate, and correctly observe that it sucks.

      For me, whatever does not fit into one "page" of my brain, is "too big".

      Looking at the average class size in Drupal, and the responses in this thread, I have to conclude that I am pretty much alone with this view. I am surrounded by "cake people", one could say.

      Or, to be fair: Most of you probably agree that separation of concerns is a good thing. We just disagree how far we want to go. And we have different ideas about the use of interfaces.

      I will therefore not further derail this issue, and move further thoughts into a blog post (which is still "under construction" while I post this).

      donquixote’s picture

      Back to the original approach (#139, unless explicitly stated otherwise):

      @dawehner (#78 interdiff):

      -    $this->extensionDiscovery = new ExtensionDiscovery($root);
      +    $this->extensionDiscovery = $this->getExtensionDiscovery();
      +  }
      +
      +  /**
      +   * Returns the extension discovery.
      +   *
      +   * @return \Drupal\Core\Extension\ExtensionDiscovery
      +   */
      +  protected function getExtensionDiscovery() {
      +    return new ExtensionDiscovery($this->root);
         }
      

      Do you remember the reason for this change?

      Also @almaudoh in #68

      Made the ExtensionDiscovery an instance property to allow ModuleExtensionList set the profile directories before scanning for modules

      And myself in #137

      We should avoid modifying objects long after they are created.

      I think this is currently a bit weird, and we can do better.

      (EDIT: This part is "fixed" in #152)

      ---------------

          if ($profile) {
            // Installation profiles are hidden by default, unless explicitly
            // specified otherwise in the .info.yml file.
            if (!isset($extensions[$profile]->info['hidden'])) {
              $extensions[$profile]->info['hidden'] = TRUE;
            }
      
            if (isset($extensions[$profile])) {
              // The installation profile is required, if it's a valid module.
              $extensions[$profile]->info['required'] = TRUE;
      

      This part is also weird. First we don't care if $extensions[$profile] is set. And further down we do care?

      In the original _system_rebuild_module_data() this was not an issue, because it was within foreach ($modules as $key => $module). So it was guaranteed that $modules[$key] is set.

        // Read info files for each module.
        foreach ($modules as $key => $module) {
          [..]
          // Installation profiles are hidden by default, unless explicitly specified
          // otherwise in the .info.yml file.
          if ($key == $profile && !isset($modules[$key]->info['hidden'])) {
            $modules[$key]->info['hidden'] = TRUE;
          }
      

      ------

            // Installation profile hooks are always executed last.
            $extensions[$profile]->weight = 1000;
      

      It looks as if this is overwritten later.

          foreach ($extensions as $name => $module) {
            $module->weight = isset($installed_modules[$name]) ? $installed_modules[$name] : 0;
      

      Duh.
      This may have been broken before already, dunno.

      -----------------

      +class ProfileExtensionList extends ExtensionList {
      

      This means that profiles are processed, by adding ->info and running them through hook_system_info_alter().
      In previous behavior, this hook was only used for modules, *after* the profile had been added to the module list.

      +  protected function doScanExtensions() {
      +    $extensions = parent::doScanExtensions();
      +
      +    // Find installation profiles.
      +    $profiles = $this->profileList->listExtensions();
      +
      +    // Include the installation profile in modules that are loaded.
      +    if ($profile = drupal_get_profile()) {
      +      $extensions[$profile] = $profiles[$profile];
      

      This means the processed profile is added into a list of unprocessed modules.

       function _system_rebuild_module_data() {
      -  $listing = new ExtensionDiscovery(\Drupal::root());
      [..]
      -  $profiles = $listing->scan('profile');
      [..]
      -  $modules = $listing->scan('module');
      -  // Include the installation profile in modules that are loaded.
      -  if ($profile) {
      -    $modules[$profile] = $profiles[$profile];
      -    // Installation profile hooks are always executed last.
      -    $modules[$profile]->weight = 1000;
      -  }
      

      This is the old behavior, where the unprocessed profile, coming directly from ExtensionDiscovery, was added to the list of unprocessed modules.

      ------------

      I am going to attempt to fix some of this, maybe not all.

      donquixote’s picture

      +  protected function ensureRequiredDependencies(Extension $module, array $modules = []) {
      +    if (!empty($module->info['required'])) {
      +      foreach ($module->info['dependencies'] as $dependency) {
      +        $dependency_name = ModuleHandler::parseDependency($dependency)['name'];
      +        if (!isset($modules[$dependency_name]->info['required'])) {
      +          $modules[$dependency_name]->info['required'] = TRUE;
      
      -function _system_rebuild_module_data_ensure_required($module, &$modules) {
      -  if (!empty($module->info['required'])) {
      -    foreach ($module->info['dependencies'] as $dependency) {
      -      $dependency_name = ModuleHandler::parseDependency($dependency)['name'];
      -      if (!isset($modules[$dependency_name]->info['required'])) {
      -        $modules[$dependency_name]->info['required'] = TRUE;
      

      Neither old nor new code check if the $modules[$dependency_name] exists.
      They just blindly write to a possibly non-existing object.
      Or what am I missing?

      EDIT: If this is intended, it needs to be documented.

      donquixote’s picture

      The patches fixes some of the issues previously mentioned, but not all of them.
      https://github.com/donquixote/drupal/compare/8.2.x...8.2.x-2208429-152-E...

      • Document distinction of processed and unprocessed extension objects.
      • Document properties that can be null during the lifetime of an ExtensionList instance.
      • Set ->addedFileNames = [] on reset and initialization, so it is never NULL.
      • Eliminate ->extensionDiscovery, and instead call ->getExtensionDiscovery() directly.
      donquixote’s picture

      There is also some confusion about available vs installed / enabled extensions.

      +  /**
      +   * Returns information about a specified extension.
      +   *
      +   * This function returns the contents of the .info.yml file for the specified
      +   * installed extension.
      +   *
      +   * @param string $name
      +   *   The name of an extension whose information shall be returned. If
      +   *   $name does not exist or is not enabled an exception is thrown.
      +   *
      +   * @return mixed[]
      +   *   An associative array of extension information.
      +   *
      +   * @throws \InvalidArgumentException
      +   *   If there is no extension with the supplied name.
      +   */
      +  public function getInfo($name) {
      

      The doc comment here is not true.

      +  /**
      +   * Returns an array of information about enabled modules or themes.
      +   *
      +   * This function returns the contents of the .info.yml file for each installed
      +   * extension.
      +   *
      +   * @return array[]
      +   *   An associative array of extension information keyed by name. If no
      +   *   records are available, an empty array is returned.
      +   */
      +  public function getAllInfo() {
      

      And neither is this one.
      Both these methods will return info arrays for available extensions, no matter if they are enabled.
      At least this is what I conclude from looking at ExtensionList::recalculateInfo().

      donquixote’s picture

      +    $this->assertEquals('test name', $test_extension_list->getName('test_name'));
      

      The assertion methods in PhpUnit are static.
      My IDE (PhpStorm) tells me I should replace this call with static::assertEquals(..);.
      Do we follow this advice?

      dawehner’s picture

      1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
        @@ -112,7 +105,6 @@ public function __construct($root, $type, CacheBackendInterface $cache, InfoPars
        -    $this->extensionDiscovery = $this->getExtensionDiscovery();
        

        +1 for doing things lazy!

      2. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
        @@ -66,37 +66,54 @@ public function __construct($root, $type, CacheBackendInterface $cache, InfoPars
        +    if (NULL !== $active_profile = $this->getActiveProfile()) {
        

        why is $active_profile = $this->getActiveProfile() not enough? A profile with an empty string in its name is certainly not supported.

      And neither is this one.
      Both these methods will return info arrays for available extensions, no matter if they are enabled.
      At least this is what I conclude from looking at ExtensionList::recalculateInfo().

      Good catch. Yeah, so to keep the core behaviour we currently have its indeed just the installed ones, see \system_get_info

      donquixote’s picture

      +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -66,37 +66,54 @@ public function __construct($root, $type, CacheBackendInterface $cache, InfoPars
      +    if (NULL !== $active_profile = $this->getActiveProfile()) {

      why is $active_profile = $this->getActiveProfile() not enough? A profile with an empty string in its name is certainly not supported.

      $active_profile is either an object or NULL. It never is a string.
      Yes, if ($active_profile = ..) would also work.
      By doing the if (NULL !== $active_profile = ..), we
      - avoid an implicit cast to bool.
      - make visible which types of values we expect from the variable.

      I picked this habit up somewhere, don't really remember where exactly. But it does make a lot of sense to me since then.
      It forces or encourages a style where variables or return values have a clearly defined range of types / values.

      Implicit casting is also slower than ===, even though this only matters for operations that are repeated a lot.

      donquixote’s picture

      Btw a lot of the problems I pointed out in previous comments still apply.
      Why do we even have an "extension.list.profile" service? Where would this be used?

      dawehner’s picture

      Why do we even have an "extension.list.profile" service? Where would this be used?

      As written on IRC, profiles are also an extension type, so there should be a listing for them. Also, at some point we might support parents there, so a listing would be even more helpful.

      dawehner’s picture

      Here is some small updates to some of the points mentioned by @donquixote. Thank you!

      Xano’s picture

      I removed an unused method parameter, made variable names more consistent, and made some small improvements to docblocks and inline code comments. No functional changes.

      dawehner’s picture

      Issue summary: View changes

      @xano
      The derived method is using it!

      Xano’s picture

      Issue summary: View changes

      Ah! I couldn't find any usages in the method I removed the method from, nor in the original code this patch replaces. Thanks for pointing this out.

      Since this is not used by anything except one of the subclasses, should we keep (and document) it there only? There's no need for the base class or any other child to have this functionality, apparently. Since the original calling code did this filtering itself, I'm not even sure we should do this in ExtensionList now.

      dawehner’s picture

      Added a small follow up in #2719315: Try to install system module like any other module

      Thank you @xano! As said on IRC we want this parameter as its part of the child methods.

      donquixote’s picture

         /**
          * Static caching for extension info.
          *
      +   * Access this property's value through static::getAllInfo().
      +   *
          * @var array[]|null
          *   Keys are extension names, and values their info arrays (mixed[]).
      +   *
      +   * @see static::getAllInfo()
          */
         protected $extensionInfo;
      

      To me this implies that ::getAllInfo() is a static method.
      Maybe the solution is to write Access this property's value through ->getAllInfo() in the comment text, but use the @see static::getAllInfo() in the @see tag?
      My IDE (PhpStorm) only understands the spelled-out @see \Drupal\Core\Extension\ExtensionList::getAllInfo(), or simply @see getAllInfo(), but nothing with self:: or static::. Do we have any convention for this? I don't see this specific case mentioned here. I think the fully spelled-out version would be the safe option. static:: is not meaningful in a @see tag, imo.


      I removed an unused method parameter

      @xano
      The derived method is using it!

        public function getAllInfo($installed = TRUE) {
      

      Imo this is a violation of the LSP. Semantically, at least. And confusing, definitely.
      In the base class, the $installed parameter has no meaning at all. In the subclasses it suddenly has a meaning.
      The solution (*) is to make this two separate methods. Different meaning -> different methods.

      Option a) Have one public method getAllInfo($installed = TRUE), which is abstract in the base class.
      Then another method getAvailableExtensionsInfo(), which is implemented in the base class and does not need overriding.

      Option b) Get rid of the parameter altogether, and have a dedicated method getInstalledExtensionsInfo().
      This method will be abstract in the base class. Or it may not exist there at all, if we think that some extension types (themes?) cannot be "installed" or "uninstalled".
      Same as in (a), introduce a method getAvailableExtensionsInfo().

      Option c) Choose one of (a) or (b). Then implement getAllInfo($installed = TRUE) or getInstalledExtensionsInfo() in the base class, and let call an abstract method which has the "installed" extension names.

        /**
         * @return array[]
         */
        public function getInstalledExtensionsInfo() {
          return array_intersect_key(
            $this->getAvailableExtensionsInfo(),
            array_fill_keys($this->getInstalledExtensionsNames(), TRUE));
        }
      
        /**
         * @return array[]
         */
        public function getAvailableExtensionsInfo() {
          if (!isset($this->extensionInfo)) {
            $cache_key = "system.{$this->type}.info";
            if ($cache = $this->cache->get($cache_key)) {
              $info = $cache->data;
            }
            else {
              $info = $this->recalculateInfo();
              $this->cache->set($cache_key, $info);
            }
            $this->extensionInfo = $info;
          }
      
          return $this->extensionInfo;
        }
      
        /**
         * @return string[]
         *   The machine names of all installed extensions of this type.
         */
        abstract protected function getInstalledExtensionsNames();
      

        public function getAllInfo($installed = TRUE) {
          if (!isset($this->extensionInfo)) {
      

      Just saying, I still prefer if ($this->extensionInfo === NULL). This lets both the IDE and the interpreter complain if we misspell the property name, or if we forget to declare it. With isset(), neither the IDE nor the interpreter will complain, because it has to assume that we intentionally work with undeclared properties.


        public function getName($extension_name) {
      

      What about getHumanName($extension_name), or getLabel($extension_name)?


         * This function returns the contents of the .info.yml file.
      [..]
        public function getAllInfo($installed = TRUE) {
      

      Does it?
      Firstly, there should be a plural somewhere in this sentence.
      Secondly, this is not the raw contents of the *.info.yml, but it is processed / modified.
      The base class adds 'mtime' and $this->defaults, and the ModuleExtensionList even adds more stuff to it.

      (And just to mention it: Even the ExtensionDiscovery can return extension objects with "processed" info arrays, because all the modifications leak down...)


        protected function doListExtensions() {
          [..]
          foreach ($extensions as $extension_name => $extension) {
            [..]
            $this->moduleHandler->alter('system_info', $extensions[$extension_name]->info, $extensions[$extension_name], $this->type);
          }
      

      What about

            $this->moduleHandler->alter('system_info', $extension->info, $extension, $this->type);
      

      What I previously (e.g. in #150) said about profiles still applies..
      We need to decide what is the intended behavior. Or try to stay as close as possible to the original behavior.


      (*) Disclaimer: I still disagree with the idea to squeeze this all into one class hierarchy. But I want to be constructive, so the above comments are about the current approach.

      almaudoh’s picture

      #164: +1 to getInstalledExtensionsInfo() and getAvailableExtensionsInfo() or maybe getAllExtensionsInfo(). Suggest we also rename getInfo() to getExtensionInfo($extension_name)

      What about getHumanName($extension_name), or getLabel($extension_name)?

      getLabel() sounds better and conforms with existing method names.

      I still disagree with the idea to squeeze this all into one class hierarchy.

      Will raise an issue for us to explore that further.

      dawehner’s picture

      Totally want with one of your suggestions!

      Also, thank you for insisting on the profile detail! Now we have more test coverage

      Status: Needs review » Needs work

      The last submitted patch, 166: 2208429-165.patch, failed testing.

      almaudoh’s picture

      1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
        @@ -296,8 +296,8 @@ protected function doListExtensions() {
        +  public function getExtensionInfo($extension_name) {
        +    $all_info = $this->getAllInstalledInfo();
             if (isset($all_info[$extension_name])) {
        

        Interestingly, getExtensionInfo() should not be affected by installation status. One should be able to get extension info for a non-installed extension and then can check <del>$info</del>$extension->status if needed.
        However, I guess we're trying to maintain existing behavior

      2. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
        @@ -108,25 +92,22 @@ protected function getActiveProfile() {
        +    if ($active_profile_name && isset($profiles[$active_profile_name])) {
        +      $extensions[$active_profile_name] = $active_profile = $profiles[$active_profile_name];
               $active_profile = $extensions[$active_profile_name];
               // Installation profiles are hidden by default, unless explicitly
        

        Wierd. Guess the second one should go.

      donquixote’s picture

      Interestingly, getExtensionInfo() should not be affected by installation status. One should be able to get extension info for a non-installed extension and then can check $info->status if needed.

      Well.. no :)
      Extension info is just the info array, $extension->info.
      The $extension->status is on the extension object, not on the info array.

      To me this just proves that we are muddling too many concerns into one class hierarchy, and as a result we are getting confused.

      donquixote’s picture

      I opened #2720447: Naming convention for parameterized getter methods (->getItem($key)). for the $extension_list->nameGetExtension($extension_name) proposal.

      donquixote’s picture

      Btw do we ever check if the extension has the correct type? E.g. $extension->info['type'] === 'module'.
      What if modules/notamodule/notamodule.info.yml says type === 'theme' ?

      Currently I think we assume the extension type based on the directory.
      The type specified in the info file is redundant at best, but it may also contradict the type we expect based on the directory.

      dawehner’s picture

      @donquixote
      Actually the extension discovery filters out extensionsbof different types. There is an entire optimisation by not using a yml parser etc. That part totally works, but well, is also not changed as part of this issue.

      dawehner’s picture

      Status: Needs work » Needs review
      FileSize
      58.64 KB
      10.42 KB

      I reintroduced the state "cache" now, as its an optimization for cold caches.

      Status: Needs review » Needs work

      The last submitted patch, 173: 2208429-173.patch, failed testing.

      Xano’s picture

      +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -380,8 +392,13 @@ public function getFilenames() {
      +      elseif ($file_names = $this->state->get($this->getFilenameCacheId())) {
      

      This needs a code comment to explain why the block is empty, partly because the assignment can easily be mistaken for a condition in this context.

      To me this implies that ::getAllInfo() is a static method.

      This is the other kind of static: http://php.net/manual/en/language.oop5.late-static-bindings.php (It's confusing indeed). This notation can be used as @return static as well and PhpStorm picks that up fine.

      The solution (*) is to make this two separate methods. Different meaning -> different methods.

      Agreed. I really like how @dawehner implemented this!

      This lets both the IDE and the interpreter complain if we misspell the property name,

      I hadn't looked at it this way, but what you say makes total sense.

      I reintroduced the state "cache" now, as its an optimization for cold caches.

      Why exactly? So we have reliable storage which can be overridden by faster caching backends? If so, good improvement, but I suggest we add some code comments to explain the rationale.

      dawehner’s picture

      Why exactly? So we have reliable storage which can be overridden by faster caching backends? If so, good improvement, but I suggest we add some code comments to explain the rationale.

      Well, doesn't it say so?

      // Store filenames to allow static::getFilename() to retrieve them
      // without having to rebuild or scan the filesystem.

      Note: #2664322: key_value table is only used by a core service but it depends on system install might help with these remaining failures.

      donquixote’s picture

      To me this implies that ::getAllInfo() is a static method.

      This is the other kind of static: http://php.net/manual/en/language.oop5.late-static-bindings.php (It's confusing indeed). This notation can be used as @return static as well and PhpStorm picks that up fine.

      No it's not.
      This method is always called with an object context. So either $this->getAllInfo(), or $list->getAllInfo().
      It is not meant to be called in a static context.

      almaudoh’s picture

      +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -387,17 +387,18 @@ protected function recalculateInfo() {
      +        // Store filenames to allow static::getFilename() to retrieve them
      +        // without having to rebuild or scan the filesystem.
               $this->state->set($cache, $file_names);
      

      This should be $cache_id as well

      Interestingly, getExtensionInfo() should not be affected by installation status. One should be able to get extension info for a non-installed extension and then can check $info->status if needed.

      This was a typo, I meant $extension->status. But I still think it's a valid point that the extension info should be available whether the extension is installed or not. The fact that one has requested info for a specific extension means he needs it and can always determine if the extension is installed or not for himself.

      donquixote’s picture

      This was a typo, I meant $extension->status. But I still think it's a valid point that the extension info should be available whether the extension is installed or not. The fact that one has requested info for a specific extension means he needs it and can always determine if the extension is installed or not for himself.

      Yes, I agree. This information has to come from somewhere else than getExtensionInfo(), but I still agree.

      almaudoh’s picture

      Status: Needs work » Needs review
      FileSize
      5.67 KB
      58.05 KB

      In this patch:
      - Fixed some of the points raised by @donquixote in #150
      - Moved the active profile additional initializations to ProfileListing::doListExtensions() which, now that I think about it is the correct place it should be.
      - Also moved the make-drupal-think-a-profile-is-a-module hack to ModuleListing::doListExtensions(), so it is only added after processing. The benefit is also that the profile is not processed as a module, but rather as a profile.
      - Fixed #178

      Also:

      +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,513 @@
      +   * Returns information about a specified extension.
      +   *
      +   * This function returns the contents of the .info.yml file for the specified
      +   * extension.
      ...
      +   * @throws \InvalidArgumentException
      +   *   If there is no extension with the supplied name.
      +   */
      +  public function getExtensionInfo($extension_name) {
      +    $all_info = $this->getAllInstalledInfo();
      +    if (isset($all_info[$extension_name])) {
      +      return $all_info[$extension_name];
      +    }
      +    throw new \InvalidArgumentException("The {$this->type} $extension_name does not exist or is not installed.");
      +  }
      +
      

      I'm still convinced this method should not be restricted to only installed extensions. Even the phpDoc and exception message indicate this. I have taken the liberty of making this change. Feel free to revert if that was never the intent of this method.

      Status: Needs review » Needs work

      The last submitted patch, 180: extension_system_part-2208429-180.patch, failed testing.

      dawehner’s picture

      Issue tags: -beta target triage

      - Also moved the make-drupal-think-a-profile-is-a-module hack to ModuleListing::doListExtensions(), so it is only added after processing. The benefit is also that the profile is not processed as a module, but rather as a profile.

      Mh, I think this issue should try to keep the existing behaviour as much as possible, so for example the processing / alter look should still make it possible.

      I'm still convinced this method should not be restricted to only installed extensions. Even the phpDoc and exception message indicate this. I have taken the liberty of making this change. Feel free to revert if that was never the intent of this method.

      Do you mind open up a follow up where we can discuss that? I'm totally on our site, but you know, we cannot just fix everything at the same time.

      almaudoh’s picture

      - Also moved the make-drupal-think-a-profile-is-a-module hack to ModuleListing::doListExtensions(), so it is only added after processing. The benefit is also that the profile is not processed as a module, but rather as a profile.
      Mh, I think this issue should try to keep the existing behaviour as much as possible, so for example the processing / alter look should still make it possible.

      The existing behavior is still maintained. Only that for profiles hook_system_info_alter(array &$info, \Drupal\Core\Extension\Extension $file, $type) is called with $type = 'profile' instead of $type = 'module' which I believe is a necessary result of the refactoring. We could always place that block of code before the processing, in which case that hook will be called twice for profiles.

      Do you mind open up a follow up where we can discuss that?

      Opened #2721603: ExtensionList cleanup and separation of concerns for general discussions and actual patches for all the architectural changes, etc.

      but you know, we cannot just fix everything at the same time.

      Yeah...

      dawehner’s picture

      Status: Needs work » Needs review
      FileSize
      58.08 KB
      816 bytes

      Let's add a todo for one of the points @donquixote has

      Status: Needs review » Needs work

      The last submitted patch, 184: 2208429-184.patch, failed testing.

      Fabianx’s picture

      High-Level feedback:

      - It would be great to have some docs in how the new extension system would work on a very high level.
      - It would be fantastic to already have a draft CR for how things change from D7 (e.g. the mapping of old API to the new one)

      Creating both should give a better idea on the overall big picture.

      Code review:

      1. +++ b/core/includes/bootstrap.inc
        @@ -200,11 +201,26 @@ function drupal_get_filename($type, $name, $filename = NULL) {
        +      catch (\InvalidArgumentException $e) {
        +        // Catch the exception and trigger error to maintain existing behavior.
        +        trigger_error(new FormattableMarkup('The following @type is missing from the file system: @name', array('@type' => $type, '@name' => $name)), E_USER_WARNING);
        +      }
        

        This definitely should use its own special Exception type.

        MissingExtensionException

      2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
        @@ -817,7 +817,10 @@ protected function initializeContainer() {
        +        /** @var \Drupal\Core\Session\AccountProxyInterface $current_user */
        +        $current_user = $this->container->get('current_user');
        +        // Ensure to not accidentally initialize the user.
        +        $current_user_id = $current_user->hasAccount() ? $current_user->id() : 0;
        

        I think this should be its own bug.

      3. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
        @@ -166,7 +166,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
        +            $module_path = \Drupal::service('extension.list.module')->getPath($name);
        
        @@ -182,10 +182,10 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
        +        \Drupal::service('extension.list.module')->reset();
        
        @@ -437,10 +437,10 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
        +      \Drupal::service('extension.list.module')->reset();
        

        Should this not be injected instead?

      4. +++ b/core/lib/Drupal/Core/Updater/Module.php
        @@ -49,8 +49,7 @@ public static function getRootDirectoryRelativePath() {
        +    return \Drupal::service('extension.list.module')->extensionExists($this->name);
        

        I like this a lot, should this be injected?

      5. +++ b/core/modules/config/src/Tests/ConfigImportAllTest.php
        @@ -97,7 +97,7 @@ public function testInstallUninstall() {
        -    $this->assertEqual(['standard', 'system', 'user'], array_keys($validation_reasons));
        +    $this->assertEqual(['system', 'user', 'standard'], array_keys($validation_reasons));
        

        Why does the order change?

      6. +++ b/core/modules/system/system.module
        @@ -915,27 +915,17 @@ function system_check_directory($form_element, FormStateInterface $form_state) {
        +    else {
        +      return $module_list->getAllInstalledInfo();
             }
           }
           else {
        

        I assume it is for cleanlyness of the diff to not re-indent this?

      7. +++ b/core/modules/system/system.module
        @@ -1065,33 +957,7 @@ function _system_rebuild_module_data_ensure_required($module, &$modules) {
        -  $modules_cache = &drupal_static(__FUNCTION__);
        -  // Only rebuild once per request. $modules and $modules_cache cannot be
        -  // combined into one variable, because the $modules_cache variable is reset by
        -  // reference from system_list_reset() during the rebuild.
        ...
        +  return \Drupal::service('extension.list.module')->reset()->listExtensions();
        

        I think this is not the same.

        The reset() should not be needed - except for the first request to this function.

      dawehner’s picture

      Thank you fabian!

      - It would be fantastic to already have a draft CR for how things change from D7 (e.g. the mapping of old API to the new one)

      Hint, we have that actually :)

      - It would be great to have some docs in how the new extension system would work on a very high level.

      I totally agree

      This definitely should use its own special Exception type.

      MissingExtensionException

      Well, catch fought hard here to not throw an exception, so we cannot convert it to an exception atm. Is there anything we can do about it then?

      Should this not be injected instead?

      Injecting things into the installer is tricky, see #2380293: Properly inject services into ModuleInstaller but sure, we could give it a try.

      Why does the order change?

      Good question. Do you think this is a big deal? I guess its just the way how we organize the code, aka. let profile extend the module one.

      I think this is not the same.

      The reset() should not be needed - except for the first request to this function.

      You are right about this, but it also feels weird to keep a static around here to check that.

      Fabianx’s picture

      The change record needs some updates for the latest patch.

      --

      Well, catch fought hard here to not throw an exception, so we cannot convert it to an exception atm. Is there anything we can do about it then?

      What I mean is:

      catch (MissingExtensionException) {
      trigger_error(...);
      }

      there might be other things that make this fail, which we don't want to catch, e.g. database errors.

      --

      The order change should be fine, but should be documented in the CR - as others might have similar tests?

      --

      It indeed feels weird, but I guess it is the best option and have reset() explicitly clear the 'system_rebuild_module_data' static also.

      That would also keep BC for the non-API for the statics, which we tried to preserve BC in the past.

      e.g. the static reset would still work - but only for code that continues to use system_rebuild_module_data().

      So likely best would be to inject the drupal_static into the handler itself - if we want to keep BC there ...

      dawehner’s picture

      catch (MissingExtensionException) {
      trigger_error(...);
      }

      there might be other things that make this fail, which we don't want to catch, e.g. database errors.

      -

      oh gotcha, great point. Thank you!

      So likely best would be to inject the drupal_static into the handler itself - if we want to keep BC there ...

      Its a good conversation to have: Should the cleanness be preferred on the outer or inner level of the BC boundary.

      Version: 8.2.x-dev » 8.3.x-dev

      Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

      almaudoh’s picture

      Title: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList » [PP 1] Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList
      Version: 8.3.x-dev » 8.2.x-dev
      Status: Needs work » Postponed
      Related issues: +#2664322: key_value table is only used by a core service but it depends on system install
      dawehner’s picture

      This doesn't solve the key_value problem, but it solves one other issue as well as rerolls the patch.

      @almaudoh
      Do you think you can address the points I made in #2664322: key_value table is only used by a core service but it depends on system install ?

      larowlan’s picture

      This is a cold review, didn't read the previous 192 comments, so ignore stuff that has been covered already

      1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
        @@ -0,0 +1,516 @@
        +    // @todo On the longrun it would be great to add the reset, but the early
        

        nit: On » In

      2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
        @@ -0,0 +1,516 @@
        +      $this->moduleHandler->alter('system_info', $extension->info, $extension, $this->type);
        

        It's a pity this is a 'per extension' alter hook and not an 'alter all the extensions in one pass' hook, like many other discovery ones are. But clearly, changing that now is an API change.

      3. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
        @@ -0,0 +1,516 @@
        +    throw new \InvalidArgumentException("The {$this->type} $extension_name does not exist or is not installed.");
        

        This method only includes installed extensions right? So the 'does not exist' is misleading, if we hit this exception its not installed right?

      4. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
        @@ -0,0 +1,516 @@
        +    foreach ($this->listExtensions() as $extension_name => $extension) {
        +      $info[$extension_name] = $extension->info;
        ...
        +    foreach ($extensions as $name => $extension) {
        +      $file_names[$name] = $extension->getPathname();
        +    }
        

        minor observation: we could use array_map here, not sure which is more efficient or if it even matters

      5. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
        @@ -0,0 +1,516 @@
        +  public function setFilename($extension_name, $filename) {
        

        I think we should tag this as @internal

      6. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
        @@ -0,0 +1,516 @@
        +   * legally be located in any of these three places:
        +   *
        +   * core/modules/foo/foo.info.yml
        +   * modules/foo/foo.info.yml
        +   * sites/all/modules/foo/foo.info.yml
        +   * sites/example.com/modules/foo/foo.info.yml
        

        three places but four are listed? Also doesn't include modules inside profiles, so technically there is five

      7. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
        @@ -0,0 +1,516 @@
        +   * while a theme 'bar' may be located in any of similar places:
        

        same comment re profiles here

      8. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
        @@ -0,0 +1,516 @@
        +    if (isset($this->addedFileNames[$extension_name])) {
        +      return $this->addedFileNames[$extension_name];
        +    }
        +    elseif (isset($this->fileNames[$extension_name])) {
        +      return $this->fileNames[$extension_name];
        +    }
        +    elseif ($file_names = $this->getFilenames() && isset($file_names[$extension_name])) {
        +      return $file_names[$extension_name];
        +    }
        

        we could combine these into one if statement and avoid the elseif if we did

        $file_names = $this->addedFileNames + $this->fileNames +  $this->getFileNames();
        if (isset($file_names[$extension_name)) {
        return ...
        }
        throw new ...
        
      9. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
        @@ -0,0 +1,193 @@
        +   * @return \Drupal\Core\Extension\Extension|null
        

        Nit, needs a comment for return

      10. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
        @@ -0,0 +1,193 @@
        +    $active_profile_name = drupal_get_profile();
        ...
        +    $active_profile_name = drupal_get_profile();
        
        +++ b/core/lib/Drupal/Core/Extension/ProfileExtensionList.php
        @@ -0,0 +1,28 @@
        +    return [drupal_get_profile()];
        

        Could we put the call to drupal_get_profile() behind a protected method, just makes later refactoring easier.

      11. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
        @@ -0,0 +1,193 @@
        +      $module->schema_version = SCHEMA_UNINSTALLED;
        

        Pity we can't move this to a class constant and deprecate the old one while we're at it.

      12. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
        @@ -166,7 +166,7 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
        +            $module_path = \Drupal::service('extension.list.module')->getPath($name);
        
        @@ -182,10 +182,10 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
        +        \Drupal::service('extension.list.module')->reset();
        
        @@ -437,10 +437,10 @@ public function uninstall(array $module_list, $uninstall_dependents = TRUE) {
        +      \Drupal::service('extension.list.module')->reset();
        
        +++ b/core/lib/Drupal/Core/Updater/Module.php
        @@ -49,8 +49,7 @@ public static function getRootDirectoryRelativePath() {
        +    return \Drupal::service('extension.list.module')->extensionExists($this->name);
        

        Can this be injected? I realise there is some tight coupling here.

      13. +++ b/core/modules/system/system.module
        @@ -957,105 +947,7 @@ function system_get_info($type, $name = NULL) {
        +  return \Drupal::service('extension.list.module')->reset()->listExtensions();
        

        nice

      14. +++ b/core/tests/Drupal/KernelTests/Core/Bootstrap/GetFilenameTest.php
        @@ -25,9 +25,7 @@ function testDrupalGetFilename() {
        +    // Rebuild the filename data.
             system_rebuild_module_data();
        

        Should we be using the new method here too?

      almaudoh’s picture

      Title: [PP 1] Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList » Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList
      Status: Postponed » Needs work
      almaudoh’s picture

      Version: 8.2.x-dev » 8.3.x-dev

      Now in 8.3.x

      dawehner’s picture

      Status: Needs work » Needs review
      Issue tags: +Dublin2016
      FileSize
      59.02 KB
      4.72 KB

      Thank you @larowlan for your great review!

      This method only includes installed extensions right? So the 'does not exist' is misleading, if we hit this exception its not installed right?

      Nope, this is about the list of available modules.

      minor observation: we could use array_map here, not sure which is more efficient or if it even matters

      I'm all for it.

      we could combine these into one if statement and avoid the elseif if we did

      Mh, that would make the code less explicit.

      Can this be injected? I realise there is some tight coupling here.

      Can we have its own issue for that? #2380293: Properly inject services into ModuleInstaller

      Should we be using the new method here too?

      I try to minimize changes here.

      Status: Needs review » Needs work

      The last submitted patch, 196: 2208429-194.patch, failed testing.

      dawehner’s picture

      Status: Needs work » Needs review

      The last submitted patch, 176: 2208429-176.patch, failed testing.

      The last submitted patch, 192: 2208429-192.patch, failed testing.

      Version: 8.3.x-dev » 8.4.x-dev

      Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

      dawehner’s picture

      Here is another reroll.

      Status: Needs review » Needs work

      The last submitted patch, 202: 2208429-202.patch, failed testing.

      dawehner’s picture

      Status: Needs work » Needs review
      FileSize
      60.16 KB
      6.14 KB

      Here is some good progress with the issue.

      * Just like HEAD we need to keep a static cache during the installer. This could allow us to drop some of the changes of core.install.inc

      Status: Needs review » Needs work

      The last submitted patch, 204: 2208429-4.patch, failed testing.