Don't include vendor test code (especially mink) in the Drupal webroot

My personal recommendation would be:

* Drop /vendor entirely
* Add a hook_requirement when dev dependencies are installed which gives a warning
* Patch composer to be able to configure the default behaviour for `composer install` to be the --no-dev variant

When we original .htaccess fix we accepted that it was not perfect. For a start we should also generate a web.config file too for anyone using IIS. And we should have a note for users of other webservers and we should have note of how to move vendor out of webroot.

But given that these limitations were discussed when we did the original fix - is this really critical?

I think we should remove the require-dev dependencies from Composer - this represents about 5Mb of code uncompressed that just does not need to be there for 99.9% of sites. Yes it will mean that developers will have to get in the habit of installing composer and doing composer install --dev if they want to develop on Drupal but this is how other projects work. But even this is not really enough. Until vendor is outside of webroot we are putting our security in our dependencies hands.

For a start we should also generate a web.config file too for anyone using IIS. And we should have a note for users of other webservers and we should have note of how to move vendor out of webroot.

I think this is admirable and ideal, but not a good thing to intermix in this issue.

The web.config files that we ship get very little attention (e.g. #1543392: Add a web.config to the several directories similar to the .htaccess file is years old, currently 7 months stale and it is for defense-in-depth for another security issue).
The typical Nginx configuration is not vulnerable to this issue since executable files are usually listed explicitly and it's unlikely someone would explicitly list this file.

My opinion, restructure so we have a web folder and keep vendor outside it. That's how every other composer based project gets around this. Fairly big at this stage, but everything else is brittle

Joomla doesn't https://github.com/joomla/joomla-cms/tree/staging/libraries/vendor ... just saying.

What about scanning for test and tests case insensitive in vendor/ and nuke it?

So let's go for the general approach but I think we need to fix the implementation so that it fails if it does not remove the directories. This is protect us from thinking we're secure if mink moves its tests.

We should use the postPackageInstall and postPackageUpdate events and if the package matches remove the specific folders and if we can't remove the folder it should throw and exception or something and fail hard.

Also can we get a followup which is also an RC target to remove all tests using this script from the dependencies since this will greatly reduce the tarball size and the amount of space a D8 instance takes.

Something like this.

Relevant discussion on composer https://github.com/composer/composer/issues/4438

Here's everything that should be removed... a 6mb patch! This is a significant saving for anyone still using ftp or the likes. It should also make the D8 package significantly smaller too.

This is blocked by #2573687: Setup GitHub OAuth for Composer on Testbots - the testbots will potentially run into githubs API limits, so that needs to be solved before we can ask the testbots to hit github.

The diff needs to be made with --binary.

@Mixologic thanks for this information. The current solution this issue is looking at is removing all the unnecessary test code from dependencies - that does not required drupalci to do anything funky with composer / github.

My heart sings at this patch. So. much. removal!

Error running composer update:
Fatal error: Call to undefined method Composer\DependencyResolver\Operation\UpdateOperation::getPackage()

Oops! sent the wrong patch! The right one one the way

Interdiff-22-25:

--- a/core/lib/Drupal/Core/Composer/Composer.php
+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -127,7 +127,7 @@ public static function ensureHtaccess(Event $event) {
    */
   public static function vendorTestCodeCleanup(PackageEvent $event) {
     $vendor_dir = $event->getComposer()->getConfig()->get('vendor-dir');
-    $package = $event->getOperation()->getPackage();
+    $package = $event->getOperation()->getTargetPackage();
     $package_name = $package->getName();
     if (isset(static::$packageToCleanup[$package_name])) {
       foreach (static::$packageToCleanup[$package_name] as $path) {

It is a bit sad that /vendor/behat/mink/driver-testsuite is removed as it has quite some good examples, but for sure this is not at all a good reason to keep it.

Now this is failing on composer install.
So apparently we need to use $package = $event->getOperation()->getPackage() for install and $package = $event->getOperation()->getTargetPackage() for update

right, --binary

@pwolanin did you actually test that? I think not. The advantage of the change I made in #16 is that this fires after each package is installed or updated. Not after everything. I've tested this and it works as expected.

@alexpott after running a composer update the following files are updated:

• vendor/wikimedia/composer-merge-plugin/README.md
• vendor/wikimedia/composer-merge-plugin/src/MergePlugin.php

Should we consider those changes in the patch ?

+++ b/core/composer.json
@@ -34,7 +34,7 @@
-    "mikey179/vfsStream": "~1.2",
+    "mikey179/vfsstream": "~1.2",

The package is uppercase, see https://packagist.org/packages/mikey179/vfsStream and https://github.com/mikey179/vfsStream/blob/master/composer.json

Well, beside that, isn't it still totally unimportant for this issue? Given that composer uses lowercase internally, everything should be alright. vfsstream has a gazillion of installations, I doubt anyone is not using some case sensitive filesystem. I'm sure that alexpott uses a proper filesystem for example.

Yep I use a case sensitive file system.

So let's please revert all those out of scope changes, please.

I would say that case insensitive file systems are pretty common. It's the default type for all Mac which is a common developer platform and I *believe* many (most?) Windows machines are also case insensitive.

I don't know if that's worth including code related to case sensitivity or not, just noting the discrepancy with the statements in #50.

Thank you @pwolanin.

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -151,6 +151,35 @@ public static function vendorTestCodeCleanup(PackageEvent $event) {
+   * @param $package_name

nitpick, could be typehinted with string

When we do #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead these would no longer get packaged at all (assuming we keep require-dev out of tarballs which I think we should).

While this looks OK for a quick fix, what happens with these scripts if the library doesn't get installed at all? Also this looks like we'd need to manually maintain the list of libraries, for example if a library gets split into extra dependencies upstream (which happened recently with Zend Feed). Could some of the listing be parsed from composer itself?

@catch. The biggest problem would be solved if we skip the dev dependencies in git and the tarball.

Just skipping the dev dependencies in git is not a solution, people who want to run tests otherwise would need to composer install and then ignore some of the changes in /vendor

If you are managing the dependencies with composer, then you can say composer install --no-dev in your deployment process. Then you don't have mink at all.

If you're expecting to run the tests packaged with e.g. mink, then with the current patch you need to say composer install --prefer-source --no-scripts, which would leave the tests, but also fail to add all our htaccess and special-case autoload dumping. So then you need to do composer run-script pre-autoload-dump and composer run-script post-autoload-dump.

I vote for:

1. Not adding the patch here.
2. Doing composer install --no-dev on the packaging script for the tarball (with an unfortunate side effect of being unable to run PHPUnit).
3. Finally removing the vendor/ directory from the repo. #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead
4. Adding a testbot step where we build with no-dev and run all the tests.
5. Documenting best-practice install processes for different workflows.

I just did a composer install --no-dev and tried to run the phpunit_example tests in the simpletest UI. Result: WSOD. So some work is needed there before packaging the tarball without dev.

Adding a testbot step where we build with no-dev and run all the tests.

I think you mean build with --dev

I think you mean build with --dev

Nope. :-) See point 2. We'd need coverage for --no-dev.

so adding a testbot step = run all the tests with both --dev and --no-dev? The testbots do not use packaged tarballs.

@Mile23

If you're expecting to run the tests packaged with e.g. mink

Clone mink - don't clone Drupal and run any version of Drupal install.

@Mixologic for obvious reasons testbot is only going to work with --dev

Clone mink - don't clone Drupal and run any version of Drupal install.

Agreed.

@Mixologic for obvious reasons testbot is only going to work with --dev

Well, no. :-) We still have SimpleTest and it still works part-way for a user driving the UI. It will also work with run-tests.sh. Try it:

$ composer install --no-dev
$ php core/scripts/run-tests.sh --list

Simpletest should discover all the tests it can run without phpunit installed, which it currently doesn't do (it assumes phpunit is around).

Then the tarball can ship with --no-dev, so at least the security concern is gone there.

If someone wants to deploy with the repo, the onus is on them to do --no-dev for production, which they're used to doing anyway if they know composer at all.

But: This means a new behavior for the tarball which needs to be tested.

The only way to test that is with another testbot step that builds with --no-dev.

@Mile23 the fact that Simpletest UI does not check whether of not PHPUnit exists is not relevant for this issue since this issue does not break it doing --no-dev breaks it.

Here's the full patch to commit.

6 MB patch?

@Mile23 the fact that Simpletest UI does not check whether of not PHPUnit exists is not relevant for this issue since this issue does not break it doing --no-dev breaks it.

Right. It's relevant to my solution which is to build with --no-dev which exists to solve the problem of security holes in your test frameworks.

@Mile23 building with --no-dev does not exist to fix security flaws in anything. Storing vendor outside of webroot should be recommended secure Drupal practice because even this fix, which deletes more test code than a --no-dev build, does nothing if there is security issue with direct access to a non testfile and the .htaccess file in vendor is not working.

The *real* issue is that composer should distinguish between distribution and source and distribution should only contain what is actually necessary for runtime and nothing else.

And yes this deletes 6mb of unnecessary test code. I just forgot the --binary switch again.

Committed/pushed to 8.0.x, thanks!

It is possible to distinguish source and distributed packages. This can be done with .gitattributes using the export-ignore feature. https://www.reddit.com/r/PHP/comments/2jzp6k/i_dont_need_your_tests_in_m...

This can be fixed upstream in Mink, for example see https://github.com/minkphp/Mink/pull/691

This is also what happened in one of the other packages; https://github.com/paragonie/random_compat/commit/e1ddb31788f16f4527074f...
That is a good thing, but broke your Composer plugin. It should not break when a folder is moved/renamed or exluded from dists. For example, when installing from source (--prefer-source) the tests folder will exist, but when using --prefer-dist, it's gone.
The packaged distribution should be dist, but when people are updating composer on their own (and hit API limits or something), you should take both cases into account.

Important followup: #2664274: Combination of --prefer-dist and .gitattributes confuses our vendor test cleanup (regarding #80).