Modules and themes of the active custom profile cannot be scanned correctly

I've encountered the same issue while trying to make an early port of my profile.

But the problem is not in the SystemListing class, its scan function is only for the detection of modules - and it does detect them, so the actual problem is somewhere else.

Unfortunately I was not able to figure out whats the issue yet

This issue seems only to exist for Modules and not themes as far I could see. (At least the theme of my problem is working without problems). Or did you encounter problems with themes too?

Debugging...

After further debugging I found out that this resolves in a fatal error because in
DrupalKernel.php (line 200)

    // Load each module's bundle class.
    foreach ($this->moduleList as $module => $weight) {
      $camelized = ContainerBuilder::camelize($module);
      $class = "Drupal\\{$module}\\{$camelized}Bundle";
      if (class_exists($class)) {
        $bundles[] = new $class();
        $this->bundleClasses[] = $class;
      }
      $filename = dirname($module_filenames[$module]) . "/$module.services.yml"; // <--- error
      if (file_exists($filename)) {
        $this->serviceYamls[] = $filename;
      }
    }

The index of the module in the profile directory not set ($module_filenames[$module]), this will trigger the current set php error handler, which is _drupal_error_handler_real() at this moment. Problem is that this function makes use of the container (it needs the Request object) - which is not initialized yet at this moment.

The $module_filenames contents originate from the getModuleFileNames() function which gets called earlier.

DrupalKernel.php (line 528)

  protected function getModuleFileNames() {
    $filenames = array();
    foreach ($this->moduleList as $module => $weight) {
      if ($data = $this->moduleData($module)) {
        $filenames[$module] = $data->uri;
      }
    }
    return $filenames;
  } 

It is iterating through the $this->moduleList - which DOES contain the module in the projects directory.
So it must get lost during the call to moduleData().

The moduleData() method now uses an instance of $modules_scanner = new SystemListing($profiles); to scan for the modules data.

So the problem here is that the SystemListing class is used instead of the SystemListingInfo class. While the SystemListingInfo class determines the profiles directory to scan for itself, the SystemListing class requires the constructor to set a list of profiles.

And it just adds the profile names as they were actually directories.

    foreach ($this->profiles($directory) as $profile) {
      $searchdir[] = $profile;
    }

Because different to SystemListingInfo::profiles() the SystemListing::profiles() function just returns the array of profile names instead the expected array of profile paths.

TL;DR

To me it seems SystemListingInfo's implementation of the profiles method does not make sense - it returns search paths instead of profile names. (So fabsor was actually really close at the issue)

So it would make sense to make SystemListingInfo's::profiles() actually just return a list of profiles and resolve their path later in the ::scan() function.

Problem is: At the state the SystemListing::scan() is called drupal_get_path() is not available yet.

What to do?

Actually I think the issue title should be more explicit, as the current implementation of these classes - are just wrong.

Dang, tagged instead changing title -.-

This feels major

As this is a bug we certainly need a test here, soory.

@dawehner
What kind of tests do we need?
manual testing?

Not sure it's possible to write a test here, so only a manual testing possible

The proposed patch does not resolve the implementation failures of SystemListing/SystemListingInfo's profiles() method as mentioned in #5

It would be maybe destable if the SystemListing class could be somehow injected or at least manually be set, so you could provide a mocked version.

@patrickd
actually i've completely done the debug way as described in #5 before I started writing the patch.
because I started my own project with my own profile.

so, the $profiles property in object of SystemListing depends on array delivered to constructor... nothing more.
i'm not sure if we really need to change SystemListing::profiles() method.
We just need to calculate correct paths which need to be scanned

does it make any sense?

Beside from how we resolve the actual bug - the problem I meant is the inconsistency of the profile() method between both classes.

class SystemListing {

  /**
   * Construct this listing object.
   *
   * @param array $profiles
   *   A list of profiles to search their directories for in addition to the
   *   default directories.
   */
  function __construct($profiles = array()) {
    $this->profiles = $profiles;
  }
  ....
  function scan($mask, $directory, $key = 'name') {
    ....
    // Search for the directory in core.
    $searchdir = array('core/' . $directory);
    foreach ($this->profiles($directory) as $profile) {
      $searchdir[] = $profile;
    }
 ....
  /**
   * List the profiles for this directory.
   *  ....
   * @return array
   *   A list of profiles.
   */
  protected function profiles($directory) {
    return $this->profiles;
  }
}

Problem of class SystemListing:
- Constructor says it gets "A list of profiles to search their directories" and currently it only gets that list of profile names
- profiles() method only returns that list of profiles
- scan() expects to get the actual search directory - the path to the profiles but only gets a list

class SystemListingInfo extends SystemListing {
  /**
   * Overrides Drupal\Core\SystemListing::profiles().
   */
  protected function profiles($directory) {
    ... // Resolves directories for all profiles and returns them.
  }
...
}

Problem of SystemListingInfo
- The profiles() method extends the one of SystemListing which says it only returns a list of profile names - BUT this one actually returns a list of directories to search.
- Instances of SystemListingInfo can correctly run the scan() method because it gets the expected array of directories

----

=> If we change what SystemListing gets into its constructor we have to change the method descriptions/comments of it accordingly to be consistent between SystemListing and SystemListingInfo.

-      $profiles = array_keys(array_intersect_key($this->moduleList, $all_profiles));
+      // Put directory names for profiles into array.
+      $profiles = array_map(function($profile) {
+        return dirname($profile->uri);
+      }, array_intersect_key($all_profiles, $this->moduleList));
+

I suggest to use a different variable for the array of profile directories (eg. $profiles_paths) and keep a $profiles available as its own variable.

You can't remove the $directory parameter from SystemListing's profiles() because SystemListingInfo extends SystemListing and SystemListingInfos profile() method needs that parameter.

Beside that I still don't have the feeling you did understand what I mean, seems like there's some general misunderstanding here.
Unfortunately I've neither enough time to resolve this my self nor to make sure that I'm actually right.. :(

@patrickd
I understood your thoughts about consistency of SystemListing and SystemListingInfo.
In my view, at this point we should fix that bug and get working functionality for succesful scanning of modules and themes which live in profile. That is start point of this issue.
Yes, I understand that we should to make review and deep refactoring of SystemListing class, but I think we need to create another issue for that.

Any ideas?

I'm preparing steps to reproduce and manual testing for this issue. Will update this thread a little bit later.

@patrickd
you are right actually, and i understood you well...
I got the same error as you described in #5 comment.
I agree with you that we don't have a good consistency between SystemListing and SystemListingInfo classes.
I think we should make refactoring of both... Because there are much descriptions of methods don't correspond to the implementation logic.
But that refactoring is out of scope of this bug in my view.

also I don't understand why do we need 2 classes for profile scanning ...

cause we cant use SystemListingInfo very early when booting the kernel, cause i think besides that it depends on config() iirc, it also parse's all info.yml files, which would mean a major performance impact on the beginning of each request
I believe SystemListing is only used by the kernel

I agree they need refactoring, mainly because when those files are being scanned/parsed, the results are not stored anywhere, resulting in possible multiple scans/parsings per request, but yeah we should probably not do it here

Hi again.
I have some motivation to complete this one, so let me introduce manual testing results and some explanations.

I'll try to describe the ways how to:
- demonstrate the bug which was described in #5 comment
- make a manual test after the #25 patch applied

pp. 2 - 4 can be completed faster with unpacking of testprofile-1974696.zip to "profiles" directory.
This archive is not a part of the fix, but created just to make the manual test faster. It can help to create a "testprofile" profile.

How to reproduce initial state before the patch applied

1. Create a clear D8 instance from the git repository

2. Create your own profile
2.1. Put the profile under the profiles directory. Let his name is "testprofile".
2.2. Create profile files: testprofile.info.yml, testprofile.profile, testprofile.install. To simplify those operations I suggest to make a clone of "minimal" profile, rename files and make some code changes for just cloned files. Let's add some lines to enable and set up Seven theme as admin theme.

3. Create new test module inside the just created profile.
For example, put new test module into "modules" directory of the "testprofile" profile.

4. Create new test theme "testtheme".
Put the new theme into "themes" directory of the "testprofile" profile.

After that you'll have profile architecture like that:

--- profiles
    |--- testprofile
            |--- modules
                    |--- testmodule
                            |--- testmodule.install
                            |--- testmodule.module
                            |--- testmodule.info.yml
            |--- themes
                    |--- testtheme
                            |--- testtheme.theme
                            |--- testtheme.info.yml
            |--- testprofile.install
            |--- testprofile.profile
            |--- testprofile.info.yml

5. Install Drupal 8
5.1. Choose "Testprofile" profile on profile selecting step

5.2. Proceed with D8 installation.
5.3. Proceed with site configuration step.
5.4. Make sure that installation was completed successfully

6. Go to "Extend" section site.com/admin/modules
6.1. Observe that "testmodule" is present and can be enabled

6.2. Mark checkbox for "testmodule" module to be enabled and click "Save" button.
6.3. Go to "Performance" page site.com/admin/config/development/performance and flush the cache
6.4. Observe an error like this:
Fatal error: Call to a member function get() on a non-object in /var/www/d8/core/lib/Drupal.php on line 147

Why does it happen?
because moduleData array of DrupalKernel object doesn't have an element related to just installed module "testmodule" which is a part of "testprofile".
lines 325, 327 of /core/lib/Drupal/Core/DrupalKernel.php

  protected function moduleData($module) {
    if (!$this->moduleData) {
      // First, find profiles.
      $profiles_scanner = new SystemListing();
      $all_profiles = $profiles_scanner->scan('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.profile$/', 'profiles');
      $profiles = array_keys(array_intersect_key($this->moduleList, $all_profiles));
      // If a module is within a profile directory but specifies another
      // profile for testing, it needs to be found in the parent profile.
      if (($parent_profile_config = $this->configStorage->read('simpletest.settings')) && isset($parent_profile_config['parent_profile']) && $parent_profile_config['parent_profile'] != $profiles[0]) {
        // In case both profile directories contain the same extension, the
        // actual profile always has precedence.
        array_unshift($profiles, $parent_profile_config['parent_profile']);
      }
      // Now find modules.
      $modules_scanner = new SystemListing($profiles);
      $this->moduleData = $all_profiles + $modules_scanner->scan('/^' . DRUPAL_PHP_FUNCTION_PATTERN . '\.module$/', 'modules');
    }
    return isset($this->moduleData[$module]) ? $this->moduleData[$module] : FALSE;
  }

when $module = 'testmodule'
we have $this->moduleData[$module] = NULL after the scanning
so, the scanning has been passed, but hasn't been found the module placed into the profile "testprofile".

Manual Testing after the #25 patch applied

Repeat pp. 1 - 4 from the previous list of steps

5. Apply the #25 patch

6. Install Drupal 8
6.1. Choose "Testprofile" profile on profile selecting step
6.2. Proceed with D8 installation.
6.3. Proceed with site configuration step.
6.4. Make sure that installation was completed successfully

7. Go to "Extend" section site.com/admin/modules
7.1. Observe that "testmodule" is present and can be enabled
7.2. Mark checkbox for "testmodule" module to be enabled and click "Save" button.
7.3. Go to "Performance" page site.com/admin/config/development/performance and flush the cache
7.4. Try to surf across the site. No more fatal error.

Can confirm that previous patch resolves the issue.

Following patch just changes some descriptions / variable names in the SystemListingInfo/SystemListing classes... so that they are actually correct.

Could anyone make review please?

Is there any way how we can pass in the systemListing from outside so we can proove that it works with a test?

Confirm that this issue exists and that this patch fixes it.

Would $profile_paths be a more appropriate variable name than $profiles_paths?

Related (has tests) #2068473: Module handler favours modules in /core/modules/MODULE over modules in core/profiles/PROFILE/MODULE

Rerolling and adding test from other issue

Rerolled patch from #31

Added the test from #2068473: Module handler favours modules in /core/modules/MODULE over modules in core/profiles/PROFILE/MODULE

According to the test there is no module named 'drupal_system_listing_compatible_test'.
However, in the testing profile, that module is there.

So it would seem that the patch does not fix the problem.

Apologies to everyone here. Extensive debugging of the installer, tests, and other subsystems revealed some more fundamental problems and misconceptions in the current SystemListing[Info] implementation, and I already had 80% of the code refactored and fixed when I found this existing issue, so I'm going to close this issue as duplicate of:

#2185559: Extension System, Part I: SystemListing clean-up + performance; remove SystemListingInfo + drupal_system_listing()

The existing patch over there is similar to the one here (with respect to DrupalKernel::moduleData()), but not only fixes that, but also some infinite recursions as well as a massive amount of unnecessary additional filesystem scans. The patch is ready, profiled, battle-tested, and could use your reviews/feedback.