Warn site admins when composer dev dependencies are installed inside of docroot

There's at least one other composer-related status page issue.

Here's a patch which makes a best-guess effort at determining whether core's dev dependencies are installed.

This works now, but won't when drupal/core is a subtree split, unless we move all of core's dev requirements to the root project dependencies.

Still to do: Tests.

lazy: bad

+++ b/core/lib/Drupal/Core/Composer/ProjectRoot.php
@@ -0,0 +1,81 @@
+    $installed_json_path = $this->appRoot . '/vendor/composer/installed.json';

What if vendor is outside doc root? As described in Moving all PHP files out of the docroot

If vendor is outside doc root, then the status page won't find any dev requirements in installed.json, and won't warn the user that they have dev requirements even if they do.

We can't handle all the special cases here. Unless we can which I'd love to see a patch for. :-) But if you have a special requirement and need for this status to report the right thing, it's all in services which you can replace. So a composer-based distro which stores vendor somewhere else could include a module to swap out services.

Hello, https://www.drupal.org/SA-2017-001

One of the vulnerabilities documented there is exploiting the existence of phpunit on the production site.

Reroll.

Still need to address the concerns of #9.

What if we just, uh, massage the scope a little? Do we need to warn if they are installed outside of docroot?

Sure, that's why you put vendor/ outside of docroot. :-) If vendor/ isn't where ProjectRoot expects it, it stops trying to figure out the dev requirements.

There are other reasons why you'd want to avoid putting dev stuff on production, such as the giant classmap for phpunit that eats up memory, so we should figure out how to tell people even if the vendor is somewhere else. I'm not sure what the solution for that is.

Added some tests which led to some tighter code in ProjectRoot. Other misc tidying.

Patch still applies, re-running test.

Updated summary.

+    $this->assertArrayHasKey('composer_dev_dependencies', $requirements);
+    foreach(['title', 'description', 'severity'] as $property) {
+      $this->assertArrayHasKey($property, $requirements['composer_dev_dependencies']);
+    }

1. Testbot found:

core/tests/Drupal/KernelTests/Core/Composer/Dependencies/DevDependencyFinderTest.php
line 52 Expected 1 space after FOREACH keyword; 0 found

+    if ($contents !== FALSE) {
+      $json = json_decode($contents, TRUE);
+    }

+    $json = file_get_contents($installed_json_path, FALSE);
+    $packages = json_decode($json);

2. Should the latter also check contents FALSE? Or the former not do so at all?

3. Tests should be marked @internal. And I think DevDependencyFinder should be as well since it doesn't implement an interface like ProjectRoot class.

---

I think that there shouldn't be any issue with adding three file reads in system requirements. The approach looks okay.

.

Re: #19: Fixed CS, added @internal to tests, though I'm pretty sure they don't need it. Added @internal to DevDependencyFinder. There's no interface for DevDependencyFinder because the only variation we want is for people to re-implement the project root finder. Or look for common places in a follow-up.

Rerolled for 8.5.x. Rock on.

I've made a quick look for the last patch (#22) and I think that patch need work.

1. Drupal root and composer root are same only in the default case but this might be changed in future (https://www.drupal.org/node/2385387). Also drupal-project is widely used and this project has a different composer and drupal roots.
So dev-dependencies should be searched only if composer root is the same as drupal root.

2. Vendor dir is 'COMPOSER_ROOT/vendor' in the default case only. Vendor dir can be specified in composer.json file and might be changed by developer in any moment. In this case composer.project_root service won't work as designed.

I suggest to use drupal-finder package for this service. You can check a patch for composer unit tests - https://www.drupal.org/node/2545344#comment-12245148 . I used DrupalFinder class there so it may be helpful for you.

A few different commenters here have talked about arbitrary vendor directories.

We can't cover all use-cases, so for now we can talk about the default location.

That issue #2545344-30: Drupal\Tests\ComposerIntegrationTest assumes there is a composer.json in DRUPAL_ROOT has some work to do, since there are a lot of failing tests. But if the drupal-finder package makes it in within that issue, then we can have a follow-up to modify the Composer services to use it.

Drupal finder allow easily use composer root, drupal root and vendor directory. And it handles the case with custom vendor dir in composer.json. Here is an example:

$drupal_root = $app_root; // $app_root here is a variable from service
$composer_root = $drupal_root;
$vendor_dir = $composer_root . '/vendor';

$finder = new \DruplaFinder\DrupalFinder()
if ($finder->locateRoot($app_root)) {
  $drupal_root = $finder->getDrupalRoot()
  $composer_root = $finder->getComposerRoot();
  $vendor_dir = $finder->getVendorDir();
}

We can't cover all use-cases, so for now we can talk about the default location.

But we can...

As for issue #2545344-30 there was a problem, that aren't related to FrupalFinder, but in any case, thanks for your note

I've updated patch #22 to use DrupalFinder.
Needs review.

Seems like I missed update the tests in the previous patch.
There is new one.

+++ b/core/lib/Drupal/Core/Composer/ProjectRootInterface.php
@@ -0,0 +1,67 @@
+  public function getProjectRootDirectory();

We don't need this since we already have the app.root service.

And really, that's the problem I find with drupal-finder: We don't need to find the app root. In fact, we want to NOT find the app root since we already know it. We want to find composer/vendor root relative to it. We're not allowed to supply app root to DrupalFinder, so it goes about discovering a directory we could have supplied. If its discovery method develops a bug, or core changes such that DrupalFinder can't find it, then we could end up in a situation where we never find a valid installed.json.

So the problem of infinitely configurable composer directories still remains, albeit now with an external library instead of one we can change in core. In fact, even if we were to require composer itself, we couldn't use it to discover all this stuff.

But.... I thought about it over dinner and realized that we have one trick up our sleeve due to the way Drupal does autoloading. There is (supposed to) always be a DRUPAL_ROOT/autoload.php file, even in drupal-project. This file returns an object that is autogenerated by Composer in a consistent way, and we can use reflection to find out where it's located. From that, we will glean the vendor directory.

So here's that version, based off #22.

This version solves the problem of where to find vendor/, but still hard-codes the merge of core's two composer.json files. That's because even if someone supplies a composer.json file somewhere way off in outer space, we still have core's two files, and they still give us enough to determine whether dev dependencies are installed or not, for most cases. Edge cases can be follow-ups.

Note that there really isn't any way to find an arbitrary composer.json file, because a) Composer does not store this information, and b) You can use a composer.json file like this: $ COMPOSER=../../../../../../../someplace/somewhere/composer.json composer install

That feeling when you thought you ran the tests locally again after the last-minute change.

Also, fixing CS errors.

1. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,87 @@
   + * @internal
   

   Awesome! Introducing this as @internal is a great idea, however I think we can do the same for the ProjectRoot class.

2. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,87 @@
   +      if (in_array($dev_requirement, $installed)) {
   +        // @todo: Check version constraints for fewer false positives.
   +        $installed_dev[] = $dev_requirement;
   +      }
   

   Do we fix this @todo in this issue or in a followup? If we do this in a followup we should create one and link to it from the comment.

3. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,87 @@
   +    // If we have the same number of installed dev packages, then tell the user
   +    // that they might need to change their site build workflow.
   +    if (!empty($installed_dev) && (count($installed_dev) == count($dev_requirements))) {
   

   The same number as what?

   Oh, I see it's the same number of installed dev dependencies as required dev dependencies. Why do we do this?
   Shouldn't the empty check be sufficient? I mean, it doesn't matter how many of those deps are installed, even 1 is too many.

   If we decided to keep this check, can we at least use strict equals?

4. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,87 @@
   +      $requirements['composer_dev_dependencies'] += [
   +        'description' => $this->t('This Drupal installation does not appear to have development packages installed. <a href=":documentation">Documentation on drupal.org</a>', [
   +          ':documentation' => 'https://www.drupal.org/docs/develop/using-composer',
   +        ]),
   +        'severity' => REQUIREMENT_OK,
   +      ];
   

   Personal preference: I try to remove as much of the else {}-blocks I can find. This one can be removed by setting it in the initial $requirements array and only overwriting if we find installed dev dependencies.

5. +++ b/core/lib/Drupal/Core/Composer/ProjectRoot.php
   @@ -0,0 +1,116 @@
   +/**
   + * Allows introversion of installed Composer dependencies.
   + */
   +class ProjectRoot implements ProjectRootInterface {
   

   I understand that this is why we created this class, however that's not the actual thing that this class does, it provides information about the project root and where the vendor directory is located.

   Can we update this docblock?

6. +++ b/core/lib/Drupal/Core/Composer/ProjectRoot.php
   @@ -0,0 +1,116 @@
   +   * The full path to the vendor directory for this Drupal installation.
   +   * @var string
   

   Needs an additional newline in between those.

7. +++ b/core/lib/Drupal/Core/Composer/ProjectRoot.php
   @@ -0,0 +1,116 @@
   +  public function __construct($app_root) {
   +    $this->appRoot = $app_root;
   +  }
   

   Needs a docblock

8. +++ b/core/lib/Drupal/Core/Composer/ProjectRoot.php
   @@ -0,0 +1,116 @@
   +    $json = [];
   +    $contents = file_get_contents($composer_file, FALSE);
   +    if ($contents !== FALSE) {
   +      $json = json_decode($contents, TRUE);
   +    }
   +    if (isset($json['require-dev'])) {
   +      return $json['require-dev'];
   +    }
   +    return [];
   

   Very micro optimisation.

   $contents = ...;
   if ($contents === FALSE) {
     return [];
   }
   $json = json_decode($contents, TRUE);
   if (!isset($json['require-dev'])) {
     return [];
   }
   
   return $json['require-dev'];
   

9. +++ b/core/lib/Drupal/Core/Composer/ProjectRootInterface.php
   @@ -0,0 +1,51 @@
   + * Allows introversion of installed Composer dependencies.
   

   Same remark as the other remark about description.

10. +++ b/core/tests/Drupal/KernelTests/Core/Composer/Dependencies/DevDependencyFinderTest.php
    @@ -0,0 +1,60 @@
    + * @todo Make this a UnitTestBase test when it no longer has dependencies on
    + *   install.inc.
    

    Let's do this todo. Should be as simple as changing the baseclass and moving the file.

Thanks, @borrison_

I think this gets pretty much covers everything except #10. We can't do that since the system under test still has a hard requirement on install.inc.

Here's an updated inline docblock, which should give some indication as to why we should be extremely limited in the types of promises we make about Composer in core:

    // If the number of dev requirements and installed dev requirements is
    // exactly the same, then we reason that the user probably issued a
    // 'composer install' command (defaulting to --dev). We then verify this by
    // checking core's dev requirement version constraints. If the version
    // constraints do not match, then either the user has an out-of-date vendor/
    // directory, or contrib has non-dev requirements on the same packages as
    // core's dev requirements, but in different versions. This is highly
    // unlikely, but still doesn't count as core dev requirements. This leaves
    // us with an edge case where contrib happens to require exactly the same
    // packages, with similar version constraints. In this case, it is
    // reasonable to issue a warning for the same reasons we'd issue the warning
    // for core's dev requirements. And we're unable to tell the difference
    // anyway.

This looks good to me, I think this is ready. Not sure if I can RTBC this or if we should wait on more reviews. You can set it back to NR if needed.

Other folks will come along and disagree as needed. :-)

Thanks.

I love this issue, great work @Mile23. Couple of observations

1. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,113 @@
   +  public function buildRequirements() {
   

   Do you think we should cache this with a tag that includes the deployment indicator. hook requirements runs a lot when admins are using the admin areas. I think this is unlikely to change unless a deployment occurs? See \Drupal\system\Controller\SystemController::overview

2. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,113 @@
   +    $installed_dev = [];
   +    $installed_package_names = array_keys($installed);
   +    foreach (array_keys($dev_requirements) as $dev_requirement_name) {
   +      if (in_array($dev_requirement_name, $installed_package_names)) {
   +        $installed_dev[$dev_requirement_name] = $installed[$dev_requirement_name];
   +      }
   +    }
   

   Any reason not to use

   $installed_dev = array_intersect_key($installed, $dev_requirements);
   

3. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,113 @@
   +    if (!empty($installed_dev) && (count($installed_dev) === count($dev_requirements))) {
   

   nit, no need for the !empty here, just !$installed_dev will do

4. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,113 @@
   +        if (!strpos($version_constraint, 'dev')) {
   

   Shouldn't this use strict comparison

   if (strpos($version_constraint, 'dev') === FALSE)
   

   0 would evaluate to FALSE

5. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,113 @@
   +      $requirements['composer_dev_dependencies']['severity'] = REQUIREMENT_WARNING;
   

   I'd make this a requirements error, on security grounds. But that's my personal preference.

6. +++ b/core/lib/Drupal/Core/Composer/ProjectRoot.php
   @@ -0,0 +1,125 @@
   +      $vendor_dir = $this->appRoot . '/vendor';
   +      $autoload = require $this->appRoot . '/autoload.php';
   +      if ($autoload instanceof \Composer\Autoload\ClassLoader) {
   +        $ref_loader = new \ReflectionClass($autoload);
   +        $vendor_dir = dirname(dirname($ref_loader->getFileName()));
   

   I think our future selves would appreciate a comment here.

   It's clear what we're doing here now, but may not be later.

   Suggest something similar to what you have in the comment above

7. +++ b/core/lib/Drupal/Core/Composer/ProjectRootInterface.php
   @@ -0,0 +1,51 @@
   + * @todo Some of this would very well be changed in
   + *   https://www.drupal.org/node/2494073
   

   The comment here doesn't meet our standards. Can we specify what we intend to change in the future?

8. +++ b/core/lib/Drupal/Core/Composer/ProjectRootInterface.php
   @@ -0,0 +1,51 @@
   +   * dev requirements for contrib. It's likely that if core and contrib's dev
   +   * requirements are similar.
   

   this sentence doesn't make sense, I think the word if isn't needed?

9. +++ b/core/tests/Drupal/KernelTests/Core/Composer/Dependencies/DevDependencyFinderTest.php
   @@ -0,0 +1,80 @@
   + * @todo Make this a UnitTestBase test when the systems it tests no longer have
   + *   dependencies on install.inc.
   

   Can we get an issue created for this and linked here.

10. +++ b/core/tests/Drupal/KernelTests/Core/Composer/Dependencies/DevDependencyFinderTest.php
    @@ -0,0 +1,80 @@
    +class DevDependencyFinderTest extends KernelTestBase {
    
    +++ b/core/tests/Drupal/KernelTests/Core/Composer/ProjectRootTest.php
    @@ -0,0 +1,33 @@
    +class ProjectRootTest extends KernelTestBase {
    
    +++ b/core/tests/Drupal/Tests/Core/Composer/ProjectRootTest.php
    @@ -0,0 +1,78 @@
    +class ProjectRootTest extends UnitTestCase {
    

    Great work here.

Thanks.

Good calls. :-) Just a few things:

#38.1: The status of this warning can change on a filesystem change. The only way to reliably invalidate the cache would be to do the discovery again. Similar problem to #1667822: Remove caching of test classes in TestDiscovery

#38.5: If you don't want people to be able to install or update a site with dev dependencies present, then yah, we can make it an error. That means whenever you do dev work you won't be able to install from your codebase unless you say composer install --no-dev. This would prevent running tests until you installed again. Also: BrowserTestBase might need to be able to install Drupal. :-)

#38.7: I'll just remove the comment. We can change things as needed later.

#38.9: @see for new issue: #2909480: Move REQUIREMENT_* constants out of install.inc file

#38.5 - yep you're right, ignore me

Let's get this back to RTBC. #39 resolves everything asked for in #38.

It's recently (okay, six months ago) come to my attention that we overuse REQUIREMENTS_WARNING: see #2880439: Status report requirements should be more defined and #2877128: [META] Evaluate status report warnings and determine which should be reduced to info, removed, etc.. So far the consensus there comes down to it a distinction between whether something might be wrong, versus something being actually wrong that needs to be fixed.

In the case of this issue, 8.2.7 resolved a known/disclosed security issue with a dev dependency. Something was definitely wrong before 8.2.7, but for 8.4.x it might be in the "might be wrong" category. That said, the dev dependencies thing was serious enough to require a PSA, an SA, and d.o infrastructure changes previously, so I'm still leaning toward warning for this one in particular.

Leaving at RTBC while I get a second opinion on it. Thanks!

This is tagged as a blocker, but @Wim Leers provided no explanation when so tagging it. What is it blocking?

I missed where @larowlan said "this is almost an error because it's so serious"; I think that (plus the fact that @cilefen posted it in the first place) is motivation/committer signoff for it to be at least a warning. So +1 for the current approach. As @cilefen said, while we don't know for sure that anything is wrong, "you have been warned."

Hum, the patch is adding a lot more code and API surface than I was expecting for a simple (maybe not totally simple) requirements warning. E.g., do we really need a ProjectRootInterface?

The idea was that this is a soft-ish blocker for #2494073-330: Prevent modules which have unmet Composer dependencies from being installed (300+ comments and in need of issue summary love) because whatever we decide on this issue makes it easier to decide things on that one.

So adding a project root and other services would be a step in the direction of helping resolve whether modules need composer dependencies and so forth. An interface for project root would enable different discovery services as needed.

I'm not sure how all this fits with #2912406: [META] Replace update_manager with a more powerful solution which seems to be the latest plan around composer, AFAICT. We could turn the composer services into a separate issue and link them in to the process on #2912406: [META] Replace update_manager with a more powerful solution

This issue still addresses a security concern, and it's easy enough to adapt to any new discovery method, and we can have it send an error instead of warning.

So, if this includes something we want added as API, let's do so in a dedicated issue that can discuss the short-term and long-term goals, and then just scope this issue to just be adding the message and implementing whatever API was reviewed and added elsewhere. DevDependencyFinder is also a pattern we haven't seen before -- basically building out a hook implementation with a value object. There's something there.

Being able to do this:

+++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
@@ -0,0 +1,107 @@
+    // Get a list of installed packages.
+    $installed = $this->projectRoot->getInstalledPackages();
+    // Get the dev requirements for the root project.
+    $dev_requirements = $this->projectRoot->getDevRequirements();

It's pretty cool, hey! But overall this feels over-architected for the purposes of this issue at least. Based on my reservations I'm tagging for framework managers to look at it too. Leaving RTBC though because I would also like to get the scope described in the summary in ASAP.

1. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,107 @@
   + * Builds hook_requirements() info about Composer dev dependencies.
   

   This doesn't reflect that we are only caring about core.

   +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,107 @@
   +   * This warning will only reflect core's dev requirements.
   

   Why do we only care about Core? I've read the issue summary and that's not really explained as far as I can see. I would have thought that contrib dev requirements being installed is also problematic.

2. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,107 @@
   +    $dev_installed = FALSE;
   +    if ($installed_dev && (count($installed_dev) === count($dev_requirements))) {
   

   Variables named $dev_installed and $installed_dev don't make for easy code reading.

3. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,107 @@
   +      foreach ($dev_requirements as $package_name => $version_constraint) {
   +        // Special case for @dev packages, which can generate unparseable local
   +        // version numbers, such as [behat/mink] => 9999999-dev.
   +        if (strpos($version_constraint, 'dev') === FALSE) {
   +          if (!Semver::satisfies($installed_dev[$package_name], $version_constraint)) {
   +            $dev_installed = FALSE;
   +            break;
   +          }
   +        }
   +      }
   

   I'm not sure why a single dev dependency that satisfies the conditions should prevent the warning.

4. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,107 @@
   +    $requirements = [
   +      'composer_dev_dependencies' => [
   +        'title' => $this->t('Composer dev requirements'),
   +        'description' => $this->t('This Drupal installation does not appear to have development packages installed. <a href=":documentation">Documentation on drupal.org</a>', [
   +          ':documentation' => 'https://www.drupal.org/docs/develop/using-composer',
   +        ]),
   +        'severity' => REQUIREMENT_OK,
   +      ],
   +    ];
   +
   +    if ($dev_installed) {
   +      $requirements['composer_dev_dependencies']['description'] =
   +        $this->t('This Drupal installation appears to include development packages. This can lead to performance and security issues. Read the <a href=":documentation">documentation on drupal.org</a>', [
   +          ':documentation' => 'https://www.drupal.org/docs/develop/using-composer',
   +        ]);
   +      $requirements['composer_dev_dependencies']['severity'] = REQUIREMENT_WARNING;
   +    }
   
   +++ b/core/modules/system/system.install
   @@ -74,6 +74,12 @@ function system_requirements($phase) {
   +    // Add warning about core's Composer-based dev requirements.
   +    $requirements = array_merge(
   +      $requirements,
   +      \Drupal::service('composer.dev_dependency_finder')->buildRequirements()
   +    );
   

   It's weird that so much requirement stuff is bleeding into the service. Well I guess that's the entire point of the service. In some ways, I think the ProjectRoot service should have a method like hasDevRequirementsInstalled() that just returns either a Boolean as in or a list of the dev requirements installed. And this stuff could be in system_requirements().

   Also for me what's odd is that this method is doing two things. One is working out if dev requirements are installed and the second is constructing the requirement array. Why isn't the first thing part of the ProjectRoot class?

   If we're going to have a requirement builder service, I think the requirement building would simpler if we were just returning the single requirement. Ie. in system_requirements() just doing:

   $requirements['composer_dev_dependencies'] = \Drupal::service('composer.dev_dependency_finder')->buildRequirement();
   

5. +++ b/core/lib/Drupal/Core/Composer/Dependencies/DevDependencyFinder.php
   @@ -0,0 +1,107 @@
   +        'description' => $this->t('This Drupal installation does not appear to have development packages installed. <a href=":documentation">Documentation on drupal.org</a>', [
   

   Not sure about the word appear here. It makes it seem very cautious which makes it very hard for a user to know what to do. Even in the warning message we use the word appear too. This could leave the user thinking how do I ensure that I've not got dev dependencies installed. Also as above we're only checking core's dev dependencies - but the message doesn't say this so you might have a module's dev dependencies installed but the requirements are telling you you've got none installed.

6. +++ b/core/lib/Drupal/Core/Composer/ProjectRoot.php
   @@ -0,0 +1,130 @@
   + * Provides information about the Composer project root.
   ...
   +class ProjectRoot implements ProjectRootInterface {
   

   I think ProjectRoot is an odd name for what this class does. I think Project is kinda overloaded already - we have \Drupal\Core\Utility\ProjectInfo - which Performs operations on drupal.org project data.. How about ComposerInfo or something like that.

7. +++ b/core/lib/Drupal/Core/Composer/ProjectRoot.php
   @@ -0,0 +1,130 @@
   +  protected function getDevRequires($composer_file) {
   ...
   +    if (isset($json['require-dev'])) {
   

   Why not getRequireDev so it is inline with the key you're getting. Less to learn.

Why do we only care about Core? [...] Not sure about the word appear here.

It's not that we only care about core. It's that the only require-dev dependencies we can determine are from drupal/drupal (and through merge plugin, drupal/core) because that's how Composer works.

It could be that contrib has a non-dev requirement that is also a core dev requirement. That's why we say 'appears.'

The rest of the review is good stuff. ProjectRoot is a thing because it was going to help out with [#12244933] so it's structured as a separate service. Now we have #2912406: [META] Replace update_manager with a more powerful solution so it's unclear what's actually needed in this issue or that one.

The new update manager seems to have slowed down a bit. But even if it was still active, I think that initiative can make use of the ProjectRoot. So I would love to get this patch in. This improvement can be really helpful for those that are not going to use the new update manager.

Patch in #39 still applies. Re-testing. Giving a poke because of interest in #2494073: Prevent modules which have unmet Composer dependencies from being installed from #1398772: Replace .info.yml with composer.json for extensions

The biggest outstanding issue here is @xjm's scope issue of #49, so if there's some kind of more specific guidance as to what to do to move forward that'd be great.

We have a composer initiative happening, in order to support a lot of different initiative changes: #2958021: Proposal: Composer Support in Core initiative It'd be great to be able to provide this service so core can do some Composer introspection and support all those other things.

Reroll.

We still have an outstanding review from #50 which need to be addressed.

We still have issues of scope from #49. Some commenters here seem to want the services, but those needs were expressed in 2017. :-)

The composer initiative doesn't seem to require that we have these things at the moment.

And here I thought I had added the patch already.

Never mind.

Weird. I need to go eat or something.

Patch applied!

Composer has an upstream issue about storing some of this info in installed.json: https://github.com/composer/composer/issues/3008

That's great for new sites after Composer 2.0 is released, but old sites would still need the complex logic here.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I think this is still relevant.
I opened a MR based on the latest patch.

Composer 2 is now a required part of installing drupal. So I think we can use that information in installed.json instead of doing all the logic in this patch?

Can #77 be explored

Yes we can read that json file and usage the 'dev' key. Another possibility would be to add this to \Drupal\Composer\Plugin\Scaffold\DrupalInstalledTemplate::getCode() as that is generated when you run composer install. That way we wouldn't be dependent on Composer's JSON. However, that has an iinteresting issue that you need to run composer install --no-dev (or composer install) twice to switch from one to the other.... not sure why and that's a bug.

I updated the MR to get the info from the dev key in installed.json.

Left some comments on the MR. Tagging for a CR for the new interface. Summary probably needs to be updated to mention that too.

Thanks.

I made the suggested changes and added a CR.

I think we don't need all the stuff in Drupal\Core\Composer we can just do \Composer\InstalledVersions::getRootPackage()['dev']; to find out if the project has dependencies installed and put all the requirements stuff in the hook class. Less complexity. We can test this with a build test - see \Drupal\BuildTests\Framework\Tests\HtRouterTest::testHtRouter for an example.

Thanks! Based on the feedback moving to NW