In #2205271: Project namespace for dependencies support was added to allow modules to list dependencies by prefixing their names with their project name, thus solving the age-old problem of module names and project names being different (*cough* CCK *cough*). This API improvement was included in v7.40.

In #2844504: Testbot failing for Metatag-D7 =) it was identified that some of Metatag's tests were failing because the testci system was unable to download certain dependencies, specifically one submodule depending upon another submodule would fail if the dependency was listed in the old [module] syntax, it had to be listed in the newer [project:module] syntax.

It was reported in #2853699: Out-of-date dependency processing in panopoly_core by asacolips that this conflicts with installation profiles, which do not support the [project:module] syntax.

A simple example is the following:

  • Download the latest Panopoly distribution release.
  • Set it up somewhere so it can be installed.
  • Download Metatag into the directory structure somewhere, e.g. sites/all/modules.
  • Edit the panopoly.info file to list Metatag as a dependency, e.g.:
    diff --git a/panopoly.info b/panopoly.info
    index 2ee445e..2752de7 100644
    --- a/panopoly.info
    +++ b/panopoly.info
    @@ -21,6 +21,8 @@ dependencies[] = file
     dependencies[] = dblog
     dependencies[] = update
     
    +dependencies[] = metatag
    +
     ; Panopoly Foundation
     dependencies[] = panopoly_core
     dependencies[] = panopoly_images
    
  • Hook up the codebase to the local web server and try to run the installer.
  • The following requirements error will be reported:

    Required modules - Required modules not found.
    The following modules are required but were not found. Move them into the appropriate modules subdirectory, such as sites/all/modules. Missing modules: Drupal:system, Ctools:ctools, Token:token

  • Further, if the dependency line in panopoly.info is changed to "dependencies[] = metatag:metatag" then the error becomes the following:

    Required modules - Required modules not found.
    The following modules are required but were not found. Move them into the appropriate modules subdirectory, such as sites/all/modules. Missing modules: Metatag:metatag

It seems like the dependency logic does not properly handle the [project:module] logic for modules available in the current codebase.

CommentFileSizeAuthor
#64 interdiff-2855026-55-64.txt1.87 KBphenaproxima
#64 2855026-64.patch4.17 KBphenaproxima
#56 2855026-55.patch1.72 KBphenaproxima
#54 2855026-54-FAIL.patch561 bytesphenaproxima
#49 interdiff-2855026-46-49.txt1.46 KBtucho
#49 drupal-support_project_module_format_dependencies-2855026-49.patch9.63 KBtucho
#46 interdiff_39-46.txt2.08 KB-enzo-
#46 2855026-46.patch9.81 KB-enzo-
#40 2855026-39.patch11.22 KBtrobey
#39 interdiff_32-39.txt5.09 KBtrobey
#34 interdiff_32-34.txt3.37 KBtrobey
#34 2855026-34.patch8.73 KBtrobey
#32 2855026-32.patch5.04 KBalexpott
#32 30-32-interdiff.txt1.4 KBalexpott
#30 interdiff-17-30.txt1.38 KBjofitz
#30 2855026-30.patch4.92 KBjofitz
#21 installation_profiles-2855026-21.patch804 bytesHaiNguyen007
#18 2855026-install-profiles-depedencies-18.patch1.2 KBHaiNguyen007
#17 interdiff-12-17.txt5.31 KBTaran2L
#17 2855026-17.patch4.86 KBTaran2L
#15 installation_profiles-2855026-15.patch812 bytestobiasb
#12 2855026-12.patch1.09 KBandypost
#5 drupal-n2855026-5.patch946 bytesDamienMcKenna
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna created an issue. See original summary.

DamienMcKenna’s picture

Issue summary: View changes
DamienMcKenna’s picture

It seems to be the logic in drupal_verify_profile() that causes the problem, that's where it processes the dependencies.

DamienMcKenna’s picture

So there are two parts to this:

  • If an installation profile lists a dependency in the format [project:module] it will fail.
  • If an sub-dependency from the profile is in the format [project:module] it will fail.

Ultimately, the list of dependencies examined in drupal_verify_profile() needs to be filtered so it only lists the modules.

DamienMcKenna’s picture

This appears to fix it.

What should I do to make this commit-worthy?

DamienMcKenna’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: drupal-n2855026-5.patch, failed testing.

DamienMcKenna’s picture

Status: Needs work » Needs review

Rerunning the tests, the failure in statistics.test appears to be unrelated.

trobey’s picture

The reason that this was not caught earlier is two-fold. First, this code does not call drupal_parse_dependency() which is in common.inc which is the core of Drupal core. While no one is forced to use this function not doing so means that when dependencies have any changes your code is likely to break. For instance, I do not see any provision for handling dependencies with versions specified(!). So the problem here is bigger than not handling the project.

Second, when the project namespace change was made all the tests passed. So the code being changed in this issue does not appear to have any test coverage. If you call drupal_parse_dependency() there is test coverage. If you do not use that function then do we need additional tests?

DamienMcKenna’s picture

Status: Needs review » Needs work

@trobey: Thanks for pointing out drupal_parse_dependency(), I'll see about reworking the patch to use it and maybe _module_build_dependencies() too.

andypost’s picture

The same affects 8.x core

andypost’s picture

Version: 7.x-dev » 8.4.x-dev
Status: Needs work » Needs review
FileSize
1.09 KB

Patch for 8.x, versions still not checked

Local site install fails

The following module is missing from the file system: drupal:action      [error]
in drupal_get_filename() (line 236 of core/includes/bootstrap.inc).
drupal_get_filename('module', 'drupal:action')
(Line: 259)
drupal_get_path('module', 'drupal:action') (Line:
635)
Drupal\Core\Config\ConfigInstaller->drupalGetPath('module',
'drupal:action') (Line: 617)
Drupal\Core\Config\ConfigInstaller->getDefaultConfigDirectory('module',
'drupal:action') (Line: 444)
Drupal\Core\Config\ConfigInstaller->checkConfigurationToInstall('module',
'drupal:action') (Line: 141)
Drupal\Core\Extension\ModuleInstaller->install(Array, ) (Line: 83)
Drupal\Core\ProxyClass\Extension\ModuleInstaller->install(Array, )
(Line: 1799)
_install_module_batch('drupal:action', NULL, Array) (Line:
252)
_batch_process() (Line: 870)
batch_process(Object, Object) (Line: 618)
install_run_task(Array, Array) (Line: 539)
install_run_tasks(Array) (Line: 116)
install_drupal(Object, Array) (Line: 726)
drush_call_user_func_array('install_drupal', Array) (Line:
711)
drush_op('install_drupal', Object, Array) (Line: 80)
drush_core_site_install_version('abilways', Array) (Line:
249)
drush_core_site_install('abilways',
'install_configure_form.site_default_country=FR',
'install_configure_form.date_default_timezone=Europe/Paris')
(Line: 422)
_drush_invoke_hooks(Array, Array) (Line: 231)
drush_command('abilways',
'install_configure_form.site_default_country=FR',
'install_configure_form.date_default_timezone=Europe/Paris')
(Line: 199)
drush_dispatch(Array) (Line: 67)
Drush\Boot\BaseBoot->bootstrap_and_dispatch() (Line: 66)
drush_main() (Line: 458)
drush_run_main(, '/', 'Phar detected. Proceeding to
drush_main().') (Line: 365)
drush_startup(Array) (Line: 114)
require('phar:///usr/bin/drush/drush') (Line: 10)

The following module is missing from the file system: drupal:action      [error]
in drupal_get_filename() (line 236 of core/includes/bootstrap.inc).
drupal_get_filename('module', 'drupal:action')
(Line: 259)
drupal_get_path('module', 'drupal:action') (Line:
169)
Drupal\Core\Extension\ModuleInstaller->install(Array, ) (Line: 83)
Drupal\Core\ProxyClass\Extension\ModuleInstaller->install(Array, )
(Line: 1799)
_install_module_batch('drupal:action', NULL, Array) (Line:
252)
_batch_process() (Line: 870)
batch_process(Object, Object) (Line: 618)
install_run_task(Array, Array) (Line: 539)
install_run_tasks(Array) (Line: 116)
install_drupal(Object, Array) (Line: 726)
drush_call_user_func_array('install_drupal', Array) (Line:
711)
drush_op('install_drupal', Object, Array) (Line: 80)
drush_core_site_install_version('abilways', Array) (Line:
249)
drush_core_site_install('abilways',
'install_configure_form.site_default_country=FR',
'install_configure_form.date_default_timezone=Europe/Paris')
(Line: 422)
_drush_invoke_hooks(Array, Array) (Line: 231)
drush_command('abilways',
'install_configure_form.site_default_country=FR',
'install_configure_form.date_default_timezone=Europe/Paris')
(Line: 199)
drush_dispatch(Array) (Line: 67)
Drush\Boot\BaseBoot->bootstrap_and_dispatch() (Line: 66)
drush_main() (Line: 458)
drush_run_main(, '/', 'Phar detected. Proceeding to
drush_main().') (Line: 365)
drush_startup(Array) (Line: 114)
require('phar:///usr/bin/drush/drush') (Line: 10)
pwolanin’s picture

Just found this bug while trying to make phpcs warnings go away. Very frustrating.

andypost’s picture

I used to patch coder https://github.com/klausi/coder/pull/25/files to fix phpcs warnings

tobiasb’s picture

With the other patch I got undefined index in install_profile_modules and _drupal_trigger_error_with_delayed_logging is trigged.

The patch is for Drupal7.

Taran2L’s picture

Taran2L’s picture

New patch attached. Changes:

  • Installation completes successfully
  • Tests
HaiNguyen007’s picture

Status: Needs review » Needs work

The last submitted patch, 18: 2855026-install-profiles-depedencies-18.patch, failed testing. View results

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

HaiNguyen007’s picture

reroll patch #15 for Drupal 7.56

HaiNguyen007’s picture

andypost’s picture

Issue tags: +Needs reroll

@HaiNguyen007 please create separate issue for d7

Taran2L’s picture

Status: Needs work » Needs review

@andypost patch works as is with D8.5, why a reroll ?

@HaiNguyen007 please create separate issue for d7, as proposed

setting this back to needs review

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

yep, it works for me, it was my local collision of patches

andypost’s picture

DamienMcKenna’s picture

DamienMcKenna’s picture

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/tests/src/Kernel/Installer/InstallerDependenciesResolutionTest.php
@@ -19,21 +19,23 @@ class InstallerMissingDependenciesTest extends KernelTestBase {
+    $this->assertNotContains('Drupal:node', $message);

+++ b/core/profiles/testing_profile_dependencies/testing_profile_dependencies.info.yml
@@ -1,10 +1,12 @@
+  - drupal:node

Why is one capital D and the other lower case?

Can we add one more case here, a missing module in {project:name} format.

And an assert to verify it shows up correctly?

I.e. we're covering the happy path, but not the unhappy path

Thanks

jofitz’s picture

Status: Needs work » Needs review
FileSize
4.92 KB
1.38 KB

Why is one capital D and the other lower case?

drupal_verify_profile() capitalises the first character, i.e. Unicode::ucfirst($module). "Drupal:node" has now been changed to "Node" because the project name is stripped off.

Can we add one more case here, a missing module in {project:name} format.

Added drupal:fictional

And an assert to verify it shows up correctly?

Added $this->assertContains('Fictional', $message);

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

This looks good. Just a couple of tidy ups.

trobey’s picture

I went to the directory for the standard profile and added metatag:metatag to the dependencies to standard.info.yml along with the Metatag project in the modules directory. An error appears:

Required modules
Required modules not found.
The following modules are required but were not found. Move them into the appropriate modules subdirectory, such as /modules. Missing modules:

Metatag:metatag

After applying the patch the error no longer appears. I had to add the Token project to them modules directory and then I could complete the installation. The code looks okay. The only question I have is if we need to add the project:drupal to the info.yml files under the profiles directory.

trobey’s picture

I added project namespaces to the three installation profiles.

alexpott’s picture

@trobey nice work - we also need to do the test profiles.

dawehner’s picture

This is really a nice nix. Explaining someone why it is different, is just really confusing.
It looks like we still have test profiles with other dependencies.

Here is a question. Module are able to specify version dependencies right now. Shouldn't profile have the same capability? This is part of the result of the dependency parsing ...

alexpott’s picture

@dawehner good point about profiles and version dependencies. At least with this it wouldn't break the installer if you added a version dependency. Definitely worth filing a followup to try to implement this.

andypost’s picture

@alexpott what about to add reports section warning for 8.6.x? It will need a sniffer for contrib or a test, assert method, follow-ups?

trobey’s picture

FileSize
5.09 KB

I added namespaces to the testing installation profiles. I did not change the testing_profile_dependencies. I can look at handling versions but it may be better to handle as a separate issue since this is tagged as a blocker. I do not know anything about the reports section warning.

trobey’s picture

alexpott’s picture

@trobey version handling is 100% a separate issue.

dawehner’s picture

Let's add the followup already :)

trobey’s picture

I will try to work on adding the follow up today. Before doing that I will need to verify version handling does not work and document how to reproduce the problem.

trobey’s picture

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

-enzo-’s picture

Reroll patch to be compatible with new release 8.6.0

Status: Needs review » Needs work

The last submitted patch, 46: 2855026-46.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

-enzo-’s picture

BTW The patch could be applied without problems, but the changes on install_profile_info are not enough with 8.6.x, something else is required

tucho’s picture

Based on the issue #2952888: Allow install profiles to require modules, profiles now have two keys to declare the modules that should be enabled during installation: dependencies and install.

From the issue:

Since we don't want to affect existing install profiles unless they opt in we need a BC layer. The BC layer moves all an install profile dependencies to the install key if the install key is not set.

Due to this, both keys must be converted from the project:module format so that the required modules are correctly processed.

The attached patch converts the install and dependencies keys in the install_profile_info function before they are unified in the install key:

$info['dependencies'] = array_map(function ($dependency) {
  return ModuleHandler::parseDependency($dependency)['name'];
}, $info['dependencies']);

// Convert install key in [project:module] format.
$info['install'] = array_map(function ($dependency) {
  return ModuleHandler::parseDependency($dependency)['name'];
}, $info['install']);
...
$ info ['install'] = array_unique (array_merge ($ info ['install'], $ required, $ info ['dependencies'], $ locale));
Rajab Natshah’s picture

Thank you :)

The Patch in #49 is working well.

Rajab Natshah’s picture

Status: Needs review » Reviewed & tested by the community
tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

The patch removes InstallerMissingDependenciesTest, I think that should not be done. At least there would need to be some reasoning for that and I don't see that being discussed.

phenaproxima’s picture

I agree that test removal is something that should only be done with heavy justification. Additionally, I think we need a fail patch to prove the problem.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
561 bytes

Here's a simple fail patch. I'm not sure exactly how many tests will break with this...but I bet it's a lot of 'em.

Status: Needs review » Needs work

The last submitted patch, 54: 2855026-54-FAIL.patch, failed testing. View results

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
1.72 KB

And, with the fix from #49. InstallerMissingDependenciesTest passes with this fix, so this patch does not remove it. The smaller the patch is, the better our chances of landing it.

phenaproxima’s picture

#54 failed exactly as hard as it should have, for the record. If you look at the console log on the dispatcher, you'll see a lot of installation-related failures. Perfect.

The last submitted patch, 54: 2855026-54-FAIL.patch, failed testing. View results

phenaproxima’s picture

Filed a postponed follow-up to change existing profiles so that the project:modulename format is used everywhere in core: #3003877: [PP-1] Namespace all core profile dependencies with a project name.

NickDickinsonWilde’s picture

Assigned: Unassigned » NickDickinsonWilde

Running manual tests

NickDickinsonWilde’s picture

Assigned: NickDickinsonWilde » Unassigned
Status: Needs review » Reviewed & tested by the community

Manual tests confirm it works correctly :)

Rajab Natshah’s picture

Thank you Adam,
Switching to use 2855026-55.patch

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@phenaproxima I agree that making the patch only provide the fix and having a test is important but the explicit test coverage that was added by #30 was really nice. Let's add that back. However I wouldn't rename the testing_missing_dependencies profile just in case (unlikely as it maybe) something in contrib / custom is using it.

Added issue credit for reviews.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
4.17 KB
1.87 KB

Restored test coverage from #30, sans rename.

NickDickinsonWilde’s picture

Status: Needs review » Reviewed & tested by the community

Test improvements make sense and look good to me

alexpott’s picture

Adding @NickWilde to credit for manual reviews and testing.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed e172a37ad8 to 8.7.x and fbbdbc2727 to 8.6.x. Thanks!

I've backported this to 8.6.x since I agree that this is a bugfix. It's rare that a bugfix deserves a change record but this one does.

  • alexpott committed e172a37 on 8.7.x
    Issue #2855026 by phenaproxima, trobey, HaiNguyen007, alexpott, Taran2L...

  • alexpott committed fbbdbc2 on 8.6.x
    Issue #2855026 by phenaproxima, trobey, HaiNguyen007, alexpott, Taran2L...
alexpott’s picture

Ah the CR is for the initial support for module dependencies. I don't think we need to add a new one or even update that one.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.