Extension System, Part II: ExtensionDiscovery

Code review, manual testing to come:

1. +++ b/core/includes/common.inc
   @@ -3182,19 +3181,6 @@ function drupal_page_set_cache(Response $response, Request $request) {
   -function drupal_system_listing($mask, $directory, $key = 'name', $min_depth = 1) {
   

   I think policy dictates we will need to leave this around, as a wrapper and mark deprecated. But you'll need to ask a committer.

2. +++ b/core/includes/install.inc
   @@ -122,22 +123,19 @@ function drupal_detect_database_types() {
   + * @todo Convert database drivers into a new + regular extension type.
   

   Out of scope, please open a new issue

3. +++ b/core/includes/install.inc
   @@ -122,22 +123,19 @@ function drupal_detect_database_types() {
   -  // Allow any valid PHP identifier.
   -  // @see http://www.php.net/manual/en/language.variables.basics.php.
   -  $mask = '/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/';
   +  // The internal database driver name is any valid PHP identifier.
   +  $mask = '/^' . DRUPAL_PHP_FUNCTION_PATTERN . '$/';
   

   good cleanup, but out of scope - separate issue?

4. +++ b/core/lib/Drupal/Core/Config/InstallStorage.php
   @@ -110,9 +112,14 @@ public function listAll($prefix = '') {
   +      $listing = new ExtensionDiscovery();
   ...
   +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -297,19 +298,28 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
   +      $listing = new ExtensionDiscovery();
   

   can you wrap this in an extensionDirectory() method like you have for the theme handler, so we can mock for phpunit

5. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -0,0 +1,154 @@
   +  protected $blacklist = array(
   +    // Object-oriented code subdirectories.
   +    'src',
   +    'lib',
   +    'vendor',
   +    // Front-end.
   +    'assets',
   +    'css',
   +    'files',
   +    'images',
   +    'js',
   +    'misc',
   +    'templates',
   +    // Legacy subdirectories.
   +    'includes',
   +    // Test subdirectories.
   +    'fixtures',
   +    // @todo ./tests/Drupal should be ./tests/src/Drupal
   +    'Drupal',
   

   should we add .git here? I would regularly have a contrib module in it's own checkout, no need to traverse into the .git folder

6. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -0,0 +1,154 @@
   +    if ($name[0] == '.') {
   

   nevermind :)

7. +++ b/core/lib/Drupal/Core/Extension/Extension.php
   @@ -0,0 +1,218 @@
   +   * The relative pathname of the extension (e.g., 'core/modules/node/node.info.yml').
   ...
   +   * The relative pathname of the main extension file (e.g., 'core/modules/node/node.module').
   

   > 80 chars

8. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -0,0 +1,395 @@
   +class ExtensionDiscovery {
   

   Should this have an interface and be a service (so can be injected etc), I realise its low-level, but there are high-level uses of drupal_system_listing() in contrib in D7

9. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
   @@ -0,0 +1,395 @@
   +   *   (optional) Whether to include test extensions. Defaults to FALSE when not
   ...
   +  public function scan($type, $include_tests = NULL) {
   

   Default mismatch, one says NULL, the other FALSE

10. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -0,0 +1,395 @@
    +    // Search the core directory.
    +    $searchdirs[static::ORIGIN_CORE] = 'core';
    +
    +    // Search the sites/all directory (replaced by top-level directory in D8).
    +    $searchdirs[static::ORIGIN_SITES_ALL] = 'sites/all';
    +
    +    // Search for contributed and custom extensions in top-level directories.
    +    // The scan uses a whitelist to limit recursion to the expected extension
    +    // type specific directory names only.
    +    $searchdirs[static::ORIGIN_ROOT] = '';
    +
    +    // Search the site-specific directory.
    +    $searchdirs[static::ORIGIN_SITE] = conf_path();
    

    Should there be an ORIGIN_PROFILE here? if so we must be missing tests. If not, then I've missed something.

11. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -0,0 +1,395 @@
    +        static::$files[$dir][$include_tests] = $this->scanDirectory($dir, $include_tests);
    

    fetching it all in once pass++

12. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -0,0 +1,395 @@
    +    // 1 0 profiles/parent_profile/modules/parent_module/parent_module.module
    

    Parent profiles? Do we support that?

13. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -0,0 +1,395 @@
    +   * @todo Move installation profiles into Settings (including directory paths).
    

    Out of scope?

14. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -0,0 +1,395 @@
    +   * @param array $profileDirectories
    +   *   A list of installation profile directories to search for extensions.
    +   *
    +   * @return $this
    +   */
    +  public function setProfileDirectories(array $paths = NULL) {
    

    mismatch $profileDirectories Vs $paths

15. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -316,12 +295,10 @@ public function rebuildThemeData() {
    -      // Give the screenshot proper path information.
    

    out of scope?

16. +++ b/core/lib/Drupal/Core/Extension/ThemeHandlerInterface.php
    @@ -81,10 +81,10 @@ public function listInfo();
    +   * @return \Drupal\Core\Extension\Extension[]
    

    Yay for less arbitrary arrays, more domain objects

17. +++ b/core/lib/Drupal/Core/Utility/ProjectInfo.php
    @@ -178,13 +178,11 @@ function processInfoList(&$projects, $list, $project_type, $status, $additional_
    -   *
    -   * @see system_get_files_database()
    

    note for other reviewers: this function is gone

18. +++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/TestSchemaStorage.php
    @@ -29,10 +30,12 @@ public function __construct() {
    +      $listing = new ExtensionDiscovery();
    

    again, makes unit testing difficult, can we use a method like for ThemeHandler, given it extends InstallStorage, should be do-able

19. +++ b/core/modules/system/lib/Drupal/system/Tests/Bootstrap/GetFilenameUnitTest.php
    @@ -47,15 +47,6 @@ function testDrupalGetFilename() {
    -    $this->assertIdentical(drupal_get_filename('script', 'test'), 'core/scripts/test/test.script');
    

    Does anything use this functionality (using drupal_get_filename with random arguments), surely it was added for a reason (and with tests). Worth grepping the git logs?

20. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/SystemListingTest.php
    @@ -7,12 +7,13 @@
    -class SystemListingTest extends WebTestBase {
    +class SystemListingTest extends DrupalUnitTestBase {
    

    Nice!

21. +++ b/core/modules/system/lib/Drupal/system/Tests/System/ThemeTest.php
    @@ -254,9 +254,12 @@ function testSwitchDefaultTheme() {
    -    $this->assertText(t('This theme requires the base theme @base_theme to operate correctly.', array('@base_theme' => 'not_real_test_basetheme')), 'Invalid base theme check succeeded.');
    -    $this->assertText(t('This theme requires the theme engine @theme_engine to operate correctly.', array('@theme_engine' => 'not_real_engine')), 'Invalid theme engine check succeeded.');
    +    $this->assertText(t('This theme requires the base theme @base_theme to operate correctly.', array('@base_theme' => 'not_real_test_basetheme')));
    +    $this->assertText(t('This theme requires the theme engine @theme_engine to operate correctly.', array('@theme_engine' => 'not_real_engine')));
    

    Out of scope?

Manual testing as follows:

• create a new module 'foo' in core/profiles/standard - ensure it shows up
• override said module 'foo' in sites/modules - ensure overrides work

so my comments about ORIGIN_PROFILE don't apply here

Overall, I like this patch alot.

1. +++ b/core/includes/theme.inc
   @@ -118,19 +119,9 @@ function drupal_theme_initialize() {
     *    An optional array of objects that represent the 'base theme' if the
   

   Do we use (optional) An array.. instead?

2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -297,19 +298,28 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
   +      $parent_profile = !empty($settings['parent_profile']) ? $settings['parent_profile'] : FALSE;
   

   This could be a much broader question/issue really. However, might as well bring it up here. Do you think this should return NULL and not FALSE. So we are not mixing bool/strings? (minor issue)

3. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -0,0 +1,154 @@
   +  public function acceptTests($flag = NULL) {
   

   Pass a bool here are the default parameter value maybe.

4. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -0,0 +1,154 @@
   +    $name = $this->current()->getFilename();
   ...
   +      if ($this->current()->getSubPath() == '') {
   ...
   +        return substr($this->current()->getPathname(), -14) == 'modules/config';
   

   Might make sense to store the value of $this->current() as a variable instead in this method?

5. +++ b/core/modules/simpletest/simpletest.module
   @@ -542,23 +545,36 @@ function simpletest_test_get_all($module = NULL) {
   +    cache()->set($cid, $extensions);
   

   Could use \Drupal::cache() directly but don't really mind. Would be pretty nice to remove cache.inc soon though. There isn't much in there nowadays.

This is looking awesome Sun. I have run into multiple issues with discovery of extensions, so to see this rewrite is great!

I tested the patch from 28. For each module, profile and theme that I created, I only added a .info.yml file as that should be the only requirement to detect the module/theme. Also, when I note a success I mean that the module was shown on the admin/extend page, the theme was shown on the admin/appearance page, or the profile was shown as an option when running install.php. I'm assuming that issues regarding actually enabling these modules, themes, and profiles, which only contain info.yml files is another issue.

Modules:

• Created a module in /modules and it worked.
• Created a module in /core/profiles/minimal/modules (The profile I used to install Drupal) and it worked.
• Created a module in /core/profiles/standard/modules (A profile that was not installed) and nothing showed up. It worked.

Themes:

• Created a theme in /themes and it worked.
• Created a theme in /core/profiles/minimal/themes (The profile I used to install Drupal) and it worked.
• Created a theme in /core/profiles/standard/themes (A profile that was not installed) and nothing showed up. It worked.

Profiles:

• Created a profile in /profiles and it worked.

Thoughts

• Themes required a cache clear before they would be detected/updated while modules did not. Is this expected behavior?
• As noted, I only made sure that these discovered extensions showed up in the proper sections of the admin UI. They all had issues being enabled, which I am assuming is out of the context of this patch/issue, and that subsequent issues would need to be raised regarding the enabling of these extensions. Is this correct?
• At the top of the issue you note that type should be at the top of the .info.yml file. I found that the extension was discovered regardless of where the type was located. Is having the type anywhere but at the top supposed to have an effect?

Is there anything else that needs to be tested? Otherwise I would consider this RTBC depending on the response to my questions. Again, awesome work Sun!

Awesome, thanks for the detailed response. I'm marking this as RTBC, let's get this committed! In addition to leading by example with core's modules, we should also make sure that the change record as well as the documentation also note this, as this is something that could easily be lost in the shuffle.

I do not plan to address #27.1 and .5 here.

27.1 - fair enough. I don't really care about that
27.5 - I was not meaning remove cache.inc in this issue. Here you are adding calls to cache() though, which is not preferable to \Drupal::cache().

This can be removed in another issues though, I don't mind either way. If you're really against changing it for whatever reason.

OK, just for reference, created #2198339: Remove cache.inc to remove cache.inc (Also see child issue that is postponed on).

The 'type' property should ideally be declared first (for file parsing performance reasons only).

Is this a requirement or optional?

As of now its optional. You can actually put the type anywhere in the info.yml but you get a performance bump if you put it first.

extension.disco_.28.patch no longer applies.

error: patch failed: core/lib/Drupal/Core/Extension/UpdateModuleHandler.php:46
error: core/lib/Drupal/Core/Extension/UpdateModuleHandler.php: patch does not apply

This seems like it'd be good for catch to review.

The usages of the cache() function still need to be changed. This is now deprecated and would reintroduce it, meaning the only two usages in core again.

Not sure why you refused to do this in my previous feedback tbh ;)

Fixed those calls. No commit credit, this is all sun's doing :)

Thanks!

extension.disco_.43.patch no longer applies.

error: patch failed: core/includes/module.inc:5
error: core/includes/module.inc: patch does not apply

Trivial re-roll, only conflicted on a new use statement.

extension.disco_.46.patch no longer applies.

error: patch failed: core/modules/system/system.module:1574
error: core/modules/system/system.module: patch does not apply

Overall looks great, few things I noticed:

1. +++ b/core/includes/bootstrap.inc
   @@ -655,7 +656,7 @@ function _drupal_request_initialize() {
   @@ -667,72 +668,31 @@ function drupal_get_filename($type, $name, $filename = NULL) {
   

   Something in me was hoping this function would completely die. It's great that it's a lot shorter, is there a part ? where this gets moved to the extension system proper? (Fine if there isn't for this patch, just wondering).

2. +++ b/core/includes/install.inc
   @@ -123,22 +124,19 @@ function drupal_detect_database_types() {
   + * @todo Convert database drivers into a new + regular extension type.
   

   Not sure about this, we don't do the same for lots of other services and seems like this would only ever be used in the installer. Could we remove the @todo and add a follow-up issue instead? The discovery here is no worse than it already is, so doesn't need a @todo (which is more of an apology than an intention to do anything usually).

3. +++ b/core/includes/module.inc
   @@ -297,14 +298,19 @@ function module_uninstall($module_list = array(), $uninstall_dependents = TRUE)
   +  $files = $listing->scan('module');
   

   purdy.

4. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -0,0 +1,152 @@
   + * To ensure the best possible performance for an extension discovery, this
   

   No need for 'an'.

5. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -0,0 +1,152 @@
   +  protected $blacklist = array(
   

   What if there's a module with the same name as one of these directories?

6. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -0,0 +1,152 @@
   +  public function acceptTests($flag = FALSE) {
   

   Does this need to be specific to tests, or could we add a setting, then override that setting when running tests? Just thinking whether there's a use case for extending the blacklisted directories during normal operation.

7. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -0,0 +1,152 @@
   +    $this->acceptTests = (bool) $flag;
   

   No need to cast to bool.

8. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -0,0 +1,152 @@
   +      // named 'config'. By explicitly testing for that case, we can skip all
   

   Should we open a follow-up issue to rename 'config' module to 'configuration? I understand the optimization here but it's a bit nasty.

9. +++ b/core/lib/Drupal/Core/Extension/Extension.php
   @@ -0,0 +1,218 @@
   +    $info = new \ReflectionObject($this);
   

   Really? Could at least do with a @todo, but I'm having trouble seeing why this is all necessary.

10. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -0,0 +1,411 @@
    +    // Search the sites/all directory (replaced by top-level directory in D8).
    

    Not really replaced.

11. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -0,0 +1,411 @@
    +        $info = $this->getInfoParser()->parse($file->getPathname());
    +        if (!isset($info['core']) || $info['core'] != \Drupal::CORE_COMPATIBILITY) {
    

    Might be worth putting in a dedicated method, we must be doing the same check elsewhere.

12. +++ b/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php
    @@ -0,0 +1,411 @@
    +    // include_paths will not be consulted). Backup the relative originating
    

    Not a backup, just 'store' or 'retain'?

13. +++ b/core/modules/block/tests/modules/block_test/block_test.module
    @@ -6,14 +6,6 @@
    - * Implements hook_system_theme_info().
    

    Nice.

I'm going to reroll this. then we can address the concerns of @catch.

auto merged with no conflicts. (patch 50)

I didn't answer catch's questions, but did fix #48 4, 7, 12 in patch 50b.

I've been struggling finding time to get back to this, but had another look through and no more complaints really - or they'll hopefully get resolved in the next set of issues.

However, this no longer applies.

51: extension.disco_.49.patch queued for re-testing.

Committed/pushed to 8.x, thanks!

Can't install into multi-site folder
EDIT
sites.php - $sites['d8.l'] = 'd8.l';

LogicException: Cannot use SplFileObject with directories in SplFileInfo->openFile() (line 354 of /home/andypost/www/core8/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php).

SplFileInfo->openFile('r')
Drupal\Core\Extension\ExtensionDiscovery->scanDirectory('sites/d8.l', )
Drupal\Core\Extension\ExtensionDiscovery->scan('profile')
install_begin_request(Array)
install_drupal()

Reverted for now.

The problem was caused by custom theme that is sym-link'ed to site's theme folder

It was not introduced here, see #2204675: Extension discovery is not discovering symlinks

andypost confirmed this was a regression introduced by this specific patch in irc.

@penyaskito No! it is introduced here, before this patch my theme and modules works fine.

So I see no reason to commit major issue and make another major to fix regression!

I've checked php 5.4 (fresh debian) and core fails too: I just placed symlink to theme folder

Additional uncaught exception thrown while handling exception.

Original

LogicException: Cannot use SplFileObject with directories in SplFileInfo->openFile() (line 354 of /var/www/drupal/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php).

SplFileInfo->openFile('r')
Drupal\Core\Extension\ExtensionDiscovery->scanDirectory('sites/default', )
Drupal\Core\Extension\ExtensionDiscovery->scan('profile')
install_begin_request(Array)
install_drupal()
Additional

LogicException: Cannot use SplFileObject with directories in SplFileInfo->openFile() (line 354 of /var/www/drupal/core/lib/Drupal/Core/Extension/ExtensionDiscovery.php).

SplFileInfo->openFile('r')
Drupal\Core\Extension\ExtensionDiscovery->scanDirectory('sites/default', )
Drupal\Core\Extension\ExtensionDiscovery->scan('theme')
Drupal\Core\Extension\ThemeHandler->rebuildThemeData()
Drupal\Core\Extension\ThemeHandler->listInfo()
list_themes()
_drupal_maintenance_theme()
drupal_maintenance_theme()
_drupal_log_error(Array, 1)
_drupal_exception_handler(Object)

@sun This looks like we must use
http://ua1.php.net/manual/en/splfileinfo.getlinktarget.php
http://ua1.php.net/manual/en/splfileinfo.islink.php
for checking if dst is symbol link and get its real target in this case.

This fix looks good to me - let's let andypost confirm and RTBC.

Now it works, just re-roll because 66 applies with offset.
Also I found that installation of modules become a bit slower but seems this caused by yml parser

Thanks! Committed/pushed to 8.x.

This also seems to have broken run-tests.sh on OS X: #2216695: run-tests.sh broken on OS X ("Too many open files").

Even for Drupal 8 which is full of stupid ideas this is a spectacularly stupid idea -- how the hell are we supposed to debug test failures if we can't switch on a test module?? Incredible!

Here are new tests documenting the behavior of ExtensionDiscovery.
#2729711: Improve test coverage for ExtensionDiscovery

I wonder if all of this is the intended behavior, or if some of it is by accident.
E.g. I remember that in D7, files in parent directories had priority over those in subdirectories with same name. In D8 it is the opposite.
Also, there is nothing to prevent extensions of different type to have the same machine name.

In aftermath of this issue, was it a good idea to let the Extension object pretend to also be the SplFileInfo object for its *.info.yml file via magic __call()?
Why did we do this?
#2959989: Deprecate Extension::__call() magic