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
Comment | File | Size | Author |
---|---|---|---|
#2 | 3088403_2.patch | 1.8 KB | Mile23 |
Comments
Comment #2
Mile23Patch.
This results in:
That's not exactly what we want from a path repo, but it's also not broken.
Comment #3
MixologicThats cause you need to, currently remember to COMPOSER_ROOT_VERSION on your foof-23 branch.
Anyhow, I was running into this bug working on the metapackage generator, applied this patch, and the bug went away.
Rad
Comment #4
alexpottCommitted 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
Comment #7
Mixologic/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?
Comment #8
Mile23Patch 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.
Comment #9
MixologicYeah, 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.
Comment #10
alexpott@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.
Comment #11
alexpottDiscussed with @catch and we agreed to backport.
Comment #14
xjm