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

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

Final reviews
Commit

User interface changes

None

API changes

New ExtensionList, ModuleExtensionList and ProfileExtensionList classes. New extension.list.module and extension.list.profile services to access information on modules and profiles.

Note: This change is still BC compatible.

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

    FAQ for reviewers

    The module installer doesn't have services injected, see #2208429-313: Extension System, Part III: ExtensionList, ModuleExtensionList and ProfileExtensionList

    CommentFileSizeAuthor
    #341 Screen Shot 2018-01-26 at 10.29.41 PM.png67.02 KBalmaudoh
    #334 interdiff-2208429.txt2.96 KBdawehner
    #333 2208429-333.patch70.34 KBdawehner
    #330 interdiff-2208429.txt1.07 KBdawehner
    #330 2208429-330.patch70.31 KBdawehner
    #321 interdiff-2208429.txt1.41 KBdawehner
    #321 2208429-321.patch70.31 KBdawehner
    #319 interdiff-2208429.txt1.92 KBdawehner
    #319 2208429-319.patch70.31 KBdawehner
    #311 interdiff-2208429-295-311.txt5.05 KBbircher
    #311 2208429-311.patch70.83 KBbircher
    #295 2208429-3-295.patch69.78 KBalexpott
    2208429-3-295.patch72.95 KBalexpott
    #291 2208429-291.patch69.55 KBalmaudoh
    #291 interdiff.txt1.09 KBalmaudoh
    #288 2208429-288.patch69.71 KBjibran
    #288 interdiff-56b862.txt1.24 KBjibran
    #286 2208429-286.patch69.7 KBjibran
    #286 interdiff-be534a.txt4.12 KBjibran
    #284 2208429-284.patch69.68 KBjibran
    #284 interdiff-cfb949.txt9.64 KBjibran
    #278 2208429-2-278.patch69.68 KBalexpott
    #278 275-278-interdiff.txt12.23 KBalexpott
    #275 2208429-2-275.patch68.94 KBalexpott
    #275 273-275-interdiff.txt2.76 KBalexpott
    #273 interdiff-2208429.txt591 bytesdawehner
    #273 2208429-273.patch69.03 KBdawehner
    #271 interdiff-2208429.txt5.26 KBdawehner
    #271 2208429-271.patch69.06 KBdawehner
    #269 interdiff-2208429.txt1.12 KBdawehner
    #269 2208429-269.patch68.98 KBdawehner
    #267 interdiff-2208429.txt12.51 KBdawehner
    #267 2208429-267.patch69.01 KBdawehner
    #266 interdiff-2208429.txt7.04 KBdawehner
    #266 2208429-266.patch69.05 KBdawehner
    #266 Screen Shot 2017-09-22 at 09.32.52.jpg126.33 KBdawehner
    #266 Screen Shot 2017-09-22 at 09.32.26.jpg527.99 KBdawehner
    #265 2208429-265.patch64.61 KBdawehner
    #255 interdiff-2208429.txt2.32 KBdawehner
    #255 2208429-255.patch64.61 KBdawehner
    #248 2208429-248.patch63.99 KBalexpott
    #248 246-248-interdiff.txt899 bytesalexpott
    #246 2208429-245.patch63.85 KBalexpott
    #246 241-245-interdiff.txt2.36 KBalexpott
    #241 2208429-241.patch63.33 KBalexpott
    #241 237-241-interdiff.txt15.88 KBalexpott
    #237 2208429-237.patch63.37 KBdawehner
    #235 interdiff-2208429.txt3.65 KBdawehner
    #235 2208429-235.patch90.6 KBdawehner
    #232 interdiff-2208429.txt893 bytesdawehner
    #232 2208429-232.patch63.36 KBdawehner
    #230 interdiff-2208429.txt2.25 KBdawehner
    #230 2208429-230.patch63.16 KBdawehner
    #227 interdiff-2208429.txt18.44 KBdawehner
    #227 2208429-227.patch62.32 KBdawehner
    #223 interdiff-2208429.txt1.58 KBdawehner
    #223 2208429-223.patch61.19 KBdawehner
    #222 interdiff-2208429.txt1.65 KBdawehner
    #222 2208429-222.patch62.04 KBdawehner
    #221 interdiff-2208429.txt13.35 KBdawehner
    #221 2208429-221.patch62.04 KBdawehner
    #219 interdiff-2208429.txt2.96 KBdawehner
    #219 2208429-218.patch61.63 KBdawehner
    #212 interdiff-2208429.txt4.95 KBdawehner
    #212 2208429-212.patch62.07 KBdawehner
    #209 interdiff-2208429.txt7.34 KBdawehner
    #209 2208429-209.patch61.03 KBdawehner
    #206 interdiff-2208429.txt4.82 KBdawehner
    #206 2208429-206.patch60.81 KBdawehner
    #204 interdiff-2208429.txt6.14 KBdawehner
    #204 2208429-4.patch60.16 KBdawehner
    #202 2208429-202.patch58.22 KBdawehner
    #196 interdiff.txt4.72 KBdawehner
    #196 2208429-194.patch59.02 KBdawehner
    #192 interdiff.txt1.51 KBdawehner
    #192 2208429-192.patch58.9 KBdawehner
    #184 interdiff.txt816 bytesdawehner
    #184 2208429-184.patch58.08 KBdawehner
    #180 extension_system_part-2208429-180.patch58.05 KBalmaudoh
    #180 interdiff.txt5.67 KBalmaudoh
    #176 interdiff.txt3.67 KBdawehner
    #176 2208429-176.patch57.98 KBdawehner
    #173 interdiff.txt10.42 KBdawehner
    #173 2208429-173.patch58.64 KBdawehner
    #166 interdiff.txt13.32 KBdawehner
    #166 2208429-165.patch58.05 KBdawehner
    #163 2208429-163.patch55.14 KBdawehner
    #160 drupal_2208429_160.patch54.42 KBXano
    #160 interdiff.txt6.08 KBXano
    #159 interdiff.txt3.72 KBdawehner
    #159 2208429-159.patch54.41 KBdawehner
    #152 D8-2208429-152-vs-139.interdiff.txt9.8 KBdonquixote
    #152 D8-2208429-152-ExtensionList.patch54.09 KBdonquixote
    #144 D8-2208429-144-ExtensionHub-ExtensionList-vs-129.interdiff.txt73.82 KBdonquixote
    #144 D8-2208429-144-ExtensionHub-ExtensionList.patch74.58 KBdonquixote
    #139 interdiff.txt2.23 KBdawehner
    #139 2208429-139.patch53.04 KBdawehner
    #129 drupal_2208429_129.patch53 KBXano
    #129 interdiff.txt7.51 KBXano
    #127 drupal_2208429_127.patch52.9 KBXano
    #127 interdiff.txt12.8 KBXano
    #116 interdiff.txt4.26 KBdawehner
    #116 2208429-116.patch51.73 KBdawehner
    #114 interdiff.txt2.61 KBdawehner
    #114 2208429-114.patch52.68 KBdawehner
    #113 interdiff.txt3.27 KBdawehner
    #113 2208429-113.patch53.03 KBdawehner
    #111 interdiff.txt3.34 KBdawehner
    #111 2208429-111.patch52.23 KBdawehner
    #106 Screen Shot 2016-02-29 at 15.33.41.png777.63 KBdawehner
    #104 56d452431030b.drupal-perf.xhprof.txt731.62 KBdawehner
    #104 56d4524f84a32.drupal-perf.xhprof.txt733.21 KBdawehner
    #103 2208429_103.patch52.14 KBMile23
    #95 interdiff.txt2.79 KBdawehner
    #95 2208429-95.patch52.61 KBdawehner
    #93 extension_system_part-2208429-93.patch49.82 KBalmaudoh
    #93 interdiff-80-93.txt3.59 KBalmaudoh
    #90 2208429-90.patch47.08 KBdawehner
    #86 2208429.patch40.17 KBalmaudoh
    #80 extension_system_part-2208429-80.patch47.06 KBalmaudoh
    #80 interdiff.txt9.02 KBalmaudoh
    #78 interdiff.txt7.97 KBdawehner
    #78 2208429-78.patch46.41 KBdawehner
    #73 extension_system_part-2208429-72.patch39.3 KBalmaudoh
    #73 interdiff.txt1.49 KBalmaudoh
    #70 extension_system_part-2208429-70.patch38.34 KBalmaudoh
    #70 interdiff.txt11 KBalmaudoh
    #68 extension_system_part-2208429-68.patch37.76 KBalmaudoh
    #68 interdiff.txt28.67 KBalmaudoh
    #63 extension_system_part-2208429-63.patch29 KBalmaudoh
    #63 interdiff.txt577 bytesalmaudoh
    #60 extension_system_part-2208429-60.patch28.44 KBalmaudoh
    #60 interdiff.txt2.81 KBalmaudoh
    #56 extension_system_part-2208429-56.patch26.71 KBalmaudoh
    #56 interdiff.txt831 bytesalmaudoh
    #54 extension_system_part-2208429-54.patch26.73 KBalmaudoh
    #54 interdiff.txt4.51 KBalmaudoh
    #51 2208429-51.patch26.78 KBdawehner
    #47 extension_system_part-2208429-47.patch25.97 KBalvar0hurtad0
    #41 interdiff-2208429-39-41.txt490 bytesamitgoyal
    #41 2208429-41.patch24.8 KBamitgoyal
    #39 2208429-39.patch24.8 KBamitgoyal
    #34 interdiff.txt1.54 KBdawehner
    #34 2208429-34.patch24.8 KBdawehner
    #30 2208429-30.patch23.47 KBstefan.r
    #25 interdiff.txt1.39 KBdawehner
    #25 2208429-25.patch23.53 KBdawehner
    #23 interdiff.txt789 bytesdawehner
    #23 2208429-23.patch23.43 KBdawehner
    #21 interdiff.txt7.74 KBdawehner
    #21 2208429-21.patch23.44 KBdawehner
    #19 interdiff.txt2.72 KBdawehner
    #19 2208429-19.patch20.57 KBdawehner
    #17 interdiff.txt689 bytesdawehner
    #17 2208429-17.patch20.42 KBdawehner
    #14 interdiff.txt8.61 KBdawehner
    #14 2208429-14.patch19.94 KBdawehner
    #12 interdiff.txt13.33 KBdawehner
    #12 2208429-12.patch23.02 KBdawehner
    #10 2208429-10.patch12.68 KBdawehner
    Support from Acquia helps fund testing for Drupal Acquia logo

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

    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

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    23.02 KB
    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
    8.61 KB

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

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    20.42 KB
    689 bytes

    Let's see whether this is enough so far.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    20.57 KB
    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
    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.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    23.43 KB
    789 bytes

    Ups, that was horrible.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    23.53 KB
    1.39 KB

    Just fixes.

    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

    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
    1.54 KB

    Can be done now?

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

    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

    Reroll of #34.

    amitgoyal’s picture

    Status: Needs work » Needs review
    FileSize
    24.8 KB
    490 bytes

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

    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.

    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.

    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.

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

    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.

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

    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

    dawehner’s picture

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

    Here is a unit test.

    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 and ThemeEngineExtensionList, also updated the @todo

    Thanks for the unit test @dawehner

    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.

    almaudoh’s picture

    Woah!! is DrupalCI down???

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

    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.

    almaudoh’s picture

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

    Fixed two test fails...

    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: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname(), 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.

    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: Deprecate drupal_get_path() and drupal_get_filename() and replace with ExtensionList::getPath() and ExtensionList::getPathname(), #2442383: Add the option to cache drupal_get_filename(), #697946: Properly deprecate module_load_include() and move it into \Drupal::moduleHandler() service.

    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

    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.

    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.

    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

    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.

    dawehner’s picture

    Status: Needs work » Needs review

    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.

    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

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    60.81 KB
    4.82 KB

    There have been two remaining problems:

    • Some code, like the DrupalKernelTest relied purely on the container.modules information. I think we should leverage that in general, as its static information which avoids reparsing on runtime
    • The order in which we merge in profile information diverged from system_module_rebuild_data(). This interdiff fixes that by making the code more parallel to the original.
    dawehner’s picture

    Issue summary: View changes
    jibran’s picture

    Can we please update the IS and mention that this change is BC.

    1. +++ b/core/includes/install.core.inc
      @@ -437,6 +431,12 @@ function install_begin_request($class_loader, &$install_state) {
      +  // @todo Remove as part of https://www.drupal.org/node/2186491
      

      This is a meta issue now. On which sub-issue does this depend? Do we need to create a new one?

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +   * @todo Replace drupal_get_profile() once
      +   *   https://www.drupal.org/node/2156401 is in.
      

      This has been fixed.

    3. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,236 @@
      +    ¶
      

      White space.

    dawehner’s picture

    Issue summary: View changes
    Status: Needs work » Needs review
    FileSize
    61.03 KB
    7.34 KB

    Thank you @jibran for your review!

    This has been fixed.

    Great point! We can clean quite a bunch of ugliness up now, I guess.

    This is a meta issue now. On which sub-issue does this depend? Do we need to create a new one?

    Well that is not 100% sure yet. This issue is one of many steps :)

    Berdir’s picture

    +++ b/core/core.services.yml
    @@ -1,4 +1,4 @@
    -parameters:
    +parameter:
       session.storage.options:
    

    I'm guessing this was not on purpose ;)

    1. +++ b/core/includes/bootstrap.inc
      @@ -207,11 +208,26 @@ function drupal_get_filename($type, $name, $filename = NULL) {
      +    /** @var \Drupal\Core\Extension\ExtensionList $extension_list */
      +    $extension_list = \Drupal::service($service_id);
      +    if (isset($filename)) {
      +      // Manually add the info file path of an extension.
      +      $extension_list->setFilename($name, $filename);
      +      return;
      +    }
      

      I'm wondering if we want to convert some calls to drupal_get_filename() already here. The patch is already pretty big but we might be able to get rid of a bunch of @todo's.. in \Drupal\Core\Config\ExtensionInstallStorage::getAllFolders() and \Drupal\Core\Config\InstallStorage::getAllFolders() for example. Although maybe the point of the @todo is to change things completely, not just call a different method?

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

      In the long run: https://www.usingenglish.com/forum/threads/135310-IN-or-ON-the-long-run

    3. +++ 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 system_rebuild_module_data() to pick up the
      -        // new module, since it merges the installation status of modules into
      -        // its statically cached list.
      -        drupal_static_reset('system_rebuild_module_data');
      +        // Clear the static cache of the "extension.list.module" service to pick
      +        // up the new module, since it merges the installation status of modules
      +        // into its statically cached list.
      +        \Drupal::service('extension.list.module')->reset();
      

      Wondering if that could be seen as an API change. I think not, as there is no reason to call this outside of tests and the ModuleInstaller. And if someone else uses it, I guess it is more likely that they used system_list_reset(), like config_installer.

    4. +++ b/core/modules/system/system.module
      @@ -992,105 +982,7 @@ function system_get_info($type, $name = NULL) {
        */
       function _system_rebuild_module_data() {
      

      any reason to not @deprecate those two functions here?

    5. +++ b/core/modules/system/system.module
      @@ -992,105 +982,7 @@ function system_get_info($type, $name = NULL) {
      - */
      -function _system_rebuild_module_data_ensure_required($module, &$modules) {
      -  if (!empty($module->info['required'])) {
      

      see #65.17 and the response in #68, not sure we can remove something just because it is not used in core.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    62.07 KB
    4.95 KB

    I'm guessing this was not on purpose ;)

    Wow, how is Drupal not broken!

    I'm wondering if we want to convert some calls to drupal_get_filename() already here. The patch is already pretty big but we might be able to get rid of a bunch of @todo's.. in \Drupal\Core\Config\ExtensionInstallStorage::getAllFolders() and \Drupal\Core\Config\InstallStorage::getAllFolders() for example. Although maybe the point of the @todo is to change things completely, not just call a different method?

    Yeah I think we should try to limit the scope of this issue as much as possible. These installer issues might be solveable properly ...

    Wondering if that could be seen as an API change. I think not, as there is no reason to call this outside of tests and the ModuleInstaller. And if someone else uses it, I guess it is more likely that they used system_list_reset(), like config_installer.

    We argued on IRC about it, and I believe we both agree now its not.

    any reason to not @deprecate those two functions here?

    I indeed added some additional deprecations.

    Mile23’s picture

    jibran’s picture

    6 coding standards messages

    Berdir’s picture

    23:14:48 Catchable fatal error: Argument 8 passed to Drupal\Core\Extension\ModuleExtensionList::__construct() must implement interface Drupal\Core\Config\ConfigFactoryInterface, instance of Drupal\Core\Extension\ProfileExtensionList given in /var/www/html/core/lib/Drupal/Core/Extension/ModuleExtensionList.php on line 70

    This is causing the CI error

    Berdir’s picture

    1. +++ b/core/includes/bootstrap.inc
      @@ -219,6 +219,7 @@ function drupal_get_filename($type, $name, $filename = NULL) {
           else {
             try {
      +        @trigger_error('drupal_get_filename() is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, you should use \Drupal::service(\'extension.list.module\')->getFileName() and \Drupal::service(\'extension.list.theme\')->getFileName(). See https://www.drupal.org/node/2709919', E_USER_DEPRECATED);
               return $extension_list->getFilename($name);
      

      Hm. drupal_get_filename() isn't used that much directly, but drupal_get_path() is just a slim wrapper around it and I'm seeing 350 calls to that in my installation, mostly by contrib modules.

      This kind of @trigger_error() isn't free, and if we have a lot of calls to it, then that adds overhead.

      Also, neither drupal_get_path() nor drupal_get_filename() are documented as deprecated at the moment. Because it isn't yet in all cases.

      Should we create a follow-up of this and the theme extension list issue to convert most/all remaining usages of those two functions in core and then deprecate it with a trigger error, for all cases?

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -169,7 +169,7 @@ public function reset() {
           // cache.
      -    // @todo On the longrun it would be great to add the reset, but the early
      +    // @todo In the longrun it would be great to add the reset, but the early
      

      better, but pretty sure it must be long run, with space.

    3. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
      @@ -717,6 +717,7 @@ public function getModuleDirectories() {
         public function getName($module) {
      +    @trigger_error('ModuleHandlerInterface::getName() is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, you should use \Drupal::service(\'extension.list.module\')->getName(). See https://www.drupal.org/node/2709919', E_USER_DEPRECATED);
           return \Drupal::service('extension.list.module')->getName($module);
      

      same here, IMHO we should at least convert the usages in core before starting to add trigger_error() warnings.

    4. +++ b/core/modules/system/system.module
      @@ -976,12 +976,40 @@ function system_get_info($type, $name = NULL) {
        *   An associative array of module information.
      + *
      + * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0.
      

      needs a message about what to use instead.

    Wim Leers’s picture

    Wow. Epic work.

    1. +++ b/core/includes/install.core.inc
      @@ -302,12 +302,6 @@ function install_begin_request($class_loader, &$install_state) {
      -  // Before having installed the system module and being able to do a module
      -  // rebuild, prime the drupal_get_filename() static cache with the module's
      -  // exact location.
      -  // @todo Remove as part of https://www.drupal.org/node/2186491
      
      @@ -437,6 +431,12 @@ function install_begin_request($class_loader, &$install_state) {
      +  // Before having installed the system module and being able to do a module
      +  // rebuild, prime the drupal_get_filename() static cache with the module's
      +  // exact location.
      +  // @todo Remove as part of https://www.drupal.org/node/2186491
      +  drupal_get_filename('module', 'system', 'core/modules/system/system.info.yml');
      

      Why do we need to move this?

      (I grepped the issue, nobody asked this earlier.)

    2. +++ b/core/includes/install.inc
      @@ -618,6 +618,13 @@ function drupal_install_system($install_state) {
      +  // exact location.
      

      "exact location" sounds like "absolute file path" to me, but apparently a relative file path is sufficient?

    3. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +  /**
      +   * A list of extension folder names directly added in code (not discovered).
      +   *
      +   * It is important to keep a separate list to ensure that it takes priority
      +   * over the discovered extension folders.
      +   *
      +   * @var string[]
      +   */
      +  protected $addedFileNames = [];
      

      This sounds edge-casey. It makes me think this is only necessary for tests.

      What is a concrete use case?

    4. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +  protected static $staticAddedFileNames;
      

      So this one is similar to $addedFileNames, but for another edge case: installer.

      Why can't both edge cases use the same?

    5. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +    // We explicitly don't reset the addedFileNames as its sort of a static
      

      s/its/it is/

    6. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +  protected function getListCacheId() {
      

      Why not just getCacheId()? "List" is kind of obvious/pointless, because it's already in the classname?

    7. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +  protected function getInfoCacheId() {
      

      Oh wait this is the other thing. I have no idea what this "info" is though.

    8. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +   * Returns the extension filenames cache ID.
      ...
      +  protected function getFilenameCacheId() {
      

      Plural "filenames" vs singular "filename".

    9. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +   *   TRUE if the extension exists (whether installed or not) and FALSE if not.
      

      "whether" sounds strange here, I think "regardless" is a better fit?

    10. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +   *   The extension name.
      ...
      +   *   If there is no extension with the supplied machine name.
      ...
      +   *   The extension name.
      ...
      +   *   Processed extension object for the given machine name.
      ...
      +   *   If there is no extension with the supplied name.
      

      "extension name" vs "machine name" not being used consistently here.

    11. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +   * Build the actual list of extensions before caching it.
      

      Builds the list of extensions.

      ("before caching it" is not something we usually add to these sorts of methods?)

    12. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +      // Merge in defaults and save.
      

      Are defaults shared? I think they're extension type-specific? Also, this is not saving anything?

      So I suggest: Merge extension type-specific defaults.

    13. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +  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.");
      +  }
      

      Why does this only work for installed extensions? The name suggests this works for any available extension, installed or not.

    14. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +   * The information is placed in cache with the "system.{extension_type}.info"
      +   * key.
      ...
      +   * The information is placed in cache with the "system.{extension_type}.files"
      +   * key.
      

      Rather than this, I'd do @see ::theDecoratingMethodThatDoesTheCaching.

      Mentioning the cache ID (not cache "key"!) here explicitly causes you to need to keep them in sync in two places. Seems unnecessary.

    15. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +      // We use $file_names below.
      +      elseif ($file_names = $this->state->get($cache_id)) {
      +      }
      +      else {
      +        $file_names = $this->recalculateFilenames();
      

      I've never seen this pattern before.

    16. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +   * @internal
      

      Good :)

      I almost wonder if we should have an installer-specific decorator for these classes, so that we only burden those decorators with the installer-specific needs?

    17. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +    if (!empty($GLOBALS['install_state'])) {
      

      This makes me wish for this to live in a decorator even more.

    18. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +   * Gets the filename for a system resource.
      ...
      +   *   The filename of the requested extension's .info.yml file.
      

      This says "filename", but I think it actually means "file path"?

      The whole point is that you get the path to the info file (and hence the module/theme), regardless of where it may be stored on disk.

      Also: "system resource" -> "extension".

    19. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +   * 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
      

      Let's prefix these with dashes so that it's a list.

      Also: this is missing an installation profile example.

    20. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,553 @@
      +   *   The drupal-root-relative path to the specified extension.
      

      Nit: s/drupal/Drupal/

    21. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,235 @@
      +  /**
      +   */
      +  protected $moduleLocations;
      

      Nit: incomplete.

    22. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,235 @@
      +      $this->setFilename($module_name, $info['pathname']);
      

      Hm, the docs for ::setFilename() said that this was only necessary in rare cases. This makes it look like it's always necessary?

    23. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,235 @@
      +   * @return array
      

      string[]?

    24. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,235 @@
      +  protected function doListExtensions() {
      +
      +    // Find modules.
      

      Nit: unnecessary \n.

    25. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,235 @@
      +      // The installation profile is required, if it's a valid module.
      

      I don't know what this means.

    26. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,235 @@
      +          $modules[$dependency_name]->info['explanation'] = $this->t('Dependency of required module @module', array('@module' => $module->info['name']));
      

      s/array()/[]/

    27. +++ 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 system_rebuild_module_data() to pick up the
      -        // new module, since it merges the installation status of modules into
      -        // its statically cached list.
      -        drupal_static_reset('system_rebuild_module_data');
      +        // Clear the static cache of the "extension.list.module" service to pick
      +        // up the new module, since it merges the installation status of modules
      +        // into its statically cached list.
      +        \Drupal::service('extension.list.module')->reset();
      

      NICE! :)

    28. +++ b/core/lib/Drupal/Core/Session/AccountProxyInterface.php
      @@ -6,6 +6,7 @@
      + * @internal
      

      +1

      (Although I do somewhat wonder if it's okay to still do this now that D8 has been out for so long.)

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    61.63 KB
    2.96 KB

    better, but pretty sure it must be long run, with space.

    This is what my IDE is telling me as well.

    I removed the trigger_errors' out of the critical path for now

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    62.04 KB
    13.35 KB

    Than you for the review wim!

    To be clear, this issue is not the end of the work here. There is more refactoring to be done and some more methods (drupal_get_path() / drupal_get_filename() will be marked as deprecated in the future), but you know, we cannot solve all problems at once.

    Note: This does not address point #1 yet, which I will try out in the patch afterwards.

    This sounds edge-casey. It makes me think this is only necessary for tests.
    What is a concrete use case?

    Well, the problem is that drupal_get_filename() used to have this feature using its static cache. We cannot just drop the feature for now, as it is a performance optimization. What we should do IMHO is to mark these explicit as internal.

    So this one is similar to $addedFileNames, but for another edge case: installer.
    Why can't both edge cases use the same?

    As explained on \Drupal\Core\Extension\ExtensionList::setFilename we need a static cache here, something which is kept during the installer. On the other hand for actual runtime we don't want to have an actual static variable floating around. I hope on the longrun we can at least clean up the installer, for example by having an installer kernel with extra services.

    Why not just getCacheId()? "List" is kind of obvious/pointless, because it's already in the classname?

    Well, we have one cache ID per cache entry in this class, of which we have multiple.

    Oh wait this is the other thing. I have no idea what this "info" is though.

    I fixed the class to actually use it, which is use on \Drupal\Core\Extension\ExtensionList::getAllAvailableInfo

    Plural "filenames" vs singular "filename".

    Good point, it should be indeed plural.

    "whether" sounds strange here, I think "regardless" is a better fit?

    Agreed

    "extension name" vs "machine name" not being used consistently here.

    We are in the domain of extensions, so I believe using extension machine name, especially to make it explicit, is the way to go.

    ("before caching it" is not something we usually add to these sorts of methods?)

    I'm happy to drop it.

    So I suggest: Merge extension type-specific defaults.

    Good point, this is better indeed.

    Why does this only work for installed extensions? The name suggests this works for any available extension, installed or not.

    Well yeah, things are tricky. This is caused by system_get_info() was used before, which is explicit just about installed extensions. We could always add an additional method afterwards.

    Rather than this, I'd do @see ::theDecoratingMethodThatDoesTheCaching.

    Mentioning the cache ID (not cache "key"!) here explicitly causes you to need to keep them in sync in two places. Seems unnecessary

    Good point. These places should not care about the caching aspect of things.

    I've never seen this pattern before.

    We can indeed do that.

    I almost wonder if we should have an installer-specific decorator for these classes, so that we only burden those decorators with the installer-specific needs?

    Well, its tricky. We discussed that before to have a dedicated installer kernel (also see the point above), but fixing this is non trivial and is A LOT OF effort. I hope marking things as internal allows us to make progress on the longrun.

    This says "filename", but I think it actually means "file path"?

    The whole point is that you get the path to the info file (and hence the module/theme), regardless of where it may be stored on disk.

    Also: "system resource" -> "extension".

    I think I agree with you. Filename is confusing, but yeah drupal_get_filename() was always about that :) Do we think we should change this idea here?

    Nit: incomplete.

    Well, we can actually drop it :)

    Hm, the docs for ::setFilename() said that this was only necessary in rare cases. This makes it look like it's always necessary?

    Note: This is an internal function call ... and as such this is an optimization (... so we can skip using a cache get in most cases). We have all this kind of information already at our hand for modules, but also just for installed modules, while getFilename supports non installed extensions as well.

    ... Note: All this technical debt exists in HEAD as well, we just make it more visible here.
    Here is an example: You could argue: calling getFilename() is a rare thing for uninstalled modules, and as such doesn't have to cached, but well, if you'd remove that, #1081266: Avoid re-scanning module directory when a filename or a module is missing would be open again.

    string[]?

    Fixed that.

    I don't know what this means.

    I guess there was some if around that in previous times.

    dawehner’s picture

    Oh let's fix the fatal first.

    dawehner’s picture

    I'm quite sure that moving things further up should totally be possible.

    jibran’s picture

    1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
      @@ -840,7 +840,10 @@ protected function initializeContainer() {
      +        // Ensure to not accidentally initialize the user.
      +        $current_user_id = $current_user->hasAccount() ? $current_user->id() : 0;
      

      WTF :(

    2. +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
      @@ -55,6 +55,13 @@ public function setAccount(AccountInterface $account) {
      +  public function hasAccount() {
      

      Should we make separate issue for this with tests?

    3. +++ b/core/modules/system/system.module
      @@ -1094,39 +1001,31 @@ function _system_rebuild_module_data_ensure_required($module, &$modules) {
      + *   Use \Drupal::service(\'extension.list.module\')->reset()->listExtensions()
      

      Why do want to escape the '?

    dawehner’s picture

    WTF :(

    Note: We have a comment about that in the issue summary ...

    Why do want to escape the '?

    Heh, this was just a c&p

    Should we make separate issue for this with tests?

    I just added some quick test coverage around that. Are you sure this is worth its own issue?

    Berdir, webflo, wim and me talked about the naming of ::getFilename vs. ::getPathname and we agreed on using ::getPathname as its in parallel to both \Drupal\Core\Extension\Extension and \SplFileInfo.

    Note: I also fixed the fatal and the coding style.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    63.16 KB
    2.25 KB

    Reverting #218.1 causes the following error:

    1) Drupal\FunctionalJavascriptTests\Ajax\AjaxTest::testAjaxWithAdminRoute
    Drupal\Core\DependencyInjection\ContainerNotInitializedException: \Drupal::$container is not initialized yet. \Drupal::setContainer() must be called with a real container.
    
    /var/www/html/core/lib/Drupal.php:129
    /var/www/html/core/lib/Drupal.php:158
    /var/www/html/core/includes/bootstrap.inc:214
    /var/www/html/core/includes/install.core.inc:309
    /var/www/html/core/includes/install.core.inc:114
    /var/www/html/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php:291
    /var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:947
    /var/www/html/core/tests/Drupal/Tests/BrowserTestBase.php:429
    

    Let's keep this behaviour so. But you know, when we fix drupal_get_filename() we might not even need this workaround any longer.

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    63.36 KB
    893 bytes

    Let's fix it, it was some testing invalidation issue.

    almaudoh’s picture

    Great to see this issue coming back. Thanks @dawehner.

    Berdir, webflo, wim and me talked about the naming of ::getFilename vs. ::getPathname and we agreed on using ::getPathname as its in parallel to both \Drupal\Core\Extension\Extension and \SplFileInfo.

    Where this may lead to ambiguity is that drupal_get_path() becomes ExtensionList::getPath() while drupal_get_filename() becomes ExtensionList::getPathname(). I'm not familiar with the usage of "pathname" to refer to the full path of the .info.yml file. Is that what we actually mean? Because that's what that method is doing.

    1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,555 @@
      +  abstract protected function getInstalledExtensionsNames();
      

      getInstalledExtensionNames() sounds better here.

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,555 @@
      +   * Gets the filename for an extension.
      

      Since getPathname() is actually returning the full path to the .info.yml file, this should really be Gets the info file path for an extension.

    dawehner’s picture

    Where this may lead to ambiguity is that drupal_get_path() becomes ExtensionList::getPath() while drupal_get_filename() becomes ExtensionList::getPathname(). I'm not familiar with the usage of "pathname" to refer to the full path of the .info.yml file. Is that what we actually mean? Because that's what that method is doing.

    You are absolute right, this is a bit confusing when you come from the world of drupal. Both the Extension and the \SplFileInfo class though provide those names already.

    getInstalledExtensionNames

    Good point! I cannot agree more.

    Since getPathname() is actually returning the full path to the .info.yml file, this should really be Gets the info file path for an extension.

    Fair, let's adapt this :)

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    63.37 KB

    The last patch had some other patch merged in.

    dawehner’s picture

    Status: Needs work » Needs review
    alexpott’s picture

    1. +++ b/core/core.services.yml
      @@ -503,6 +503,12 @@ services:
      +  extension.list.module:
      +    class: Drupal\Core\Extension\ModuleExtensionList
      +    arguments: ['@app.root', 'module', '@cache.default', '@info_parser', '@module_handler', '@state', '@config.factory', '@extension.list.profile', '%install_profile%', '%container.modules%']
      +  extension.list.profile:
      +    class: Drupal\Core\Extension\ProfileExtensionList
      +    arguments: ['@app.root', 'profile', '@cache.default', '@info_parser', '@module_handler', '@state', '%install_profile%']
      

      I wish there were less dependencies.

    2. +++ b/core/includes/install.core.inc
      @@ -1063,7 +1063,7 @@ function install_base_system(&$install_state) {
         // Prime the drupal_get_filename() static cache with the user module's
      -  // exact location.
      +  // location.
      

      out-of-scope :) and these exact words are used later.

    3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
      @@ -840,7 +840,10 @@ protected function initializeContainer() {
      -        $current_user_id = $this->container->get('current_user')->id();
      +        /** @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;
      
      +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
      @@ -55,6 +55,13 @@ public function setAccount(AccountInterface $account) {
      +  public function hasAccount() {
      +    return isset($this->account);
      +  }
      +
      +  /**
      +   * {@inheritdoc}
      +   */
      

      Not sure this is necessary anymore. Since #2753733: AccountProxy can do unnecessary user loads to get an ID landed.

    4. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,555 @@
      +    // @todo In the long run it would be great to add the reset, but the early
      +    //   installer fails due to that.
      

      If we want a todo here we should file the follow up and link it here. I think just explaining why we don't reset it is okay. Or we could use a different extension list in the early installer... if we think that that is worth exploring we could file the followup.

    5. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,555 @@
      +   * @param string $extension_name
      +   *   The name of an extension whose information shall be returned.
      ...
      +   * @param string $extension_name
      +   *   The name of the extension for which the filename is requested.
      ...
      +   * @param string $extension_name
      +   *   The name of the extension for which the pathname is requested.
      ...
      +   * @param string $extension_name
      +   *   The name of the extension for which the path is requested.
      

      Let's state that it's the machine name - so it's not confused with ::getName()

    6. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,230 @@
      +  use StringTranslationTrait;
      ...
      +          $modules[$dependency_name]->info['explanation'] = $this->t('Dependency of required module @module', ['@module' => $module->info['name']]);
      

      Let's just create s TranslatableMarkup object - less dependencies down here is always nice.

    7. +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
      @@ -306,6 +306,9 @@ public function getModuleDirectories();
      +   *
      +   * @deprecated in Drupal 8.4.0, will be removed before Drupal 9.0.0.
      +   *   Use \Drupal::service('extension.list.module')->getName() instead.
      

      I think we should file follow-ups to do the deprecations. Doing the deprecations and triggering errors and doing the replacements in this patch will make it huge.

    8. +++ b/core/modules/book/tests/src/Kernel/BookUninstallTest.php
      @@ -76,7 +76,7 @@ public function testBookUninstall() {
      -    $module_data = _system_rebuild_module_data();
      
      +++ b/core/modules/system/system.module
      @@ -1094,39 +1000,31 @@ function _system_rebuild_module_data_ensure_required($module, &$modules) {
      + * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0.
      + *   Use \Drupal::service('extension.list.module')->reset()->listExtensions()
      + *   instead. Note: You probably don't need the reset() method.
      + */
      +function _system_rebuild_module_data() {
      +  @trigger_error("_system_rebuild_module_data() is deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0. Instead, you should use \\Drupal::service('extension.list.module')->reset()->listExtensions(). See https://www.drupal.org/node/2709919", E_USER_DEPRECATED);
      +  return \Drupal::service('extension.list.module')->reset()->listExtensions();
      

      This can be deprecated here because all usages are accounted for.

    9. +++ b/core/modules/system/system.module
      @@ -1094,39 +1000,31 @@ function _system_rebuild_module_data_ensure_required($module, &$modules) {
      + *
      + * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0.
        */
       function system_rebuild_module_data() {
      

      We need to do this in a follow-up. There are way too many calls to system_rebuild_module_data() to handle in this issue.

      Also, it's weird that system_rebuild_module_data() and _system_rebuild_module_data() are the same. Are we sure there is no performance impact to removing the static caching from system_rebuild_module_data()?

    10. +++ b/core/modules/system/tests/src/Kernel/Extension/ModuleHandlerTest.php
      @@ -105,7 +105,7 @@ public function testDependencyResolution() {
      -    drupal_static_reset('system_rebuild_module_data');
      +    \Drupal::service('extension.list.module')->reset();
      

      This has me thinking about whether we need to implement something in drupal_static_reset() to support calling the service instead. This would preserve a sort of BC. Not sure.

    11. +++ b/core/tests/Drupal/Tests/Core/Extension/ExtensionListTest.php
      @@ -0,0 +1,259 @@
      +class ExtensionListTest extends UnitTestCase {
      

      This is missing coverage of the reset method.

    alexpott’s picture

    Patch addresses #240 - 2, 3, 5, 6, 7, 8, removes the deprecation mentioned in 9, and adds some missing test coverage.

    alexpott’s picture

    system_rebuild_module_data() used to have a static cache... but now it calls \Drupal::service('extension.list.module')->reset()->listExtensions(). Are we sure this doesn't have some awful performance implications?

    alexpott’s picture

    Standard + simpletest install times on php 7.1:

    With patch

    real	0m17.901s
    user	0m9.291s
    sys	0m1.942s
    
    real	0m17.649s
    user	0m9.185s
    sys	0m1.877s
    
    real	0m18.048s
    user	0m9.373s
    sys	0m1.945s
    

    Without patch

    real	0m15.752s
    user	0m8.166s
    sys	0m1.810s
    
    real	0m14.364s
    user	0m7.559s
    sys	0m1.612s
    
    real	0m15.882s
    user	0m8.051s
    sys	0m1.757s
    

    Seems like we have a performance regression here somewhere.

    alexpott’s picture

    So here's a blackfire comparison of the minimal install test scenario... https://blackfire.io/profiles/compare/259259f8-8213-417e-86e5-7f28d550e3...

    We're calling Drupal\Core\Extension\ModuleHandler::parseDependency 11000+ more times :) - for example.

    catch’s picture

    That sounds like we need to replace some of the usages of system_rebuild_module_data() in this patch to avoid the regression - more or less anywhere that's not doing a drupal_static_reset() before the call but at least install_profile_modules() should help?

    I think it's OK for system_rebuild_module_data() itself to have a performance regression though as long as the important spots in core are updated.

    alexpott’s picture

    diff --git a/core/includes/install.core.inc b/core/includes/install.core.inc
    index 1f404f0..907b686 100644
    --- a/core/includes/install.core.inc
    +++ b/core/includes/install.core.inc
    @@ -1497,7 +1497,7 @@ function install_profile_modules(&$install_state) {
       install_core_entity_type_definitions();
    
       $modules = \Drupal::state()->get('install_profile_modules') ?: [];
    -  $files = system_rebuild_module_data();
    +  $files = \Drupal::service('extension.list.module')->listExtensions();
       \Drupal::state()->delete('install_profile_modules');
    
       // Always install required modules first. Respect the dependencies between
    

    Makes no difference :( I'm guessing because installing a module forces a container rebuild. I think this all comes back to the idea that we should have an internal container that is never rebuilt and only contains a handful of services necessary for a running Drupal engine.

    Yep that's it. I think we should make ExtensionList::$pathName static and not rebuild it on container rebuild. A module can not change location during a request. And if it does so be it.
    Comparison to #241... https://blackfire.io/profiles/compare/17977d6e-abb9-4b91-bf2e-11844734cf... - 11,000+ less calls to Drupal\Core\Extension\ModuleHandler::parseDependency

    Here's a comparison to HEAD: https://blackfire.io/profiles/compare/df6d8410-6fc4-4b97-9f32-95434e97b9... - so here we can see the cost of the mostly the dependency container and calls to this thing that needs to be redone throughout the install because the container is being rebuilt. If we had an inner container of services that can't change on module install this could be avoided.

    alexpott’s picture

    Status: Needs work » Needs review
    FileSize
    899 bytes
    63.99 KB

    Whoops - also a proper static reset in the container - removes the performance improvement of using the static.

    andypost’s picture

    alexpott’s picture

    We reset the list twice during a module install... once via a direct call to \Drupal::service('extension.list.module')->reset(); and once via \Drupal::service('theme_handler')->refreshInfo();

    alexpott’s picture

    dawehner’s picture

    Thank you @alexpott for the amazing work. I feel bad that I haven't profiled it for a while.
    I think for now its fine to add this static variable, but you know, we can always see whether we can actually improve it.

    Do you think we need further work here?

    +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
    @@ -76,6 +76,10 @@
    +   * Extension folders cannot change during a request therefore this is a static
    +   * property to prevent rescanning all the directories and parsing all the
    +   * info.yml files.
    +   *
        * @var string[]|null
        */
    

    That is helpful, thank you!

    alexpott’s picture

    @dawehner there are the follow-ups we need to open for deprecation mentioned in #240 - I didn't address everything there. I noted what I did in #241.

    larowlan’s picture

    +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
    @@ -0,0 +1,558 @@
    +  protected $extensionInfo;
    ...
    +  protected static $pathNames;
    

    Question: Should this be initialized to [] so we don't juggle types (at moment there's array or null)?. Would we ever need to distinguish between [] and null? I see strict checks for === NULL but would these ever be empty arrays?

    dawehner’s picture

    Question: Should this be initialized to [] so we don't juggle types (at moment there's array or null)?. Would we ever need to distinguish between [] and null? I see strict checks for === NULL but would these ever be empty arrays?

    Yes at the moment this is used to lazy fill them:

      public function getPathnames() {
        if ($this->pathNames === NULL) {

    and

      public function getAllAvailableInfo() {
        if ($this->extensionInfo === NULL) {
    

    #240.1

    I wish there were less dependencies.

    Well, at least the users have less now ...

    #240.4

    If we want a todo here we should file the follow up and link it here. I think just explaining why we don't reset it is okay. Or we could use a different extension list in the early installer... if we think that that is worth exploring we could file the followup.

    I referencing to the system module installation issue.

    #240.9

    We need to do this in a follow-up. There are way too many calls to system_rebuild_module_data() to handle in this issue.
    Also, it's weird that system_rebuild_module_data() and _system_rebuild_module_data() are the same. Are we sure there is no performance impact to removing the static caching from system_rebuild_module_data()?

    Sure, let's open it up: #2873051: Replace usages of system_rebuild_module_data with the extension.list.module service

    #240.10

    This has me thinking about whether we need to implement something in drupal_static_reset() to support calling the service instead. This would preserve a sort of BC. Not sure.

    I remember we had an issue exactly about that I believe but I think we postponed it for concrete usecases.

    #240.11

    This is missing coverage of the reset method.

    There is some test coverage for it now. I think testing reset() is a little bit tricky to be honest.

    jibran’s picture

    Status: Needs review » Reviewed & tested by the community

    All the feedback is addressed let's see what maintainers think about this.

    alexpott’s picture

    I think given the significance of the architecture change it needs at least one more framework manager to review this. It is big and complex. I'll ping effulgentsia and catch.

    alexpott’s picture

    1. Sorry but what follows is mostly a naming things review. Much of it I'm not sure about but because this is a new API it is well worth asking these questions before we commit to the names
    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,556 @@
      +  public function extensionExists($extension_name) {
      

      Is extension necessary? Tricky. Maybe this could be just has()?

    3. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,556 @@
      +  public function getExtension($extension_name) {
      

      Given the above discussion - get()?

    4. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,556 @@
      +  public function listExtensions() {
      

      list()? Also this vs getAllAvailableInfo() seems interesting - why do we need both?

    5. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,556 @@
      +      $extension->info = $this->infoParser->parse($extension->getPathname());
      ...
      +  public function getAllAvailableInfo() {
      +    if ($this->extensionInfo === NULL) {
      +      $cache_id = $this->getInfoCacheId();
      +      if ($cache = $this->cache->get($cache_id)) {
      +        $info = $cache->data;
      +      }
      +      else {
      +        $info = $this->recalculateInfo();
      +        $this->cache->set($cache_id, $info);
      +      }
      +      $this->extensionInfo = $info;
      +    }
      +
      +    return $this->extensionInfo;
      +  }
      

      We have a cache of all the available info - how come we're not using it - or is it important that this is fresh?

    6. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,556 @@
      +  public function getExtensionInfo($extension_name) {
      

      Does extension in this method name add anything?

    7. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,556 @@
      +  abstract protected function getInstalledExtensionNames();
      

      This name is odd given the getName() method. But maybe it is okay - it seems we've settled on a convention that the human readable label is called 'name' and the machine name is 'extension name'. Not sure we can do better.

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

      This is a funny distinction. Maybe getPath() could be getDirectory()?

    Berdir’s picture

    8. getPath() and getPathname() are consistent with SplFileInfo as well as our existing Extension class.

    dawehner’s picture

    Status: Reviewed & tested by the community » Needs review

    I totally believe that looking at the method names again could be valuable. Especially the first point though could cause an infinite discussion loop so we should be more thoughtful about that .

    Is extension necessary? Tricky. Maybe this could be just has()?
    Given the above discussion - get()?
    Does extension in this method name add anything?

    We had a discussion earlier in the issue, like on https://www.drupal.org/node/2208429#comment-11161611 ... It was argued that get/has doesn't give you enough context to know what it might be. I think get/has is clear, but getInfo maybe a bit less?

    list()? Also this vs getAllAvailableInfo() seems interesting - why do we need both?
    We have a cache of all the available info - how come we're not using it - or is it important that this is fresh?

    We indeed don't actually need both, but I think we added getAllInstalledInfo to have straightforward function to be called inside system_get_info.
    We could switch over to extension objects, when we don't use system_get_info() in the critical path, in which just getting the info, without the object, would be more efficient. I cannot find any instance in which we do that, so we could try to get rid of all that,
    and maybe keep a cache inside system_get_info()?

    This name is odd given the getName() method. But maybe it is okay - it seems we've settled on a convention that the human readable label is called 'name' and the machine name is 'extension name'. Not sure we can do better.

    Yeah this is kinda what we settled on. Its tricky, but if you have better ideas ... maybe we could have the concept of module IDs ;) Let's be honest, I think we should try to not move away too much from \Drupal\Core\Extension\Extension

    This is a funny distinction. Maybe getPath() could be getDirectory()?

    Note: This is coming from the same method name on Extension, see \Drupal\Core\Extension\Extension::getPath, which is inherited from https://secure.php.net/manual/en/splfileinfo.getpath.php

    Berdir’s picture

    Blackfire still reports a 8% slower (55 instead of 59s) install time. A considerable amount of that is overhead due to more function calls, tracking with time is 43s instead of 41.6.

    What I'm seeing is 24k more calls to Extension::getMTime(), which actually goes through __call() and creates an SplFileInfo object, as far as I see just for that piece of information. We could possibly micro-optimize that and add an actual implementation of that method. that directly uses filemtime()? And going even further, wondering if we should just deprecate "$extension->info['mtime']"? Outside of tests, I'm not seeing anything in core using that information and I'm also not exactly sure why someone would need this? But we're talking just about 100ms or so.

    But anyway, the real problem is that we now store this information in a service that we rebuild after every module install as alexpott pointed out. Before it was drupal_static() which was not reset across module installs.

    I'm worried this is too much of a difference to get in as-is. The problem is that the the additional calls/time grows exponentionally with larger install profiles because we both have more re-parse cycles as well as more data to parse. I think I agree with @alexpott that we should for now store this in a static property. Once it is internal and reset through the API, we can change this in the future if we e.g. have that low-level container that we do not rebuild.

    dawehner’s picture

    But anyway, the real problem is that we now store this information in a service that we rebuild after every module install as alexpott pointed out. Before it was drupal_static() which was not reset across module installs.

    I'm worried this is too much of a difference to get in as-is. The problem is that the the additional calls/time grows exponentionally with larger install profiles because we both have more re-parse cycles as well as more data to parse. I think I agree with @alexpott that we should for now store this in a static property. Once it is internal and reset through the API, we can change this in the future if we e.g. have that low-level container that we do not rebuild.

    An alternative for now is to provide installer only services which have all the statics, does that sound reasonable?

    Berdir’s picture

    I guess that would work. Always using a static could cause troubles in tests I suppose. Likely requires quite a bit of code duplication though :(

    One argument could be that the static would also help if you enable a lot of modules at once (a drush en tmgmt_demo enables 10 modules) but I think we can live with that being a bit slower.

    If you agree on the mtime thing (deprecate the key in info (or possibly everything in info is deprecated already?) and make getMtime() an actual method on that oject) then I can open a follow-up for that.

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

    Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.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

    For now this is just a reroll.

    dawehner’s picture

    This change introduces a static cache during the installer, not just the early installer but throughout the entire installer process ...

    I tried to profile the patch now with this better caching ... and I no longer see any problem.

    Before

    https://blackfire.io/profiles/0f632792-16ae-4e98-84ae-1049bf13dde0/graph

    After

    https://blackfire.io/profiles/8c206b1b-0fb7-4305-98a4-f883c6d1817c/graph

    Compare

    https://blackfire.io/profiles/compare/341d1b5c-cacb-4aaa-be0d-431ac4d9ee...

    Screenshot of the comparison

    https://www.drupal.org/files/issues/Screen%20Shot%202017-09-22%20at%2009...
    https://www.drupal.org/files/issues/Screen%20Shot%202017-09-22%20at%2009...

    @berdir It would be great if you could retest your installation profile as well

    dawehner’s picture

    #258

    1. No worries
    2. I went with exists given that we use that in other places as well
    3. Done
    4. Well, one is optimized for getting the parsed info, one for the extension objects.
    5. Well, getAllAvailableInfo depends on recalculateInfo , depends on list, depends on doList
    6. At least for now we use extension_name to make it clear its the machine name. ¯\_(ツ)_/¯
    7. Yeah see 6.
    8. This is a funny distinction. Maybe getPath() could be getDirectory()? I put the answer of berdir into a comment ...

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    68.98 KB
    1.12 KB

    I forgot to adapt the unit test after the method renames ...

    phenaproxima’s picture

    This patch is a serious happy-maker. It is so very much clearer than the current system. Really, I didn't find anything grievously wrong with it -- although I'm not versed in the dark corners of the extension system anyway, so I'm not sure I'd see dragons even if one was staring me in the face. But I would still love to see this land, because what is clear is that this is a significantly improved API.

    1. +++ b/core/core.services.yml
      @@ -504,6 +504,12 @@ services:
      +  extension.list.module:
      

      I like this naming pattern.

    2. +++ b/core/includes/bootstrap.inc
      @@ -215,11 +216,26 @@ function drupal_get_filename($type, $name, $filename = NULL) {
      +    if (isset($filename)) {
      +      // Manually add the info file path of an extension.
      +      $extension_list->setPathname($name, $filename);
      +      return;
      

      Is it a BC break to return void here?

    3. +++ b/core/includes/install.core.inc
      @@ -435,6 +430,9 @@ function install_begin_request($class_loader, &$install_state) {
      +    // drupal_get_filename is called both with 'module' and 'profile', see ¶
      

      Nit: There's an extra character of white space at the end of this line.

    4. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,539 @@
      +abstract class ExtensionList {
      

      Maybe this should import UseCacheBackendTrait, so that caching can be enabled or disabled by external code (useful for testing, perhaps)?

    5. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,539 @@
      +   * @internal
      +   */
      +  protected $addedPathNames = [];
      

      Is @internal needed? It's not like outside code can get to it, so it's not part of the API...

    6. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,539 @@
      +  public function reset() {
      

      Should this be called resetCache() instead, for clarity?

    7. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,539 @@
      +    catch (\Exception $e) {
      +      // Ignore exceptions caused by a non existing {key_value} table in the
      +      // early installer.
      +    }
      

      If possible, we should catch a more specific exception.

    8. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,539 @@
      +    // We explicitly don't reset the addedFileNames as it is sort of a static
      +    // cache.
      

      This is an important detail and should therefore be in the method's doc comment.

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

      Super useful. I love it.

    10. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,539 @@
      +   * @throws \InvalidArgumentException
      +   *   If there is no extension with the supplied extension human name.
      +   */
      +  public function getName($extension_name) {
      

      Do we still need the @throws if this method does not directly throw an exception?

    11. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,230 @@
      +    if ($active_profile = $this->getActiveProfile()) {
      

      I believe the coding standards now dictate that we no longer put assignments inside if statements.

    12. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,230 @@
      +    $settings = $this->configFactory->get('simpletest.settings')->get();
      

      Nit: We could just do $parent_profile = $this->configFactory->get('simpletest.settings')->get('parent_profile').

    13. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,230 @@
      +    $profile_extension_discovery = new ExtensionDiscovery($this->root);
      +    $profiles = $profile_extension_discovery->scan('profile');
      

      This seems a bit icky. Can't we inject the extension.list.profile service and get the profile's info from there, to keep the abstraction sealed?

    14. +++ b/core/lib/Drupal/Core/Installer/InstallerModuleExtensionList.php
      @@ -0,0 +1,54 @@
      +class InstallerModuleExtensionList extends ModuleExtensionList {
      

      Needs a doc comment. Also, why is the class name prefixed with Installer? This is already in the Installer namespace.

    15. +++ b/core/lib/Drupal/Core/Session/AccountProxyInterface.php
      @@ -6,6 +6,7 @@
      + * @internal
        */
       interface AccountProxyInterface extends AccountInterface {
      

      Does marking this @internal constitute a BC break? I can't imagine it does, really, but it's worth asking.

    16. +++ b/core/lib/Drupal/Core/Updater/Module.php
      @@ -49,8 +49,9 @@ public static function getRootDirectoryRelativePath() {
      +    $module_extension_list = \Drupal::service('extension.list.module');
      

      Can this be injected?

    dawehner’s picture

    1.

    I like this naming pattern.

    Yeah its all about namespacing.

    2.

    Is it a BC break to return void here?

    Site small catch!

    3.

    Nit: There's an extra character of white space at the end of this line.

    Gone

    4. What about filing a follow up for that? Its absolutely an interesting idea.

    5. Is @internal needed? It's not like outside code can get to it, so it's not part of the API...
    Does it hurt? Its an indication that this property will not stay around ideally.

    6. There are actually quite a lot of reset() functions out there, the language manager, config factory etc. IF you look at the actual code it also resets some state, so its not just about cache really at that point.

    7.Good point. I used the DB exception wrapper for that

    8. Good point!

    9. :+1:

    10. I would say yes, how else can people know about it?

    11. I believe the coding standards now dictate that we no longer put assignments inside if statements.
    I couldn't find any hint for that, nor I see phpcs failing, so ... let's not bikeshed, which is the entire point of phpcs

    12. Nice catch!

    13.

    This seems a bit icky. Can't we inject the extension.list.profile service and get the profile's info from there, to keep the abstraction sealed?

    I'm happy given it a try, but I could imagine that there might be big issues we run into

    14. Maybe the right answer is to move it into the extension folder?

    15. Mhhh

    16. Maybe, I have no idea where its actually used

    dawehner’s picture

    Status: Needs work » Needs review
    FileSize
    69.03 KB
    591 bytes

    Sorry, that's the wrong patch with the right interdiff. I didn't had the right local branch for some reason.

    phenaproxima’s picture

    Discussed with @dawhener on Slack. I think this looks good, and I'd RTBC it if I was more familiar with the extension system. But I'm not, so I'm tagging this for framework manager review in order to catch any bugs that might be lying in wait.

    I don't mind RTBCing this if it passes muster with the framework managers.

    alexpott’s picture

    Re-rolled since there was a conflict in system.module due to #2901562: Fix 'Squiz.WhiteSpace.SuperfluousWhitespace' coding standard. Also fixed a couple of coding standards things.

    alexpott’s picture

    1. +++ b/core/includes/bootstrap.inc
      @@ -215,11 +216,23 @@ function drupal_get_filename($type, $name, $filename = NULL) {
      +      // Catch the exception and trigger error to maintain existing behavior.
      +      trigger_error(new FormattableMarkup('The following @type is missing from the file system: @name', ['@type' => $type, '@name' => $name]), E_USER_WARNING);
      

      I'm not sure that this is not tested. Also I think potentially there is a way to not duplicate this code. Since this exact same message is emitted later in the method.

    2. +++ b/core/includes/install.core.inc
      @@ -1067,7 +1071,7 @@ function install_base_system(&$install_state) {
         // Prime the drupal_get_filename() static cache with the user module's
      -  // exact location.
      +  // location.
      

      Out of scope change - and you use the exact same phrase later on in install.inc for a non-exact later. Lol I made the same point in #240.

    3. +++ b/core/includes/install.inc
      @@ -622,6 +624,13 @@ function drupal_install_system($install_state) {
      +  // rebuild, prime the ModuleExtensionList static cache with the module's
      +  // exact location.
      +  // @todo Try to install system as any other module, see
      +  //   https://www.drupal.org/node/2719315.
      +  \Drupal::service('extension.list.module')->setPathname('system', 'core/modules/system/system.info.yml');
      

      The word exact is unnecessary and not correct as per #218

    4. +++ b/core/lib/Drupal/Core/DrupalKernel.php
      @@ -841,7 +841,10 @@ protected function initializeContainer() {
      -        $current_user_id = $this->container->get('current_user')->id();
      +        /** @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;
      
      +++ b/core/lib/Drupal/Core/Session/AccountProxy.php
      @@ -52,6 +52,13 @@ public function setAccount(AccountInterface $account) {
      +  /**
      +   * {@inheritdoc}
      +   */
      +  public function hasAccount() {
      +    return isset($this->account);
      +  }
      +
      
      +++ b/core/lib/Drupal/Core/Session/AccountProxyInterface.php
      @@ -6,6 +6,7 @@
        * Defines an interface for a service which has the current account stored.
        *
        * @ingroup user_api
      + * @internal
        */
       interface AccountProxyInterface extends AccountInterface {
       
      @@ -43,4 +44,12 @@ public function getAccount();
      
      @@ -43,4 +44,12 @@ public function getAccount();
          */
         public function setInitialAccountId($account_id);
       
      +  /**
      +   * Checks whether an account is currently wrapped.
      +   *
      +   * @return bool
      +   *   TRUE, if an account is currently wrapped. FALSE otherwise.
      +   */
      +  public function hasAccount();
      +
      

      I think we fixed this in #2753733: AccountProxy can do unnecessary user loads to get an ID therefore all these changes are unnecessary and out-of-scope.

    5. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
      @@ -717,8 +717,7 @@ public function getModuleDirectories() {
         public function getName($module) {
      -    $info = system_get_info('module', $module);
      -    return isset($info['name']) ? $info['name'] : $module;
      +    return \Drupal::service('extension.list.module')->getName($module);
         }
      
      +++ b/core/lib/Drupal/Core/Extension/ModuleHandlerInterface.php
      @@ -306,6 +306,9 @@ public function getModuleDirectories();
      +   *
      +   * @deprecated in Drupal 8.4.0, will be removed before Drupal 9.0.0.
      +   *   Use \Drupal::service('extension.list.module')->getName() instead.
          */
         public function getName($module);
      

      I think we should file a followup to deprecate this and remove the usages rather than deprecating here.

    6. +++ b/core/modules/system/system.module
      @@ -961,27 +960,17 @@ function system_check_directory($form_element, FormStateInterface $form_state) {
       function system_get_info($type, $name = NULL) {
      

      Need a followup to deprecate and remove usages.

    7. +++ b/core/modules/system/system.module
      @@ -1088,6 +992,9 @@ function _system_rebuild_module_data() {
      + *
      + * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0. This
      + *   function is no longer used in Drupal core.
        */
       function _system_rebuild_module_data_ensure_required($module, &$modules) {
      

      Need to actually trigger the deprecation.

    alexpott’s picture

    +++ b/core/modules/system/system.module
    @@ -1103,40 +1010,32 @@ function _system_rebuild_module_data_ensure_required($module, &$modules) {
    + *
    + * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0.
      */
     function system_rebuild_module_data() {
    

    I don't think we should deprecate this function in this issue. I think we should do this in a followup.There's lots of usages. We should deprecate and swap out for the new service in a separate issue. This deprecation is causing hundreds of testing fails now we test for deprecations.

    alexpott’s picture

    Issue summary: View changes
    Status: Needs review » Reviewed & tested by the community
    Issue tags: -Needs framework manager review

    I think this patch is rtbc - all my contributions have in reviewing the issue. We've overcome the performance issues and we've created followups for the additional deprecations that are possible once this lands. The patch is a huge step towards refactoring extension listing into a maintainable service and encapsulating all the logic and statics. Great work @dawehner and all the others that have contributed to this issue.

    almaudoh’s picture

    Great to see this issue RTBC finally. Awesome work by @dawehner, @alexpott and all the other folks. All I have are docs nits.

    1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,542 @@
      +   *   The extension machine name of the extension.
      ...
      +   *   The extension machine name.
      ...
      +   *   The extension machine name.
      

      Some consistency needed here - maybe "The machine name of the extension"

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,542 @@
      +   *   If there is no extension with the supplied extension human name.
      

      s/extension human name/machine name/

    3. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,542 @@
      +   *   Processed extension objects, keyed by extension machine name.
      ...
      +   *   Unprocessed extension objects, keyed by extension machine name.
      ...
      +   *   Processed extension objects, keyed by extension machine name.
      

      "keyed by machine name" sounds better here.

    4. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,542 @@
      +   *   The name of the extension for which the filename is requested.
      

      The machine name of the extension.

    5. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,542 @@
      +   *   The name of the extension for which the pathname is requested.
      

      The machine name of the extension....

    6. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,542 @@
      +   *   If there is no extension with the supplied name.
      ...
      +   *   The name of the extension for which the path is requested.
      

      s/name/machine name/

    7. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,542 @@
      + * The extension list is per extension type, like module theme and profile.
      

      Nit: "like module, theme and profile"

    8. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,542 @@
      +   * - /profiles/baz/baz.info.yml
      

      Should be - profiles/baz/baz.info.yml for consistency.

    9. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,542 @@
      +   * Calling ExtensionList::getFilename('foo') will give you one of the above,
      

      s/getFilename/getPathname/

    10. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,542 @@
      +   *   The filename of the requested extension's .info.yml file.
      

      The drupal-root relative filename and path...

    11. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
      @@ -0,0 +1,228 @@
      +
      

      Nit: extra space.

    12. +++ b/core/modules/system/system.module
      @@ -1088,8 +992,14 @@ function _system_rebuild_module_data() {
      + * @deprecated in Drupal 8.4.x and will be removed before Drupal 9.0.0. This
      

      @deprecated in Drupal 8.5.0 ....

    almaudoh’s picture

    I added additional tests on PHP5.5+ because I saw a warning in my IDE. The ExtensionList::list() method is not allowed in PHP < 7 because it is a reserved keyword.

    If we want PHP5 compatibility, we have to change that method name.

    alexpott’s picture

    Status: Reviewed & tested by the community » Needs work

    @almaudoh great catch re #281! Definitely needs work for that and the other nits in #280.

    alexpott’s picture

    I think list() can just be getList() - I'll roll a new patch over the weekend unless someone beats me to it.

    jibran’s picture

    Status: Needs work » Needs review
    FileSize
    9.64 KB
    69.68 KB

    Night fairy stikes again.

    Status: Needs review » Needs work

    The last submitted patch, 284: 2208429-284.patch, failed testing. View results

    jibran’s picture

    PHPStorm missed some renamings.

    Status: Needs review » Needs work

    The last submitted patch, 286: 2208429-286.patch, failed testing. View results

    jibran’s picture

    jibran’s picture

    Status: Needs work » Needs review
    almaudoh’s picture

    A few more docs nits :)

    1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,543 @@
      +   *   Processed extension object for the given extension machine name.
      

      A processed extension object for the extension with the specified machine name.

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,543 @@
      +   * Returns installed extension machine names.
      

      Returns a list of machine names of installed extensions.

    3. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,543 @@
      +   *   The extension machine names of all installed extensions of this type.
      

      The machine names...

    I have also updated the change record to reflect the new API.

    almaudoh’s picture

    I've fixed the small nits in #290

    Change record has been updated. I have gone through the latest patch a second time and it looks good to go. Since all I've added to this version of the patch is three small docs nitpicks, I guess I'm allowed to re-RTBC this?

    almaudoh’s picture

    Issue summary: View changes

    The two fails in PHP7.0-dev and PHP7.1-dev are fails in HEAD (https://www.drupal.org/pift-ci-job/819421 and https://www.drupal.org/pift-ci-job/819423)

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 291: 2208429-291.patch, failed testing. View results

    oriol_e9g’s picture

    Status: Needs work » Reviewed & tested by the community

    Per #292

    alexpott’s picture

    FileSize
    69.78 KB

    Needed a re-roll... git took care of it...

    First, rewinding head to replay your work on top of it...
    Applying: Init commit
    Using index info to reconstruct a base tree...
    M	core/core.services.yml
    M	core/includes/install.core.inc
    Falling back to patching base and 3-way merge...
    Auto-merging core/includes/install.core.inc
    Auto-merging core/core.services.yml
    
    dawehner’s picture

    git++

    The last submitted patch, 2208429-3-295.patch, failed testing. View results

    The last submitted patch, 291: 2208429-291.patch, failed testing. View results

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 295: 2208429-3-295.patch, failed testing. View results

    Anonymous’s picture

    Status: Needs work » Reviewed & tested by the community

    Amazing work! Back to RTBC after random failure. #2926309: Random fail due to APCu not being able to allocate memory

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 295: 2208429-3-295.patch, failed testing. View results

    Anonymous’s picture

    Status: Needs work » Reviewed & tested by the community

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 295: 2208429-3-295.patch, failed testing. View results

    alexpott’s picture

    Status: Needs work » Reviewed & tested by the community

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 295: 2208429-3-295.patch, failed testing. View results

    alexpott’s picture

    Hmmm maybe this patch is causing the memory problems :(

    Anonymous’s picture

    Status: Needs work » Reviewed & tested by the community

    At the moment, many issues have this memory problems. #2926309: Random fail due to APCu not being able to allocate memory

    larowlan’s picture

    This is looking good, couple of questions, leaving at rtbc

    1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,544 @@
      +
      

      nit whitespace error here

    2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
      @@ -717,8 +717,7 @@ public function getModuleDirectories() {
         public function getName($module) {
      

      should we deprecate this method in favour of using the new service directly? Follow up is ok

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

      I assume this can't be injected because of a circular dependency?

      Do we have a plan to resolve that?

      A setter on the module handler that we call in the constructor of extension.list.module?

      Doesn't have to be resolved here, just keen to know the plan

    4. +++ b/core/lib/Drupal/Core/Installer/InstallerModuleExtensionList.php
      @@ -0,0 +1,57 @@
      +    // In the early installer the container is rebuilt multiple times. Therefore
      +    // we have to keep the added filenames across those rebuilds. This is not a
      +    // final design, but rather just a workaround resolved at some point,
      +    // hopefully.
      

      Can we create a follow up and add a @todo here, rather than just 'hoping' :)

    alexpott’s picture

    @vaplas yes but it seems to happen more on this issue.

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 295: 2208429-3-295.patch, failed testing. View results

    bircher’s picture

    Status: Needs work » Needs review
    FileSize
    70.83 KB
    5.05 KB

    actually it can be injected..
    I guess it was not done because that method is littered with other things that are not injected..
    That said, I have not opened up the follow-up issue and the place where the link goes is kind of empty.

    Apart from that I looked over the patch but couldn't spot where the test failures would come from. then again the time right now is limited.

    dawehner’s picture

    Issue summary: View changes

    I guess it was not done because that method is littered with other things that are not injected..

    Well, its not that easy.

    First we have an issue which would be the right solution for this general injection problem: #2350111: Introduce an event to decouple the module installer from the entity manager. All this code should not be coupled that horrible to each other.

    The actual reason though why this is not was commented in #187. When you install a module, you rebuild the container and after that you should really not reuse anything from the previous service container. Even this patch might be green, there could be some really hard problems hidden. I would recommend to not try this as part of this issue.

    vijaycs85’s picture

    Just did a profiling with backfire locally. Between current 8.5.x HEAD vs 8.5.x HEAD + patch from 311. Comparision is available at https://blackfire.io/profiles/compare/257f3a85-6fec-47cf-9d24-356161ce26.... Seems we have bit of improvement on time(Time-14 ms (-1.8%) 757 ms → 744 ms) and memory(Memory-5.77 kB (-0.022%) 26.1 MB → 26.1 MB).

    dawehner’s picture

    @vijaycs85
    Thank you for your profile. This looks like a nice improvement!
    Which bit did you actually profiled?

    In #166I focused on the installer.

    vijaycs85’s picture

    @dawehner it was just home page (on clean install) after a `drush cr`.

    Berdir’s picture

    I don't think a cached frontpage call even calls the code being changed here. You can see that there are no call differences, so that's probably just random differences.

    For a meaningful comparison, you would need to compare the first request after manually clearing the cache (truncate all cache tables, a drush cr or so would already populate them again). And you need to disable aggregation.

    dawehner’s picture

    Looking at the comparison: https://blackfire.io/profiles/compare/257f3a85-6fec-47cf-9d24-356161ce26...() is the main difference, using the same amount of calls. Given that this is basically a measurement of your file system / APCU cache :)

    vijaycs85’s picture

    Thanks @Berdir and @dawehner for the input.

    For a meaningful comparison, you would need to compare the first request after manually clearing the cache (truncate all cache tables, a drush cr or so would already populate them again). And you need to disable aggregation.

    I did this (hopefully you meant blackfire aggregation) and the result seems much better now(at least from execution time POV): https://blackfire.io/profiles/compare/5b932935-ed5f-4170-9688-969b72de08...

    dawehner’s picture

    I started with #295 again and addressed the feedback in #308

    nit whitespace error here

    ✔️

    should we deprecate this method in favour of using the new service directly? Follow up is ok

    Yeah, we have a follow up already.

    I assume this can't be injected because of a circular dependency?

    Do we have a plan to resolve that?

    A setter on the module handler that we call in the constructor of extension.list.module?

    Doesn't have to be resolved here, just keen to know the plan

    See below.

    Can we create a follow up and add a @todo here, rather than just 'hoping' :)

    I added the issue: #2934063: Remove the workaround in \Drupal\Core\Installer\InstallerModuleExtensionList::setPathname

    jibran’s picture

    1. +++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
      @@ -14,6 +14,11 @@
      + * each installation uninstallation of a module.
      

      s/installation uninstallation/installation or uninstallation

    2. +++ b/core/lib/Drupal/Core/Installer/InstallerModuleExtensionList.php
      @@ -28,6 +28,7 @@ public function setPathname($extension_name, $pathname) {
      +    // @todo Remove as part of https://www.drupal.org/project/drupal/issues/2934063
      

      > 80

    dawehner’s picture

    dawehner’s picture

    @jibran
    Nope :) There are issues adressing this problem already.

    bircher’s picture

    Status: Needs review » Reviewed & tested by the community

    Yes actually the service should not be injected, the comment on the class makes that clear and also explains why all the other ones are also not injected.

    dawehner’s picture

    Thank you @bircher!

    Version: 8.5.x-dev » 8.6.x-dev

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

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, 321: 2208429-321.patch, failed testing. View results

    jibran’s picture

    Status: Needs work » Reviewed & tested by the community
    catch’s picture

    Status: Reviewed & tested by the community » Needs review

    Only a partial review, but wanted to try to get a start on reviewing this again now it's RTBC. Overall I didn't notice anything major yet but I did spot these relatively minor points. Most of the things I'd complain about have follow-up issues, which is fine.

    1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,543 @@
      +   * The type of the extension, such as "module" or "theme".
      

      Since we only have 'profile' otherwise should we just add it here without the 'such as'?

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,543 @@
      +
      +  /**
      +   * The statically cached extensions.
      +   *
      +   * @var \Drupal\Core\Extension\Extension[]|null
      +   */
      +  protected $extensions;
      

      Since these aren't static properties can we call this something else?

    3. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,543 @@
      +   * The state.
      

      state service?

    1. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,543 @@
      +      elseif (!$path_names = $this->state->get($cache_id)) {
      +        $path_names = $this->recalculatePathnames();
      +        // Store filenames to allow static::getFilename() to retrieve them
      +        // without having to rebuild or scan the filesystem.
      

      I feel like I'm missing something, but there's no ::getFileName() method on this class, is it getPathName()?

      Also what is the situation where the cache will be cleared but the state entry won't be (except for say a memcache eviction)?

    2. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,543 @@
      +   *
      +   * @see ::getFilename
      +   */
      

      getPathName()?

    dawehner’s picture

    Thank you for your review @catch!

    1. Since we only have 'profile' otherwise should we just add it here without the 'such as'?

      Sure

    2. Since these aren't static properties can we call this something else?

      Good point, let's remove "statically" and be done.

    3. state service?

      The state store seems to be popular in other areas in Drupal core.

    4. I feel like I'm missing something, but there's no ::getFileName() method on this class, is it getPathName()?

      Also what is the situation where the cache will be cleared but the state entry won't be (except for say a memcache eviction)?

      Yes it is, see #227

    5. Also what is the situation where the cache will be cleared but the state entry won't be (except for say a memcache eviction)?

      Well yes this was the idea. Avoid disc IO as much as possible.

    6. getPathName()?

      Good catch

    jibran’s picture

    Status: Needs review » Reviewed & tested by the community

    Back to RTBC as last round of feedback has been addressed.

    catch’s picture

    Status: Reviewed & tested by the community » Needs work
    1. +++ b/core/includes/bootstrap.inc
      @@ -226,43 +226,45 @@ function drupal_get_filename($type, $name, $filename = NULL) {
      +    elseif (!isset($files[$type][$name])) {
      +      // If still unknown, retrieve the file list prepared in state by
      +      // system_rebuild_module_data() and
      +      // \Drupal\Core\Extension\ThemeHandlerInterface::rebuildThemeData().
      

      The comment has only been moved, but it should be updated to reference wherever this actually happens now.

    2. +++ b/core/includes/install.core.inc
      @@ -444,6 +439,9 @@ function install_begin_request($class_loader, &$install_state) {
      +    // drupal_get_filename is called both with 'module' and 'profile', see
      

      Missing ().

    3. +++ b/core/includes/install.inc
      @@ -622,6 +624,13 @@ function drupal_install_system($install_state) {
      +  // rebuild, prime the ModuleExtensionList static cache with the module's
      

      Should this reference the fully qualified name? And remove 'static' here too?

    4. +++ b/core/lib/Drupal/Core/Extension/ExtensionList.php
      @@ -0,0 +1,543 @@
      +        // Store filenames to allow static::getFilename() to retrieve them
      

      This still says getFileName() but it should be getPathName().

    5.Also what is the situation where the cache will be cleared but the state entry won't be (except for say a memcache eviction)?

    Well yes this was the idea. Avoid disc IO as much as possible.

    I'm not sure about this, it means that it will persist past a bin flush. For example what happens if you move a module from modules/custom to modules/contrib?

    dawehner’s picture

    Assigned: Unassigned » catch
    Status: Needs work » Needs review
    FileSize
    70.34 KB
    2.96 KB

    The comment has only been moved, but it should be updated to reference wherever this actually happens now.

    ✔️

    Missing ().

    ✔️

    Should this reference the fully qualified name? And remove 'static' here too?

    Added the fully qualified name. I kept the static cache, because it is actually a static cache.

    This still says getFileName() but it should be getPathName().

    ✔️

    I'm not sure about this, it means that it will persist past a bin flush. For example what happens if you move a module from modules/custom to modules/contrib?

    Well, in this case \Drupal\Core\Extension\ExtensionList::reset is called, which removes the state entry. Given that it is totally able to move modules.
    Also keep in mind, we do have \Drupal::state()->set('system.module.files', $files); already in core as it is. We are "just" moving it around basically. Does that makes sense?

    I'm assigning the issue to catch, given they seem to do a continue review at this moment in time. Everyone else is welcome to give more reviews as well :)

    dawehner’s picture

    Assigned: Unassigned » catch
    Status: Needs work » Needs review
    FileSize
    70.34 KB
    2.96 KB

    The comment has only been moved, but it should be updated to reference wherever this actually happens now.

    ✔️

    Missing ().

    ✔️

    Should this reference the fully qualified name? And remove 'static' here too?

    Added the fully qualified name. I kept the static cache, because it is actually a static cache.

    This still says getFileName() but it should be getPathName().

    ✔️

    I'm not sure about this, it means that it will persist past a bin flush. For example what happens if you move a module from modules/custom to modules/contrib?

    Well, in this case \Drupal\Core\Extension\ExtensionList::reset is called, which removes the state entry. Given that it is totally able to move modules.
    Also keep in mind, we do have \Drupal::state()->set('system.module.files', $files); already in core as it is. We are "just" moving it around basically. Does that makes sense?

    I'm assigning the issue to catch, given they seem to do a continue review at this moment in time. Everyone else is welcome to give more reviews as well :)

    catch’s picture

    Assigned: catch » Unassigned
    Status: Needs review » Reviewed & tested by the community

    Thanks for the changes and the explanations. Moving back to RTBC.

    • catch committed 725641c on 8.6.x
      Issue #2208429 by dawehner, almaudoh, alexpott, jibran, Xano, donquixote...
    catch’s picture

    Status: Reviewed & tested by the community » Fixed

    Committed 725641c and pushed to 8.6.x. Thanks!

    almaudoh’s picture

    Wow! This is great!! \o/

    dawehner’s picture

    @almaudoh Thank your for all your help here!

    Mile23’s picture

    I think this broke my contrib.

    Try installing ajax_example from the examples project.

    Here's the failing test in 8.6.x: https://www.drupal.org/pift-ci-job/870918

    And the passing test in 8.5.x: https://www.drupal.org/pift-ci-job/870920

    almaudoh’s picture

    Try installing ajax_example from the examples project.

    I did this on simplytest.me and it installed just fine. https://d0gs.ply.st/admin/modules

    Edit: I just realized that simplytest.me was installing an older version of Drupal. I'll check this out on my local machine now.

    alexpott’s picture

    Yep this issue introduced a breaking change to system_get_info() - see #2939904: Fix system_get_info() for non-installed modules

    almaudoh’s picture

    Created a meta issue to track all the deprecations from this issue and the ThemeExtensionList one: #2940190: [meta] Deprecations of old functions in Extension system

    almaudoh’s picture

    Status: Fixed » Closed (fixed)

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