Problem/Motivation
Package Manager needs to be able to run Composer in order to work properly.
It will try to autodetect the path to Composer at runtime, but if that fails, it will allow the site builder to explicitly set the path to Composer (in config). If the hosting environment has a too-old version of Composer, though, that won't be sufficient either.
A project can have a copy of Composer that is local to itself (e.g., vendor/bin/composer) to get around that, of course. But it would be helpful, and reduce friction, if Package Manager would automatically detect this condition and configure itself accordingly.
Proposed resolution
If a path to Composer is set in configuration, use that. This is the current behavior, and is unchanged.
But if Composer's path is not set in configuration, then use the copy Composer installed in the current project, if it exists. This should happen automatically in Package Manager's ExecutableFinder service.
Issue fork drupal-3528139
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3528139-use-local-composer
changes, plain diff MR !12302
Comments
Comment #2
phenaproximaComment #4
phenaproximaReviewable, but not ready quite yet since tests are needed.
Comment #5
phenaproximaI can't easily write a test for this, because although composer/composer is a dev dependency of core, vendor/bin/composer doesn't exist because it's cleaned up by the vendor hardening plugin, which has no way to prevent a package from being cleaned up. This also means that end users can't take advantage of this improvement if they have the vendor hardening plugin at all.
Not sure how to proceed. Should I change our build tests to confirm that this works? Should we file a blocking issue to allow the vendor hardening plugin to skip certain packages if configured to do so?
Comment #6
phenaproximaAn update here -- if we can get #3501582: Package Manager should always run Composer through the PHP interpreter, rather than directly in, this becomes much more viable, and would immediately improve things for Drupal CMS users -- if Composer is locally installed and
vendor/composer/composer/bin/composerexists, Package Manager would use that by default, no questions asked.The vendor hardening plugin would still be an obstacle for some sites -- although Drupal CMS does not currently use it -- but my idea there is to change it to not delete Composer's binaries outright, but rather just
chmodthem to 644 so that they cannot be executed directly. Then Composer simply becomes another PHP script run by the PHP interpreter, rather than something that can be invoked as its own process. All that would need to happen in a separate issue, though.Postponing on the related issue.
Comment #7
phenaproximaComment #8
phenaproximaBlocker landed!
Comment #9
phenaproximaFigured out how to write a unit test (a rare case where reflection is the way to go). This is reviewable!
Comment #10
tim.plunkettReviewed this with @phenaproxima and everything makes sense. Test coverage looks good.
Comment #11
catchOverall this looks good, except for one comment on the MR - think we can avoid an unnecessary try/catch.
Comment #12
phenaproximaEasy enough. Since this was such a simple change with no logic implications, tentatively restoring Tim's prior RTBC.
Comment #15
catchNoticed one more thing - the static variable was unnecessary, @phenaproxima moved it to a normal property. We could maybe have done a separate method for testing instead but it's 50/50 so not worth further changes.
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
Comment #18
catchAs with #3537668: [PP-1] Dynamically figure out the actual path to Composer's binary, and make it read-only I think we need to think about this a bit more so I'm reverting this for now, we can commit again depending on the discussion.
edit: this comment was on the wrong issue.
Comment #19
smustgrave commented@catch I don't see a revert commit just wanted to follow up on that one?
Comment #20
catchThe comment was on the wrong issue. The three commits that were reverted, were:
#3511972: Allow Composer and rsync location to be configured via the UI
#3537668: [PP-1] Dynamically figure out the actual path to Composer's binary, and make it read-only
#3534278: The vendor hardening plugin should provide a way to skip cleaning certain packages was also reverted, but later re-committed again with some changes.
Moving this one to fixed, thanks for checking!