Problem/Motivation

Doing some experimentation, I discovered that some non-radical Composer operations broke drupal/core-vendor-hardening. Here’s a log:

Unsound:drupal paul$ git checkout -b foof-23
Switched to a new branch 'foof-23'
Unsound:drupal paul$ composer update drupal/core-vendor-hardening
> Drupal\Core\Composer\Composer::ensureComposerVersion
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 0 installs, 2 updates, 0 removals
  - Removing drupal/core-vendor-hardening (8.9.x-dev), source is still present in /Users/paul/projects/drupal/vendor/drupal/core-vendor-hardening
  - Installing drupal/core-vendor-hardening (8.9.x-dev 3e396cb): Downloading (100%)         

Fatal error: Uncaught TypeError: Argument 1 passed to Drupal\Composer\Plugin\VendorHardening\VendorHardeningPlugin::removeBinBeforeCleanup() must be an instance of Composer\Package\CompletePackage, instance of Composer\Package\AliasPackage given, called in /Users/paul/projects/drupal/composer/Plugin/VendorHardening/VendorHardeningPlugin.php on line 120 and defined in /Users/paul/projects/drupal/composer/Plugin/VendorHardening/VendorHardeningPlugin.php:156
Stack trace:
#0 /Users/paul/projects/drupal/composer/Plugin/VendorHardening/VendorHardeningPlugin.php(120): Drupal\Composer\Plugin\VendorHardening\VendorHardeningPlugin->removeBinBeforeCleanup(Object(Composer\Package\AliasPackage))
#1 [internal function]: Drupal\Composer\Plugin\VendorHardening\VendorHardeningPlugin->onPrePackageUpdate(Object(Composer\Installer\PackageEvent))
#2 phar:///Users/paul/bin/composer/src/Composer/EventDispatcher/EventDispatcher.php(176): call_user_func(Array, Object(Composer\Installer\PackageEvent))
#3 phar:///Users/paul/bin/composer/src/Compose in /Users/paul/projects/drupal/composer/Plugin/VendorHardening/VendorHardeningPlugin.php on line 156

The reasons Composer is sending an AliasPackage to this event handler are complex and don’t actually matter that much for this issue… They mostly revolve around the fact that Composer has generated a new version constraint based on the branch name, and must install a new plugin to replace an existing one that is currently running.

All that aside, we should make the code here more defensive so that we don’t break people’s installations, or complicate the dev workflow for core developers.

Proposed resolution

Given that removeBinBeforeCleanup() is the only method that takes a package object as a parameter, we can make this method more defensive against non-spec package types.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3088403_2.patch1.8 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Status: Active » Needs review
FileSize
1.8 KB

Patch.

This results in:

$ git checkout -b foof-23
Switched to a new branch 'foof-23'
$ composer update

[...]

  - Removing drupal/core-vendor-hardening (8.8.x-dev), source is still present in /Users/paul/projects/drupal/vendor/drupal/core-vendor-hardening
  - Installing drupal/core-vendor-hardening (8.8.x-dev efa83b6): Downloading (100%)   

That's not exactly what we want from a path repo, but it's also not broken.

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Composer initiative

Thats cause you need to, currently remember to COMPOSER_ROOT_VERSION on your foof-23 branch.

$ git checkout -b foof-23
Switched to a new branch 'foof-23'
$ COMPOSER_ROOT_VERSION=8.9.x composer update

Anyhow, I was running into this bug working on the metapackage generator, applied this patch, and the bug went away.

Rad

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed 3072f841ab to 9.0.x and cf3f6a38d4 to 8.9.x. Thanks!

Asking a release manager to approve backporting to 8.8.x

  • alexpott committed 3072f84 on 9.0.x
    Issue #3088403 by Mile23, Mixologic: Defensive coding: Vendor hardening...

  • alexpott committed cf3f6a3 on 8.9.x
    Issue #3088403 by Mile23, Mixologic: Defensive coding: Vendor hardening...
Mixologic’s picture

/me is unsure if this is supposed to be RTBC or Patch (to be ported) ? and when to use one or the other.

This patch should already apply to 8.8.x, so does that mean we RTBC it?

Mile23’s picture

Patch in #2 still applies to 8.8.x.

Re: #7: I think the idea here is that core maintainers have to consult to figure out if this can be backported to 8.8.x. It's a bugfix and a minor change, so I'm optimistic.

Mixologic’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

Yeah, I was just making sure that it was actually going to be on their radar, as this one #3087482: Add 'next steps' info to Composer installation output went from Patch (to be ported) to Needs Review to RTBC, so I wouldn't want this to get missed if they thought this was still waiting on a Patch.

I think I'll RTBC it with the caveat that @catch or @xjm need to make the call first. Patch (to be ported) just seems like its saying there is more work to do before its ready for their judgement.

alexpott’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

@Mixologic both @catch and I use the "patch to be ported" status to separate these from the other rtbc's that are yet to be committed to any branch. It makes it less likely to be missed.

alexpott’s picture

Status: Patch (to be ported) » Fixed

Discussed with @catch and we agreed to backport.

  • alexpott committed f45fef9 on 8.8.x
    Issue #3088403 by Mile23, Mixologic: Defensive coding: Vendor hardening...

Status: Fixed » Closed (fixed)

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

xjm’s picture