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.
Comment | File | Size | Author |
---|---|---|---|
#64 | interdiff-2855026-55-64.txt | 1.87 KB | phenaproxima |
#64 | 2855026-64.patch | 4.17 KB | phenaproxima |
#56 | 2855026-55.patch | 1.72 KB | phenaproxima |
#54 | 2855026-54-FAIL.patch | 561 bytes | phenaproxima |
#49 | interdiff-2855026-46-49.txt | 1.46 KB | tucho |
Comments
Comment #2
DamienMcKennaComment #3
DamienMcKennaIt seems to be the logic in drupal_verify_profile() that causes the problem, that's where it processes the dependencies.
Comment #4
DamienMcKennaSo there are two parts to this:
Ultimately, the list of dependencies examined in drupal_verify_profile() needs to be filtered so it only lists the modules.
Comment #5
DamienMcKennaThis appears to fix it.
What should I do to make this commit-worthy?
Comment #6
DamienMcKennaComment #8
DamienMcKennaRerunning the tests, the failure in statistics.test appears to be unrelated.
Comment #9
trobey CreditAttribution: trobey as a volunteer commentedThe 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?
Comment #10
DamienMcKenna@trobey: Thanks for pointing out drupal_parse_dependency(), I'll see about reworking the patch to use it and maybe _module_build_dependencies() too.
Comment #11
andypostThe same affects 8.x core
Comment #12
andypostPatch for 8.x, versions still not checked
Local site install fails
Comment #13
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedJust found this bug while trying to make phpcs warnings go away. Very frustrating.
Comment #14
andypostI used to patch coder https://github.com/klausi/coder/pull/25/files to fix phpcs warnings
Comment #15
tobiasbWith 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.
Comment #16
Taran2LComment #17
Taran2LNew patch attached. Changes:
Comment #18
HaiNguyen007 CreditAttribution: HaiNguyen007 commentedThis patch work for D7.56
Comment #21
HaiNguyen007 CreditAttribution: HaiNguyen007 commentedreroll patch #15 for Drupal 7.56
Comment #22
HaiNguyen007 CreditAttribution: HaiNguyen007 commentedComment #23
andypost@HaiNguyen007 please create separate issue for d7
Comment #24
Taran2L@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
Comment #25
andypostyep, it works for me, it was my local collision of patches
Comment #26
andypostBlocker for
- #2821499: Enable phpcs rule DrupalPractice.InfoFiles.NamespacedDependency
- #2857856: Allow not namespaced dependencies in theme info YAML files
Comment #27
DamienMcKennaI created #2905520: Installation profiles do not support project:module format for dependencies (backport to D7) for the D7 backport.
Comment #28
DamienMcKennaI rerolled @HaiNguyen007's patch and uploaded it to #2905520: Installation profiles do not support project:module format for dependencies (backport to D7).
Comment #29
larowlanWhy 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
Comment #30
jofitz CreditAttribution: jofitz at ComputerMinds commenteddrupal_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.Added drupal:fictional
Added
$this->assertContains('Fictional', $message);
Comment #32
alexpottThis looks good. Just a couple of tidy ups.
Comment #33
trobey CreditAttribution: trobey as a volunteer commentedI 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.
Comment #34
trobey CreditAttribution: trobey as a volunteer commentedI added project namespaces to the three installation profiles.
Comment #35
alexpott@trobey nice work - we also need to do the test profiles.
Comment #36
dawehnerThis 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 ...
Comment #37
alexpott@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.
Comment #38
andypost@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?
Comment #39
trobey CreditAttribution: trobey as a volunteer commentedI 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.
Comment #40
trobey CreditAttribution: trobey as a volunteer commentedComment #41
alexpott@trobey version handling is 100% a separate issue.
Comment #42
dawehnerLet's add the followup already :)
Comment #43
trobey CreditAttribution: trobey as a volunteer commentedI 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.
Comment #44
trobey CreditAttribution: trobey as a volunteer commentedComment #46
-enzo- CreditAttribution: -enzo- at weKnow Inc for Department of Premier and Cabinet - Victoria, Australia commentedReroll patch to be compatible with new release 8.6.0
Comment #48
-enzo- CreditAttribution: -enzo- at weKnow Inc for Department of Premier and Cabinet - Victoria, Australia commentedBTW 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
Comment #49
tuchoBased 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:
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:
Comment #50
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedThank you :)
The Patch in #49 is working well.
Comment #51
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedComment #52
tstoecklerThe 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.Comment #53
phenaproximaI 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.
Comment #54
phenaproximaHere'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.
Comment #56
phenaproximaAnd, 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.
Comment #57
phenaproxima#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.
Comment #59
phenaproximaFiled 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.
Comment #60
NickDickinsonWildeRunning manual tests
Comment #61
NickDickinsonWildeManual tests confirm it works correctly :)
Comment #62
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedThank you Adam,
Switching to use 2855026-55.patch
Comment #63
alexpott@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.
Comment #64
phenaproximaRestored test coverage from #30, sans rename.
Comment #65
NickDickinsonWildeTest improvements make sense and look good to me
Comment #66
alexpottAdding @NickWilde to credit for manual reviews and testing.
Comment #67
alexpottCommitted 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.
Comment #70
alexpottAh 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.