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
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.- 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 ofdrupal_get_filename()
, which causes the first of these calls to trigger yet another filesystem scan for installation profiles. - 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. - 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
-
Fix the infinite recursion logic error in
SystemListingInfo
. Doing so makesSystemListingInfo
obsolete.→ Merge into
SystemListing
. - Fix the installer to prime
drupal_get_filename()
after scanning for installation profiles. - Add a static cache to
SystemListing
to prevent multiple filesystem scans that would yield the same results. - Change the cache property of
InfoParser
into a static property to prevent the same info files from being re-parsed again. - Replace all instances of
drupal_system_listing()
withSystemListing
and removedrupal_system_listing()
.
API changes
drupal_system_listing()
has been replaced withDrupal\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');
- (8.x-only)
SystemListingInfo
has been merged intoSystemListing
, just change the class name.
Notes
-
Further work on #2186491: [meta] D8 Extension System: Discovery/Listing/Info will move
Drupal\Core\SystemListing
intoDrupal\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.
Comment | File | Size | Author |
---|---|---|---|
#15 | interdiff.txt | 51.88 KB | sun |
#14 | interdiff.txt | 1.52 KB | sun |
#14 | extension.discovery.14.patch | 49.88 KB | sun |
#12 | interdiff.txt | 8.32 KB | sun |
#12 | extension.discovery.12.patch | 49.42 KB | sun |
Comments
Comment #2
sunFixed 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™.
Comment #5
sunComment #7
sunComment #9
sunComment #10
sunComment #11
sunReplaced issue summary with a proper one.
Comment #12
sunComment #13
dawehnerIt would be cool if you could explain this change.
Will this pattern be hidden inside ExtensionDiscovery?
The comment should be adapted a bit.
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)
Comment #14
sunre: #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.
Comment #15
sunI spent some good time today to proceed and advance on this topic.
→ Replacing
SystemListing
with a highly performance-optimizedExtensionDiscovery
: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:
.info.yml
files for all extension types./sites/all
) for the requested extension type only, scan it once for all extensions (of all types).'tests'
directories at regular runtime (~400% performance increase on its own).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?
Comment #16
BerdirA 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.
Comment #17
twistor CreditAttribution: twistor commentedAny thoughts on using Symfony's Finder component?
http://symfony.com/doc/current/components/finder.html
Comment #18
sun@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 newExtensionDiscovery
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 properRecursiveFilterIterator
), 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 forExtensionDiscovery
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.Comment #19
BerdirSure, 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 ;)
Comment #20
sunAlright, 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.
Comment #21
sun#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.
Comment #22
sunMarking 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 :-)