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

Command icon 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:

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Issue summary: View changes

phenaproxima’s picture

Status: Active » Needs review
Issue tags: +Needs tests

Reviewable, but not ready quite yet since tests are needed.

phenaproxima’s picture

I 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?

phenaproxima’s picture

Title: Package Manager should use a copy of Composer that is local to the current project, if available » [PP-1] Package Manager should use a copy of Composer that is local to the current project, if available
Status: Needs review » Postponed
Related issues: +#3501582: Package Manager should always run Composer through the PHP interpreter, rather than directly

An 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/composer exists, 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 chmod them 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.

phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Title: [PP-1] Package Manager should use a copy of Composer that is local to the current project, if available » Package Manager should use a copy of Composer that is local to the current project, if available
Status: Postponed » Needs review
Issue tags: -Needs tests

Blocker landed!

phenaproxima’s picture

Figured out how to write a unit test (a rare case where reflection is the way to go). This is reviewable!

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed this with @phenaproxima and everything makes sense. Test coverage looks good.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Overall this looks good, except for one comment on the MR - think we can avoid an unnecessary try/catch.

phenaproxima’s picture

Status: Needs work » Reviewed & tested by the community

Easy enough. Since this was such a simple change with no logic implications, tentatively restoring Tim's prior RTBC.

  • catch committed 52758cae on 11.2.x
    Issue #3528139 by phenaproxima, tim.plunkett, catch: Package Manager...

  • catch committed 42209999 on 11.x
    Issue #3528139 by phenaproxima, tim.plunkett, catch: Package Manager...
catch’s picture

Version: 11.x-dev » 11.2.x-dev
Status: Reviewed & tested by the community » Fixed

Noticed 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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

catch’s picture

Status: Closed (fixed) » Needs review

As 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.

smustgrave’s picture

@catch I don't see a revert commit just wanted to follow up on that one?

catch’s picture

Status: Needs review » Fixed

The 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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.