Call to a member function getPath() when installing Profiles with unknown modules as requirement

I can confirm this issue. It's occurring when you have a module set in dependencies, and the module doesn't exist.

When a module is not existing. This should be added to the requirements array that is returned in drupal_check_profile.

Can you please provide the steps to reproduce the issue.

Use profile in attachment. This will trigger an error (the menu module doesn't exist)

This seems like a drush issue.

Maybe similar to: https://www.drupal.org/node/1971072?

https://www.drupal.org/node/1971072#comment-10687236 thanks @zuuperman

It's not a drush issue. I also had this error when installing via the browser.

@zuuperman The issue summary reads "via drush". Can you update it please?

Never noticed the drush in description. Seems like another issue then, because he is also having it with copies of minimal / standard.

It would be better to throw an exception with a helpful message, something like this.

The path in #11 does indeed makes it a lot more obvious what the problem is...

+1 for the error message!

Going to see if I can recreate the issue in a test and I will expect an error message.

I have a test only patch and a patch which incorporates the code from #11 with the minor adjustment of using the InstallerException.

I have placed the tests in the wrong location. I will prepare the patches again

Good news that the test only failed and the test+fix passed testing. I will do more work on this to improve the Exception and perhaps create a new Exception for this which extends InstallerException.

Nice work! I'm quite a fan of clear exceptions and like your idea.

+++ b/core/modules/system/tests/src/Kernel/Installer/InstallerMissingDependenciesTest.php
@@ -0,0 +1,35 @@
+    $this->setExpectedException('Drupal\Core\Installer\Exception\InstallerException', t('The Testing missing dependencies profile depends on the missing module missing_module.'));

Since this is the only thing you are testing, the @expectedException and @expectedExceptionMessage annotations in the docblock make more sense. https://phpunit.de/manual/current/en/appendixes.annotations.html#appendi...

Thanks for the feedback @neclimdul, I wasn't even aware of the annotations. It works perfectly.

I am attaching a screenshot of the message that is display to the end user when they encounter this issue as well as the new patch for review.

Credit due to @mariancalinro who helped review my patch in person at Dev Days.

With the patch applied, the installer UI will only list a single missing dependency at a time, even if there are many dependencies.

It would be better to list all the missing dependencies, the way that Drupal 7 did.

It would be better to list all the missing dependencies, the way that Drupal 7 did.

This would be possible by first verifying all dependencies (similar to how config finds missing dependencies), and then throwing an exception (or using an assertion) listing all of them.

I'm attaching a screenshot of the message that is display to the end user when they encounter this issue as well as the new patch for review.

+++ b/core/profiles/testing_missing_dependencies/testing_missing_dependencies.info.yml
@@ -0,0 +1,9 @@
+dependencies:
+  - missing_module

I think the idea is to have more then one dependent module so exception will list them all

It seems interdiff from #37 is not quite real. I applied the changes from interdiff #37 in patch #34.

This should fix the remaining test.

Looks good to go

I'm going to review this patch.

This looks good to me. I've read through the code and cannot spot anything that would raise concern.

This is a major improvement on the approach initially proposed which throws an Exception when the first missing dependency is detected. It now displays all missing dependencies along side all other requirement problems.

Just changing the issue tag for consistency. Don't credit me for that.

1. +++ b/core/includes/install.inc
   @@ -930,27 +930,35 @@ function drupal_requirements_url($severity) {
   -
   ...
   -
   -  // Performs an ExtensionDiscovery scan as the system module is unavailable and
   -  // we don't yet know where all the modules are located.
   +  // Performs an ExtensionDiscovery scan as the system module is unavailable
   +  // and we don't yet know where all the modules are located.
   

   why?

2. +++ b/core/includes/install.inc
   @@ -930,27 +930,35 @@ function drupal_requirements_url($severity) {
   +    // Checking for key existence is enough because the "$module_list" will
   

   Enough for what? There's an @see where I can probably figure it out but the comment should tell us what does the keys existence actually tell us?

3. +++ b/core/includes/install.inc
   @@ -930,27 +930,35 @@ function drupal_requirements_url($severity) {
   +    if (array_key_exists($module, $module_list)) {
   

   Is there a reason to use array_key_exists instead of isset($module_list[$module])? i.e. does the code care about a set null value for some reason?

1. Because we are on installation phase.
2. Try to read after "because $module_list will definitely contain...". And @see annotation available as well.
3. There will not be any null items. Please, try to spend more time for review and not do useless review.

1) No, why did you change the spacing on the documentation but not change the documentation.
2) I did. that doesn't tell me that "enough" is. what does its existence mean?
3) then use isset instead of array_key_exists and don't be rude.

1. Yes. I've changed it because it was in contact with a border of 80 chars.
2. Please, follow the @see reference there. Execution of (new ExtensionDiscovery())->scan() will return an array of \Drupal\Core\Extension\Extension.
3. I have not changed previously used array_key_exists and not going to do this now.

Bringing it back to RTBC since no blockers here.

P.S. I'm sorry.

I don't agree that this is RTBC and that's generally the reviewers role in core issues.

1) That's what I figured. Border is not a documentation problem though. It'd be fine to use that in new docs but lets leave the old one's alone. Smaller patch for committer to review and more likely to go in quickly and keeps the blame history more concise.
2) Right, I could follow it. But why would we rely on that when we could just tell people the existence of the key tells us the module doesn't exist and we don't want to run the registration code when that's the case. That's will be a lot more useful to future developers looking at this. Also, I missed it in my first review but we require FQDN on @see annotations.
3) Cool, it is being added in this patch which is why I reviewed it. The actual fix for the issue even ;). Drupal avoids array_key_exists unless its absolutely necessary for various reasons and will be a red flag for most maintainers so lets not use it here.

RTBC++

I worried only about

+++ b/core/includes/install.inc
@@ -932,17 +932,17 @@ function drupal_check_profile($profile) {
-    if (array_key_exists($module, $module_list)) {
...
+    if (isset($module_list[$module])) {

there was array_key_exists() for reason
Last time I debugged this key was in array but value was NULL

Cool! NULL is not a valid extension so if there where NULL values in the array isset is actually fixing an additional bug.

I support the changes too. Thanks for weighing in @neclimdul.

@andypost, @neclimdul NULL is impossible there since scan() method of \Drupal\Core\Extension\ExtensionDiscovery has only one return from it - return $this->process($files);. So, if you follow to the process() method, you'll see that there is triggering the getName() method for each extension... So, fatal error will be earlier than you'll see NULL.

Let's finish with this. RTBC+

Committed/pushed to 8.3.x, thanks!

This looks fine for a patch release, so setting 'to be ported' against 8.2.x for cherry-pick once 8.2.0 is out.

Cherry-picked to 8.2.x