Project namespace for dependencies

As discussed in #2047557: Support Drupal 8-style *.info.yml files we have the problem today that modules cannot define a dependency on https://www.drupal.org/project/php. Every test run fails completly and makes automated testing impossible. GA depends on php filter module and always fail, see https://qa.drupal.org/pifr/test/480418.

After more reading the webform case I do not understand how this proposed change could be backwards compatible. There are many modules out that use a version string in dependencies. e.g.

test_dependencies[] = autoload (>7.x-1.5)
dependencies[] = autoload (>7.x-1.5)

Prefixing the version with a project name should fail the same way like prefixing the module name with a project. Untested, but I think this cannot work without changes. Unbelivable... This cannot wait for D9 and needs to be backported to D7, too.

As it may require changing every .info.yml file.

Wrong tag.

Aside... We could fix this in a lot cleaner way by adding a new key to info files that only project dependency uses. Like we already done with test_dependency. What do you think about this?

Example:
test_project_dependencies[] = php

This could force project dependency to download these listed projects as preferred projects. If the test_dependencies modules are found in these projects the project dependencies module stops trying to download the dependend modules from random projects. This should be maintainable in project dependency module, too.

@trobey: Does this work for you?

That is a good start. There may be some more changes required. I cannot remember for sure, but I thought update module will download dependend modules automatically... Isn't it? I could be wrong as I'm using drush for such a long time...

+++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
@@ -496,21 +496,21 @@ public function testDependencyParsing($dependency, $expected) {
-      array('system', array('name' => 'system')),
-      array('taxonomy', array('name' => 'taxonomy')),
-      array('views', array('name' => 'views')),
-      array('views_ui(8.x-1.0)', array('name' => 'views_ui', 'original_version' => ' (8.x-1.0)', 'versions' => array(array('op' => '=', 'version' => '1.0')))),
+      array('system', array('name' => 'system', 'module' => 'system')),
+      array('taxonomy', array('name' => 'taxonomy', 'module' => 'taxonomy')),

You shouldn't have to change all of these expectations. The code should assume that the module is the same. You should just add a couple test cases where the module is different, I guess?

Do we really need backward compatibility in D8? I do not think so. I would use module and project as keys and may backport a workaround.

I'm confused about views:views_ui. Views UI is in project drupal:views_ui as I know. We should try adding php module and see how project dependency behaves :-)

Code looks good, has green tests and I have no idea what else could I test manually than what the tests have tested. Let's get this in before beta.

+ * A dependency string, for example 'project:module (>=8.x-4.5-beta5, 3.x)'.

*blinks slowly*

Thanks for working on this but this comment needs a lot of better examples of what "project" and "module" can be. Not to mention that just "module ()" still works and that's not visible from the comment.

That sounds like we should write a change notice that every module should specify dependency only in the new format and the old module-only format is deprecaded? I'm fine with this change if it is a permanent fix. This makes this patch again RTBC, isn't it?

So why did both #17 and #19 both pass? #17 was wrong, it should have failed.

No, they pass as we only define the dependency. The test validates that splitting the dependency string works properly. In project dependency module we can test if downloading the dependency works. We could also add a test module to core that downloads php filter module and this will for sure fail on today test robot. But before we implement this, we need a core patch that has been accepted.

I'm not sure if we want a test in core that depends on a contrib project... Next core version produces a test failure if there is no contrib dev available.

Aside... I think we should very hard discourage people from creating colliding namespaces. We could also discuss unpublishing modules doing this. I fear that people start doing this if we force the project name...

If this about requiring Drupal developers to put in drupal: for every version dependency then count me as squarely opposed to this issue. Such a change would require wide buy in from the community and I don't see that here.

@chx: version is not required. It may be arguable if we require the "project:", but I would not require this. The problem today is simply that I cannot require php module as dependency. See https://qa.drupal.org/pifr/test/480418 how it fails. We need an optional project name in dependency string to solve the test bot inability. We also have the same troubles in drush that cannot resolve the project name and may download a wrong project, too. This are really heavy problems that may cause a lot of troubles for maintainers.

Then write an issue summary that makes the use case understandable because by now I absolutely have no idea what this issue is about. Concrete examples would be welcome. For eg. is project Views? Or what? etc.

e.g: this fails, because there are core alpha versions that have the php module integrated. The reason is the project dependency module tries to find a good match and finds drupal:php (project: drupal, module: php)

test_dependencies:
  - php

This will not fail (project: php, module: php)

test_dependencies:
  - php:php

Both projects drupal had and php project have the module php. Than my test downloads the required modules and tries to enable "php" module, but fails as core (DEV) does not have php embedded any longer. Root cause - project dependency think it still has (as there is a php module in an alpha release). The info.yml setting will force the testbot and drush to download php module from php project and not drupal.

Does this makes things clearer?

Now we know what are we talking about, if we want to make the project string required then a followup and a (very) wide community buy in is required for that. I recommend not holding the issue with it. However, it is optional at the moment + // Split out the optional project name. so we might've misunderstood each other and noone wanted required in the first place?

This seems like a very drupally way of trying to specify which repository to use for a given library (module) dependency, which sounds like the same problem space as configuring tools like composer to use a different fork. If we followed that model, the directive might be on a separate line.

dependencies[] = webform
repository[webform] = webform_forked

@grayside: That looks like my #5 idea. That is really backward compatible I think. I like it, too.

To be considered: the "project" line does not exist as far as Drupal is concerned. Look here http://cgit.drupalcode.org/views/tree/views_ui.info

So we need to decide whatever way we add this to dependencies on what do we do if the project value is not available? It won't be from a git checkout.

Side note:

> The core maintainers always assume that Drupal 7 core only has to understand Drupal 7 info files but this is not true. I have to use Drupal 7 core to read Drupal 6 and Drupal 8 info and info.yml files.

That's off topic. If you want, please file a separate meta issue and get the community to agree with that. Until then, D7 code works with D7 modules.

After a brief discussion on IRC: there's a sister issue to this, this is a followup to it: enact a policy (no code whatsoever) that project: line in *.info.yml is mandatory and add it to the core files. That can't be done past beta because if from beta1 to beta2 modules won't even be recognized as such then we sort of failed in "we are not turning the world upside down anymore past beta". If I may, I recommend putting considerable effort in making your case succinctly and precisely. That was not apparent in this issue. I am interested enough to condense the problem into an IS for this issue (which you probably want to reuse for the other one too) but trying to change policy this close to beta is an issue too controversial for my tastes so I can't help with that one. Don't be surprised if this change will need to wait for 8.1.

Once we have done such policy, this for D8 becomes an easy issue.

Then we can decide what do with D7. That's an entirely separate thing and I am not sure whether it's solvable *at all*. We can add an optional policy perhaps. That will also be fun to get through.

Well, you post an issue something like https://www.drupal.org/node/2134513 this and then somehow get attention to it and get it accepted. It's very informal. There's no formal process. Wish there was.

So far, I think we agreed on it being optional. It would be quite a blow to enforce for every dependency a project when so often it'd be X:X anyways.

I'm totally confused what this case has to do with project: key in info.yml. Can we set this back to RTBC now?

How do you plan to get the project from the info if it's not in git? Yes, the packaging script puts in there but if it's not in git then git checkouts have no way of gaining it. Not to mention custom modules. Also, we can't because I haven't seen better comments written...?

I have not reviewed the project dependency code, but today we find the projects. Project key is not in the scope of this case. This is RTBC. Don't set it back, please. Create followups for other issues, please.

Postponed on #2296241: [policy, no patch] Project namespace for dependencies

Also why did you RTBC this despite I asked for better comments? You know what the community thinks of you for this behavior, right? Do not re-RTBC this. I am actually trying to help you despite your constant issue queue trolling.

@Chx: as trobey said - you have not understand anything in this issue. Sorry. The last ~30 comments are ALL offtopic and have nothing - absolutly nothing to do with the issue here. You only confuse this case. Trobey has rewritten the intro and i have no idea how someone can explain this more detailed. I'm not trolling here, You do not understand the issue at all. The other confusing case has been closed. This patch is RTBC. Try understanding the issue first before you write another 20 nonsensical comments, please.

An issue summary is a concise overview of a full issue report.

This issue seems like only half the work or the testing is not complete. I would have expected test modules declaring dependencies on project:module configurations that exist or not not exist. Reading the code I can't see how the dependency graph is fixed for modules of the same name being provided by different projects. Are we only adding this information for https://www.drupal.org/project/project_dependency?

Setting back to "needs work" to get answers.

is it obvious that a missing fe_block module requires installation of the Features Extra project?

No it's completely not obvious - but I really can't see how this issue fixes that. Yes it would allow this to be fixed if modules included their dependencies like this. But this patch does not make that required or give any much of a clue that this is even possible. We need more documentation since at the moment it is not there to help module authors do the right thing. Plus this is a change from previous versions of Drupal so we need a change record.

The documentation should be added to the doc block for InfoParserInterface since this line is would be incomplete with this patch applied:

   * - dependencies: An array of shortnames of other modules this module requires.

And since there are no examples in core you will need to provide a valid one.

change records? Add one: http://drupal.org/node/add/changenotice | Instructions: https://drupal.org/contributor-tasks/draft-change-record | New change record process: https://groups.drupal.org/node/402688 | See also: needs change record? missing change record? for lists of issues. change record improvements?

trobey has written change notice, project dependency integration. Setting back to RTBC as before. No changes to the patch made.

So if I have both webform and webform_patched in my /modules directory and I install a module that depends on webform_patched's webform module how does the module installer choose the correct one to install?

To me this solution still seems half baked and we have work to do in core.

Sorry I didn't mean to imply that there has not been thought put into it - I was meaning to say I feel it is incomplete.

What I'm trying to say is that if we introduce this then we need to bake this into module dependency discovery so that the system behaves as the user expects. If a developer adds the project to the dependency info and the system enables the wrong module then this is going to be super confusing.

Whilst I understand that this enables Project Dependency to download the correct module and that this is very important - meeting developer expectations is important too.

Also one of the advantages of the current system is that you can replace modules like webform with webform_patched and modules that depend on webform will still think their dependencies are met. If we start specifying projects then we don't get this flexibility. Obviously the current solution does not break this flexibility but if we enforced this type of dependency management in core we would have this issue. And what happens if there is a webform_patchedNG module?

@alexpott: as maintainer of a module it is my decission on what module my module depends. I'm testing with php:php and not with random:php_patchedNG or any other. I need to make sure that only compatible dependencies are installed and not any random modules with the same name just for the reason that an idiot created a duplicate project and have not helped me fixing bugs in my module. I have test failures in google analytics that need to be fixed and I cannot workaround. This makes it impossible to share patches in queue as every "needs review" status ends with a testbot failure and a "needs work" status in *every* issue! This makes project issue queue and automated testing useless and failing in 100%. Is this what we want?

This is really a good solution and we do not force anything. Maintainers can also add test_dependency key with "php:php" and dependency key with "php" only to allow these suxxx flexibility described by you in the info files. I cannot and will never support random module integrations like webform_patched and webform_patchedNG if my module is only supported with webform. The others are outside the game. The developers of the random modules may have not understood how we maintain modules in this community and that we *fix* modules and do not create projects like webform_patched. This only suxxx. We are not on wordpress site where you have no idea what google analytics module you should download out of ~10-20 available ones. These theoretical flexibility is a lot less our problem than the issues we really have.

Independed of the issue here I would force a policy. Unpublish and delete such duplicate projects on d.o. But this is all OT here.

I'm reclassifying this as a 'task' as it will help address multiple issues for Testbot, Drush, the packaging scripts, the project pages on drupal.org and Drupal's module installation page. I'm also moving this back to 'needs review' because I don't feel like alexpott's questions have been answered appropriately. I certainly appreciate all the work that has gone into this patch, but I also feel it could use a bit more discussion based on alexpott's feedback. Thanks everyone!

This issue, from a Drush perspective.

1. Drush will attempt to automatically download a module "foo" when it sees a module "foo" in the dependency list of some other module "bar" (during `drush pm-enable bar`) if there is no "foo" in any downloaded module in the current Drupal site. This patch will help Drush in instances where "bar" wants the "foo" module from the "foo_patched" project. The current workaround is to just manually download "foo_patched" before enabling "bar".

2. Drush has a larger problem when it needs a module "foo" that only exists in some project that is not named "foo". Some folks like to put the "baz_bob" module into the "bazbob" project, for example; less commonly, someone has a dependency on a submodule of a project without depending on the primary module (which is named after the project).

This proposal could help Drush out quite a bit to the extent that folks use it. From a Drush perspective, I am not concerned that "bar" might require "foo:foo" when the end user might prefer "foo_patched:foo"; Drush could simply treat the project name as advisory, and print a warning to the user if it decided to use "foo_patched:foo" ("foo_patched" module already downloaded) instead of the "foo:foo" that was requested (foo module not downloaded).

I don't think Drush will correctly handle downloading two differently-named projects that contain the same module name(s), but that is a Drush issue that does not affect this task. Drush could also really benefit from a modulename-to-projectname mapping service, for existing modules without the project info in the dependency list, but that is also a Drush issue that is perhaps orthogonal to this request (since knowing the project names does not help the testbot select which one should be used).

In conclusion, then, I think that this enhancement is a net win for Drush, and I for one would be interested in seeing it adopted if it meets the needs of other interested parties.

@alexpott: What exactly is not already answered here?

We should be adding a test module with a missing project dependency so that we can test the new behaviour. Much happier now that module page is reporting the missing dependency - but this needs testing. Also since the UI has new behaviour it'd be nice to have a screenshot of this working.

1. +++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
   @@ -27,7 +27,11 @@
   +   *   - (>x.y): Version requirement with operator and version. (Optional)
   +   *   Example: drupal:system (>=7.22)
   

   Maybe we should suggest a more complex pattern having also comma, telling that the version requirement can be more complex?

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -217,7 +217,8 @@ public function buildModuleDependencies(array $modules) {
   +          $graph[$module->getName()]['edges'][$dependency_data['module']]
   +            = $dependency_data;
   
   +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -355,50 +356,78 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +            $row['#requires'][$module_name]
   +              = $this->t('!module (<span class="admin-warning">mismatch</span>)', array('!module' => Xss::filter($name)));
   ...
   +            $row['#requires'][$module_name]
   +              = $this->t('!module', array('!module' => Xss::filter($name)));
   

   This an unusual way to wrap lines. Better leave them on a single line.

3. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -664,6 +665,12 @@ protected function verifyImplementations(&$implementations, $hook) {
   +    if (strpos($dependency, ':')) {
   +      list($project_name, $dependency) = explode(':', $dependency);
   +      $value['project'] = $project_name;
   +    }
   

   In the unlikely case of having $dependency equal to :views_ui the if (...) {...} block will not be executed because strpos() will be 0, leaving the $dependency dirty, with a colon in the first place.

4. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -355,50 +356,78 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +        $project = l(Unicode::ucfirst($dependency['project']), 'https://drupal.org/project/' . $dependency['project']) . ':';
   ...
   +          $project = l(Unicode::ucfirst($dependency['project']), 'https://drupal.org/project/' . $dependency['project']);
   ...
   +          $project = l(Unicode::ucfirst($dependent['project']), 'https://drupal.org/project/' . $dependent['project']) . ':';
   

   Use $this->l() instead l() — I think is available in the class.

   Maybe www. on d.o URL?

#2. yeah, should be one line.

#3: I do not think we need to handle this as this is not supported and in this case it should break intentionally.

This suxxx every day more. #2061157: Test stuck in queue. Please commit!!!

This replaces l() with $this->l().

Can we get this committed now, please? RTBC this again.

+++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
@@ -27,7 +27,11 @@
+   * - dependencies: An array of strings of the form project:module (>x.y).

This comment doesn't agree with the issue summary: is the project: part optional or not?

Also, AFAIK, current policy is that an unpublished change record must be written before the patch can be committed.

This issue was a major task now critical. To evaluate this and whether we should make this change during the beta, we need to update the issue summary according to https://www.drupal.org/core/beta-changes to explain what improvements it makes and what the disruption of the change would be. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

Also considering #91 contains no justification for the criticality according to https://www.drupal.org/core/issue-priority demoting back to major.

@Joachim: please read the case first. As noted several times "project:" is optional. A change notice for optional stuff seems not required from my point of view.

If it's optional, then the documentation in the patch is wrong.

What does the words "Recommended for uniqueness" in the patch tell You?

For me this means it is recommended, but not required and therefore optional.

+++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
@@ -27,7 +27,11 @@
+   * - dependencies: An array of strings of the form project:module (>x.y).

This bit says it has to be of that form. (And while we're at it, the format should be quoted.)

+++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
@@ -27,7 +27,11 @@
+   * - dependencies: An array of strings of the form project:module (>x.y).
+   *   - project: Project shortname. (Recommended to ensure uniqueness)
+   *   - module: Module shortname within the project. (Required)
+   *   - (>x.y): Version requirement with operator and version. (Optional)
+   *   Example: drupal:system (>=8.22, <8.28)

This is more than clear. Can you read the details and tests, please?

> + * - dependencies: An array of strings of the form project:module (>x.y).

That is untrue, because you can have a string of the form just 'module', which is not the given form. I feel I'm banging my head on a brick wall here.

That is untrue, because you can have a string of the form just 'module', which is not the given form.

Can you read the other 3 lines, please? These explain what you are looking for. If you ignore them you cannot understand the full picture. The full documentation of how the info strings may look like are on a d.o documentation page. We cannot cover all these complexity and examples in 80 chars inline documentation.

I feel I'm banging my head on a brick wall here.

If I only read the first line and ignore the 3 other lines than maybe.

https://www.drupal.org/node/542202#dependencies is where the documentation goes after this has been committed. All previous syntax will be supported plus the new syntax with project name.

Yes, I can read them.

+++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
@@ -27,7 +27,11 @@
+   * - dependencies: An array of strings of the form project:module (>x.y).
+   *   - project: Project shortname. (Recommended to ensure uniqueness)
+   *   - module: Module shortname within the project. (Required)
+   *   - (>x.y): Version requirement with operator and version. (Optional)
+   *   Example: drupal:system (>=8.22, <8.28)
    * - package: The name of the package of modules this module belongs to.

Ok, so a developer who doesn't know Drupal will see that the 'project' part is optional, and therefore write ':module' with the colon...

Look, all that's going to happen is that someone is going to file a documentation bug on this at some point in the future, and we'll shake our heads and sigh at patches that get in without clearly-written documentation :/

> All new code should have the project name supplied.

What about custom modules that are in a site's codebase and have dependencies on other custom modules? Those are not on d.org and therefore don't have projects.

> Once this is committed I do not want to spend an entire day tracking down a problem with the testbots only to have to tell them they should supply the project name in their dependencies. It is a waste of my time and their time.

That's a very good point. How about we word it to say that modules hosted on d.org must include the project: prefix?

1. As per #93 we still need a beta evaluation completed.

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -224,7 +224,7 @@ public function buildModuleDependencies(array $modules) {
   -          $graph[$module->getName()]['edges'][$dependency_data['name']] = $dependency_data;
   +          $graph[$module->getName()]['edges'][$dependency_data['module']] = $dependency_data;
   
   @@ -655,9 +661,8 @@ public static function parseDependency($dependency) {
   -    $value['name'] = trim($parts[0]);
   +    $value['module'] = trim($parts[0]);
   
   +++ b/core/modules/system/system.install
   @@ -528,7 +528,7 @@ function system_requirements($phase) {
   -        $required_module = $requirement['name'];
   +        $required_module = $requirement['module'];
   

   This change is a shame because then the dependency information has nothing that matches the Extension::getName() method. And it makes the current dependency code more difficult to share with themes in future. I'm not sure that this change is really necessary.

3. +++ b/core/modules/system/css/system.admin.css
   @@ -167,6 +167,9 @@ small .admin-link:after {
    .admin-missing {
      color: #f00;
    }
   +span.admin-warning {
   +  color: #fb0;
   +}
   

   No need for the span also the use of colour here should be UX tested. Warnings elsewhere have a different colour.

4. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -374,50 +375,79 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +        $url = Url::fromUri('https://www.drupal.org/project/' . $dependency['project']);
   +        $project = $this->l(Unicode::ucfirst($dependency['project']), $url) . ':';
   ...
   +          $url = Url::fromUri('https://www.drupal.org/project/' . $dependent['project']);
   +          $project = $this->l(Unicode::ucfirst($dependent['project']), $url) . ':';
   

   I don't think we should be assuming the project is stored on drupal.org. Removing this will reduce the amount of change on the ModulesListForm since all the change to use Xss::filter can be removed.

5. +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -374,50 +375,79 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +            $row['#requires'][$module_name] = $this->t('!module (<span class="admin-warning">mismatch</span>)', array('!module' => Xss::filter($name)));
   
   +++ b/core/modules/system/src/Tests/Module/DependencyTest.php
   @@ -14,6 +14,49 @@
   +    $this->assertRaw('Requires: <a href="https://www.drupal.org/project/project_namespace_test">Project_namespace_test</a>:System project namespace dependency (<span class="admin-warning">mismatch</span>)', 'Project mismatch warning works.');
   ...
   +    $this->assertRaw('Requires: <a href="https://www.drupal.org/project/project_namespace_test">Project_namespace_test</a>:System project namespace dependency (<span class="admin-warning">mismatch</span>)', 'Project mismatch warning works.');
   

   How is the user going to know what mismatch means?

Re #108 afaik drush will already download missing dependencies when you try enable a module with missing dependencies.

#4. That is true. project status url = "http://example.com/release-history" could be set in .info files and than the download is not on d.o. This is possible. As I know the project URL is available in update status module and can be for sure outside of d.o.

Re #108 afaik drush will already download missing dependencies when you try enable a module with missing dependencies.

I do not think that this will work any longer for the php project as it was moved outside of core.

I wonder if this patch is just trying to do too much. The problem space especially for the UI is extremely complex and this patch only adds a warning that probably will only serve to confuse the user since it does not prevent them from installing the module.

Given that potentially a module could be provided by any number of projects and those individual projects be depended on in conflicting ways, what we are really after is to know where to get the dependency from if it is not the expected place. For example, if a module depends on the webform module in webform_patched we need to know to download http://www.drupal.org/project/webform_patched. But it is possible for another variant to exist as well ie. http://www.drupal.org/project/webform_patched_better and other modules depend on that :(

Another issue is that the project information is only added by the packaging script so if you use git or drush with git deploy your module's info.yml files will not contain the project key. So whether or not the UI works will depend on how someone gets their modules. And even if the project is downloaded through a tarball only the top level .info.yml file has the project information added.

The original problem is that testbot (and drush) can not always determine where to get a dependency of a project. We should focus on that and leave the UI issues behind. The test should just ensure that if the project information is present in the dependency information it is ignored by Drupal's module installer when checking dependencies.

+++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
@@ -27,7 +27,13 @@
+   * - dependencies: An array of strings of the form project:module (>x.y).
+   *   - project: Project shortname. (Recommended to ensure uniqueness*)
+   *   - module: Module shortname within the project. (Required)
+   *   - (>x.y): Version requirement with operator and version. (Optional)
+   *   Example: drupal:system (>=8.22, <8.28)
+   *   * Custom modules may not have a project. In this case the project name
+   *     may be omitted (e.g. system (>=8.22, <8.28)).

I think the use of the * here is weird. Just put everything together. Also a bit more text on the subject of uniqueness could be helpful using the examples in this issue.

TBH, if you create a webform module in a new project webform_patched, or use it, then you're asking for trouble. Project names on d.org are unique and modules within projects should be namespaced: if people stuck to that we'd be fine.

The webform_patched project shouldn't exist -- its maintainers should have become branch maintainers for the older branches of webform that they wanted to update.

Another issue is that the project information is only added by the packaging script so if you use git or drush with git deploy your module's info.yml files will not contain the project key.

That is not correct. We are talking about dependency key and not what the packaging does. Packaging does not interfere here at all.

A few if not You only asked for the UI... :-)

Re #118:

1. The project key is used in the UI, is only added by d.o packaging and it can not be guaranteed to be there - this is a problem.
   

   +++ b/core/modules/system/src/Form/ModulesListForm.php
   @@ -374,50 +374,76 @@ protected function buildRow(array $modules, Extension $module, $distribution) {
   +          if (isset($dependency['project']) &&
   +            isset($modules[$module_name]->info['project']) &&
   +            $dependency['project'] != $modules[$module_name]->info['project']) {
   

   It is used here

2. @hass I guess you think #64 is what asked for a UI. All I was pointing out is that the user does not know which webform to install. And they still don't because the webform module dependency can be satisfied by both webform and webform_patched and all they will get is a warning. But yes you are correct that the final patch will probably look more like #61 than what we have now BUT we will have the improved tests ensuring that if the additional project information is in the dependency then it is ignored.

+++ b/core/tests/Drupal/Tests/Core/Extension/ModuleHandlerTest.php
@@ -499,6 +499,7 @@ public function dependencyProvider() {
+      array('drupal:views_ui(>8.x-1.x)', array('project' => 'drupal', 'module' => 'views_ui', 'original_version' => ' (>8.x-1.x)', 'versions' => array(array('op' => '>', 'version' => '2.x')))),

This should be 'name' => 'views_ui'

Also I think we should still include test modules to ensure that having project in dependency information does not break anything.

1. +++ b/core/modules/system/src/Tests/Module/DependencyTest.php
   @@ -14,6 +14,36 @@
   +   /**
   +   * Checks functionality of project namespaces for dependencies.
   +   */
   +  function testProjectNamespaceForDependencies() {
   

   The method should have a public scope. Also the docblock indentation of the first line is incorrect.

2. +++ b/core/modules/system/tests/modules/system_project_namespace_test/system_project_namespace_test.info.yml
   @@ -0,0 +1,8 @@
   +  - drupal:taxonomy
   

   Let's make this dependent on filter so the test does not have to install so many modules.

3. Whilst thinking about this issue I wonder if we should stop trying to cram everything into a single string. We could still support the short format but we could also support a long format like:

   dependencies:
     - webform:
         project: webform_patched
         version: >8.x-1.x
   

   This would have the advantage of making dependency information more extensible in the future.

It would be great if the test can explain what it is testing - ie. that project namespaces in dependencies are ignored. To that end...

+++ b/core/modules/system/tests/modules/system_project_namespace_test/system_project_namespace_test.info.yml
@@ -0,0 +1,8 @@
+dependencies:
+  - drupal:filter

Let's change drupal to something that should obviously be ignored and and more information to the test.

Regarding #127.1 - two wrongs don't make a right.

How is #127.3 expanding the scope? We're adding more information to the dependencies stored in .info.yml - this is just another way. That said this problem does not just affect Drupal 8 so going with a backportable solution probably makes sense.

Also the suggested structure is not easy to port to .info...

dependencies:
  - webform:
      project: webform_patched
      version: >8.x-1.x
  - node

... would be something like...

dependencies[0][webform][project] = webform
dependencies[0][webform][project] = >8.x-1.x
dependencies[1] = node

So possible but hard coding numeric keys is ugly.

It may be a lot easier to implement the more flexible structure for D8 now and backport the feature itself to D7 with a single string variant. I'm not sure how many discussions we will run into about this if we have it different between D8 and D7, but I also think the suggested structure looks cleaner. For future we can make it therefore better, but for the current D6/D7 state of the art we have at least a workaround with the string. Not every maintainer will add the project name to D7, but for D8 we can do better from day one. Should we go this direction now? Will we really get this in?

@hass I've looked at this a bit more. It's just not worth it. We should refactor this in 9.0.x there is too much madness in ModuleHandler::parseDependency()... for instance rebuilding the original version information...

      $value['original_version'] = ' (' . $parts[1];

... a sticking a space on the front. Why would we want that? :(

Got to love Drupal... a later we remove it and the brackets

        $compatibility = drupal_check_incompatibility($requirement, $version);
        if ($compatibility) {
          $compatibility = rtrim(substr($compatibility, 2), ')');

I was asked in IRC to see if I could help with the documentation in this patch... several patches ago.

There are two chunks of documentation, and I think both of them are at best misleading, and in addition, they both have some formatting and punctuation/grammar problems.

a) InfoParserInterface - in patch:

+   * - dependencies: An array of strings of the form project:module (>x.y).
+   *   - project: Project shortname. (Recommended to ensure uniqueness.
+   *     Multiple projects may have modules with the same name which breaks
+   *     automated testing among other things.  Custom modules may not have a
+   *     project name.  in this case just the module name can be specified.)
+   *   - module: Module shortname within the project. (Required)
+   *   - (>x.y): Version requirement with operator and version. (Optional)
+   *   Example for project hosted on drupal.org:
+   *     drupal:system (>=8.22, <8.28)
+   *   Example for a custom module:
+   *     system (>=8.22, <8.28)

The two Example lines are not indented right, and there are grammar/punctuation problems... how about:

   * - dependencies: An array of dependency strings. Each is in the form
   *   'project:module (versions)'; with the following meanings:
   *   - project: (optional) Project shortname, recommended to ensure
   *     uniqueness, if the module is part of a project hosted on drupal.org.
   *     If omitted, also omit the : that follows.
   *   - module: (required) Module shortname within the project.
   *   - (versions): Version information, consisting of one or more
   *     comma-separated operator/value pairs or simply version numbers, which
   *     can contain "x" as a wildcard. Examples: (>=8.22, <8.28), (8.x-3.x).

b) ModuleHandler - from the patch:

+   *   A dependency string, for example 'project:module (>=8.x-4.5-beta5, 3.x)'.

My suggestion for this would be (this needs to be wrapped at 80 character lines):

A dependency string, which specifies a module dependency, and optionally the project it comes from and versions that are supported. Supported formats: 'module', 'project:module', 'project:module (>=8.x-4.5-beta5, 8.x-3.x)'.

Thank you!

So in the second piece:

+   *   A dependency string, which specifies a module dependency, and optionally
+   *   the project it comes from and versions that are supported. Supported
+   *   formats:
+   *     'module'
+   *     'project:module',
+   *     'project:module (>=8.x-4.5-beta5, 8.x-3.x)'

The list of examples should either use list format (see https://drupal.org/node/1354#lists) or the formats should just be in-line separated by commas.

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -633,7 +633,12 @@ protected function verifyImplementations(&$implementations, $hook) {
+   *   the project it comes from and versions that are supported. Supported
+   *   formats:
+   *     - 'module'
+   *     - 'project:module',
+   *     - 'project:module (>=8.x-4.5-beta5, 8.x-3.x)'

Either a missing comma or a misplaced comma. I think the comma is unnecessary.

Also this list looks exhaustive, because of the words Supported formats:. I suggest changing this to Supported formats include:. Or including module (versions). Also mixing project:module with (>=8.x-4.5-beta5, 8.x-3.x) is weird since we're mixing placeholders with real values.

@trobey re #132 there's far too much I in that comment. You seem to be forgetting that Drupal is about we.

The issue summary needs to be updated (and probably the CR) to reflect the fact the including the project dependency information has 0 impact on module installation and dependency calculation within Drupal core code. Since it is totally reasonable to expect the information to have some effect. Also the code documentation should be updated to reflect this too.

The docs formatting in ModuleHandler is still not quite right:

+   *   A dependency string, which specifies a module dependency, and optionally
+   *   the project it comes from and versions that are supported. Supported
+   *   formats include:
+   *     - 'module'
+   *     - 'project:module'
+   *     - 'project:module (>=version, version)'

The - should line up with the previous line. See https://www.drupal.org/node/1354#lists

Thanks! Docs look good to me now. I will leave the rest of the review to others.

Just reviewing this, will see if I can do a reroll of this for D7.

Can we get this patch committed now, please?

All green

Committed 305ed67 and pushed to 8.0.x. Thanks!

Thank you for adding the beta evaluation to the issue summary.

I just tried to add this to Payment 8.x-2.x. The testbot handles it fine, but core seems to have problems with the new format.

The testbot is not yet able to use the new format. The project dependency patch has not been deployed to d.o yet. I guess this is #2047557: Support Drupal 8-style *.info.yml files

Nice - #153 found an existing bug :) see #2410151: _system_rebuild_module_data_ensure_required does not parse dependencies

@trobey: Do you plan to backport this to D7? You know we cannot RTBC our own patches... *hint*

@trobey: I haven't gotten to it yet, feel free to give it a try.

Patch looks good, thanks for the backport

Committed to 7.x - thanks! This looks like a nice improvement.

I fixed the following on commit (the first because it was in the Drupal 8 commit but seemed to just be flat-out missing from the backport, the second I reverted because it wasn't in the Drupal 8 commit and in fact looks inaccurate in a couple ways... we should have a followup issue to add that documentation though):

diff --git a/includes/common.inc b/includes/common.inc
index 91370e5..acef70a 100644
--- a/includes/common.inc
+++ b/includes/common.inc
@@ -7368,7 +7368,16 @@ function drupal_write_record($table, &$record, $primary_keys = array()) {
  * Information stored in a module .info file:
  * - name: The real name of the module for display purposes.
  * - description: A brief description of the module.
- * - dependencies: An array of shortnames of other modules this module requires.
+ * - dependencies: An array of dependency strings. Each is in the form
+ *   'project:module (versions)'; with the following meanings:
+ *   - project: (optional) Project shortname, recommended to ensure uniqueness,
+ *     if the module is part of a project hosted on drupal.org. If omitted,
+ *     also omit the : that follows. The project name is currently ignored by
+ *     Drupal core but is used for automated testing.
+ *   - module: (required) Module shortname within the project.
+ *   - (versions): Optional version information, consisting of one or more
+ *     comma-separated operator/value pairs or simply version numbers, which
+ *     can contain "x" as a wildcard. Examples: (>=7.22, <7.28), (7.x-3.x).
  * - package: The name of the package of modules this module belongs to.
  *
  * See forum.info for an example of a module .info file.
@@ -7662,8 +7671,7 @@ function debug($data, $label = NULL, $print_r = FALSE) {
  *
  * @return
  *   An associative array with three keys:
- *   - 'project' includes the name of the project containing the module.
- *   - 'name' includes the name of the module to depend on.
+ *   - 'name' includes the name of the thing to depend on (e.g. 'foo').
  *   - 'original_version' contains the original version string (which can be
  *     used in the UI for reporting incompatibilities).
  *   - 'versions' is a list of associative arrays, each containing the keys

I created a followup issue for the above-mentioned documentation change, and also updated the change notice for this issue so it applies to Drupal 7 also. Please review: https://www.drupal.org/node/2299747/revisions/view/8461810/8979767

Coming here from #2660992: Due incorrect dependency information, the module can not installed via composer and #2551703: Store composer.json if present.

I know it's a bit late to do this, so my apologies - but I think this should be reverted entirely unless I'm missing something. The entire premise of this change is based on the idea that I should be able to choose the project namespace that the "webform" module comes from, and I don't think that's a valid claim. We encourage people to collaborate on things for a reason: we don't want a zillion different webform projects that do the same thing, except for tiny variances. That's just setting people up for pain. hass had the problem that specifying a dependency on the "php" module was not actually downloading the contrib php module. That sounds like an issue with either the testbot or project_dependency, and really isn't something that core should have to work around. "If there's no PHP module available, then google_analytics can't be installed" is about as far as core should go. With this patch, though, if there is a valid reason to use webform_patched instead of just webform, I can't do that anymore if someone specifies the project namespace.

I'd suggest that a very simple fix here would involve leaving the dependencies key alone, and adding a new key that was something like "testbot_project_dependencies" or something, which would simply be a list of project namespaces (and not module names) to download as part of a test run.

If nothing else, does anyone have an objection to softening the language in the change notice? I don't think all new code needs to specify " - pathauto:pathauto (>=8.1.x)" when a simple " - pathauto" will do. I don't see any reason for encouraging people to do this unless they specifically need a module from another project namespace, or if the project namespace isn't clear (i.e. Features Extra submodules).

There was not a lot of support for doing this other than from trobey and maybe hass, and there was explicit disagreement on that point from chx and maybe others (I read the issue very quickly, so may have missed someone). This needs wider discussion before we say that the more verbose way is the "recommended" way of doing things.

EDIT: Removed some words that didn't make sense after I reordered the points in my comment.

I thought the main reason for this was to support automatically downloading modules that don't have the same name as the project they live in (and not just by the testbot, but also Drush).

For example if you have dependencies[] = date_api in an info file, Drush has no way to know that https://www.drupal.org/project/date is the project it needs to download to resolve that dependency. But if you use dependencies[] = date:date_api then it can.

Following the link from the change notice to https://github.com/drush-ops/drush/issues/1731 it seems like Drush might support that now (since the patch here was committed) but I haven't tested it myself.

I agree with you about the change notice though:

• An example like the above would make more sense to include there than the "webform_patched" example.
• "pathauto:pathauto" sounds redundant to me also.

There is test_dependencies for testing only. I only use this in ga. However I think this is a valid aproach and must not rolled back.

If module should not support webform_patched than it is not allowed to use it. It may be hard, but it's also a way to tell people to collaborate and not fork. It also helps not ending in the wordpress mess. Hard, but maybe intentionally. I personally do not use this (and see it as last resort decission), but I see the reasons for doing this and like it.

The Date/Date API example is the primary reason for this functionality, and I think it's a great improvement as well. The webform_patched example might be an alternative use of this, but not the main reason it was added.

I updated the documentation for writing .info files at https://www.drupal.org/node/542202#dependencies.

I think my major problem is that the change record and documentation seem to indicate that this is the only way to specify dependencies, and that in most cases, like, if you depend on ctools or token modules, this results in additional confusion over 'ctools:ctools' and 'token:token', in addition to modules possibly breaking people using Drupal < 7.40 when attempting to run database updates.

I would assume the new syntax should be used in the cases of date:date_api where project name <> module name, but that is not very clear.

For example, #2753203: Incorrect dependencies format in amp.info

This change in the documentation made sense to me, because it's clearly about ambiguous dependencies, and indicates that it requires Drupal 7.40: https://www.drupal.org/node/542202/revisions/view/9119944/9541547

The change in the above general section about dependencies is what confused me and I think should be reverted: https://www.drupal.org/node/542202/revisions/view/9541547/9556989

I made the following edit to the section on .info files in D7, leaving most of the explanation about project dependencies in the project dependency section, and noting that in most cases, just the simple non-project dependency information is needed. https://www.drupal.org/node/542202/revisions/view/9557153/9800823

Bad news. This patch did not address all of the places in core that introspect the .info files.

Apparently installation profiles *also* check to validate their required dependencies and assume that any module listed in an install profiles .info file becomes a required module, and it parses each listed modules info files, and makes the assumption that the name is the module that needs to be listed.

Please refer to #2855026: Installation profiles do not support project:module format for dependencies .

Re #2983584: Comprehensive solution for potx config project:module format dependencies

also some config translatables are missing now due to this.