Provide optional support for using composer.json for dependency metadata

A scattered, disorganized list of things to consider:

Dependency information in composer vs dependency information in drupal - composer dependencies are considered resolved when a particular module exists on the filesystem, and is part of the autoloader (and meets all versioning requirements), whereas in drupal a dependency isnt resolved unless its actually *installed* in drupal.

Version numbers do not exist in composer.json, so we'd have to rely on vendor/composer/installed.json to determine which versions of software we have available. Drupal.org's packaging system adds the version numbers, but, this doesnt work for dev branches and git repositories - needing things like git_deploy and composer_deploy modules.

Info.yml files have a bunch of keys that we'll probably need to figure out where to put in composer.json:

• version
• dependencies
• test_dependencies
• 'install' (alias of dependencies for install profiles)
• type
• base theme
• engine
• screenshot
• description
• package
• php

Also, hook_system_info_alter is kindof still a thing. maybe it doesnt matter how we store on-disk metadata though.

Drupal core still doesn't have a concept of a 'project' beyond maybe the 'package' in the info files. Thats a drupal.org-ism.

Hi,
I don't know why we needed a new ticket for this, considering that #1398772: Replace .info.yml with composer.json for extensions already exists.
I will not mark it as "Duplicate" yet, because maybe I am missing something :)

I am opposed to such a change, unless we completely change the nature of Drupal modules.
See my comments #58 and #93. #96 in the other issue.

Module info files and module package composer.json files contain different information for different consumers, for different entities (package !== module). I see no reason why they should be combined.

Nope, this is definitely a duplicate.

I'll make some comments on that other issue.

As part of #3009338: [META] Support semantic versioning for extensions (modules, themes, etc) in Drupal core, and allow modules to be compatible with Drupal 8 and 9 at the same time I've been doing a spike on this - I have discovery and info parsing working.

I now need to get version constraint comparison working which will be easier with #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer.

WIP code attached which I'll push along further in the coming days.

More work

This installs, Drupal seems to work, and I can get test runs

Also includes #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer as need that logic.

Also includes a console command to auto convert your info files to composer.json (Cause I wasn't going to do the 400+ in core by hand right!)

usage php core/scripts/drupal upgrade-info-files modules/contrib/token -d

Fixes phpcs issues

Given this is green and there are no info.yml files, its no longer a spike/experiment

Here's a patch without the info.yml->composer.json file changes and #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer

This is ridiculously awesome work. I am thunderstruck.

Also, postponing on #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer, since landing that one first will make this easier to review/commit.

Updating remaining tasks to add this:

---

All the core tests pass because core .info.yml files have VERSION: version
What we need is support for composer.json files without version as we have no way to tell the version.

This will require
a) reading composer.lock for installs using composer
b) d.o to inject the version into the composer.json for tarballs.

Updated the deprecation notice to indicate the actual extension with the old .info file.

Removed dependency parsing for the install key of install profiles, as this was triggering deprecation notices, just in-lined that code.

Correct me if I'm wrong but I don't see how this works in practice for project like commerce. Where you have dependencies like commerce:commmerce_cart - the composer package is drupal/commerce but the dependency is on a sub-module and making the dependency on the commerce module is wrong.

@alexpott, I'm sure you can simply run composer require drupal/commmerce_cart for submodules - documentation.

I wonder, could submodules be replaced by subtree splits?

@pingwin4eg yes but the point of some sub modules is that some people has used the project:module to disambiguate cases where there has been a namespace clash.

Where you have dependencies like commerce:commmerce_cart - the composer package is drupal/commerce but the dependency is on a sub-module and making the dependency on the commerce module is wrong.

I think the composer facade covers that, you can run composer require "drupal/actions_permissions" which is a submodule of views_bulk_operations.

yes but the point of some sub modules is that some people has used the project:module to disambiguate cases where there has been a namespace clash.

Can you give an example of this outside tests, e.g. I know contact_storage has a test module that clashes with one in core (it came to core from the contrib module). I think we'd have a serious issue if this was widespread.

Working on failing tests locally, ModuleListForm is still looking for the core key, which is part of the motivation behind this work - unlocking modules to work on d8 and d9 at the same time

I wonder, could submodules be replaced by subtree splits

I think modules would need complete refactoring, then.

As far as I'm aware, namespacing the dependencies was mostly to work around the Composer facade due to modules not having a composer.json

Submodules generally always require the root module. Well, not Devel which provides Kint. But maybe we have the root module in `require` and Drupal module dependencies declared in `extra`

I am really excited about doing away with .info.yml in favor of composer.json — but maybe I'm misunderstanding the implications of the proposal to replace submodules with a subtree split. It sounds like replacing submodules with a subtree split would mean that I have to maintain the submodules as if they were modules of their own.

But my ideal developer experience would be to arbitrarily add, rename, and remove submodules without the formality of having to "create", "move", or "abandon" them, because most of the submodules I end up writing don't really make sense to be their own project — most of the submodules I create fit into one of the following categories:

1. The main module is an API that is (mostly) useless on its own, and the sub-modules are the default implementation(s) of an API defined in the main project (e.g.: neither clean_markup, d3_sankey make sense to use on their own without the default implementation in the submodule)
2. The main module is an API that is (mostly) useless on its own, and the sub-modules are meant to be "examples" demonstrating how to use the API but no practical uses exist because those would vary depending on your project (e.g.: scanner_fixer_api is an API to help you automate fixing problems; but I can't predict what problems exist on your site so I can't write a default implementation for you)
3. The sub-modules are hidden modules used for automated testing (IIRC no public projects of mine yet; but find . -name '*.info.yml' | grep 'test' in drupal core)

Thank you for your patience with me as I try to understand that part of the proposal!

This also needs #2024043: Add Module, Theme, Profile, and Extension value objects to be less ick - ModuleListForm should not know how to calculate if a module has dependencies or can be installed 🤮

This will require
a) reading composer.lock for installs using composer

Instead of reading composer.lock to find version information, could we use something like https://packagist.org/packages/jean85/pretty-package-versions or https://packagist.org/packages/ocramius/package-versions?

I posted an issue to ask about LTS for PHP 7.0 https://github.com/Ocramius/PackageVersions/issues/86

Re #26:

It sounds like replacing submodules with a subtree split would mean that I have to maintain the submodules as if they were modules of their own.

Thats not what it would mean.

A subtree split allows the maintainers to have one issue queue, and one git repo that they maintain for their project, and it would automatically get split into smaller git repos/releases that composer can use.

We currently do this with the core components, and this is also how symfony manages their repository as well.

@Mixologic, ah, perfect, thank you for the clarification. Sounds fine to me.

{
    "type": "drupal-module",
    "name": "drupal/aggregator",
    "extra": {
        "drupal": {
            "name": "Aggregator",
            "package": "Core",
            "configure": "aggregator.admin_settings"
        }
    },
    "description": "Aggregates syndicated content (RSS, RDF, and Atom feeds) from external sources.",
    "version": "VERSION",
    "require": {
        "drupal/core": "~8",
        "drupal/file": "*",
        "drupal/options": "*"
    }
}

So here's an example composer.json from the patch. Looking at it I'm pondering a couple of things... not show stoppers just things to think about.

1. The description being part of the root but the root name being "drupal/aggregator" and having to put the Drupal UI name in the extra Drupal dumping ground.
2. The use of the word package in the drupal extra dumping ground - this feels problematic as Composer is a package manager...
3. I'm also pondering the developer experience of changing / adding keys to the drupal extra area - at least with the yaml everything was a root key value or a sequence which is a bit simpler - maybe we need a cli for this.
4. For install profiles the format of the install key and the require key is totally different - and you have the problem of them not being next to each other - tricky.

How are we looking at handling things like global styles/javascript or even regions?

I was kind of enjoying the consistency we were starting to see with Modules and themes using similar language in .info.yml. I think if we are looking at doing this, then it should be done in the theme layer as well to at least keep things consistent.

Another Q on this: is there a way to extend the composer.json schema (https://github.com/composer/composer/blob/master/res/composer-schema.json) to validate "drupal" options under "extra"?

The use of the word package in the drupal extra dumping ground - this feels problematic as Composer is a package manager...

Yeah, I feel like the info.yml is useful for this kind of info but not declaring dependencies.

The extension name, type, and version could be extracted from Composer but keep the info.yml for Drupal-y metadata.

I haven't read #1398772: Replace .info.yml with composer.json for extensions fully so perhaps someone can provide a TL;DR; here before we have duplicate discussions in both issues. It's really cool that you've created a working implementation for this already. However I feel that we don't have consensus yet. Personally I'm not convinced and think this could break quite a few things.

An example of where this would introduce a namespace clash would be in the Open Social profile. It uses module names that also exist as projects on Drupal.org for various reasons. This currently just works but I see this horribly breaking if we put all dependencies under the drupal/* namespace.

I also very much dislike the mingling of a module and a package definition. Sure we can put this in the "extra" key of a composer.json but for an installation profile that provides a lot of its own modules that don't make sense outside of the profile this would create one hellish composer.json file.

What would happen with modules contained in a subsplit? For example in Open Social there is the social_event module which according to this new scheme would be a tree subsplit of the social project also available under drupal/social_event. That module itself has some optional related functionally which exists in submodules. Would this then be a nested subsplit, does that work?

In the other issue I also see that the lack of comments in composer.json is listed as a minus. I think the step backwards that this would be is quite big. Documenting an .info.yml file with why some choices were made or why some things were omitted or even what a certain key is used for can be tremendously helpful especially for newer beginners. I've already spent quite some time using composer, trying to find the right documentation on the internet for an extension that used the composer extra field to figure out what that value meant and why it was the case.

EDIT: Another question that I thought was not an issue but now does seem to be one. It was also referenced here: https://www.drupal.org/project/ideas/issues/1398772#comment-13084116

In the above scenario, how would the social_event module ensure that its optional modules get downloaded but not necessarily installed. It can't put them in the composer.json require key because that would cause them to be installed (correct?). Would these need to be hoisted to the install profiles require key because it has a separate install key? That feels like it would be unclear and could cause issues if it's ever decided to spin the package out of the install profile.

Since the benefit of using composer.json is for dependencies, why not move dependency logic into composer. And deprecate it from .info.yml. Then we could keep the info details somewhere else. Maybe move them into another yml file or files that is less of a dumping ground. For example, over in #2936365: Migrate UI - allow modules to declare the state of their migrations we've been working on adding a yaml file describe migration status. But the original plan was to dump it into the info.yml file, for that reason. Don't use it for dumping. Themes could have their "own" yml file; modules another, etc. Then we could have a tight contract between yaml and a schema. We've already started that trend with libraries.yml, workflows.yml, etc, etc, etc. Should we continue it?

If you're replacing .info.yml files with composer.json for both modules AND themes, then that heavily implies that themes would be able to declare dependencies on modules, which currently isn't the case (#474684: Allow themes to declare dependencies on modules). Right now themes only have the ability to set a single base theme, which isn't really what composer.json is designed to do in terms of declaring a dependency. There are a lot of other important keys in info.yml for themes, and putting those into composer.json feels like putting square pegs into a round hole. That's not really what Composer is for, right?

Maybe I'm missing something, but it feels like we could really use a good summary of here's what's great about using composer.json files for modules and themes and what sort of information is good to put into composer.json. And then perhaps look at what sort of information goes into info.yml for modules and themes that *doesn't* fit into that category and look at what are the best options for how to deal with that. Does putting that into composer extras really make sense? Does keeping around an info.yml file for just those bits make sense, or does that just cause confusion? Or is there a new .yml file that would be a better spot, but that would make deprecation and conversion more difficult?

Maybe I've missed these discussions, so if I'm suggesting something too basic, my apologies.

#35 (@Kingdutch)

However I feel that we don't have consensus yet. Personally I'm not convinced and think this could break quite a few things.

My feeling as well.

This needs a consensus and well-documented spec before and if we want to go ahead.

Maybe I'm missing something, but it feels like we could really use a good summary of here's what's great about using composer.json files for modules and themes and what sort of information is good to put into composer.json.

The composer.json has to contain all information that is consumed and required by Composer for its installation and package dependency resolution process.

The info.yml has to contain all information that is consumed by Drupal core when scanning available modules that can be installed or uninstalled.

Combining these two files means that both Drupal and Composer will have to process some information they do not care about.

There is some overlap of course:
Package dependencies and Drupal module/extension dependencies are technically different.
But in many cases they will be the same information in different format, or one will be a subset of the other.
Exceptions:
- submodules. These were discussed above. Someone suggested subtree split, in Drupal 7 Composer integration these become meta packages.
- dependencies to non-Drupal packages. This means in one "require" list, anything marked as drupal-module also needs to be installed in Drupal itself, whereas anything without that only needs to be downloaded by Composer as a package.

From a DRY perspective I can see the urge to combine the two files.
From a "separation of concerns" perspective, I am rather skeptical.

If we would generate one file from the other, with an option for hard override, this would make me happy on both fronts. But we are already doing that I think :)

Thanks everyone for their feedback - and for @Gábor for promoting it so we got some more eyes on it

So in light of this we've shown that we could remove info files and replace them with composer.json. But this doesn't mean we should - because as the patch shows, we're stuffing a square peg in a round hole in some cases - evidenced by the extra key entries.

So let's get back to the issues we're trying to solve here:

- core: 8.x in an info file is restrictive
- we invented our own dependency syntax
- we want to move away from having the major version in the module version (e.g. 8.x-1.x » 1.0.0)
- we want to move core away from version: VERSION so we can do subtree splitting
- we want contrib modules to be able to declare their composer dependencies - e.g my module 1.3 needs symfony/yaml 4.0
- as a bonus, themes could rely on modules

So after some discussions in slack - it makes sense to use composer.json for

- discovery of themes/modules/profiles/engines
- dependency declaration
- version numbering

Everything else belongs in info.yml files and is bending composer to suit our means.

So re-titling and updating issue summary.

Shouldn't also the test_dependencies key be deprecated in favor of Composer's require-dev?

Shouldn't also the test_dependencies key be deprecated in favor of Composer's require-dev?

Agree, however there is no runtime code in core for that (unless I'm mistaken), its all part of the test-bot

The test_dependencies key is parsed by drupal.org and converted into require-dev for the composer facade. *however* its somewhat hacky, and would be much, much better if test_dependencies was also moved into require-dev in the composer.json.

I strongly favor keeping the composer data as "metadata for dependency managment" and all other metadata in the module.yml/theme.yml file (er well, module.info.yml/theme.info.yml) +1

Some thoughts: Name spacing is going to be a challenge.

1. Dependencies must be properly namespaced. The current system allowed for really weird things like "as long as there is *some* module named views, the dependency is satisfied". Which may have been an expedient way to do some things in the past when dependencies were simpler, the way composer accomplishes that is with the replace key.

2. That means that currently, there are several modules throughout the ecosystem that have the exact same name, most often 'submodules' found within projects. many are things like a 'defaults' module or test.module etc.

3. There is no concept in core of a 'submodule', nor is there one on drupal.org. We mentally acknowledge subordinate/supporting modules in a project's folder structure and refer to them as such, but theres no true indicator as to what is or isn't a submodule.

4. There is no concept in core of a "project", but thats what we host on drupal.org, and thats what the version numbers are tied to, and thats what is actually released, which in turn is what a dependency relies on. The only place core interacts with project names is when it looks at the info.yml files and sees the project:module dependency pattern. Which the only thing is does is discard the project portion. When it checks for updates, it sends in module names, which drupal.org has preconverted into project/module names.

5. The current namespaces are derived from the project information on drupal.org, and the composer facade has some heuristics built into it on how to handle collisions, primarily 'whichever module was most popular when the facade was built, and moving forward, whichever module we see first, gets the 'bare' namespacedrupal/modulename as its namespace'. If there is a collision, then in the future the module gets the namespace of drupal/projectname-modulename

6. Modules do things like start off in contrib, then become experimental modules (i.e. json_api as the most recent example). Modules also get moved out of core and back into contrib. We'll need to address what it means to depend on json_api.

So given that, I would suggest that moving forward, the namespaces should follow the following pattern:
drupal/projectname-modulename *unless* the projectname and modulename are the same, then the name becomes drupal/projectname.

That way if you have a 'rules_support.module' file in your project, your composer.json can say its name is "drupal/my_project-rules_support" and it wont collide with everybody elses rules_support module out there.

So given that, I would suggest that moving forward, the namespaces should follow the following pattern:
drupal/projectname-modulename *unless* the projectname and modulename are the same, then the name becomes drupal/projectname

ok, I'll keep that in mind when this is unpostponed

#46.6: this is exactly what we encountered in JSON:API Extras' dependencies (#3043975: Unable to install JSON:API Extras with core JSON:API using composer) and what we're currently dealing with in #3053700: Impossible to update drupal/core to 8.7.0 with Composer >= 1.7.3 if drupal/jsonapi is installed.

I'm also looking at using ocramius/package-versions in #3054002: Parse project name and version from composer.json.

composer.json implements a third-party data schema.

Here's what the schema says about extra:

extra

Arbitrary extra data for consumption by scripts.

This can be virtually anything. To access it from within a script event handler, you can do:

$extra = $event->getComposer()->getPackage()->getExtra();

In this context, script means a Composer script.

While it's possible to store our metadata within this schema, I'd say it's not something we should do, because it's not our schema. That is, we don't control it.

The list of issues we're trying to solve from #39 don't seem like they need for us to deprecate info files and/or add extra keys to composer.json. The Composer facade is doing all the heavy lifting already for most of the items in that list. It seems like the problem is not with the concept of info files, but with the schema within them.

I'd vote for an info.yml schema v.2.0 that makes everything as easy as possible for the facade to consume, and also for maintainers to maintain. For instance, make it illegal to have a core key. Also, add a schema_version key. This should be formalized so that we can validate info files.

What's the plan for custom modules? This was raised in a Drupal 9 readiness meeting earlier. Since those would be local and not composer-managed, you cannot just add a composer.json to them(?) Let's assume you need to add dependencies on the global composer scope for the project. How would then Drupal about dependencies of custom modules? Would that be handled entirely different from contrib module's dependency declaration?

One thing I have been using for custom modules is the ability to define a local path repository in the root composer.json
https://getcomposer.org/doc/05-repositories.md#path

You don't even have to define a repository for each custom modules since you can use expansions in the path like this:

{
    "repositories": [
        {
            "type": "path",
            "url": "web/modules/custom/*"
        }
    ],
    "require": {
        "drupal/custom_module": "*"
    }
}

This way composer is aware of your custom/local packages/modules

Have you considered this?

All of this seems a bit like throwing the baby out with the bathwater. The solutions to the side effects seem to be adding more complexity, thus restrictions than what the issue originally set out to resolve.

- core: 8.x in an info file is restrictive, remove it from the schema. Modules have been able to add a hook_requirements function for some time. So version checking could/should be handled there.

- we invented our own dependency syntax that is overly complex because of the optional core major version. This can again simply be dropped with the leverage of hook_requirements in modules/themes instead.

- we want to move core away from version: VERSION so we can do subtree splitting. Again, with us removing the core and relegating that into the install file, this can be done away with to allow for subtree additionally, this seems more like an edge case

- we want contrib modules to be able to declare their composer dependencies - e.g my module 1.3 needs symfony/yaml 4.0. We can do this in composer files inside of modules/themes now along with the use of https://github.com/wikimedia/composer-merge-plugin

- as a bonus, themes could rely on modules. This is the only one we can't do today, so adding/extending the theme system to allow for using hook_requirements here would seem like the smaller path to victory

Another thing that could be done would be to extend the modules list page a bit more to allow displaying those defined requirements on that page. This I think would allow for quite a bit of flexibility.

Unless I am missing something, the path of using hook_dependency more to replace the current functionality of dependencies in .info seems like a better way to go.

IMHO, using hook_requirements would be a regression, because we want the Composer do the majority of dependencies resolving.

If we're moving this way, then the easiest way for custom extensions I see is to comply with contribs and to provide minimal composer.json with dependencies only. Are there blockers for this? The core then would use it to check whether the extension could be enabled/disabled. So the same logic could be applied to customs, contribs, submodules, etc. And AFAIK the wikimedia/composer-merge-plugin is not needed for this.

There's also no need to require drupal/custom_something in the root composer.json as long as the extension is in the same VCS repository.

P.S.: In the IS I don't see any replacement solution for the type key. How will the core recognize an extension type then?

It won't be long before the merge plugin is gone from your production Drupal: #2912387: Stop using wikimedia/composer-merge-plugin That doesn't mean you can't add it back into your project as needed.

How will the core recognize an extension type then?

Taken from https://github.com/composer/installers, the following types are used. I don't like the "custom" ones, but they're used for controlling placement of the dependency in the typical "themes/custom" or "modules/custom" directory.

drupal-core
drupal-module
drupal-theme
drupal-library
drupal-profile
drupal-drush
drupal-custom-theme
drupal-custom-module

Dependency namespacing in .info.yml file

@shubham.prakash this patch doesn't seem to address this issue. Setting metadata back to befor #57

Basing this comment off the patch in #18. I didn't reroll it but applied it previous commit to review.

While I like the idea of this issue I am little confused as to why the changes in #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer are needed. This while it gets rid of own version parsing logic it replaces it with a much more complicated conversion of current constraint strings to composer format. I don't think this is actually necessary to deprecate .info.yml files in favor of composer.json files

In #7 @larowlan

I now need to get version constraint comparison working which will be easier with #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer

So is this just to get semantic version comparison working? #2641658: Module version dependency in .info.yml is ineffective for patch releases has much less disruptive change that I think gets it working.

#3004459: [PP-1] Fold in dependency parsing wisdom from project_composer currently will not pass tests after #3066448: Harden test coverage for dependency Constraint class is committed, at least because it didn't cover <> and more than 2 version constraints. Maybe other problems too.
#2641658: Module version dependency in .info.yml is ineffective for patch releases is less disruptive and the additional tests #3066448: Harden test coverage for dependency Constraint class won't be broken.

From my understanding of the patch in #18 \Drupal\Component\Version\Constraint will only be used in \Drupal\Core\Extension\Dependency which is itself deprecated. All composer.json files will use \Drupal\Core\Extension\Dependency\Composer for isCompatible() checks. \Drupal\Core\Extension\Dependency\Composer doesn't need anything analogous to \Drupal\Component\Version\Constraint because its implementation of \Drupal\Core\Extension\Dependency\DependencyInterface::isCompatible() is just

public function isCompatible($version) {
   return Semver::satisfies($version, $this->constraint);
}

Why couldn't we do the minimal changes required to \Drupal\Component\Version\Constraint to get it to work with semantic versioning comparisons as demonstrated in #2641658: Module version dependency in .info.yml is ineffective for patch releases,

I do understand that the new \Drupal\Core\Extension\Dependency::getConstraintString() is used in \Drupal\Core\Command\UpgradeInfoFilesCommand to upgrade info.yml files to composer.json files but we could just move the logic over UpgradeInfoFilesCommand::convertToComposerConstraint.

I think having it in a tool like UpgradeInfoFilesCommand is a lot less dangerous than having in code that is actually going to determine if modules can be enabled.

#9: wow, epic work @larowlan! 😮🤩👏

#32 + #3 + #37 -> +1 for only moving dependency-related things into composer.json and keeping *.info.yml for everything else.

#51's question about how to deal with custom modules has not been confirmed by core composer experts. (#52 answers it, but I'd like to see some confirmation.)

#60:

• RE: adopting composer's constraint format. How can we adopt composer.json-powered dependency handling if we can't parse all possible Composer constraints? The constraint expressions supported by Drupal's *.info.yml are more limited, but if we're going to adopt composer.json, then modules will use more complex constraints, and hence we will need to support it. Or am I missing something?
• RE: <>. Wow. Superb observation! 👏 But can't we just convert <> in *.info.yml to != in composer.json? I feel like I'm missing something, hopefully you can tell me what.
• Oh wait … I think I better understand your earlier question now! I think you're wondering why we're pulling in all of #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer, which includes adding support for evaluating much more complex dependency expressions in *.info.yml, if we are trying to move those expressions over to composer.json anyway?

  But … AFAICT @larowlan already ommitted #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer in his #13 reroll? But it's indeed still present in #18. Is this simply a misunderstanding?

• RE: minimal changes to get Drupal to work with semver comparison: I know that's what you're working on, but that that's not what this issue is trying to achieve. This issue is trying to solve that as a side effectof moving dependency information out of *.info.yml and into composer.json. If I understand it correctly.

1. re #61

   RE: minimal changes to get Drupal to work with semver comparison: I know that's what you're working on, but that that's not what this issue is trying to achieve. This issue is trying to solve that as a side effectof moving dependency information out of *.info.yml and into composer.json. If I understand it correctly.

   Yeah what I was saying was that #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer does much more than is necessary to solve this problem. I think you could do the whole current patch just without #3004459 and replace it with #2641658: Module version dependency in .info.yml is ineffective for patch releases because they both solve the problem.

2. Right now the patch in #18 has comments in it that indicate we would deprecate dependencies in info files by 8.x and removed by 9.0.0. I am not sure if that is the intention because it is getting very late to make such a big deprecation.

   It would also have an affect on a couple other issues.

   #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning
   Because the current plan there is to basically use \Composer\Semver\Semver::satisfies() for the core: key in info files. One of the big advantages there would be it would allow core: ^8 || ^9. But of course if the core key is not supported in info files in 9.x then that is not necessary.

   #2807145: [policy, no patch] Allow contrib projects to specify multiple major core branches(which would be solved by #2313917) would also not be necessary.

   If we plan to deprecate dependencies in info files in the 9.x cycle then #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning would still make sense.

I put together a parent META that summarizes all the thoughts and plans and strategies surrounding our dependency management features. #3069795: [meta] Improve dependency management for extensions

TL;DR - I think we are too late to deprecate anything in .info.yml files. But I also think we can add support for using composer.json's as an optional, additional method that is more feature rich and supports semver. We still need #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning so that existing modules can work with both 8 and 9.

This would essentially mean that *if* you need features like declaring a dependency on a specific version of core, you'll need to use the newer, more feature rich method of composer.json files. So we wouldnt fix #2641658: Module version dependency in .info.yml is ineffective for patch releases for .info.yml files. Nor would we necessarily need #3004459: [PP-1] Fold in dependency parsing wisdom from project_composer except for any bugfix like stuff.

@Mixologic thanks for making the Meta

This would essentially mean that *if* you need features like declaring a dependency on a specific version of core, you'll need to use the newer, more feature rich method of composer.json files.

Actually the patch in #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning would allow core: =8.8.0 or core: ~8.6.7 which allow specific versisons of core. There is test case in \Drupal\Tests\system\Functional\Module\DependencyTest::testCoreVersionDependency() in the patch

You would be able to declare semantic version dependencies for modules though but we could support that in composer.

Ah, sorry. it was late and I misspoke. I meant to say "

*if* you need features like declaring a dependency on a specific version of another module you'd need to use composer.json files for that.

. (assuming that we end up with semver for extensions).

Hid an irrelevant patch.

Agree we're running out of time here and if we can solve the issues listed in the OP in other ways for now, we should do so

Im going to retitle this with a bit more of the plan that has formulated based off of the meeting we had, summarized here: https://www.drupal.org/project/drupal/issues/3069795#comment-13200882

TL;DR - We like to add composer.json metadata as an optional method that maintainers can use to express dependencies and compatibility, and wait until drupal 9 to deprecate their usage in .info.yml files.

We reached a consensus on the call on the following things:

1. Complete replacement of .info.yml files with composer.json files isnt the best path forward. We'd like to use both files, for the things they're most suited for. composer.json files are for dependency management, and .info.yml are for all other extension metadata.
2. It's too late in the 8.x development cycle to deprecate dependency management metadata in .info.yml files, as it affects *all* modules.
3. We still need the ability to support semver for contrib, and advanced dependency declarations that come along with it like ~/^ etc. We'll do that by adding optional support for composer.json.
4. If a contrib maintainer wishes drupal core to use composer.json to determine if its dependencies are met and if it is compatible with drupal core, then the maintainer should explicitly remove the core, dependencies, and test_dependencies keys from their .info.yml files and insted place them into a composer.json file as 'require/require-dev' alongside the modules and submodules in their project.

What this means is that when drupal.org starts allowing semver for contrib modules, if a maintainer needs to express a dependency on a patch/bugfix/security release (i.e. 1.4.1), or they wish to use advanced composer syntax like ^2.3, they would need to opt into using a composer.json.

Given that, I'd like to take the patch in #18, remove any deprecation notices, remove the changes to the existing .info.yml ->composer.json conversions, remove the conversion script, and split those into separate issues, and focus this on solely making it so that drupal allows an alternate, more feature rich path for maintainers to express them in a composer.json and not an .info.yml.

Im also going to go ahead and remove needs release manager review, since 2 of them were on the call and vetted the plan. If we decide they need to review the implementation, we can always re-add it.

Thanks, @Mixologic, for bringing clarity to this issue cluster by creating #3069795.

+1 for the plan in #67, which is an abridged version of the plan at #3069795-14: [meta] Improve dependency management for extensions.

Assigning to myself start a limit reroll. It should be a more limited patch as per #67, for instance we will still always have an .info.yml file.

Here is the more limited patch. This should still be wait till #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning.

Started on changes need for new scope in #67

I have checked a lot of the failing test and ran them locally. They all pass locally so I am retesting that patch

it was the deprecation on \Drupal\Core\Extension\Dependency we can't actually remove uses of it right now.

Fixing test failures

Good start 👍 Here's an initial review.

1. +++ b/core/includes/install.inc
   @@ -1129,17 +1129,38 @@ function install_profile_info($profile, $langcode = 'en') {
   +      // Favour the legacy .info.yml file.
   

   🤓 Übernit: s/Favour/Favor/

2. +++ b/core/includes/install.inc
   @@ -1129,17 +1129,38 @@ function install_profile_info($profile, $langcode = 'en') {
   +    // Modern composer.json file.
   

   🤔 This comment looks like it's misplaced, I think it belongs with the else { a few lines earlier?

3. +++ b/core/lib/Drupal/Core/Extension/Dependency/DependencyInterface.php
   @@ -0,0 +1,37 @@
   +  /**
   +   * Determines if the provided version is compatible with this dependency.
   +   *
   +   * @param string $version
   +   *   The version to check, for example '4.2'.
   +   *
   +   * @return bool
   +   *   TRUE if compatible with the provided version, FALSE if not.
   +   */
   +  public function isCompatible($version);
   

   🤔 This is confusing. "The version to check" — but which version is this? That of the dependee, of the dependent, or of the dependent's constraint on the dependee?

   EDIT: oh wow, this is just lifted from \Drupal\Core\Extension\Dependency::isCompatible() which was added in 8.7.0. 😑 Maybe I'm the only one who finds this confusing?

   In the change record it's somewhat less confusing because it's a concrete example: https://www.drupal.org/node/2756875

   I don't want to derail this issue, but if we're adding an interface for this, I think we need to set the bar for clarity a bit higher. I am hopeful we can fix this with just improved variable names and code docs.

4. +++ b/core/lib/Drupal/Core/Extension/Discovery/RecursiveExtensionFilterIterator.php
   @@ -158,8 +158,8 @@ public function accept() {
   +      // Only accept extension info or composer.json files.
   +      return $name === 'composer.json'|| substr($name, -9) == '.info.yml';
   

   Nit: "only" should be removed now :)

5. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -15,52 +15,111 @@ class InfoParserDynamic implements InfoParserInterface {
        if (!file_exists($filename)) {
   -      $parsed_info = [];
   +      return [];
        }
   -    else {
   -      try {
   -        $parsed_info = Yaml::decode(file_get_contents($filename));
   -      }
   ...
   +  protected function doParseInfoYmlFile($filename) {
   +    try {
   +      $parsed_info = Yaml::decode(file_get_contents($filename));
   +    }
   

   🤔 If we would just keep this file_exists() check, the changes would be much easier to understand, because then HEAD's ::parse() would just be renamed to ::doParseInfoYmlFile().

6. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -15,52 +15,111 @@ class InfoParserDynamic implements InfoParserInterface {
   +  protected function getRequiredKeys() {
   +    return ['type', 'name'];
   +  }
   ...
   +    // @todo Actually handle core constraints
   +    // https://www.drupal.org/project/drupal/issues/2313917.
   

   🤔 Shouldn't we reroll this on top of #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning? That needs to go in first anyway, and that also changes this file. It's not yet clear how the two would interact right now. Specifically in the quoted hunks. If we would make this patch be based on #2313917, we could get that clarity :)

7. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
   @@ -219,8 +219,20 @@ protected function getInstalledExtensionNames() {
   +      if (!empty($module->info['composer_dependencies'])) {
   

   🤔 Hm, this is not actually stored in the *.info.yml file, but we shoehorn it into the existing data structure anyway to simplify the implementation and because the existing implementation is already a legacy spaghetti ball anyway. Is that right?

@Wim Leers thanks for the review

re 6)
Yes rerolling on top of #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning now that is RTBC and hopefully won't big changes

Will address the other points after this.

Not sure if it's needed, but I +1 this issue!

@mixologic I think it may behoove us to revise the problem/motivation. What do you think of this:

Currently, the following problems exist:

• There are two ways to manage dependencies in Drupal: 1) with a bespoke Drupal .info file or 2) with a composer.json file.
• The bespoke Drupal .info file approach has limitations that composer.json does not. For example, Drupal's bespoke approach:
• Does not support semantic versioning
• Does not allow one module to declare compatibility with multiple major versions of Drupal. E.g., 8 and 9.
• Does not allow modules to declare dependencies on third party PHP libraries, like Symfony components.
• Does not allow themes to rely on modules
• Does not locally record version information for dev versions of modules

Proposed resolution

We can solve these problems if we use composer.json, and not .info.yml, for:

• discovery of themes/modules/profiles/engines
• dependency declaration
• version numbering

Addressing other points in #77

1. fixed
2. This logic is not longer valid because I think we are only allowing dependency metadata in composer.json. You will still need a .info.yml file. Fixing
3. This is the dependee version updating.
4. Same as 2) I think you need a .info.yml file still. So removing changes to this file.
5. When I rerolled this in #78 I got rid of doParseInfoYmlFile ()
6. rerolled in #78
7. Yes. I wonder if before we do this issue we should actually do something like
   Module extends Extension

   So we could

   /**
      * Gets dependencies.
      * 
      * @return \Drupal\Core\Extension\Dependency\DependencyInterface[]
      */
     public function getDependencies();
   

   It seems weird that put all this stuff in $module->info

   We could do that in another issue after #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning

   Then in this issue we could introduce ModuleParser which would call both the existing info_parser and a new composer_json_parser service.

   Though you probably won't need composer_json_parser since you could just json_decode(file_get_contents($filename), TRUE)

   and then it would be ModuleParser responsibility to determine if any errors should be thrown because the combo of info file values and composer.json files values is not valid.
   Then the Module extends Extension object would store the dependencies info internally and everywhere else we would get them by $module->getDependencies() and they would be evaluated by \Drupal\Core\Extension\Dependency\DependencyInterface::isCompatible() and we won't have to care anywhere else if they were composer or legacy dependencies.

   Technically we could do ModuleParser and Module extends Extension before this issue if we wanted to make it smaller.

re #79
That lays out the larger problem well but I don't all of that is going be addressed in this issue
For example #2494073: Prevent modules which have unmet Composer dependencies from being installed will be a follow up.
For right now this issue is keeping the ability to use info.yml files for dependencies but in info.yml files but you will have to choose 1 or the other. We won't merge dependencies between the 2.
I think the idea is a lot of the problem you mention will basically only be solved if you choose to use composer.json for dependencies. info files for dependencies will be consider legacy. They won't get new good stuff because the current logic around dependencies is broken in many ways.

See #3069795: [meta] Improve dependency management for extensions for the larger plan without having to go through all the comments in this issue as the scope has changed a few times.

1. #78: Thanks, that made this much easier to understand and review!
2. #80.2+4 🥳(That was going to be my next question! 😀)
3. #80.5: Wow, can we?!
4. #79 to clarify what @tedbow wrote at the end of #80: read only the issue summary, and if something is unclear, point out what that is and ask for the issue summary to be improved.

Patch review

1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -57,12 +57,23 @@ public function parse($filename) {
   +        if (isset($parsed_info['dependencies'])) {
   +          throw new InfoParserException("If the 'dependencies' key is used the 'core' or 'core_version_requirement' key is required in $filename");
   +        }
   

   🤔 This is really interesting. If I understand it correctly, this is saying that if you don't use dependencies: …, that you also don't need core: … or core_version_requirement: …. Is that correct?

2. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -57,12 +57,23 @@ public function parse($filename) {
   +        if (!file_exists($composer_filename)) {
   +          throw new InfoParserException("If the 'core' or 'core_version_requirement' key is not provided a composer.json file is required in $filename");
   +        }
   

   Ah, yes, looks like it is! 👍

   🤔 But … what about modules that are compatible with both Drupal 8.7.x and Drupal 9?

   Shouldn't this logic be version-dependent? Much like #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning also takes great care of ensuring we don't break existing sites.

3. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -167,4 +178,36 @@ protected static function isConstraintSatisfiedByPreCoreVersionRequirementCoreVe
   +   * @return array
   +   *   Parsed info file.
   

   Nit: this is not an "info file".

4. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -167,4 +178,36 @@ protected static function isConstraintSatisfiedByPreCoreVersionRequirementCoreVe
   +      $parsed_info['composer_dependencies'][$name] = $constraint;
   

   🤔 So this is where that 'composer_dependencies' key is set!

   I think it'd be helpful to turn this into a constant, because that makes it much easier to discover all the places where this is being used.

re #81

1. 👍
2. Yep. Fixed. throwing an error if they don't have core info in info.yml and in composer.json drupal/drupal is less that 8.8.0

   made the previous methods from #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning that check for versions before 8.7.7 now take a $version argument so they can used for checking for support before 8.7.7 and before 8.8.0.

3. fixed
4. fixed

Forgot a use statement

Two things:

1.

+++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
@@ -60,7 +144,105 @@ public function parse($filename) {
+      if ($name === 'drupal') {

This should be drupal/core

drupal/drupal is essentially deprecated, and shouldn't show up in composer.json dependencies.

2.

+++ b/core/lib/Drupal/Core/Extension/Dependency/Composer.php
--- /dev/null
+++ b/core/lib/Drupal/Core/Extension/Dependency/DependencyInterface.php

Part of me sees why this is like this, but I think the abstraction model isn't quite right.

Composer.php seems to be more of a variant of core/lib/Drupal/Component/Version/Constraint.php.

It seems like DependencyInterface should actually be a ConstraintInterface.

Constraint has isCompatible, and a __toString() function, but lacks a 'getName()' method -> which I think is the correct model there.

Since we're already discarding the namespace portion of the name in parseComposerFile, so the name on lib/Drupal/Core/Extension/Dependency doesn't really need to be decorated with an additional 'name'

I just re-read your comment on #80.7 and am in full agreement. The number of places we have to check the dependency type is awkward.

EDIT: I had a talk with Ted in slack, posting these just to not lose the ideas.

@Mixologic thanks for the review

re #85

1. fixed
2. I think I agree with this but haven't tried to implement it. Not addressing right now

other problems fixed

1. +++ b/core/includes/install.inc
   @@ -1130,16 +1131,27 @@ function install_profile_info($profile, $langcode = 'en') {
   +
   

   Unrelated change

2. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -11,10 +11,17 @@
   +  /**
   + * The earliest Drupal version that supports the 'core_version_requirement'.
   

   Copied comment should refer to composer.json

3. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -11,10 +11,17 @@
   +  const COMPOSER_DEPENDENCIES = 'composer_dependencies';
   

   Needs a comment

1. reroll after #2313917: Core version key in module's .info.yml doesn't respect core semantic versioning was committed 👍
2. Added more test modules and test coverage
3. Add some @todo's for more test coverage that we have to add in this issue

From a Drupal distribution/profile perspective, how can we maintain the integrity of the dependencies of packages?

Not sure what you are asking? What do you mean by integrity?

For example,

As a distribution/profile maintainer, I want to manage and test every package in the distribution/profile.

The site which uses/implements this distribution/profile shall have to follow the parent package restrictions or centralized dependencies of packages.

- we want contrib modules to be able to declare their composer dependencies - e.g my module 1.3 needs symfony/yaml 4.0
- as a bonus, themes could rely on modules

Can this new feature allow contrib/custom module or theme to require any dependencies defined in the distribution/profile layer and overwrite them?

Ah, yes, I understand now.

So, this particular issue is about the about optionally allowing contrib modules to rely on a composer.json to specify its drupal module and core requirements using composer.json as an alternative to info.yml.

If a custom or contrib module has a specific dependency, that could potentially override a distribution maintainers intention. However, there are methods that can be used to ensure that doesn't happen.

However, this issue doesn't address those methods, so we should probably deal with that separately. Maybe we should open a plan issue in core 's queue so we can ensure this use case is fully articulated.

(it's very similar to something we just solved in the composer initiative so I think we have a good plan)

I'm afk for the next week or so but if there's a new issue, we can discuss there later.

thanks @Mixologic

Just adding more test coverage for installing in kernel and functional tests.

reroll patch #93

rerolled and against 8.9.x now

1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -69,8 +103,21 @@ public function parse($filename) {
   +          $parsed_info += $this->parseComposerFile($composer_filename);
   

   This was in the wrong block so it wasn't working when the module package was testing. This caused a functional test to fail but the unit test for this class. Fixed and added unit test

2. +++ b/core/modules/system/tests/modules/composer_core_incompatible_test/composer.json
   @@ -0,0 +1,7 @@
   +  "name": "drupal/composer_core_incompatible_test",
   +  "type": "drupal-module",
   +  "require":  {
   +    "drupal/core": "^100"
   +  }
   +}
   

   The test composer files are no longer valid since because we run validation on composer files in core.

1. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -56,6 +89,7 @@ public function parse($filename) {
            if (strpos($filename, 'core/') === 0 || strpos($filename, $this->root . '/core/') === 0) {
              // Core extensions do not need to specify core compatibility: they are
   @@ -69,8 +103,23 @@ public function parse($filename) {
   
   @@ -69,8 +103,23 @@ public function parse($filename) {
              $parsed_info['core_version_requirement'] = \Drupal::VERSION;
            }
   

   This means we can't have any test files that use composer.json. This failed the functional tests that where testing composer.json functionatlity
   So added test around this that doesn't set core_version_requirement if it is testing module and it has a composer.json file.

2. I also realized were weren't testing composer.json files that should be ignored because core or core_version_requirement is set in info.yml file.

   I updated all the test methods in InfoParserUnitTest that made sense with dataProvider so the methods could be run with and without a composer.json file. This makes sure we don't get info from composer.json when we shouldn't.

   This is important because we don't want to parse the composer.json unless a developer specifically opts in by removing the keys from the info.yml files. There will be a lot of contrib projects that have existing composer.json files and things could break hard if core suddenly starts to read those.

   @todo We should add a functional test for composer.json file that should be ignored.

Exciting to see this moving forward again! :)

Adding back #474684: Allow themes to declare dependencies on modules as related.

In that issue we are just adding the ability to depend on modules but not specify version constraints. We should determine if composer.json support for dependencies should also be supported by themes. This would allow setting constraints on modules by themes.

@tedbow:

We should determine if composer.json support for dependencies should also be supported by themes.

TL;DR. Isn't this issue about all types of extensions, including themes?

@pingwin4eg before #474684: Allow themes to declare dependencies on modules we had no logic for themes to depend on modules. So there is not logic to stop you from installing a theme if dependencies are not met.

As far as remember and can from looking at the patch there is no logic to prevent you from installing a theme if composer.json dependencies aren't met.

The logic \Drupal\Core\Extension\InfoParserDynamic would apply to themes but that doesn't actually prevent a theme from being installed.

I didn't realize this was a blocker for modules to depend on other modules that are using semver. Yikes! Given the widespread adoption of contrib semver already, the lack of this "feature" is probably a bug that we should consider backporting.

Just gave this a thorough review. Mostly looks okay, although I found lots of small problems in comments, exception text, and other cosmetic things. A few concerns of substance (e.g. version numbers are now invalid, does composer.json or info.yml take precendence?, etc)...

1. +++ b/core/lib/Drupal/Core/Extension/Dependency.php
   @@ -102,16 +95,10 @@ protected function getConstraint() {
   +  public function isCompatible($dependee_version) {
   +    return $this->getConstraint()->isCompatible($dependee_version);
   

   See #3138770-14: Replace incorrect words for Dependee in States API. We shouldn't add "dependee" to core. I believe we mean 'dependency' here. Or maybe just '$version' is good enough for this one.

2. +++ b/core/lib/Drupal/Core/Extension/Dependency/Composer.php
   @@ -0,0 +1,59 @@
   +   * @param $name
   +   */
   

   Missing type and explanation.

3. +++ b/core/lib/Drupal/Core/Extension/Dependency/Composer.php
   @@ -0,0 +1,59 @@
   +  public function isCompatible($dependee_version) {
   +    return Semver::satisfies($dependee_version, $this->constraint);
   

   s/dependee/dependency/ (or something)

4. +++ b/core/lib/Drupal/Core/Extension/Dependency/DependencyInterface.php
   @@ -0,0 +1,37 @@
   +   * @param string $dependee_version
   +   *   The dependee version to check, for example '4.2'.
   ...
   +  public function isCompatible($dependee_version);
   

   s/dependee/dependent/ or something.

5. +++ b/core/lib/Drupal/Core/Extension/Dependency/DependencyInterface.php
   @@ -0,0 +1,37 @@
   +   * Gets dependency name.
   

   Gets the dependency name.

6. +++ b/core/lib/Drupal/Core/Extension/Dependency/DependencyInterface.php
   @@ -0,0 +1,37 @@
   +   * Gets constraint string from the dependency.
   

   Gets the constraint ...

7. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -23,6 +23,39 @@ class InfoParserDynamic implements InfoParserInterface {
   +  const FIRST_COMPOSER_JSON_SUPPORTED_VERSION = '8.9.0';
   

   :( No longer true. Maybe 9.1.0? Yikes.

8. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -23,6 +23,39 @@ class InfoParserDynamic implements InfoParserInterface {
   +  /**
   +   * The key for composer dependencies under extension 'info'.
   +   */
   +  const COMPOSER_DEPENDENCIES = 'composer_dependencies';
   +
   

   I'm not sure what the comment means.

9. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
   @@ -23,6 +23,39 @@ class InfoParserDynamic implements InfoParserInterface {
   +   * This method uses \Composer\Semver\Semver::satisfies() but returns FALSE if
   +   * the version or constraints are not valid instead of throwing an exception.
   

   So there's no way to distinguish between an invalid constraint and something that's not satisfied? Is that a good idea? I guess this is protected, so perhaps this is wise, but it seems dangerous and likely to mask errors. I'll keep reading the patch to see where this is used...

   p.s. I got to the end and I don't see where this is used anywhere in the patch. Is this whole thing dead code?

10. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -23,6 +23,39 @@ class InfoParserDynamic implements InfoParserInterface {
    +   * @param string $constraints
    +   *   The constraints.
    

    If it's a single string, should it be "constraint" (singular)? Even if your constraint has multiple clauses, it's still a single constraint, right?

11. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -56,21 +89,41 @@ public function parse($filename) {
    -          // Non-core extensions must specify core compatibility.
    ...
    +            // Non-core extensions must specify core compatibility.
    

    I think this comment made more sense at the previous location. It's true for both of the specific cases being tested...

12. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -56,21 +89,41 @@ public function parse($filename) {
    +            throw new InfoParserException("If the 'dependencies' key is used the 'core' or 'core_version_requirement' key is required in $filename");
    

    ... key is used, the ...

13. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -56,21 +89,41 @@ public function parse($filename) {
    +            throw new InfoParserException("If the 'core' or 'core_version_requirement' key is not provided a composer.json file is required in $filename");
    

    ... not provided, a composer.json ...

14. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -56,21 +89,41 @@ public function parse($filename) {
    +        if (isset($parsed_info['core']) && !preg_match("/^\d\.x$/", $parsed_info['core'])) {
    +          throw new InfoParserException("Invalid 'core' value \"{$parsed_info['core']}\" in " . $filename);
    +        }
    

    Do we really want this after adding the composer.json stuff? If we're getting an invalid value from composer.json, the $filename is wrong. If this can only be from info.yml, why are we doing it this late in the game?

15. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -56,21 +89,41 @@ public function parse($filename) {
    +        if (static::isConstraintSatisfiedByPreviousVersion($parsed_info['core_version_requirement'], static::FIRST_COMPOSER_JSON_SUPPORTED_VERSION)) {
    

    Not sure what "previous" has to do with this check. This would benefit from an explanatory comment.

16. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -56,21 +89,41 @@ public function parse($filename) {
    +          throw new InfoParserException('Core versions before ' . static::FIRST_COMPOSER_JSON_SUPPORTED_VERSION . " must be specified in the module info.yml in $filename");
    

    "module info.yml in $filename" seems redundant. Isn't $filename already the right info.yml file?

17. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -207,4 +260,36 @@ protected static function getAllPreviousCoreVersions($version) {
    +   * Parses a composer.json file.
    +   *
    +   * @param string $filename
    +   *   The file name.
    

    If this only parses a composer.json file, it seems like "$filename" would more accurately be called "$file_path" or something, no?

18. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -207,4 +260,36 @@ protected static function getAllPreviousCoreVersions($version) {
    +   * @return array
    +   *   Parsed composer.json file.
    

    Maybe we should mention the special handling for 'core_version_requirement' and COMPOSER_DEPENDENCIES here? It doesn't just parse the file, it interprets the 'require' key, too...

19. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -207,4 +260,36 @@ protected static function getAllPreviousCoreVersions($version) {
    +   */
    

    Should also document the @throws in here.

20. +++ b/core/lib/Drupal/Core/Extension/InfoParserDynamic.php
    @@ -207,4 +260,36 @@ protected static function getAllPreviousCoreVersions($version) {
    +      throw new InfoParserException("The require key must at least specify a 'drupal/core' version in $filename");
    

    The 'require' key ... ?

21. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
    @@ -209,8 +209,20 @@ protected function getInstalledExtensionNames() {
         if (!empty($module->info['required'])) {
    

    Totally out of scope, but yikes, this is potentially confusing if we have 'required' vs 'require'...

22. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
    @@ -209,8 +209,20 @@ protected function getInstalledExtensionNames() {
    +      // Modern composer.json file.
    

    "Modern"? ;)

23. +++ b/core/lib/Drupal/Core/Extension/ModuleExtensionList.php
    @@ -209,8 +209,20 @@ protected function getInstalledExtensionNames() {
    +      if (!empty($module->info[InfoParserDynamic::COMPOSER_DEPENDENCIES])) {
    +        $dependencies = array_keys($module->info[InfoParserDynamic::COMPOSER_DEPENDENCIES]);
    

    This is only set for composer.json files that defines 'require' for something other than core. I guess that's what we want here, but just checking.

24. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
    @@ -223,7 +224,12 @@ protected function add($type, $name, $path) {
    +      if (isset($module->info[InfoParserDynamic::COMPOSER_DEPENDENCIES]) && is_array($module->info[InfoParserDynamic::COMPOSER_DEPENDENCIES])) {
    +        foreach ($module->info[InfoParserDynamic::COMPOSER_DEPENDENCIES] as $name => $constraint) {
    +          $graph[$module->getName()]['edges'][$name] = new Composer($constraint, $name);
    +        }
    +      }
    +      elseif (isset($module->info['dependencies']) && is_array($module->info['dependencies'])) {
    

    I thought the intention was that if .info.yml still defines 'dependencies' that should take precedence over composer.json. Isn't this backwards?

25. +++ b/core/modules/system/tests/modules/composer_dependencies_test/composer_dependencies_test.info.yml
    @@ -0,0 +1,5 @@
    +description: 'Support module for test composer dependencies.'
    

    s/test/testing/

26. +++ b/core/modules/system/tests/modules/composer_incompatible_core_composer_depend_test/composer_incompatible_core_composer_depend_test.info.yml
    @@ -0,0 +1,5 @@
    +description: 'Support module for testing system dependencies.'
    
    +++ b/core/modules/system/tests/modules/composer_incompatible_core_depend_test/composer_incompatible_core_depend_test.info.yml
    @@ -0,0 +1,5 @@
    +description: 'Support module for testing system dependencies.'
    

    These descriptions are identical, but the modules are different. Can we make both of these more verbose to clarify the difference and make sure they're not the same description in both cases?

27. +++ b/core/modules/system/tests/modules/composer_incompatible_module_version_dependencies_test/composer.json
    --- /dev/null
    +++ b/core/modules/system/tests/modules/composer_incompatible_module_version_dependencies_test/composer_incompatible_module_version_dependencies_test.info.yml
    
    +++ b/core/modules/system/tests/modules/composer_incompatible_module_version_dependencies_test/composer_incompatible_module_version_dependencies_test.info.yml
    @@ -0,0 +1,5 @@
    +name: 'Incompatible module version dependencies test using composer'
    ...
    +description: 'Support module for testing system dependencies using composer.'
    

    This one also doesn't make much sense. My eyes are starting to glaze over trying to make sense of all these new test modules and what cases they test. Would love for all this to be more clear somehow -- both a more standard naming convention and more verbose names + descriptions for everything.

28. +++ b/core/modules/system/tests/modules/system_composer_dependencies_test/composer.json
    --- /dev/null
    +++ b/core/modules/system/tests/modules/system_composer_dependencies_test/system_composer_dependencies_test.info.yml
    
    +++ b/core/modules/system/tests/modules/system_composer_dependencies_test/system_composer_dependencies_test.info.yml
    @@ -0,0 +1,5 @@
    +name: 'System composer dependency test'
    ...
    +description: 'Support module for testing system composer dependencies.'
    

    And here...

29. +++ b/core/modules/system/tests/modules/system_core_composer_test/system_core_composer_test.info.yml
    --- /dev/null
    +++ b/core/modules/system/tests/modules/system_incompatible_core_composer_depend_test/system_incompatible_core_composer_depend_test.info.yml
    
    +++ b/core/modules/system/tests/modules/system_incompatible_core_composer_depend_test/system_incompatible_core_composer_depend_test.info.yml
    @@ -0,0 +1,8 @@
    +name: 'System incompatible core version dependencies using composer.json test'
    ...
    +description: 'Support module for testing system dependencies.'
    

    I don't see a composer.json in here, so what's "composer" doing in the name?

    Also too vague a description to be helpful.

30. +++ b/core/modules/system/tests/src/Functional/Form/ModulesListFormWebTest.php
    @@ -74,8 +74,7 @@ public function testModulesListFormWithInvalidInfoFile() {
    +    $this->assertSession()->pageTextContains("If the 'core' or 'core_version_requirement' key is not provided a composer.json file is required in " . $path . '/broken.info.yml');
    

    If we fix the message above to include the comma, this will also need to be fixed.

31. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
    @@ -37,6 +37,7 @@ public function testProjectNamespaceForDependencies() {
    +    // @todo Add a composer.json test case.
    

    Yes, please. ;)

32. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
    @@ -62,39 +63,62 @@ public function testEnableWithoutDependency() {
    +    $this->assertRaw(t('@module (<span class="admin-missing">missing</span>)', ['@module' => Unicode::ucfirst('missing_composer_dependency')]), 'A module with missing dependencies is marked as such.');
    

    I thought we don't want t() in test assertions.

    Also I don't think we want the final message text here, we're moving away from those, too, unless they provide essential context to know which assertion is failing.

33. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
    @@ -62,39 +63,62 @@ public function testEnableWithoutDependency() {
    +    // Test that the system_incompatible_module_version_dependencies_test which
    +    // uses an info.yml file for dependencies is marked as having an
    +    // incompatible dependency.
    

    ..._test, which uses an info.yml file for dependencies, is ...

34. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
    @@ -62,39 +63,62 @@ public function testEnableWithoutDependency() {
         $this->assertRaw(t('@module (<span class="admin-missing">incompatible with</span> version @version)', [
    

    If replacing the xpath() test for the disabled checkbox is in scope, why not simplify this to the responseContains() approach, too?

35. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
    @@ -62,39 +63,62 @@ public function testEnableWithoutDependency() {
    +    // Test that the composer_incompatible_module_version_dependencies_test
    +    // which uses a composer.yml file for dependencies is marked as having an
    +    // incompatible dependency.
    

    same here: commas around the inner "which uses..." part.

36. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
    @@ -62,39 +63,62 @@ public function testEnableWithoutDependency() {
    +    // Test that module using an info.yml file with a dependency that is using a
    +    // info.yml with a incompatible core version is marked as having an
    +    // incompatible dependency.
    

    Grammar and consistency problems. Something like:

    Test that a module using an info.yml file with a dependency that is also using an info.yml file that requires an incompatible version of core is marked as having an incompatible dependency.

    Or something... ;)

37. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
    @@ -62,39 +63,62 @@ public function testEnableWithoutDependency() {
    +    $assert_session->elementContains('css', '#edit-modules-system-incompatible-core-version-dependencies-test-enable-description', 'System incompatible core version test (<span class="admin-missing">incompatible with</span> this version of Drupal core)');
    

    Out of scope and not the fault of this test, but the markup we're looking for doesn't make much sense. :( Why does "incompatible with" have it's own span, and why does it have a class of "admin-missing" instead of "admin-incompatible" or something?

38. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
    @@ -62,39 +63,62 @@ public function testEnableWithoutDependency() {
    +    // Test that module using an composer.json file with a dependency that is
    +    // using a info.yml with a incompatible core version is marked as having an
    +    // incompatible dependency.
    

    I think this should be:

    Test that a module using a composer.json file with a dependency that is using an info.yml with an incompatible core version is marked as having an incompatible dependency.

    ? (!) Wow.

39. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
    @@ -62,39 +63,62 @@ public function testEnableWithoutDependency() {
    +    // Test that module using an info.yml file with a dependency that is using a
    +    // composer.json with a incompatible core version is marked as having an
    +    // incompatible dependency.
    

    s/that module/that a module/
    s/a incompatible/an incompatible/

40. +++ b/core/modules/system/tests/src/Functional/Module/DependencyTest.php
    @@ -62,39 +63,62 @@ public function testEnableWithoutDependency() {
    +    // Test that module using an composer.json file with a dependency that is
    +    // using a composer.json with a incompatible core version is marked as
    +    // having an incompatible dependency.
    

    s/an composer/a composer/
    s/a incompatible/an incompatible/

41. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
    @@ -119,27 +119,81 @@ public function providerTestInvalidCoreInstall() {
    +  public function testDependencyInvalidCoreInstall($module, $dependency) {
    

    Don't we need to document these args with @param?

42. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
    @@ -119,27 +119,81 @@ public function providerTestInvalidCoreInstall() {
    +  /**
    +   * Dataprovider for testDependencyInvalidCoreInstall().
    +   */
    +  public function providerDependencyInvalidCoreInstall() {
    

    And/or we should document @return here...

43. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
    @@ -119,27 +119,81 @@ public function providerTestInvalidCoreInstall() {
        * Tests no dependencies install with a dependency with invalid core.
    

    Out of scope, but this comment makes little sense to the uninitiated.

44. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
    @@ -119,27 +119,81 @@ public function providerTestInvalidCoreInstall() {
    +  public function testDependencyInvalidCoreInstallNoDependencies($module) {
    

    @param string $module ... ?

45. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
    @@ -119,27 +119,81 @@ public function providerTestInvalidCoreInstall() {
    +      'info with info dependency' => [
    ...
    +      'composer.json with info dependency' => [
    

    s/info/info.yml/ for all of these?

46. +++ b/core/tests/Drupal/KernelTests/Core/Extension/ModuleInstallerTest.php
    @@ -119,27 +119,81 @@ public function providerTestInvalidCoreInstall() {
    +      'composer.json with composer.json dependency' => [
    +        'composer_incompatible_core_composer_depend_test',
    +      ],
    +      'info with composer.json dependency' => [
    +        'composer_incompatible_core_composer_depend_test',
    +      ],
    

    These are the same filename, but different comments. I think one is a copy/paste error?

47. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -27,6 +28,16 @@ class InfoParserUnitTest extends UnitTestCase {
    +   * DataProvider to create test cases based creating a composer.json file.
    

    A) "Provides data for ..."

    B) Document @return?

48. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -81,8 +92,9 @@ public function testInfoParserBroken() {
    +  public function testInfoParserMissingKeys($create_composer_json) {
    

    @param bool $create_composer_json

49. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -114,8 +128,6 @@ public function testMissingCoreCoreVersionRequirement() {
    -dependencies:
    -  - self_awareness
    

    What happened to this test coverage?

50. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -125,15 +137,14 @@ public function testMissingCoreCoreVersionRequirement() {
    +    $exception_message = "If the 'core' or 'core_version_requirement' key is not provided a composer.json file is required in vfs://modules/fixtures/missing_core_and_core_version_requirement";
    

    This will also need a comma.

51. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -125,15 +137,14 @@ public function testMissingCoreCoreVersionRequirement() {
    -    }
    -    catch (InfoParserException $exception) {
    +    } catch (InfoParserException $exception) {
    
    @@ -225,8 +244,7 @@ public function testCoreCoreVersionRequirement88() {
    -    }
    -    catch (InfoParserException $exception) {
    +    } catch (InfoParserException $exception) {
    
    @@ -266,8 +284,7 @@ public function testInvalidCore() {
    -    }
    -    catch (InfoParserException $exception) {
    +    } catch (InfoParserException $exception) {
    
    @@ -307,8 +324,7 @@ public function testCoreVersionRequirementInvalid($test_case, $constraint) {
    -    }
    -    catch (InfoParserException $exception) {
    +    } catch (InfoParserException $exception) {
    
    @@ -345,19 +362,20 @@ public function testInfoParserMissingKey() {
    -    }
    -    catch (InfoParserException $exception) {
    +    } catch (InfoParserException $exception) {
    

    Aren't all these out of scope?

52. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -144,8 +155,9 @@ public function testMissingCoreCoreVersionRequirement() {
        * @covers ::parse
    +   * @dataProvider providerCreateComposerJson
    ...
    +  public function testTestingPackageMissingCoreCoreVersionRequirement($create_composer_json) {
    
    @@ -155,21 +167,25 @@ public function testTestingPackageMissingCoreCoreVersionRequirement() {
        * @covers ::parse
    +   * @dataProvider providerCreateComposerJson
    ...
    +  public function testCoreVersionRequirement88($create_composer_json) {
    

    @param bool $create_composer_json ?

53. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -331,8 +347,9 @@ public function providerCoreVersionRequirementInvalid() {
    +  public function testInfoParserMissingKey($create_composer_json) {
    

    @param?

54. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -402,8 +424,9 @@ public function testInfoParserCommonInfo() {
    +  public function testInfoParserCoreInfo($create_composer_json) {
    
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +"drupal/core": "^8.8",
    +"drupal/field": "~8.0",
    +"smurfcore/log": "~1.0"
    

    Indent these?

55. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +double_colon: dummyClassName::method
    

    I don't see any assertion dealing with this. We should either add an assertion, or remove this from the input.

56. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +"version": "VERSION"
    

    Isn't the interesting thing about this that it's something which isn't going to override the value in the .info.yml file (since we're using += not array_merge() when we add the parsed values from composer.json)? Should this be more self-documenting in that regard?

57. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +    foreach (['1', '2'] as $file_delta) {
    

    I don't understand what this gives us. We put the same stuff in two different folders and run the same assertions on each copy?

    This either needs a comment, or to be ripped out.

58. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +double_colon: dummyClassName::method
    

    And here

59. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +"drupal/core": "^8.8",
    +"drupal/field": "~8.0",
    +"smurfcore/log": "~1.0"
    

    Indent?

60. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +    foreach (['1', '2'] as $file_delta) {
    +      $folder = "fixtures_$file_delta";
    

    And here -- what's this doing for us?

61. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +   * Tests a composer.json file with invalid 'drupal/core' value.
    

    ... with an invalid ...

62. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +simple_string: 'A simple string'
    ...
    +double_colon: dummyClassName::method
    ...
    +simple_string: 'A simple string'
    ...
    +double_colon: dummyClassName::method
    

    Unused

63. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +      'fixtures1' => [
    +        'common_no_dependencies.info.yml' => $common_info,
    +        'composer.json' => $common_json,
    +      ],
    +      'fixtures2' => [
    +        'common_no_dependencies.info.yml' => $common_info,
    +        'composer.json' => $common_json,
    +      ],
    

    Different pattern, but I still don't understand why.

64. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +    // Set the expected exception for the 2nd call to parse().
    +    $this->expectException(InfoParserException::class);
    +    $this->expectExceptionMessage('Core versions before 8.9.0 must be specified in the module info.yml in vfs://modules/fixtures2/common_no_dependencies.info.yml');
    

    Don't follow why we're doing it like this.

    a) What's different about the 2 calls, other than the filename?

    b) Why have 2 calls at all?

    c) If this is actually needed, can we document why?

    d) If so, can we put the expectException* stuff right before the call we're making that we expect to generate the exception?

65. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +    $this->expectExceptionMessage('Core versions before 8.9.0 must be specified in the module info.yml in vfs://modules/fixtures2/common_no_dependencies.info.yml');
    ...
    +      $this->assertSame('Core versions before 8.9.0 must be specified in the module info.yml in vfs://modules/fixtures1/common_no_dependencies.info.yml', $exception->getMessage());
    

    Version # and formatting will need to be fixed to match above.

66. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +   * Tests composer.json file with 'drupal/core' specified.
    

    "... not specified."

67. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +    $this->expectExceptionMessage("The require key must at least specify a 'drupal/core' version in vfs://modules/fixtures_broken/composer.json");
    

    Will need "The 'require' key..." if we fix above.

68. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +   * Tests that missing required keys are detected.
    ...
    +  public function testInfoParserDependenciesNoCore() {
    +    $missing_composer = <<<MISSINGCORE
    

    Summary, method name, variable name ($missing_composer) and MISSINGCORE don't agree about what this is testing.

69. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +# info.yml for testing missing name, description, and type keys.
    ...
    +type: module
    

    Is type missing or not? ;)

70. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +        'dependencies_no_core.info.txt' => $missing_composer,
    ...
    +    $this->expectExceptionMessage("If the 'dependencies' key is used the 'core' or 'core_version_requirement' key is required in vfs://modules/fixtures/dependencies_no_core.info.txt");
    +    $this->infoParser->parse(vfsStream::url('modules/fixtures/dependencies_no_core.info.txt'));
    

    s/info.txt/info.yml/ ?

71. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +   * Adds a composer.json file to the fixtures.
    ...
    +   * @param bool $create_composer_json
    

    This is kinda confusing. "Adds a composer.json file to the fixtures." unless you pass FALSE, in which case it's a no-op. :/ Maybe this could have a better name?

    Or at the minimum, the summary should say "Conditionally adds..."

72. +++ b/core/tests/Drupal/Tests/Core/Extension/InfoParserUnitTest.php
    @@ -524,14 +567,288 @@ public function testUnparsableCoreVersionRequirement() {
    +  private function addIgnoredComposerJson($create_composer_json, &$fixtures) {
    

    A) Probably wants the void return type now.

    B) What's "Ignored" have to do with it? Oh, it's only used in cases where we're going to ignore composer.json. So maybe it could say so. And perhaps @see self::assertComposerJsonIgnored() about it?

My drupal core version is 8.9.2 and the patch in #101 doesn't apply.

I also tried the other patches for 8.9 and they didn't apply either.

@StevanieV: The patch is supposed to apply to the 9.1.x branch, since that's the newest development focus. This will probably get backported as @dww points out, so the sooner it's in 9.1.x the better.

Unfortunately the patch in #101 doesn't apply to 9.1.x right now, either, so it needs a reroll.

Reroll of #101. Still needs to address #107.

Thanks for rerolling this, @Mile23!

NW for #107. ;)

Probably the most important part of that to address is:

does composer.json or info.yml take precedence?

I'd fix all the cosmetic stuff myself, but then I'm not eligible to RTBC. :/

Hmm, I probably wouldn't actually backport this, or at least I'd want to think about it.

@dww, given the scope of the cosmetic fixes (72 bullets, lol) maybe you could provide a patch/interdiff that cleans up the stuff that's non-controversial? That would indeed mean those changes would need an additional peer review, but I think that'll still be faster than having someone else clean up 70 things. :)

#111 has failing tests. I'm not addressing those, just churning through some of the items in #107.

I made it to #107.54. :-)

Comments:

9: My IDE couldn’t find any uses, so I removed it. I’m new to this code, too, and this just looks like an exception-safe wrapper for Semver::satisfies(), which we don’t need.

10: Looks like the reroll took away $constraints.

14: Still needs to be addressed.

23, 24: Still needs an answer. I’m here to move the issue forward. :-)

26, 27, 28, 29: Really hard to make out what these modules are supposed to demo, so maybe someone who wrote it can rename a few things.

31: Agreed.

32: Other assertions in this test use t(), so we might need it.

34: Still needs to be addressed.

37: OOS, but also yeah.

41, 42, 44, 47, 48, 52, 53: Test methods have different coding standards as of the last time I was doing this kind of stuff (granted, it was 8 months ago…) The code is fairly readable so we don't need to add too much documentation. Some more clarity with those test module names will make it even easier.

46: Eyes glaze over. Still needs to be addressed.

49: Still needs to be addressed.

51: Leaving in because we don’t actually have a CS for try/catch braces, and I’m trying to get through these. :-)

Re: #115: Right, I should have said *might* get backported instead of *probably* will get backported.

Re: #107:

51: Ok, maybe we do have rules for try/catch braces. See CS report for #116.

64: testInvalidCore(): I think we need to call twice for the same file name because the result is cached per file name. It should throw the same exception twice, to make sure we don’t accidentally cache an invalid set of dependencies. This isn’t what the test was testing, however. I changed it.

69: testInfoParserBroken() seems to be testing whether the yaml is well-formed, so I think it’s OK to have required keys so we don’t generate any other exceptions.

14, 23, 24, 26, 27, 28, 29, 31, 34, 46, 49, 55, 56, 57, 58, 60, 63, 68: Still need attention

There are a bunch of failing tests, because, well, they are tests that fail. :-) Once the question in #114 is answered, we can make the tests pass.

Also, pet peeve: The word ‘simply’ should never be in technical documentation.

Here's the interdiff back to #111.

Tagging this with Automatic Updates Initiative because if by Drupal 10 we required dependencies to be only in composer.json files we would not have to check dependencies in a Drupal specific way to determine if all extensions were compatible with the new Core version

Do we intend/hope to do this before Drupal 10? Per #121, it sounds like that would be helpful?

Drupal 8 reached EOL, we no longer need BC for 8.

Re: #123, this is something we would like to see as an API addition at any time. It's not a hard blocker for D10, but it's still a useful improvement for any major it lands in.

Hi,

I tried to convert patch from comment 117 into a MR to more easily update it.

After trying to figure out in more details what it is was doing, I think I will open a separated issue for #3440128: version in info.yml for general project.

In my case, I only want to try to extract versions from Composer if not present in .info.yml to provide site admin versions in admin pages.