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
Comment | File | Size | Author |
---|---|---|---|
#125 | interdiff.txt | 2.59 KB | Mile23 |
#125 | 3057094_125.patch | 46.52 KB | Mile23 |
#123 | 3057094-123.patch | 45.37 KB | hussainweb |
#123 | interdiff-120-123.txt | 1.05 KB | hussainweb |
#120 | 3057094-120.patch | 45.24 KB | hussainweb |
Comments
Comment #2
Mile23Comment #3
Mile23Note 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.
Comment #4
Mile23Here'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: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.
Comment #5
Mile23Added 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:
When we update we find that some of our test directories have moved:
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.
Comment #6
xjmComment #7
xjmSince 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
Comment #8
xjmMeant 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.
Comment #9
Mile23Comment #10
Mile23Let's add composer/composer: #3057459: Add composer/composer as a dev dependency of core so that we can test Composer plugins
Comment #11
alexpottThis makes me uncomfortable - I don't think we should start adding dev dependencies to the root composer.json in this issue. See
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.
This does not need to be public API
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.
Comment #12
Mile23#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
Comment #13
alexpottRegarding #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.
Comment #14
Mile23Thanks for the reviews...
Re #13:
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.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 theirextra
, 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
Comment #15
MixologicSince 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.
Comment #16
Mile23OK, 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.
Comment #17
Mile23Once more, this time without dev dependencies in the component.
Comment #18
Mixologiclooks 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...
Comment #19
Mile23Weird. Not sure how those got in there. Reverted now.
Comment #20
MixologicAdding related, blocking issue. Also, since it blocking on that I think we may as well postpone here right?
Comment #21
Mile23Added CR: https://www.drupal.org/node/3059717
Comment #22
Mixologiclol: "Alternately, you could use the vendor cleanup plugin to cut down on disk space requirements in your project." -me gusta
Comment #23
Mile23#3057459: Add composer/composer as a dev dependency of core so that we can test Composer plugins is in, yay!
Here's a reroll.
Comment #24
Mile23Woops, neglected to update the list of packages to clean up from #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 and #3055040: Roll AbstractEventDispatcherTest into ContainerAwareEventDispatcherTest
Comment #25
Mile23Adding 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.
Comment #26
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI 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.
Comment #27
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedI 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.
Comment #28
Mile23OK, 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.
Comment #29
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOn 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.
Comment #30
Mile23Over 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.
Comment #31
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis is looking pretty good. I see one easy optimization:
This could be replaced with Composer\Util\Filesystem::removeDirectory() https://github.com/composer/composer/blob/1.8.6/src/Composer/Util/Filesy...
Comment #32
Mile23Great idea. :-)
Comment #33
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedOperating 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.
Comment #34
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpdated 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.
Comment #35
ultimikeI'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 mustcomposer 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
Comment #36
Mile23The 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
Comment #37
Chi CreditAttribution: Chi commentedHow 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 likerm -rf vendor && composer install
to clean vendors after the project creation.Comment #38
Mile23This 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.
Not true any more.
Comment #39
Mile23If you want to use the plugin, apply
use_plugin_DO_NOT_TEST.patch
and docomposer update drupal/core-vendor-cleanup
Comment #40
Mile23You know... You work hard to make a good patch and then you don't because you forgot to rebase.
Let's try that again.
Comment #41
ultimike@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
Comment #42
Mile23The 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/
: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 isolationComment #43
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedFinished 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.
Comment #44
MixologicGiven 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.
Comment #45
alexpottI'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 :(
Comment #46
catchIt'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:
I think this can just be 'automatically'.
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?
Comment #47
alexpott/me has no idea what I reviewed... ah lolz I get it. I reviewed #4. Because it was top in the issue summary. How does that happen? Ignore #45
Comment #48
Mixologic@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?
Comment #49
Mile23Fixing #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 forVendorCleanup
. (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
.Comment #50
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis 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.
Comment #51
Mile23Not totally happy with
str_repeat()
, but #46. If there's a better way then all suggestions welcome.Switched all
write()
towriteError()
.Picked up some indentations I missed before for some reason.
Normalized on
sprintf()
rather than concatenation.Comment #52
catchSince 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.
Comment #53
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented$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.Comment #54
Mile23OK, how about this?
Comment #55
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedPutting 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.
Comment #56
catch#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?
Comment #57
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented@catch: It does not appear that the core/composer.json changes have any effect on the composer.lock at the root of the project:
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.
Comment #58
bojanz CreditAttribution: bojanz at Centarro commentedNitpick time!
Missing docblock for the constructor.
"Get" should be "Gets'. None of the docblocks seem to be using the third person here, per convention?
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?
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.
Why would we say "Convenience method to..." instead of just "Gets a list of installed packages from Composer."?
Comment #59
Mile23Excellent. :-)
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.
Comment #60
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#59 addresses #58 and tests are green - back to RTBC.
Comment #61
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis patch is now postponed on #2912387: Stop using wikimedia/composer-merge-plugin
Comment #62
Mile23This 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. :-)
Comment #63
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedSure, I'm hip to re-roll after a merge here, whichever goes in first. +1 on re-RTBC'ing here.
Comment #64
MixologicComment #65
Mile23Comment #66
Mixologicdoh. unsure why all that form stuff updated.
Comment #67
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis 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.
Comment #68
MixologicAlso, 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' ?
Comment #69
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented+1 for "vendor hardening" or some similar name.
Comment #70
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedHi 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!
Comment #71
Mile23Thanks @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.
Comment #72
Mile23Missed a file rename.
Comment #74
andypostIn related issue "security logger" introduced
probably special core component could be used in both places
Comment #75
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThis will need to be relocated to the 'Plugin' directory per https://www.drupal.org/project/drupal/issues/2912387#comment-13204485
Comment #76
Mixologicpackage 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.
Comment #77
Mile23If 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.
Comment #78
Mile23#2878269: Modify TestDiscovery so the testbot runs all the tests is now in, so we can proceed here.
Comment #79
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedYeah, if we follow the pattern of the Scaffold plugin, there will be more warnings with
composer install -o
, but it will work.Comment #80
Mile23It 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:Comment #82
Mile23I was unable to repro the failing tests other than
ComposerIntegrationTest
. I think the testbot didn't re-performcomposer install
, even though I changed the composer.json file.This is the fix to
ComposerIntegrationTest
.Comment #84
Mixologicyeah, testbot is looking for changes to composer.lock for core changes.
Comment #85
Mile23Fixed typo, added a line to composer.lock to trick the testbot.
Update: Trick successful.
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...
Comment #86
Mile23Given 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: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:
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.
Comment #87
Mile23Needed a reroll after reverting #2912387: Stop using wikimedia/composer-merge-plugin
Comment #88
Mile23Added testbot issue to deal with orphaned composer.json changes: #3072484: Composer dumpautoload if composer.json is modified but lock file is not
Comment #89
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedMarking 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.Comment #90
Mile23#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.
Comment #91
Mile23In 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:
Comment #93
Mile23Woops. 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
Comment #94
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAfter 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 pathscomposer/Plugin
andcomposer/Template
seems like it would make sense.Comment #95
MixologicAdding a couple of random composer issues that will probably get wontfixed when this goes in.
Comment #96
Mile23Moved to composer/Plugin.
Updated ComposerIntegrationTest with new path.
Includes composer.lock workaround for the testbot.
Comment #97
Mile23Semi-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.
Comment #98
Mile23A real related issue that might be in conflict: #3013210: Add drupal/core-filesecurity component for writing htaccess files
Comment #99
Mile23#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.
Comment #100
MixologicLGTM.
The only concern I have is with the cross component requirement on drupal/core-php-storage:
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 withdrupal/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
Comment #101
Mile23Good 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 sayscomposer 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 tocomposer update
rather thancomposer install
because they committed their lock file.Setting this as postponed on that.
Comment #102
MixologicHah. 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.
Comment #103
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedMy 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?
Comment #104
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commented#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.
Comment #105
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedAdded a follow-on task to the proposed resolution in the issue summary.
Comment #106
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedWe 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.
Comment #107
Mile23#3013210: Add drupal/core-filesecurity component for writing htaccess files is in, so we can kick it here.
Comment #108
Mile23Amended to use drupal/core-file-security:^8.8.
Comment #109
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThe Scaffold plugin has already been relocated, so removing the follow-on work here.
This is working well now! RTBC
Comment #110
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedUpdate issue summary to link to the follow-on issue #3076684: Remove deprecated vendor cleanup scripts.
Comment #111
larowlanwe can return here and avoid the else
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?
we could do away with this nesting if we used continue to return early instead of else
Comment #112
hussainwebJust addressing the feedback in #111 right now. Anything that removes
else
blocks is good for me. :)Comment #113
hussainwebI 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.Comment #114
hussainwebCASE_LOWER is actually the default and we don't need to say that, but I put it to make it clearer.
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:
I don't know if everyone finds this readable enough, though.
Comment #115
larowlanI'd be +1 on that too, we have php7, lets use it
Comment #117
hussainwebFixing the test, making the change as per #115 and another small change I noticed.
Comment #118
hussainwebActually, it could be even shorter and easier to understand.
Comment #120
hussainwebAh, the failure was because I didn't read the test correctly.
getAllCleanupPaths
would always return keys with lowercase now.Comment #122
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedRegarding #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.
Comment #123
hussainwebWell, 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.
Comment #124
MixologicI 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.
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 :
Comment #125
Mile23Changed 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.
Comment #126
greg.1.anderson CreditAttribution: greg.1.anderson at Pantheon commentedThanks @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.
Comment #128
larowlanchuckle
Committed a42b1c5 and pushed to 8.8.x. Thanks!
Comment #129
larowlanRemoving 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
Comment #131
rfaySo... 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.
Comment #132
Mixologicre #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)
Comment #133
Mile23#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.
Comment #134
rfayThis 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
Comment #135
cilefen CreditAttribution: cilefen as a volunteer commented