Discovered in #1351352-33: Distribution installation profiles are no longer able to override the early installer screens

Part I of #2186491: [meta] D8 Extension System: Discovery/Listing/Info

Problem

  1. SystemListingInfo::scan() triggers an infinite recursion to itself when scanning for installation profiles, because it tries to retrieve the installation profile directories to be scanned, which makes no sense.
  2. The installer scans for all available installation profiles and uses drupal_get_path('profile', $name) later on, but does not prime the static filepath lookup cache of drupal_get_filename(), which causes the first of these calls to trigger yet another filesystem scan for installation profiles.
  3. The file scan results of SystemListing are not cached, even though a scan for a certain extension type will yield the exact same results for the same directories.
  4. Especially in the installer and tests, the InfoParser is re-instantiated and invoked very often for the same info files. Because it is re-instantiated, the cached property of the last instance is gone and info files are needlessly parsed again.

Proposed solution

  1. Fix the infinite recursion logic error in SystemListingInfo. Doing so makes SystemListingInfo obsolete.

    → Merge into SystemListing.

  2. Fix the installer to prime drupal_get_filename() after scanning for installation profiles.
  3. Add a static cache to SystemListing to prevent multiple filesystem scans that would yield the same results.
  4. Change the cache property of InfoParser into a static property to prevent the same info files from being re-parsed again.
  5. Replace all instances of drupal_system_listing() with SystemListing and remove drupal_system_listing().

API changes

  1. drupal_system_listing() has been replaced with Drupal\Core\SystemListing.

    Drupal 7

    $available_modules = drupal_system_listing('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.module$/', 'modules');

    Drupal 8

    $listing = new SystemListing();
$available_modules = $listing->scan('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.module$/', 'modules');
  2. (8.x-only) SystemListingInfo has been merged into SystemListing, just change the class name.

Notes

  1. Further work on #2186491: [meta] D8 Extension System: Discovery/Listing/Info will move Drupal\Core\SystemListing into Drupal\Core\Extension\ExtensionDiscovery and change its API and implementation to be fully tailored towards core extension types.

    The ultimate goal for D8 is to arrive at this:

    -$listing = new SystemListing();
-$listing->scan('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.module$/', 'modules');
+$discovery = new ExtensionDiscovery();
+$discovery->findModules();

    However, that is not supposed to be changed here. The plan is to move forward in manageable, reviewable, and well-scoped steps.

CommentFileSizeAuthor
#15 interdiff.txt51.88 KBsun
#14 interdiff.txt1.52 KBsun
#14 extension.discovery.14.patch49.88 KBsun
#12 interdiff.txt8.32 KBsun
#12 extension.discovery.12.patch49.42 KBsun
#11 interdiff.txt31.14 KBsun
#11 extension.discovery.11.patch43.91 KBsun
#7 interdiff.txt17.81 KBsun
#7 extension.discovery.7.patch32.33 KBsun
#5 interdiff.txt6.57 KBsun
#5 extension.discovery.5.patch22.3 KBsun
#2 interdiff.txt6.05 KBsun
#2 extension.discovery.2.patch18.76 KBsun
extension.discovery.0.patch15.81 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, extension.discovery.0.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
extension.discovery.2.patch18.76 KB
interdiff.txt6.05 KB

Fixed various bogus indirect calls to SystemListingInfo.

I already know where this will end — a new ExtensionDiscovery service that replaces SystemListing* + drupal_get_path() + drupal_get_filename() + system_list() + system_rebuild_module_data() + system_rebuild_theme_data(), etc.pp...

But for now, I'm really trying hard to limit the scope of this to ExtensionDiscovery Part I™.

Status: Needs review » Needs work

The last submitted patch, 2: extension.discovery.2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: extension.discovery.2.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
extension.discovery.5.patch22.3 KB
interdiff.txt6.57 KB
  1. Fixed InfoParserUnitTest.
  2. Fixed ThemeHandlerTest.
  3. Reverted SystemListing to SystemListingInfo.
  4. Fixed infinite recursion caused by drupal_maintenance_theme() → Theme\Registry → Config\InstallStorage → SystemListingInfo prior to having profile info.

Status: Needs review » Needs work

The last submitted patch, 5: extension.discovery.5.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
extension.discovery.7.patch32.33 KB
interdiff.txt17.81 KB
  1. Fixed logic error: SystemListingInfo cannot getProfileDirectories() if being asked to search for profile directories.
  2. Fixed bogus filename regex in ThemeHandler.

Status: Needs review » Needs work

The last submitted patch, 7: extension.discovery.7.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: ExtensionDiscovery » Extension System, Part I: SystemListingInfo clean-up + performance
Issue tags: +Extension system
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Parent issue: » #2186491: [meta] D8 Extension System: Discovery/Listing/Info
sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Extension System, Part I: SystemListingInfo clean-up + performance » Extension System, Part I: SystemListing clean-up + performance; remove SystemListingInfo + drupal_system_listing()
Component: base system » extension system
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +API change, +API clean-up
FileSize
extension.discovery.11.patch43.91 KB
interdiff.txt31.14 KB
  1. Minor: Adjusted InfoParser to complain about missing 'type' key first.
  2. Removed SystemListingInfo.

Replaced issue summary with a proper one.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
extension.discovery.12.patch49.42 KB
interdiff.txt8.32 KB
  1. Fixed stale documentation referencing file_scan_directory() instead of SystemList.
  2. Added persistent cache to simpletest_classloader_register(), because simpletest_test_get_all() has one, too.
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/includes/install.core.inc
@@ -494,7 +494,16 @@ function install_begin_request(&$install_state) {
-  $module_handler->load('system');
+  $module_handler->loadAll();

    It would be cool if you could explain this change. 

  2. +++ b/core/includes/install.core.inc
@@ -494,7 +494,16 @@ function install_begin_request(&$install_state) {
+  $listing = new SystemListing();
+  $install_state['profiles'] += $listing->scan('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.profile$/', 'profiles');

    Will this pattern be hidden inside ExtensionDiscovery? 

  3. +++ b/core/includes/module.inc
@@ -285,14 +286,22 @@ function module_uninstall($module_list = array(), $uninstall_dependents = TRUE)
   // An installation profile is required and one must always be loaded.
...
+  // The only exception is the installer.
+  if ($profile = drupal_get_profile()) {
+    $required[] = $profile;
+  }

    The comment should be adapted a bit. 

  4. +++ b/core/lib/Drupal/Core/DrupalKernel.php
--- a/core/lib/Drupal/Core/Extension/InfoParser.php
+++ b/core/lib/Drupal/Core/Extension/InfoParser.php

+++ b/core/lib/Drupal/Core/Extension/InfoParser.php
@@ -22,7 +22,7 @@ class InfoParser implements InfoParserInterface {
-  protected $parsedInfos = array();
+  protected static $parsedInfos = array();

    As far as I understand the reason for this change is that we rebuild the container quite often during reinstall, so we want to keep the static information. What about introducing a new method to be able to reset the static information? (FOLLOW UP)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
extension.discovery.14.patch49.88 KB
interdiff.txt1.52 KB
  1. Added docs for using ModuleHandler::loadAll() in install_begin_request().
  2. Polished comment in drupal_required_modules().

re: #13.2: Yes, that's the ultimate goal beyond this issue; see issue summary.

re: #13.4: I considered that as well while working this patch, but then concluded that this InfoParser class is located in the Extension component, and within this component, the assumption that .info.yml files are not going to change within a request is a fair and safe one to do. Hence, I don't think it makes sense to provide an option to reset and thus override that assumption. But yeah, if deemed to be necessary, can happen in a separate issue.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
interdiff.txt51.88 KB

I spent some good time today to proceed and advance on this topic.

→ Replacing SystemListing with a highly performance-optimized ExtensionDiscovery:

Bench: 10 scans for 'modules'
SystemListing (HEAD): 6.825 seconds
ExtensionDiscovery:   0.391 seconds

The branch/patch is based on this patch here and only 15KB larger: https://drupal.org/node/1766036#comment-8440975

It uses a different approach, which I mentioned in the meta issue already:

  1. Require .info.yml files for all extension types.
  2. Instead of scanning a particular base directory (e.g., /sites/all) for the requested extension type only, scan it once for all extensions (of all types).
  3. Use a highly optimized filter implementation for the actual recursive filesystem scan, which is 100% tailored to the specific task of discovering extensions.
  4. Do not recurse into 'tests' directories at regular runtime (~400% performance increase on its own).
  5. Also, return proper SplFileInfo objects for all discovered extensions.

→ Substantial extension discovery performance increase. Installer responds like a breeze.

However. I'd need core maintainer input now, because I'm not sure what the best way forward is:

The revised approach is certainly not done yet, whereas this patch/issue here is pretty much RTBC from my perspective. Since the revised approach is based on this branch/patch here, I wouldn't mind at all if this would land first, but other core work + contrib possibly do, so it might make sense to skip the intermediate step. OTOH, I want to keep the scope of the required extension system changes manageable and reviewable, because this is bad-ass base system stuff... :-/

To get a better understanding, I'm attaching an interdiff between this branch and the revised; in other words, the patch against 8.x if this patch here was committed already.

Any recommendations?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

A bit worried about #15.4: It's obvious that it does have a huge impact as we have a crazy amount of test modules and themes. There are however also use cases for enabling test modules on e.g. a development site ( I have contrib modules with pluggable backends with a dummy implementation in a test module that I also use for development), this could be somewhat unexpected behavior.

Sure, I just need to move it out of the tests folder, but wondering where to document this, no idea where you would look if Drupal tells you that your test module does not exist.

About your benchmarks, wondering if you have an SSD or not? I expect that the difference will be much smaller on a disk that is optimized for fast lookups/reads. I think that testbots even have the drupal installation in tmpfs, so I wouldn't expect large differences there. It could make a huge difference for all the people that currently claim that the installation takes forever for them, though.

twistor’s picture

twistor CreditAttribution: twistor commented

Any thoughts on using Symfony's Finder component?

http://symfony.com/doc/current/components/finder.html

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

@Berdir: @xjm raised a similar concern about test modules for development/manual-testing purposes in IRC. Given the very substantial difference in discovery performance, I think we should think about a flag in settings.php for that use-case, if necessary.

But yeah, at regular runtime, there is zero point in discovering and processing test extensions. We definitely want to avoid that.

Regarding SSD: Possible. But I'm experiencing the slow discovery on my local system (Windows), too. I don't think we should bake any assumptions about certain disks/filesystems into Drupal core.

@twistor: Symfony Finder is a performance drain at this point, because it does not properly apply its filters to a recursive filesystem scan. See #1451320: Evaluate Symfony2's Finder Component to simplify file handling + https://github.com/symfony/symfony/issues/5951.

Since a proper RecursiveFilterIterator implementation is indeed rocket science right now (the implementation of the new ExtensionDiscovery took me quite some time to get right), PHP 5.4 finally introduced a new RecursiveCallbackFilterIterator. Once Finder has been re-architected and rewritten to leverage that (or implement a proper RecursiveFilterIterator), we may be able to look into it for Drupal core. I'd love to add Finder to Drupal core myself, but I personally would not recommend to do that before it has been fixed for recursive scans.

That said, the RecursiveExtensionFilterIterator I developed for ExtensionDiscovery hard-codes a range of further assumptions to optimize the filesystem scan performance for the specific case of Drupal extensions, so even if/when Finder becomes an option in the future, it might not be a good fit here.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Sure, I can see that ignoring tests directory is a huge performance boost, I've been thinking about that too (I think there have also been discussions to remove test and test modules from packaged core releases for this purpose). Just trying to point out that there *are* use cases where you want to scan it and therefore wondering how/where to document this so that people find it when they need to.

Also agreed about the file system stuff, I was just trying to say that the testbot performance might not increase as much as you'd expect based on your local performance. +100 for improving performance on non-optimized hardware, because it will be people on such hardware/servers that will create the Drupal 8-version of http://drupal.stackexchange.com/questions/724/why-is-drupal-7-so-slow ;)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Alright, in order to keep the discussion here focused on the concrete patch at hand, I've created a separate issue for the work and additional improvements I mentioned in #15:

#2188661: Extension System, Part II: ExtensionDiscovery

Feedback is very welcome over there. Based on that, we will need to decide whether we're going to mark this issue as a duplicate (to skip the intermediate step of this patch here), or whether we're going to proceed with this patch as an intermediate step.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

#2188661: Extension System, Part II: ExtensionDiscovery has a working, tests-passing, and IMHO much cleaner patch now. I'd personally recommend to skip this intermediate step and directly go with that instead.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Closed (duplicate)

Marking this as a duplicate now, since #2188661: Extension System, Part II: ExtensionDiscovery is way superior to this patch, both in architecture, implementation, as well as performance.

Oh, and it's waiting for final reviews :-)