This issue is part of the composer initiative: #2958021: Proposal: Composer Support in Core initiative

This is a child issue of #2982674: [meta] Composer Initiative Phase 1: Add composer build support to core

Originally, #2982684: Add a composer scaffolding plugin to core was going to cover this behavior, but it was decided that we could split it out to gain developer velocity on other Composer Initiative issues. #3053800: Agenda for 5-15-2019

Problem/Motivation

Because we plan to have Composer's vendor/ directory inside the public-accessible docroot for some projects, we have to make a good-faith effort to sanitize it. This would mean removing directories that could be exploited for execution, such as test directories. See: https://www.drupal.org/forum/newsletters/security-advisories-for-drupal-... and #2585165: Don't include vendor test code (especially mink) in the Drupal webroot

This has been handled by Drupal\Core\Composer\Composer::vendorTestCodeCleanup(), called as an event script in drupal/drupal's composer.json like this:

    "scripts": {
        "post-package-install": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",
        "post-package-update": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",

We want to change this, however, because we want to be able to create a Composer template that generates the 'tarball' download product, and which also allows users of that product to do Composer-y operations like update their codebase. #2982680: Add composer-ready project templates to Drupal core

Some work has been done on this, in #2990257: Determine how to handle composer scripts in drupal/drupal and #2982684: Add a composer scaffolding plugin to core.

Specs:

  • The vendor cleanup behavior should remove test files at known locations within individual packages.
  • Verbose output should inform the user that test directories were not where they were expected to be, with a message about filing an issue.
  • The implementation should update when the user updates their codebase. This means the implementation should be a Composer plugin.

Proposed resolution

Duplicate the behavior of Drupal\Core\Composer\Composer::vendorTestCodeCleanup() into a Composer plugin.

This plugin is located in composer/Plugin, which behaves like a Drupal component, except it is not packaged with the drupal/core subtree split.

Remaining tasks

None

Follow-on tasks

For this issue, we'll leave the existing Composer script methods for drupal/drupal #3057094-15: Add Composer vendor/ hardening plugin to core and we'll need to decide in an subsequent issue when to replace those script methods. See #3076684: Remove deprecated vendor cleanup scripts

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

CommentFileSizeAuthor
#125 interdiff.txt2.59 KBMile23
#125 3057094_125.patch46.52 KBMile23
#123 3057094-123.patch45.37 KBhussainweb
#123 interdiff-120-123.txt1.05 KBhussainweb
#120 3057094-120.patch45.24 KBhussainweb
#120 interdiff-118-120.txt667 byteshussainweb
#118 3057094-118.patch45.24 KBhussainweb
#118 interdiff-117-118.txt542 byteshussainweb
#117 3057094-117.patch45.26 KBhussainweb
#117 interdiff-113-117.txt1.7 KBhussainweb
#113 3057094-113.patch45.38 KBhussainweb
#113 interdiff-112-113.txt1.5 KBhussainweb
#112 3057094-112.patch45.83 KBhussainweb
#112 interdiff-108-112.txt4.63 KBhussainweb
#108 interdiff.txt1.99 KBMile23
#108 3057094_108.patch45.91 KBMile23
#99 interdiff.txt191 bytesMile23
#99 3057094_99.patch46.84 KBMile23
#96 run_locally_no_test.patch2.09 KBMile23
#96 interdiff.txt5.66 KBMile23
#96 3057094_96.patch47.02 KBMile23
#91 run_locally_do_not_test.patch1.97 KBMile23
#91 interdiff.txt6.16 KBMile23
#91 3057094_91.patch46.6 KBMile23
#87 3057094_87.patch47 KBMile23
#85 interdiff.txt829 bytesMile23
#85 3057094_85.patch47.01 KBMile23
#82 3057094_82.patch46.82 KBMile23
#82 interdiff.txt706 bytesMile23
#80 interdiff.txt10.9 KBMile23
#80 3057094_80.patch46.89 KBMile23
#80 3057094_80_local_do_not_test.patch1.36 KBMile23
#72 interdiff.txt375 bytesMile23
#72 3057094_72.patch44.62 KBMile23
#71 interdiff.txt11.25 KBMile23
#71 3057094_71.patch44.61 KBMile23
#59 interdiff.txt2.59 KBMile23
#59 3057094_59.patch44.67 KBMile23
#54 interdiff.txt3.48 KBMile23
#54 3057094_54.patch44.73 KBMile23
#51 interdiff.txt4.06 KBMile23
#51 3057094_51.patch44.72 KBMile23
#49 interdiff.txt3.08 KBMile23
#49 3057094_49.patch44.64 KBMile23
#48 40-48_interdiff.txt922 bytesMixologic
#48 3057094_48.patch44.58 KBMixologic
#4 3057094_4.patch36.15 KBMile23
#5 3057094_5.patch64.43 KBMile23
#5 interdiff.txt33.93 KBMile23
#14 3057094_14.patch65.06 KBMile23
#14 interdiff.txt19.56 KBMile23
#16 3057094_16.patch69.69 KBMile23
#16 interdiff.txt12.1 KBMile23
#17 3057094_17.patch69.56 KBMile23
#17 interdiff.txt539 bytesMile23
#19 3057094_20.patch53.86 KBMile23
#19 interdiff.txt15.71 KBMile23
#23 3057094_23.patch37.08 KBMile23
#24 3057094_24.patch37.95 KBMile23
#30 3057094_30.patch38.88 KBMile23
#30 interdiff.txt2.67 KBMile23
#32 3057094_32.patch38.18 KBMile23
#32 interdiff.txt2.82 KBMile23
#34 3057094_32-to-34-interdiff.txt13.3 KBgreg.1.anderson
#34 3057094_34.patch39.9 KBgreg.1.anderson
#39 3057094_39.patch138.01 KBMile23
#39 interdiff.txt14.67 KBMile23
#39 use_plugin_DO_NOT_TEST.patch1.26 KBMile23
#40 interdiff.txt11.42 KBMile23
#40 3057094_40.patch43.68 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Mile23’s picture

Note that there are a few existing packages out there in the world that already do this.

One very thorough option is https://packagist.org/packages/dg/composer-cleaner which removes everything not essential for running the package in non-dev mode. That is, it analyzes the autoloaders in composer.json and then deletes everything not necessary for that. Since we rely on existing tests, this is not currently an option for us, but issues like #3055040: Roll AbstractEventDispatcherTest into ContainerAwareEventDispatcherTest might help us with this. Of the options available, I'd choose this package because it has zero dependencies.

There are others such as https://packagist.org/packages/mediamonks/composer-vendor-cleaner which have many dependencies including symfony/finder.

Mile23’s picture

Status: Active » Needs review
Issue tags: +Needs change record, +Needs documentation
FileSize
36.15 KB

Here's a first go.

This plugin is called drupal/core-vendor-cleanup. Left on its own it has the same behavior as Drupal\Core\Composer\Composer::vendorTestCodeCleanup(). However, you can also configure it in the root project, like this:

"extra": {
  "drupal-core-vendor-cleanup": {
    "vendor/package": ["test", "documentation"]
  }
}

Under the assumption that drupal/drupal is the 'dev distribution,' this component does not have a replace in composer.json like the other components. It is installed through a path repository. In #2982680: Add composer-ready project templates to Drupal core we'll make a Composer project template that only requires drupal/core-vendor-cleanup and doesn't do this repository dance.

Still to do: CR, Make the README pretty, Add documentation.

Mile23’s picture

Added some tests.

In order for the tests to work, we have to use the dev dependencies of drupal/core-vendor-cleanup.

Since we don't merge that composer.json file and instead require it, we have to re-declare those dev dependencies in drupal/drupal.

If components were tested independently, we wouldn't have to do this. Just sayin'. :-) #2943856: [meta] Reorganize Components so they are testable in isolation

Anyway, here's what it looks like:

$ rm -rf vendor/
$ composer install -v

[...]

 Extracting archive  - Installing symfony/yaml (v3.4.26): Loading from cache
 Extracting archive    Package cleanup for symfony/yaml

  - Installing squizlabs/php_codesniffer (3.4.2): Loading from cache
 Extracting archive  - Installing drupal/coder (8.3.2): Cloning 44c80c21074df43572652f35bec4f184f9eae5e7 from cache
    Package cleanup for drupal/coder

  - Installing ircmaxell/password-compat (v1.0.4): Loading from cache
 Extracting archive  - Installing twig/twig (v1.38.4): Loading from cache
 Extracting archive    Package cleanup for twig/twig

  - Installing jcalderonzumba/gastonjs (v1.0.2): Loading from cache
 Extracting archive    Package cleanup for jcalderonzumba/gastonjs

[ etc...]

When we update we find that some of our test directories have moved:

$ rm -rf vendor/
$ composer update -v

[ ... ]

  - Installing sebastian/comparator (2.1.3): Loading from cache
 Extracting archive    Package cleanup for sebastian/comparator

  - Installing doctrine/instantiator (1.2.0): Loading from cache
 Extracting archive    Package cleanup for doctrine/instantiator
      Directory '/Users/paul/projects/drupal/vendor/doctrine/instantiator/tests' does not exist.

[ etc ]

It's possible that we could allow for halting the build in this circumstance, through a config in extra and/or an environmental variable. This way we could test builds.

xjm’s picture

xjm’s picture

Since this involves adding new dev dependencies for core, we should add evaluation of the criteria to the IS: https://www.drupal.org/core/dependencies#criteria

xjm’s picture

Meant to add, see #3036210: Add justinrainbow/json-schema as a composer dependency so JSON:API can use it to validate its responses for an example of where we went through this process for another dev dependency recently.

Mile23’s picture

Issue summary: View changes
Mile23’s picture

alexpott’s picture

  1. Note the list of cleanups needs a massive clean up - see #3057462: Add missing packages to \Drupal\Core\Composer\Composer::$packageToCleanup
  2. +++ b/composer.json
    @@ -5,8 +5,14 @@
    +    "require-dev": {
    +        "composer/composer": "^1.8@stable",
    +        "mikey179/vfsstream": "^1.2",
    +        "phpunit/phpunit": "^6.5"
    +    },
    

    This makes me uncomfortable - I don't think we should start adding dev dependencies to the root composer.json in this issue. See

  3. +++ b/core/lib/Drupal/Component/VendorCleanup/VendorCleanupPlugin.php
    @@ -0,0 +1,148 @@
    +    // Use defalt configuration if none is specified.
    +    $this->config = new Config($this->composer->getPackage());
    +    if ($this->config->isUnconfigured()) {
    +      $this->config = new DefaultConfig($this->composer->getPackage());
    +    }
    

    I get why want to allow composer.json to add to this. But i'm not understanding why you'd want to override. Shouldn't this be a merge. Also rather than maintaining the default list in code shouldn't we maintain the list in core/composer.json? Or alternatively do we need any of this complexity? Can we add the list to the root composer.json shipped in git and then have a single list that's maintained for all composer projects. I guess the answer there is no because how do we add new dependencies to the list. Adding new dependencies to the list is also a concern for the current behaviour here which is to override. I think we need to merge the default list with what is in the root composer.json.

  4. +++ b/core/lib/Drupal/Component/VendorCleanup/VendorCleanupPlugin.php
    @@ -0,0 +1,148 @@
    +  /**
    +   * Clean out the package.
    +   *
    +   * @param \Composer\Package\CompletePackageInterface $package
    +   */
    +  public function cleanPackage($vendor_dir, CompletePackageInterface $package) {
    

    This does not need to be public API

  5. +++ b/core/lib/Drupal/Component/VendorCleanup/composer.json
    @@ -0,0 +1,23 @@
    +  "require-dev": {
    +    "composer/composer": "^1.8@stable",
    +    "mikey179/vfsstream": "^1.2",
    +    "phpunit/phpunit": "^6.5"
    +  },
    

    We're not added dev dependencies for any components so far and I don't think we should start without considering doing it for the rest and considering how we're going to manage them going forward.

Mile23’s picture

#11.2: The patch is from before #3057459: Add composer/composer as a dev dependency of core so that we can test Composer plugins so hopefully we can get that part solved over there.

#11.3: Having defaults is an easy way to get the same behavior without adding new config to drupal/drupal. As we can see, people get particular about their composer.json. :-) We can config either way moving forward, or we can just say: Always use the config in extra.

#11.5: We had started some work on that with #2943856: [meta] Reorganize Components so they are testable in isolation

alexpott’s picture

Regarding #11.3 I think the important point is that we need to merge the default list because the default list will change as we add dependencies.

Another thought is that this could be solved if we didn't support having vendor in webroot. Then this whole cleanup stuff is moot.

Mile23’s picture

Thanks for the reviews...

Re #13:

this could be solved if we didn't support having vendor in webroot.

Yes, that's why this is a plugin instead of continuing to use the script. #2982680: Add composer-ready project templates to Drupal core gives us a way to build the tarball using Composer. Then when someone says composer update to the tarball, the scaffold and this cleanup plugin can update with new config.

Regarding #11.3 I think the important point is that we need to merge the default list because the default list will change as we add dependencies.

So given that we expect this plugin to be updated along with core, we can't expect the root package extra field to be updated. Also, since the user could add new dependencies which they *could* maintain in their extra, you're right, it should merge.

This patch does that. It does not include the improvements from #3057462: Add missing packages to \Drupal\Core\Composer\Composer::$packageToCleanup. It includes the changes to root's composer.json, just so the tests will run. And of course, we'll finalize that in #3057459: Add composer/composer as a dev dependency of core so that we can test Composer plugins

This issue is basically postponed on #3057459: Add composer/composer as a dev dependency of core so that we can test Composer plugins

Mixologic’s picture

Since we are not going to be relying on drupal/drupal to do vendor cleanup for much longer, I propose that we do not try and make it work with this new plugin at all, and preserve the existing code in /core/lib/Drupal/Core/Composer/Composer.php.

drupal/drupal would only rely on that cleanup for as long as we're producing tarballs that do 'git clone drupal;composer install' which would only be until we get the other parts of the composer initiative in (this plugin, the scaffold, the template file). Once those are in we'd stop doing vendor cleanups in drupal/drupal, and they'd be handled by this plugin natively.

So, IMO, lets not touch anything in the root composer.json, and leave all the code in /core/lib/Drupal/Core/Composer/Composer.php

Once #3057459: Add composer/composer as a dev dependency of core so that we can test Composer plugins and #3057462: Add missing packages to \Drupal\Core\Composer\Composer::$packageToCleanup are in, we should incorporate the fixes from #3057462: Add missing packages to \Drupal\Core\Composer\Composer::$packageToCleanup.

Mile23’s picture

OK, here are the changes from #15.

We don't use the plugin, we just add it.

The dev dependencies are declared in core/composer.json. This duplicates #3057459: Add composer/composer as a dev dependency of core so that we can test Composer plugins which I thought had to be a separate issue. (#7 says add to IS, #8 says 'Here's an example of how you do it.') Anyway, we might still be blocked on that issue here, or maybe not, I dunno.

Edit: Arg. Neglected to remove the dev dependencies from the component.

Mile23’s picture

Once more, this time without dev dependencies in the component.

Mixologic’s picture

Status: Needs review » Needs work

looks like theres some extraneous stuff in the patch jsonapi/file module/ etc changes.

so if https://www.drupal.org/project/drupal/issues/3057459 goes in, we also wont need the drupal/core/composer.json or the root composer.lock changes

not sure if needs work or postponed...

Mile23’s picture

Status: Needs work » Needs review
FileSize
53.86 KB
15.71 KB

Weird. Not sure how those got in there. Reverted now.

Mixologic’s picture

Adding related, blocking issue. Also, since it blocking on that I think we may as well postpone here right?

Mile23’s picture

Mixologic’s picture

lol: "Alternately, you could use the vendor cleanup plugin to cut down on disk space requirements in your project." -me gusta

Mile23’s picture

Mile23’s picture

Issue summary: View changes
Issue tags: +Needs followup

Adding to the IS to point out that we need a follow-up to remove the usage of the event scripts from drupal/drupal, using the plugin instead. I'm not filing the followup issue because it might just be #2982680: Add composer-ready project templates to Drupal core, and we might decide to just keep the event scripts in drupal/drupal.

Re: #22... Check out the description of the aforementioned https://packagist.org/packages/mediamonks/composer-vendor-cleaner Its selling point is freeing up disk space, so there might be a need beyond ours.

BTW, it might be that we could get away with using https://packagist.org/packages/dg/composer-cleaner now that #3055040: Roll AbstractEventDispatcherTest into ContainerAwareEventDispatcherTest is in. It uses the autoload section from each package to determine what *not* to throw away, and then throws away everything else.

greg.1.anderson’s picture

I looked at https://packagist.org/packages/dg/composer-cleaner, and while it looks promising, it has a couple of shortcomings.

1. It cleans too aggressively. It may end up throwing away non-code assets in your projects that are still needed.
2. It cleans too often. While it can be configured to ignore some/all projects for cleaning, this switch must be added to the root composer.json file. The result is that if we added this as a dependency to a template Drupal project, then it would always remove the test directories, and there would be no opportunity to ever test the resulting project. It seems to me that this project should only clean after a Composer operation is run in `--no-dev` mode.

In the short term, I'd recommend continuing with our own Composer plugin in its current form. Later on, it might be worthwhile to try submitting a PR to dg/composer-cleaner to fix up these shortcomings, or perhaps just fork the project if the maintainer is not interested in the enhancements.

greg.1.anderson’s picture

I also looked at https://packagist.org/packages/mediamonks/composer-vendor-cleaner, and it aggressively cleans dependencies using the same logic as dg/composer-cleaner. Perhaps this means that it is rare for a Composer project to provide non-code assets; however, it seems imprudent to leave this to chance. If we use an auto-cleaner, I'd rather see it remove the autoload-dev directories, and leave everything else.

Mile23’s picture

Status: Needs review » Needs work

OK, let's have the plugin also look at autoload-dev and remove whatever's there. That way we can at least catch autoloadable test classes for new packages or ones where the test directory has moved.

greg.1.anderson’s picture

Status: Needs work » Needs review

On further reflection, I do not think that auto-cleaning based only on autoload-dev is sufficient. Some packages might have .php files that are not autoloaded at all, but still pose a security risk.

Maybe the best way to auto-clean would be to use the existing algorithm (clean everything NOT autoloaded), but only clean executable files (`*.php`). If the web server was configured to execute somethng other than `*.php` files, though, then this too would be insufficient.
In the short term, I think sticking with the existing blacklist code is safest.

Mile23’s picture

Over in #2873160: Implement core management of 3rd-party FE libraries (and there have to be other similar issues as well), there's discussion of using Composer for CSS and JS framework installation, so yes, an aggressive removal of files not autoloaded by PHP is probably a bad idea.

And over in #3039344: Use Symfony's simple-phpunit we're contemplating using Symfony's test runner as our test runner. That is a script which loads another script from a file ending in .php, so let's not perform the aggressive cleaning at all.

Note that in an ideal world, we wouldn't be asking anyone to manage CSS or JS with Composer, and we'd have a build-out tool for Drupal. But we don't, so alas. :-)

Added docs: https://www.drupal.org/docs/develop/using-composer/using-drupals-vendor-...

From that process I updated the README for clarity and also to be fake markdown, so there's a new patch.

greg.1.anderson’s picture

Status: Needs review » Needs work

This is looking pretty good. I see one easy optimization:

+++ b/core/lib/Drupal/Component/VendorCleanup/VendorCleanupPlugin.php
@@ -0,0 +1,145 @@
+  protected function deleteRecursive($path) {

This could be replaced with Composer\Util\Filesystem::removeDirectory() https://github.com/composer/composer/blob/1.8.6/src/Composer/Util/Filesy...

Mile23’s picture

Status: Needs work » Needs review
FileSize
38.18 KB
2.82 KB

Great idea. :-)

greg.1.anderson’s picture

Assigned: Unassigned » greg.1.anderson
Status: Needs review » Needs work

Operating scenarios:

1. composer.lock has core-vendor-clean project, vendor does not exist, run `composer install`: works
2. neither composer.lock nor vendor exists, run `composer install`: works
3. vendor exists, drupal/core-vendor-clean is installed, drupal/core is not installed, run `composer require drupal/core`: works
4. vendor exists, drupal/core is installed, drupal/core-vendor-clean is not installed, run `composer require core-vendor-clean`: does not work

The problem with scenario #4 is that the post-package install event is only called for the drupal/core-vendor-clean project; it is not called for any package that was installed before the event that installed drupal/core-vendor-clean. Once you are in this state, the already-installed packages will not be cleaned unless they are removed first for some reason (e.g. either via `composer remove`, or due to an `rm -rf vendor` or similar) and then re-installed, or if they are updated (a new version is tagged).

I'm working on an updated patch to address this feature gap.

greg.1.anderson’s picture

Assigned: greg.1.anderson » Unassigned
Status: Needs work » Needs review
FileSize
13.3 KB
39.9 KB

Updated patch includes post-install and post-update command hooks to ensure that all expected scenarios are covered.

- Existing post-package-install hook covered scenarios 1-3, but not scenario 4.
- New post-install / post-update hook covers scenarios 1, 2 and 4, but not scenario 3.

ultimike’s picture

I'm trying to get up-to-speed on this Composer plug-in, and I'm confused about one thing. This is going to be part of Drupal core (core/lib/Drupal/Component/), so when someone requires drupal/core for their project, they're going to get this plugin as well. But, the README indicates that to use this component, one must composer require drupal/core-vendor-cleanup - won't that trigger the plug-in to be installed in the /vendor directory of the project as well?

I'm not saying there's an issue here, I'm just saying I don't understand how it works :)

-mike

Mile23’s picture

The plan is that this plugin will be used as part of a composer project that will mimic the current 'tarball' download you get from drupal.org. #2982680: Add composer-ready project templates to Drupal core

As it is now in the patch, the plugin will live in core/lib/, so when you require drupal/core, the code for this plugin will be included with the core codebase.

However, it won't act as a plugin until you require drupal/core-vendor-cleanup in your composer.json. It will be a subtree split that will turn into its own Composer package.

It might be that a follow-up will place this plugin (and other ones) somewhere else in the codebase, so it's not inside the core/ directory. However, in this issue we're following the lead of #2982684-48: Add a composer scaffolding plugin to core

Chi’s picture

How will this plugin work with composer create-project command? Since the plugin itself is not installed at that moment I suppose it would require to run something like rm -rf vendor && composer install to clean vendors after the project creation.

Mile23’s picture

Status: Needs review » Needs work

This is a child issue of #2982680: Add composer-ready project templates to Drupal core

The goal is to be able to say composer create-project drupal/drupal-project-legacy and have it build a Drupal that is identical to the tarball layout, without any of the down-sides of just using the repo.

If that doesn't make any sense, then come on over to #composer on drupal.slack.com and start asking. :-)

Note that in #34 Greg made it operate at a post-command scope, which addresses the concern in #37.

That leads to a NW status for updating the readme and the docs page.

+++ b/core/lib/Drupal/Component/VendorCleanup/README.txt
@@ -0,0 +1,60 @@
+Require this Composer plugin into your project:
+
+    composer require drupal/core-vendor-cleanup
+
+Then perform a clean install or update:
+
+    $ rm -rf vendor/
+    $ composer install

Not true any more.

Mile23’s picture

Status: Needs work » Needs review
FileSize
138.01 KB
14.67 KB
1.26 KB
  • Updated the readme.
  • Shuffled responsibilities around for outputting messages.
  • Renamed some things for clarity.
  • Added a getInstalledPackages() method so we can test it with less mocking.
  • Added tests for the methods introduced in #34.

If you want to use the plugin, apply use_plugin_DO_NOT_TEST.patch and do composer update drupal/core-vendor-cleanup

Mile23’s picture

You know... You work hard to make a good patch and then you don't because you forgot to rebase.

Let's try that again.

ultimike’s picture

@Mile23 - thanks for the explanation to my question in comment 35 - much appreciated.

So, to put it another way, the drupal/core-vendor-cleanup plugin's code is part of Drupal core. When the Drupal.org packager runs, it will split out drupal/core-vendor-cleanup as it's own Composer package (this is the "subtree split" stuff). So, it will act like a normal Composer plugin and only be available after it is added as a dependency to a project. All correct?

The result of this is that the drupal/core-vendor-cleanup code will be in two places in a Drupal project: in core/lib/ as well as in the project's vendor directory, correct?

-mike

Mile23’s picture

The overall goal is that you'll be able to say composer create-project drupal/drupal-project-legacy. That's what #2982680: Add composer-ready project templates to Drupal core is all about.

@ultimike is correct: Because the plugin in the patch is currently in core/lib/Drupal/Components/, after you issue that command, it will be in two places in the filesystem:

  • core/lib/Drupal/Components/VendorCleanup
  • vendor/drupal/core-vendor-cleanup/

Composer will use the one in vendor/ to do the cleanup.

There are two reasons the plugin's code is in core/lib/Drupal/Components/:

  • @alexpott said the scaffold plugin should be there in #2982684-48: Add a composer scaffolding plugin to core so I put it there, too.
  • We currently don't have a way to run tests that are not in core/tests, and if we were to locate this plugin somewhere other than to mimic the other components, we'd have to invent that.

In my ideal world, we'd do both of those things: We'd move this plugin to somewhere other than core/, and we'd figure out how to run its tests. But I don't maintain Drupal core, so it's not my call. :-) Here's a postponed issue dealing with retooling the test system to do that kind of thing: #2943856: [meta] Reorganize Components so they are testable in isolation

greg.1.anderson’s picture

Finished reviewing and did not find any further issues. I think this patch is RTBC, but not setting the status because I provided part of the patch.

Mixologic’s picture

Status: Needs review » Reviewed & tested by the community

Given that this is mostly reimplementing existing composer scripts as a component/plugin that already exists in core, and that it's only purpose is to help the security of a use case that we discourage (having vendor in webroot), and can hopefully deprecate someday, I can't find anything that needs adjusting, polishing, or improving.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/composer.json
@@ -59,6 +58,10 @@
+        {
+            "type": "path",
+            "url": "./core/lib/Drupal/Component/VendorCleanup"
         }

+++ b/composer.lock
@@ -674,6 +674,38 @@
+            "version": "3057094.x-dev",

I'm not sure we should be using this way of including components. The version added to composer.lock is completely wrong. We need to fix that somehow.

This was a review of #4 :(

catch’s picture

It's not clear to me whether the merge vs. require issues were actually solved in this issue or deferred to #2982680: Add composer-ready project templates to Drupal core. Agreed with not trying to autoclean - maybe we can get there one day but not yet.

Couple of minor nits:

  1. +++ b/core/lib/Drupal/Component/VendorCleanup/README.txt
    @@ -0,0 +1,56 @@
    +auto-magically.
    

    I think this can just be 'automatically'.

  2. +++ b/core/lib/Drupal/Component/VendorCleanup/VendorCleanupPlugin.php
    @@ -0,0 +1,222 @@
    +      $this->io->write(sprintf("    Cleaning directories in <comment>%s</comment>", $package_name), TRUE, IOInterface::VERY_VERBOSE);
    

    Why all the left padding everywhere? If we need the left padding, should we move it to a variable or constant to save having to type exactly the right amount of spaces if we add a new message?

Also I don't know what I'm doing wrong but I can't even see the composer.json and composer.lock changes in the latest patch?

alexpott’s picture

Mixologic’s picture

@mile23, correct me if Im wrong, but I think the left padding was merely replicated from the original script, which looks like was introduced by @alexpott here: https://www.drupal.org/project/drupal/issues/2664274#comment-10952557.

I believe the left padding is there for console readability. Sometimes we indent 4 spaces, sometimes 6, depending on the context. Im not sure if we'd want to introduce a constant if it would end up being TAB.

Regarding the composer.json, yes, we should probably add this to core's composer.json like we do for all the other components - with a replace and a merge -> I've added them in this patch.

We have a test to catch issues like that for modules: https://git.drupalcode.org/project/drupal/blob/8.8.x/core/tests/Drupal/T...

But not for components. Perhaps we should have one?

Mile23’s picture

Fixing #46.

The padding is there for console readability, which is an imperfect science anyway... But now it has str_repeat(). For some other application we might have a more formalized pretty console layer but this is small fish.

Re: #48

ComposerIntegrationTest is interesting... It hardcodes all the component paths, and you can see in the patch that it's updated for VendorCleanup. (BTW, Scaffold is missing.)

But... There's not a test that says all components should have a replace/merge. I'm not sure all of them should. Let's figure that out in a followup.

I'd added 'needs followup' tag in #25, but I think it's pretty clear that we're going to iterate all over this thing in #2982680: Add composer-ready project templates to Drupal core. Leaving the tag so we remember to update ComposerIntegrationTest.

greg.1.anderson’s picture

This is pretty minor, but I'm -1 on using str_repeat() for padding, since Composer does not do this. Here's an example of how Composer handles padding:

https://github.com/composer/composer/blob/1.8.6/src/Composer/Downloader/...

Speaking of this, we should use 'writeError' rather than 'write', as Composer does, here and in the Scaffold component.

We can keep the str_repeat() if folks are more comfortable with that.

Mile23’s picture

Not totally happy with str_repeat(), but #46. If there's a better way then all suggestions welcome.

Switched all write() to writeError().

Picked up some indentations I missed before for some reason.

Normalized on sprintf() rather than concatenation.

catch’s picture

Since these are all in the same method could we do something like this? I know it's minor but the spaces look like a mistake and the str_repeat() looks a bit clunky.

  $padding = '    ';

  $this->io->write($padding . sprintf("Cleaning directories in <comment>%s</comment>", $package_name), TRUE, IOInterface::VERY_VERBOSE);

greg.1.anderson’s picture

$padding4 = str_repeat(' ', 4);, maybe?

The str_repeat is not great, but I think that factoring out the padding without using str_repeat is a half-measure that does not improve readability enough. We should go completely one way or the other. Also note that although the padding is all consistent in this instance, depending on the context, Composer will use a different amount of padding. To me, it seems more confusing to have both $padding = ' '; and $padding = ' '; than to just have variable padding hardcoded into each string. The Scaffold plugin has a lot of padded messages as well, and I imagine whatever we decide here will eventually be adopted there as well.

Mile23’s picture

OK, how about this?

$this->io->writeError(sprintf('%sCleaning: <info>%s</info>', str_repeat(' ', 4), $package_name));
greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

Putting this back to RTBC because we need a core committer to decide on #54 (which looks good to me). I think this is probably okay once this code style issue is decided on.

catch’s picture

#54 looks good and intelligible to me, we can always change later just better to have something obviously intentional.

composer.json changes also look fine - just to 100% confirm this has no impact on composer.lock?

greg.1.anderson’s picture

@catch: It does not appear that the core/composer.json changes have any effect on the composer.lock at the root of the project:

[3057094_54] ~/Code/open-source/drupal/drupal-8.8.x$ composer validate
./composer.json is valid
[3057094_54] ~/Code/open-source/drupal/drupal-8.8.x$ composer install
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update
Package phpunit/phpunit-mock-objects is abandoned, you should avoid using it. No replacement was suggested.
Generating autoload files
> Drupal\Core\Composer\Composer::preAutoloadDump
> Drupal\Core\Composer\Composer::ensureHtaccess
   Skipped installation of bin bin/composer for package composer/composer: file not found in package

Composer would complain that the composer.lock was out-of-date if even-insigificant changes were made to the root composer.json file; insignificant changes in dependencies composer.json files do not matter, though.

bojanz’s picture

Status: Reviewed & tested by the community » Needs work

Nitpick time!

+
+  public function __construct(RootPackageInterface $root_package) {

Missing docblock for the constructor.

+  /**
+   * Get the configured list of directories to remove from the root package.
+   *
+   * This is stored in composer.json extra:drupal-core-vendor-cleanup.
+   *
+   * @return array[]
+   *   An array keyed by package name. Each array value is an array of paths,
+   *   relative to the package.
+   */

"Get" should be "Gets'. None of the docblocks seem to be using the third person here, per convention?

+  /**
+   * If there is no configuration in the root package, it is unconfigured.
+   *
+   * @return bool
+   *   TRUE if the root package does not have a list of packages with
+   *   directories to clean.
+   */
+  public function isUnconfigured() {
+    return empty($this->getAllCleanupPaths());
+  }

It's a bit odd to have an isUnconfigured() method, I'd expect a isConfigured() instead.
The docblock is also odd, I'd expect something like "Determines whether..."
Furthermore, it doesn't seem that this method is actually used anywhere?

+/**
+ * A Composer plugin to clean out your project's vendor directory.
+ *
+ * A style guide for verbose messages:
+ * - Verbose is for things that happened that would be annoying if we set them
+ *   to normal.
+ * - Very verbose is for verbose things that we tried whether they happened or
+ *   not.
+ */

I'd love an actual example here for what it means to "clean out my vendor directory". What's removed and for what purpose?
The style guide bit also feels like a development note. I have no clue "that would be annoying if we set them to normal" means.

+  /**
+   * Convenience method to get all installed packages from Composer.
+   *
+   * @return \Composer\Package\PackageInterface[]
+   *   The list of installed packages.
+   */

Why would we say "Convenience method to..." instead of just "Gets a list of installed packages from Composer."?

Mile23’s picture

Status: Needs work » Needs review
FileSize
44.67 KB
2.59 KB

Nitpick time!

Excellent. :-)

Addressed everything.

isUnconfigured() was vestigial, it turns out, so it's been removed.

getInstalledPackages() was a 'convenience' method because it exists to be mocked in tests. Better description now.

Thanks.

greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

#59 addresses #58 and tests are green - back to RTBC.

greg.1.anderson’s picture

Status: Reviewed & tested by the community » Postponed
Mile23’s picture

Status: Postponed » Reviewed & tested by the community

This isn't actually postponed on #2912387: Stop using wikimedia/composer-merge-plugin, but one or the other will need a re-roll if the other commits first.

Re-setting to RTBC from #60, so we know if we need a re-roll before then. :-)

greg.1.anderson’s picture

Sure, I'm hip to re-roll after a merge here, whichever goes in first. +1 on re-RTBC'ing here.

Mixologic’s picture

Mile23’s picture

Status: Needs review » Reviewed & tested by the community
Mixologic’s picture

doh. unsure why all that form stuff updated.

greg.1.anderson’s picture

Status: Reviewed & tested by the community » Needs work

This needs a re-roll now that #2912387: Stop using wikimedia/composer-merge-plugin has been committed. Pretty much just needs the path repository, and make sure that it is required, not replaced.

Mixologic’s picture

Also, I was reviewing https://www.drupal.org/project/drupal/issues/2990257 and wondering about the ensurehtaccess script (https://git.drupalcode.org/project/drupal/blob/8.8.x/core/lib/Drupal/Cor...) and whether or not those should be files that are scaffolded, and whether /vendor is a place we can scaffold *to*.

And Greg pointed out in https://www.drupal.org/project/drupal/issues/3067645#comment-13203701 that we should probably move that functionality to here, in this plugin, since it really is a 'securing vendor that shouldnt be in you docroot anyhow, but here, we'll fix that bad idea for you as best we can' plugin.

So, I wonder if maybe we dont want to name this "cleanup" and maybe want to add that .htaccess/web.config stuff here and call it a 'vendor hardening plugin' ?

greg.1.anderson’s picture

+1 for "vendor hardening" or some similar name.

leolandotan’s picture

Hi guys,

I wanted to contribute even just a little and worked on re-rolling the patch from comment 59. I followed the steps from https://www.drupal.org/patch/reroll but I encountered a patch conflict which is due to the mentioned change in #2912387. I looked into it and noticed that the whole `extra` section was removed in favor of using the path repository(not very familiar) so in the conflict I removed the whole `extra` section as well.

As for the "vendor hardening". I'm not very familiar on what to do so I'll just not change the status to Needs Review for now.

Hope everything is in order.

Thanks!

Mile23’s picture

Status: Needs work » Needs review
FileSize
44.61 KB
11.25 KB

Thanks @leolando.tan. :-)

This patch accomplishes the rename.

Adding the .htaccess stuff means changing the way config works, so we can turn it on or off, or to allow the use of a bespoke file in the package, so that will require more work than I'm going to do this evening. :-)

So this is still 'needs work' even if the tests pass.

Mile23’s picture

Missed a file rename.

Status: Needs review » Needs work

The last submitted patch, 72: 3057094_72.patch, failed testing. View results

andypost’s picture

Related issues: +#2620304: htaccess functions should be a service

In related issue "security logger" introduced
probably special core component could be used in both places

greg.1.anderson’s picture

This will need to be relocated to the 'Plugin' directory per https://www.drupal.org/project/drupal/issues/2912387#comment-13204485

Mixologic’s picture

+++ b/core/composer.json
@@ -98,6 +98,7 @@
+        "drupal/core-vendor-cleanup": "self.version",

+++ b/core/lib/Drupal/Component/VendorHardening/composer.json
@@ -0,0 +1,20 @@
+  "name": "drupal/core-vendor-hardening",

package ought to match.

Re: #74 interesting, This is all in a composer plugin, so we cant use any drupal services etc. And it looks like we're both relying on Drupal\Component\PhpStorage\FileStorage to get the contents of the .htaccess, so Im not sure there's much more that can be abstracted out.

Mile23’s picture

Status: Needs work » Postponed
Related issues: +#2878269: Modify TestDiscovery so the testbot runs all the tests

If I move the plugin to core/lib/Drupal/Plugin, that means the tests should move to core/tests/Drupal/Tests/Plugin.

However, we can't do that because the testbot won't run tests in that directory: #2878269: Modify TestDiscovery so the testbot runs all the tests

This issue is postponed on that issue.

Also we can't/shouldn't use Drupal services (or even really the deprecated functions) in a Composer plugin, so #2620304: htaccess functions should be a service doesn't matter here. We're going to replicate Drupal\Core\Composer\Composer::ensureHtaccess() here, with some added configurable magic.

Mile23’s picture

Status: Postponed » Needs work
greg.1.anderson’s picture

Yeah, if we follow the pattern of the Scaffold plugin, there will be more warnings with composer install -o, but it will work.

Mile23’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
46.89 KB
10.9 KB

It turns out #2620304: htaccess functions should be a service is a bit relevant, because there's some discussion over there of de-component-ifying drupal/core-php-storage, and it turns out that's now a dependency of drupal/core-vendor-hardening for DRY reasons. If the htaccess writer from that issue were a component, it would be very handy here, and after that issue is committed we might revisit how these two things fit together.

So thanks, @andypost.

This patch always places .htaccess and web.config in the vendor directory. If there is hue and cry, I can add a configuration to turn that off, or to place an alternate file as needed.

There's also a patch called 3057094_80_local_do_not_test.patch. Apply this along with the other one and then do: composer update drupal/core drupal/core-vendor-hardening --with-dependencies in order to run it. We include drupal/core because you might be on a branch other than 8.8.x, and without the merge plugin and with a lock file there will be conflicts.

I suggest removing the vendor directory and then doing composer install to see all the stuff happen, like this:

  - Updating guzzlehttp/psr7 (1.4.2 => 1.6.1): Loading from cache
    Cleaning: guzzlehttp/psr7
  - Updating brumann/polyfill-unserialize (v1.0.3 => v1.0.4): Loading from cache
    Cleaning: brumann/polyfill-unserialize
Package phpunit/phpunit-mock-objects is abandoned, you should avoid using it. No replacement was suggested.
Writing lock file
Generating autoload files
Hardening vendor directory with .htaccess and web.config files.
    Skipped installation of bin bin/composer for package composer/composer: file not found in package
Nothing scaffolded because no packages are allowed in the top-level composer.json file.
Cleaning vendor directory.

Status: Needs review » Needs work

The last submitted patch, 80: 3057094_80.patch, failed testing. View results

Mile23’s picture

Status: Needs work » Needs review
FileSize
706 bytes
46.82 KB

I was unable to repro the failing tests other than ComposerIntegrationTest. I think the testbot didn't re-perform composer install, even though I changed the composer.json file.

This is the fix to ComposerIntegrationTest.

Status: Needs review » Needs work

The last submitted patch, 82: 3057094_82.patch, failed testing. View results

Mixologic’s picture

yeah, testbot is looking for changes to composer.lock for core changes.

Mile23’s picture

Title: Add Composer vendor/ cleanup plugin to core » Add Composer vendor/ hardening plugin to core
Status: Needs work » Needs review
FileSize
47.01 KB
829 bytes

Fixed typo, added a line to composer.lock to trick the testbot.

Update: Trick successful.

15:46:42 ----------------   Starting update_dependencies   ----------------
[..]
15:46:42 Generating autoload files

We should make a followup for the testbot to update the dependencies if composer.json is patched even if composer.lock isn't. I'm pretty sure there's an issue about not using a lock file in the first place...

Mile23’s picture

Given that we removed the merge plugin from core....

I had to do all that stuff with adding a line to the lock file because when you're on a work branch in git (in this case named for the issue id: 3057094), a composer version of self.version will become 3057094.x-dev. The dev branch for core is 8.8.x, leading to a locked version requirement of 8.8.x-dev for drupal/core. However, since we're on another branch, Composer can't satisfy a need for drupal/core 8.8.x-dev, because it only has drupal/core 3057094.x.dev.

This means that if you try to perform composer update --lock, it will show you something like this:

$ composer update --lock
Loading composer repositories with package information
Updating dependencies (including require-dev)
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Can only install one of: drupal/core[3057094.x-dev, 8.8.x-dev].
    - Can only install one of: drupal/core[8.8.x-dev, 3057094.x-dev].
    - Can only install one of: drupal/core[8.8.x-dev, 3057094.x-dev].
    - Installation request for drupal/core 3057094.x-dev -> satisfiable by drupal/core[3057094.x-dev].
    - Installation request for drupal/core (locked at 8.8.x-dev, required as self.version) -> satisfiable by drupal/core[8.8.x-dev].

So the way around this is: While working, you can say composer update drupal/core --with-dependencies and you'll be able to change composer requirements. This is no big deal at this point, as long as you don't generate a patch. If you were to generate a patch, and the lock file contained a reference to your branch name version, the testbot won't be able to build the codebase because it isn't using that branch name. It will see Composer error message similar to the one above.

So when it comes time to generate a patch, you should do the following:

First, you can check whether you actually changed the lock file during work:

  • git checkout 3057094
  • git diff 8.8.x composer.lock

If there's no difference, then you're golden and so get on with making a patch.

If there is a difference, however, you'll need to do the branch dance:

  • Commit all your changes to your work branch.
  • git checkout 8.8.x
  • git merge 3057094 --squash (This will add changes but not commit them.)
  • git checkout -- composer.lock
  • composer update --lock

Now you can generate a patch for your lock file: git diff composer.lock > my_lock_file.patch, go back to your work branch and apply it, commit it, and then make a patch.

Piece of cake, right?

However................

In the case of this particular issue, changing the autoload-dev section didn't actually change the lock file at all, which means that the testbot is still broken for this use case. I had to add an extra line at the end of composer.lock to get it to re-run composer install after applying the patch, so that's the hacky workaround.

The testbot is looking for changes to core/composer.json or composer.lock, but not the root composer.json. Since we're doing a lot of work on the root composer.json, we should probably modify the testbot to deal with it.

Mile23’s picture

Mile23’s picture

Added testbot issue to deal with orphaned composer.json changes: #3072484: Composer dumpautoload if composer.json is modified but lock file is not

greg.1.anderson’s picture

Status: Needs review » Postponed

Marking as postponed on #2912387: Stop using wikimedia/composer-merge-plugin

One of our learnings from that issue is that if we commit a plugin at one path, and then move it to another location, then folks can have trouble running composer install unless they remove their vendor directory first. Let's keep things simple and add this for the first time after the relocation to Plugins is done.

Mile23’s picture

Status: Postponed » Needs review

#87 patches to core/lib/Drupal/Plugin/ because you asked me to in #75. In fact as per #15 the plugin itself is unused and contains no path repositories so there will not be any conflict with installed plugins.

It should be soft-postponed on #3072484: Composer dumpautoload if composer.json is modified but lock file is not, only because it adds an extra newline to composer.lock to trick the testbot into dumping the autoload. If the existence of an extra newline in composer.lock is acceptable until that file gets changed again in a later commit, then we don't need to postpone based on that.

And: As long as we have decided to put these plugins in core/lib/Drupal/Plugin, there will not be a conflict with the scaffold move from #2912387: Stop using wikimedia/composer-merge-plugin. I'd again argue that we should put Composer plugins outside of core/, but no one but me seems to think it matters, despite the problems arising from needing to move things around.

Mile23’s picture

In a chat with @greg.1.anderson, we decided to move the plugins to a separate composer/ folder, outside of core/.

This patch makes the move for this Composer plugin. A separate follow-up will eventually move the scaffold plugin similarly. Tests remain in core/tests/ because otherwise the test system doesn't know how to run them. We'll reconcile these issues in #2943856: [meta] Reorganize Components so they are testable in isolation or a similar issue.

As always, to see it in action, do this:

$ git apply run_locally_do_not_test.patch 
$ composer update drupal/core-vendor-hardening --with-dependencies

Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 2 installs, 0 updates, 0 removals
  - Installing drupal/core-php-storage (3057094.x-dev): Symlinking from core/lib/Drupal/Component/PhpStorage
  - Installing drupal/core-vendor-hardening (3057094.x-dev): Symlinking from composer/VendorHardening
Package phpunit/phpunit-mock-objects is abandoned, you should avoid using it. No replacement was suggested.
Writing lock file
Generating autoload files
> Drupal\Core\Composer\Composer::preAutoloadDump
Hardening vendor directory with .htaccess and web.config files.
    Skipped installation of bin bin/composer for package composer/composer: file not found in package
Cleaning vendor directory.

The last submitted patch, 91: 3057094_91.patch, failed testing. View results

Mile23’s picture

Woops. Forgot to hack composer.lock. Let's just wait on results from #3072484: Composer dumpautoload if composer.json is modified but lock file is not

greg.1.anderson’s picture

After our chat, I was wondering if we should move these to composer/Plugin/VendorHardening & c. instead. The reason I suggest this is that in #2982680-25, "composer" or "composer_templates" was suggested as a location to store the template projects. Having paths composer/Plugin and composer/Template seems like it would make sense.

Mixologic’s picture

Adding a couple of random composer issues that will probably get wontfixed when this goes in.

Mile23’s picture

Moved to composer/Plugin.

Updated ComposerIntegrationTest with new path.

Includes composer.lock workaround for the testbot.

Mile23’s picture

Semi-related issue seeks to change the contents of .htaccess: #2619250: Make .htaccess usage work for the widest possible configurations without relaxing security and document pitfalls

Still looking at #2620304: htaccess functions should be a service because it might change the way the component works.

Mile23’s picture

Mile23’s picture

#3072484: Composer dumpautoload if composer.json is modified but lock file is not is in, so here's a reroll without the hacked lock file.

Mixologic’s picture

LGTM.

The only concern I have is with the cross component requirement on drupal/core-php-storage:

+++ b/composer/Plugin/VendorHardening/composer.json
@@ -0,0 +1,21 @@
+    "drupal/core-php-storage": "self.version"

If this plugin is required directly by the template file, it may have the effect of 'locking' the requirement on drupal/core-php-storage at whatever version was installed, which in turn can cause drupal/core to be locked to that same version.

i.e. The composer create-project drupal/drupal-legacy-template may end up with drupal/core-php-storage: 8.8.0 in the lockfile, and at a later date when the user goes to update core to 8.8.1 they may see the ol' composer message where it says that it cannot upgrade to 8.8.1 because of the hard requirement on 8.8.0.

Im wondering if it would be better to not use 'self.version' and instead use whatever the most recent php-storage is that would work for our purposes, which looks like ^8.6

Mile23’s picture

Status: Needs review » Postponed

Good point. Since the only thing that depends on drupal/core-vendor-hardening is the root project template, self.version will always map to that. If a user says composer update drupal/core --with-dependencies, there could be a conflict.

The other thing is that we must decide on #3013210: Add drupal/core-filesecurity component for writing htaccess files before we move here, for the same reason. If drupal/core-php-storage no longer provides the .htaccess content in the future, then users would need to composer update rather than composer install because they committed their lock file.

Setting this as postponed on that.

Mixologic’s picture

Hah. What if we said "drupal/core-php-storage: 8.7.0"

We know that one works and if core meddles with the future of php-storage we can adapt later on.

Not sure if I'm kidding or not.

greg.1.anderson’s picture

My suggestion would be to COPY htaccessLines here, and then DRY it up after #3013210: Add drupal/core-filesecurity component for writing htaccess files lands. That would allow us to un-postpone this issue, wouldn't it?

greg.1.anderson’s picture

#3013210: Add drupal/core-filesecurity component for writing htaccess files should be pretty easy to finish up and get in, so I changed my mind about adding more technical debt per #103.

greg.1.anderson’s picture

Issue summary: View changes

Added a follow-on task to the proposed resolution in the issue summary.

greg.1.anderson’s picture

We should probably also do #3076234: Relocate Scaffold plugin outside of 'core' directory before continuing here, so that we do not have our two plugins located in two different locations.

Mile23’s picture

Status: Postponed » Needs work
Mile23’s picture

Status: Needs work » Needs review
FileSize
45.91 KB
1.99 KB

Amended to use drupal/core-file-security:^8.8.

greg.1.anderson’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup

The Scaffold plugin has already been relocated, so removing the follow-on work here.

This is working well now! RTBC

greg.1.anderson’s picture

Issue summary: View changes

Update issue summary to link to the follow-on issue #3076684: Remove deprecated vendor cleanup scripts.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs framework manager review
  1. +++ b/composer/Plugin/VendorHardening/Config.php
    @@ -0,0 +1,191 @@
    +    if (isset($config[$package])) {
    +      $paths = $config[$package];
    +    }
    +    else {
    

    we can return here and avoid the else

  2. +++ b/composer/Plugin/VendorHardening/Config.php
    @@ -0,0 +1,191 @@
    +        if (strtolower($key) == strtolower($package)) {
    

    we should be using === here, but instead - why don't we just normalize the package names in the ::getAllCleanupPaths method - support for upper case package names is deprecated in composer, so should we just lower case them in that method?

  3. +++ b/composer/Plugin/VendorHardening/VendorHardeningPlugin.php
    @@ -0,0 +1,249 @@
    +        if (is_dir($cleanup_path)) {
    +          if ($fs->removeDirectory($cleanup_path)) {
    +            $this->io->writeError(sprintf("%sRemoving directory <info>'%s'</info>", str_repeat(' ', 4), $cleanup_item), TRUE, IOInterface::VERBOSE);
    +          }
    +          else {
    +            // Always display a message if this fails as it means something
    +            // has gone wrong. Therefore the message has to include the
    +            // package name as the first informational message might not
    +            // exist.
    +            $this->io->writeError(sprintf("%s<error>Failure removing directory '%s'</error> in package <comment>%s</comment>.", str_repeat(' ', 6), $cleanup_item, $package_name), TRUE, IOInterface::NORMAL);
    +          }
    +        }
    +        else {
    +          // If the package has changed or the --prefer-dist version does not
    +          // include the directory. This is not an error.
    +          $this->io->writeError(sprintf("%s<comment>Directory '%s' does not exist.</comment>", str_repeat(' ', 6), $cleanup_path), TRUE, IOInterface::VERY_VERBOSE);
    +        }
    

    we could do away with this nesting if we used continue to return early instead of else

hussainweb’s picture

Status: Needs work » Needs review
FileSize
4.63 KB
45.83 KB

Just addressing the feedback in #111 right now. Anything that removes else blocks is good for me. :)

hussainweb’s picture

I also like the idea of changing the case of package names in :: getAllCleanupPaths method itself. This completely removes the else block in ::getPathsForPackage method. I did this in another patch so that we could make a decision separately.

hussainweb’s picture

+++ b/composer/Plugin/VendorHardening/Config.php
@@ -139,7 +139,7 @@ public function getAllCleanupPaths() {
+      $this->configData = array_change_key_case($package_config['drupal-core-vendor-hardening'], CASE_LOWER);

CASE_LOWER is actually the default and we don't need to say that, but I put it to make it clearer.

+++ b/composer/Plugin/VendorHardening/Config.php
@@ -0,0 +1,175 @@
+      if (isset($this->configData[$package])) {
+        $this->configData[$package] = array_merge($this->configData[$package], $paths);
+      }
+      else {
+        $this->configData[$package] = $paths;
+      }
+    }

I would personally reduce this to use a ternary operator but I don't know if everyone here likes the idea, so I didn't do that. In fact, I would have probably reduced it to something like this:

$this->configData[$package] = array_merge($this->configData[$package] ?? [], $paths);

I don't know if everyone finds this readable enough, though.

larowlan’s picture

$this->configData[$package] = array_merge($this->configData[$package] ?? [], $paths);

I'd be +1 on that too, we have php7, lets use it

Status: Needs review » Needs work

The last submitted patch, 113: 3057094-113.patch, failed testing. View results

hussainweb’s picture

Status: Needs work » Needs review
FileSize
1.7 KB
45.26 KB

Fixing the test, making the change as per #115 and another small change I noticed.

hussainweb’s picture

Actually, it could be even shorter and easier to understand.

The last submitted patch, 117: 3057094-117.patch, failed testing. View results

hussainweb’s picture

Ah, the failure was because I didn't read the test correctly. getAllCleanupPaths would always return keys with lowercase now.

The last submitted patch, 118: 3057094-118.patch, failed testing. View results

greg.1.anderson’s picture

Status: Needs review » Needs work

Regarding #115, in the past xjm rejected PHP 7 use because it makes it harder to backport bugfixes to earlier versions that need to run on PHP 5. One might think it was still okay to use PHP 7 here, since the Vendor Hardening Plugin will never be backported. However, the last instance was in the Scaffold plugin, which similarly will not be backported.

In summary, my understanding is that Drupal 8.8.x is PHP 7+ for dependencies only, and we need to wait until Drupal 9 to use PHP 7+ constructs ourselves.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
1.05 KB
45.37 KB

Well, that's not fun but here we go. I have not reverted to the previous code but changed it in a way which makes it easier to change back to PHP 7 version whenever we want.

Mixologic’s picture

Status: Needs review » Needs work

In summary, my understanding is that Drupal 8.8.x is PHP 7+ for dependencies only, and we need to wait until Drupal 9 to use PHP 7+ constructs ourselves.

I think the main thing was that *some* php7+ constructs require us to update our phpcs polices so that we dont end up with a variety of patterns to use the same thing.

However, I do believe that usages of the null coalesce operator are acceptable as there isn't really more than one way to do that thing, and there are already examples in core.

+++ b/composer/Plugin/VendorHardening/Config.php
@@ -0,0 +1,175 @@
+    'mikey179/vfsStream' => ['src/test'],

We'll need to make sure all of these configs are lowercase as well.

I just tried this on a local install and it doesnt match, thus doesnt clean :

...
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Installing mikey179/vfsstream (v1.6.5): Loading from cache
> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
  - Installing phpdocumentor/reflection-docblock (2.0.4): Loading from cache
    Cleaning: phpdocumentor/reflection-docblock
...
Mile23’s picture

Status: Needs work » Needs review
FileSize
46.52 KB
2.59 KB

Changed the defaults to be lowercase.

Defensively performs lowercase on the defaults, because we'll patch the defaults and they'll invariably be mixed-case.

Adds a test to prove those things work.

greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @hussainweb and @Mile23 for the updates! Tested the new plugin, and found it to be working well. Issues raised in #111 seem to be resolved. Setting back to RTBC.

  • larowlan committed a42b1c5 on 8.8.x
    Issue #3057094 by Mile23, hussainweb, greg.1.anderson, Mixologic,...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/core/tests/Drupal/Tests/Composer/Plugin/VendorHardening/ConfigTest.php
@@ -83,4 +83,40 @@ public function testRootMergeConfig() {
+      'BeHatted/Mank' => ['tests'],
+      'SymFunic/HTTPFoundational' => ['src'],

chuckle

Committed a42b1c5 and pushed to 8.8.x. Thanks!

larowlan’s picture

Removing the 'needs release manager review' tag as that was added when earlier versions of this patch were adding new dev dependencies, this was later removed

Status: Fixed » Closed (fixed)

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

rfay’s picture

So... the composer complaint "Skipped installation of bin bin/composer for package composer/composer: file not found in package" results in a dangling link (vendor/bin/composer pointing to nonexistent vendor/composer/composer/bin/composer - even the composer/bin dir doesn't exist) I believe, which breaks a tar package. Can anybody enlighten me about what causes the "file not found in package"? This seems to be taken for granted in this issue.

Mixologic’s picture

re #131: So I've seen that message, frequently, but I've never actually stopped to investigate it because it didn't seem to affect anything that I was doing, and for some reason just assumed it was something upstream.

But what it is, is a bona fide bug that escaped our attention, despite it fanning its plumage for us, often : #57 / #80/ #91

The patch in #24 was supposed to do additional cleanups because of https://www.drupal.org/project/drupal/issues/3057459, and added: 'composer/composer' => ['bin'], to the cleanup configuration, however we never really discussed whether that was the right thing to do.

I think the idea was that "if a user needs the vendor cleanup plugin, its because they have vendor installed under their web root, and composer is never meant to be *secure* in a web environment, so lets get rid of its bin dir so that nobody can rummage around in there looking for a vulnerability. But, because composer/composer's composer.json says its there, composer is making the bad symlinks (which is weird. I'd think it would just fail without making nonexistant links)

Anyhow, I think we should probably open up a followup and not clean out the composer/bin dir.

OTOH, @rfay, I would also suggest that you might want to add --no-dev to your composer command when you're trying to do tar packaging - do you need development dependencies in a tarball? (stranger things have happened, so Im not ruling it out, just giving you a potential way to move forward if this is blocking you)

Mile23’s picture

#132 is correct. And just to say: The composer that's not available is the one that's there for dev purposes, not the composer binary you'll run at the command line. If your packaging adds a local composer under vendor and you want to tarball *that*, then we might be able to add some kind of accommodation to the plugin, or figure out another solution.

@rfay please if you could file a child issue to this one so we can understand the use case and work with it. Thanks.

rfay’s picture

This is for quicksprint, the package used at Drupalcon contribution time, does need to be the full-on dev package. I worked around the problem so it's not an issue for quicksprint now, but I agree it's a bug. I opened https://www.drupal.org/project/drupal/issues/3082866

cilefen’s picture