Problem/Motivation

In #3032686: Remove references to unused packages in Drupal 9's vendor hardening we added a test to ensure that packages that had been removed from composer.lock were also removed from our vendor hardening code.

We can go a step further and ensure that all packages listed in composer.lock are also listed in the vendor hardening code, even if there is nothing to harden; this ensures we have at least considered hardening for any new dependency.

Proposed resolution

Make the test in ComposerIntegrationTest::testVendorCleanup() stricter.

Remaining tasks

Decide if this is a good idea.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#20 3133903-20.patch11.08 KBKapilV
#6 3133903-6.patch11.7 KBlongwave
#2 3133903-2.patch11.63 KBlongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Active » Needs review
FileSize
3133903-2.patch11.63 KB

So I suspect this is more important than I first thought; since we first added directories to the vendor hardening, we have updated various dependencies and a number of the directories have moved. We have also added new dependencies and not hardened them. This means that, especially in packages installed from source, the vendor directories often contain a number of tests that could be safely removed. This is mitigated by the fact that many packages now use .gitattributes to remove their tests from their dist (zipfile) builds, which is what I think most people will end up installing. However, not all packages do this correctly.

I have updated the test to ensure that at least an empty array exists for each dependency, forcing us to evaluate them for hardening in the future.

The way I tested this was to run

rm -rf vendor
composer install --prefer-source

I then looked through the vendor directories for tests and added them to the hardening config. I also removed a few that now seem obsolete.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Status: Needs review » Needs work 
+++ b/composer/Plugin/VendorHardening/Config.php
@@ -20,38 +20,65 @@ class Config {
   protected static $defaultConfig = [
+    'asm89/stack-cors' => ['test'],

@@ -70,22 +101,37 @@ class Config {
+    'typo3/phar-stream-wrapper' => ['tests'],
+    'webmozart/assert' => ['tests'],

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -16,38 +16,65 @@
   protected static $packageToCleanup = [
+    'asm89/stack-cors' => ['test'],

@@ -66,23 +97,37 @@ class Composer {
+    'typo3/phar-stream-wrapper' => ['tests'],
+    'webmozart/assert' => ['tests'],

The patch does not apply. $defaultConfig and $packageToCleanup look the same, can we make one of them reusable?

One concern is that if those files are removed, no way to get them back. For example, while developing, for some reasons, I want to check those tests or docs inside the vendor directory. So I am ponding if we could make it optional.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Status: Needs work » Needs review

The patch does not apply Sorry, I was wrong, applying the patch to the wrong branch.

Status: Needs review » Needs work

The last submitted patch, 2: 3133903-2.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
3133903-6.patch11.7 KB

Rerolled to include symfony/polyfill-php80.

longwave’s picture

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

Re: #3 you can tell composer to ignore plugins and the hardening will not happen:

$ rm -rf vendor
$ composer install --no-plugins
...
$ ls vendor/squizlabs/php_codesniffer/tests
AllTests.php  bootstrap.php  Core  FileList.php  Standards  TestSuite7.php  TestSuite.php
$ composer install
...
$ ls vendor/squizlabs/php_codesniffer/tests
ls: cannot access 'vendor/squizlabs/php_codesniffer/tests': No such file or directory
jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented

re #7, got it. thanks!

$defaultConfig and $packageToCleanup look the same, can we make one of them reusable?

The above is another question in #3, any thoughts?

longwave’s picture

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

I think Drupal\Core\Composer\Composer is legacy (the old way of running Composer scripts from composer.json) and Drupal\Composer\Plugin\VendorHardening\Config is newer (the modern Composer plugin way) but I also think unifying these into a single script is out of scope for this issue; these arrays have previously been kept in sync manually, let's open a new issue to discover if we can merge them.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thanks @longwave for your explaination in #9

It looks like vendorTestCodeCleanup() is not being used and redundant as we have drupal/core-vendor-hardening doing its job.

Filed #3155625: Deprecate \Drupal\Core\Composer\Composer::vendorTestCodeCleanup() as the follow-up, re-scoping it if necessary. BTW. removing \Drupal\Core\Composer\Composer::vendorTestCodeCleanup() will remove $packageToCleanup.

The patch in #6 is good to me as the test revealed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 3133903-6.patch, failed testing. View results

longwave’s picture

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

Random fail.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed f0f4922 and pushed to 9.1.x. Thanks!

This is definitely a good idea. Nice work @longwave

  • alexpott committed f0f4922 on 9.1.x 
    Issue #3133903 by longwave, jungle: Ensure all packages in composer.lock...

  • alexpott committed adfed3f on 9.1.x 
    Revert "Issue #3133903 by longwave, jungle: Ensure all packages in...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Fixed » Needs review

Hmmm... so there's an issue with this patch. It's very fragile as it'll fail if you have drush installed locally for example. But my biggest concern is that this will make updating to PHPUnit 9 in a test run very hard too. I think we need to go back to the drawing board and come up with something less fragile. Imo we should only target prod dependencies.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

It also needs to add SF/Mime and check other dependencies

KapilV’s picture

KapilV CreditAttribution: KapilV as a volunteer and at Innoraft for Drupal Care, Drupal Association commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3133903-20.patch11.08 KB

Hear a reroll patch.

Status: Needs review » Needs work

The last submitted patch, 20: 3133903-20.patch, failed testing. View results

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.