Problem/Motivation

In #3057094: Add Composer vendor/ hardening plugin to core we added the drupal/core-vendor-hardening plugin to Drupal. This replaced the behavior of equivalent scripts which ran from the drupal/drupal composer.json file. We made the change so that the behavior would be portable across different Composer template installations.

Change record for that: https://www.drupal.org/node/3059717

The scripts are no longer called from drupal/drupal's composer.json file, but the code which the scripts called was not removed.

This code is not marked for deprecation, but there was already a pre-existing issue to delete the unused code: #3076684: Remove deprecated vendor cleanup scripts That issue is now postponed on this one.

Proposed resolution

Mark the vendor hardening code in Drupal\Core\Composer\Composer::vendorTestCodeCleanup() (and whatever else is vestigial) as deprecated.

Eventually remove the code in #3076684: Remove deprecated vendor cleanup scripts for the 10.0.0 release.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3280589

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:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Assigned: Unassigned » Spokje
Status: Active » Needs work

Added Change Record

Spokje opened merge request !2425

Spokje opened merge request !2427

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Status: Needs work » Needs review

Right...

I've created two MRs for this one:

1) MR !2425, which is basically exactly what the IS asks for: Deprecate Drupal\Core\Composer\Composer::vendorTestCodeCleanup() and create a test for that deprecation.
2) MR!2427, where I looked at #3076684: Remove deprecated vendor cleanup scripts and found a patch in there (3076684-27.patch). There's a lot more deprecating going on in there, as well as marking some methods @internal. I've basically converted that patch into an MR with some extra polish. The only big change is that the patch also deprecates Drupal\Core\Composer\Composer::upgradePHPUnit(). That's something we can't do in 9.5.x-dev since that still supports PHP 7.3 and still uses that method. Also I think it was decided (somewhere) to keep this method for the PHPUnit 10.0 upgrade.

I'll let Bigger Brains decide which way to go on this one.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Assigned: Spokje » Unassigned
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

I think we need something in between the two, so commented on 2427.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented

Oi, there was definitely no option #3...;)

Closed MR !2425 since we're not going to take that route.

Spokje closed merge request !2425

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented

Changes made, asked two times for the same clarification.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

Responded in the MR.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 commented

"Any "scripts" section mentioning this in composer.json can be safely removed."

The purpose of the script is to harden a vendor directory, so therefore not using it is potentially unsafe, so don't say 'safely.'

Here's an alternate text:

'Calling ' . __METHOD__ . ' from composer.json is deprecated in drupal:9.5.0 and is removed from drupal:10.0.0. Any "scripts" section mentioning this in composer.json can be removed and replaced with the drupal/core-vendor-hardening Composer plugin, as needed. See https://www.drupal.org/node/3260624'

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Status: Needs work » Needs review

Thanks to both @longwave and @Mile23, sense = made in both cases.

Made the requested changes.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented

Updated CR.

The one thing I'm unsure about are protected method/static that are only used by the methods we're deprecating in this issue (\Drupal\Core\Composer\Composer::findPackageKey and \Drupal\Core\Composer\Composer::$packageToCleanup). Is there the need to deprecate those, or can they just be removed in #3076684: Remove deprecated vendor cleanup scripts?

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Thanks!

findPackageKey() is now @internal so that is fine to remove, I think $packageToCleanup will be OK to remove as well - I don't think anyone will be using it outside of this code, and there's a replacement in the plugin anyway.

To me this is RTBC, it will be good to get rid of these old scripts finally.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented

Thanks for the explanation @longwave!

  • catch committed e22bcf1 on 10.0.x 
    Issue #3280589 by Spokje, longwave, Mile23: Deprecate Composer Vendor...
  • catch committed a9c1a68 on 10.1.x
    Issue #3280589 by Spokje, longwave, Mile23: Deprecate Composer Vendor...
  • catch committed 07660f9 on 9.5.x
    Issue #3280589 by Spokje, longwave, Mile23: Deprecate Composer Vendor...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Agreed with deprecating these in 9.5.x, not really an API as such and related to composer 2/2.2 updates.

Committed/pushed to 10.1.x, 10.0.x and 9.5.x, thanks!

Spokje closed merge request !2427

  • catch committed a1efd20 on 10.0.x 
    Issue #3292992 by Spokje: [#3280589] broke HEAD on 10.0.x and 10.1.x
  • catch committed c90e87c on 10.1.x
    Issue #3292992 by Spokje: [#3280589] broke HEAD on 10.0.x and 10.1.x

  • catch committed eb8e65e on 9.5.x 
    Issue #3292992 by Spokje: [#3280589] broke HEAD on 10.0.x and 10.1.x

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

Changed the CR to be, hopefully, easier to read and published.