Combination of --prefer-dist and .gitattributes confuses our vendor test cleanup

Removed composer.lock from the diff. This will cause merge fails on latest 8.1.x.

If a directory changes, it shouldn't block the entire instal process..
Why not check if the folder actually exists? This can happen to any library, that adds the test folder to .gitattributes.

A better solution would be to not have the vendor directory in the public folder, but I'm guessing that's discussed in a different issue already..

If a project adds the test folder to .gitattributes, it will be ignored when creating the archive. But if you install packages with --prefer-source, for example because of API rate limits, it will still include the tests folder.

So not checking will leave some people vulnerable.

FYI, I also have a composer plugin with some common folders/file to delete, like docs and examples; https://github.com/barryvdh/composer-cleanup-plugin

The patch does need to include the composer.lock changes for paragonie/random_compat alone as well as the actual updated library.
The trick is in running "composer update paragonie/random_compat" instead of just composer update.

Hm, looking at https://github.com/paragonie/random_compat/tree/v1.2.1 I still see the tests/ directory, and get no error running composer update?
I'm confused.

Here's the patch that updates the library, in any case.

As I said before, the directory still exists, but is excluded with the .gitattributes export-ignore feature. See https://github.com/paragonie/random_compat/commit/e1ddb31788f16f4527074f...

This means, that on dist downloads, they are excluded, but on source downloads they are still there. So you don't know for sure if the folder exists before running the removal.
This will happen with every library that uses this feature (which are a lot of them..)

We definitely need to do #5 then.

The current code is:

        if (is_dir($dir_to_remove)) {
          if (!static::deleteRecursive($dir_to_remove)) {
            throw new \RuntimeException(sprintf("Failure removing directory '%s' in package '%s'.", $path, $package->getPrettyName()));
          }
        }
        else {
          throw new \RuntimeException(sprintf("The directory '%s' in package '%s' does not exist.", $path, $package->getPrettyName()));
        }

We just need to kill that last "else".

Hello guys,
I am an average/advanced computer user and I find it difficult to manage all these changes. Could you make them more "user friendly"? I spent about 1 week and unable to solve this problem.
Or Drupal Commerce is ONLY for people with advance knowledge in coding?

@British-Link
Drupal Commerce for Drupal 8 is incomplete pre-alpha software, aimed at developers looking to contribute.
Regular users are advised to use the Drupal 7 version until Commerce hits beta or RC1 (depending on how comfortable you are chasing bugs and doing custom coding).

Working on a new patch.

The only way to support both --prefer-source and --prefer-dist is to not throw an exception. Patch attached.

Note: there were no tests to change.
Changing priority to major since this crashes Composer.

8.0 production sites that use composer are now affected by this. I see this is a patch against 8.1 but we need this for 8.0 as well. It seems to apply cleanly to 8.0 and it works there as far as I can tell.

Confirmed. We need this in all branches.

Makes sense, thanks for working on it.

I think we have to show a notice to keep $packageToCleanup up to date in the future

and we should update $packageToCleanup by removing 'tests' from 'behat/mink' element, there is another patch for that #2682003: Update $packageToCleanup, 'tests' cleanup are no longer required for Behat/Mink package

I disagree with #23.

It will output a warning on --prefer-source for every library that uses the .gitattributes approach (which will soon be all of them).
That will result in a lot of bogus reports, like the one about Mink.

I still think we need to go with #19, otherwise we'll have to keep explaining this day after day.

Regarding #24, repeating what I said in https://www.drupal.org/node/2682003#comment-10949795:

No, that is incorrect. Mink still has a tests directory: https://github.com/Behat/Mink/tree/master/tests
And if you install/update with --prefer-source, you will get it.

I agree, showing the warning will probably lead to a lot of issues/questions.

+1 for just removing the Exception.

@bojanz yes, you're right
#19 works fine, RTBC

How about we use composer's IOInterface to solve this for us... and only output stuff when -vv or more is used. The reason we tried to log something here is because the presence of tests in Behat was actually a security risk!

I think the patch attached gives us the best of both world's - a working composer and information on what is happening inside Drupal\Core\Composer\Composer

But just to be clear, you're keeping this list inside the core of Drupal? So everytime a package changes, it needs a core update, before it's safe?
Why not create a Composer plugin, so it can be updated independ of the core, at the same time as the packages?

And are you going to accept PRs for *every* package that can be used with Drupal? Eg., say I want to use dompdf in my site/module, so I require it in my root composer.json. But I don't want the tests/examples, so should I create a PR to Drupal core to add the dompdf package rules to it? Seems like a lot of work/time for this..

Imho this plugin is still a workaround for the vendor dir being in the webdir. Or is this configurable?
When people can use the Composer CLI, I'm assuming they probably also can change the webroot or at least symlink it to a different dir, right?
So why not create a build script to give users a 'safe' version, but for the devs who want to user Composer to manage deps, place it outside the webroot?

@Barryvdh yes this is just a work around - and yes vendor can be configured to be outside webroot - you just have to edit the autoload.php in Drupal's root - it exists to make this possible. The point of this is nuke the tests and unnecessary stuff that would be shipped with core... for two reasons (a) test code if available to the web has security holes and (b) the downloadable package dropped over 1mb due to this.

If a project or contributed module adds another dependency to that Drupal site it is up to them to make recommendation about to safely deploy that dependency. It's great to see that composer projects are finally starting to exclude tests and other unnecessary files if one uses the --prefer-dist option... that said the (ab)use of .gitattributes to do this seems very very icky.

Yes I understand why you do this, it really does make sense for when you ship in in the web dir. But imho that should be just for the '1-zip install', not for when people use composer to handle it. Then it should just be outside the vendor dir. But okay, if it's not feasible to expect this from developers, then this is at least better then nothing..

(As a note, the security issue that started all this, xss in Mink, has been fixed: https://github.com/minkphp/Mink/pull/694)

Discussed with @bojanz on IRC - we agreed that a missing directory is not an error so we're just going to output an informational in this case. However if the recursive delete fails we should always display an error as something has gone very wrong.

Tested by running composer update -vv --prefer-dist on core.

Sample output:

Works for me, thanks.

Also I'm wrong about .gitattributes being a "very very icky" solution. @bojanz pointed me to http://git-scm.com/book/en/v2/Customizing-Git-Git-Attributes see "exporting your repository"... in fact it is a super elegant solution that makes it no composer's responsibility to do - nice.

I guess what we could do is file PR's against all the dependencies that don't have a .gitattributes file to have one. And then we can remove this and a note about using composer with --prefer-source in production not being supported by Drupal somewhere on d.o.

@alexpott Indeed, best would be that every project does that, but not everyone agrees about this.
FYI, I've asked Mink to do that, which removed the tests dir ( https://github.com/minkphp/Mink/pull/691) , but driver-suite was needed by other packages, to some more refactoring for that is needed. If you want, you can ask them to follow up on this.
But mink and other tests tools aren't need in production anyways.

Hmmm actually I think we can do a bit better with the logging...

After applying the patch in #38, I get the error detailed here: https://www.drupal.org/node/2686639#comment-10965209

Patch #38 applied successfully and I was able to run-through composer update on Travis CI with Drupal 8.1.x branch.

• Before - Expand the composer line to see the issue.
• After - Expand the composer line to see that composer update was successful and did not revert.

@RobKoberg, are you applying this patch on Drupal 8.0.x instead of Drupal 8.1.x? I think the patch only applies on 8.1.x and 8.2.x because 8.0.x does not have the Composer IO class. I also did not need to apply this patch for 8.0.x when using Composer Manager 8.x-1.x.

@mradcliffe
The IO class comes from Composer itself, there are no differences between Drupal versions when it comes to this bug.

After applying patch in #38, I got:

composer drupal-update
> Drupal\composer_manager\Composer\Command::update
Loading composer repositories with package information
Updating dependencies (including require-dev)
  - Removing paragonie/random_compat (1.1.1)
  - Installing paragonie/random_compat (v1.2.2)
    Loading from cache

> Drupal\Core\Composer\Composer::vendorTestCodeCleanup
PHP Fatal error:  Undefined class constant 'VERY_VERBOSE' in /var/www/drupal_projects/drupal81_dev/core/lib/Drupal/Core/Composer/Composer.php on line 163

Fatal error: Undefined class constant 'VERY_VERBOSE' in /var/www/drupal_projects/drupal81_dev/core/lib/Drupal/Core/Composer/Composer.php on line 163

Testing on a fresh Drupal 8.1-dev installation.

Did you run `composer self-update`? Try with the latest Composer.
Verbosity support is added 28 jan, so you should have been receiving 30-day warnings for a couple of weeks.
https://github.com/composer/composer/commit/49d7d65933c78e163354c2c58088...

The patch was applied to the latest 8.0.x version (downloaded the tar.gz from drupal.org yesterday). I had also just installed PHP7 yesterday (for cli and apache). I ran through the instructions at http://docs.drupalcommerce.org/v2/install.html for an Existing Site. D8 had previously installed fine. The Composer Manager and Drupal Console also was installed new yesterday.

This breaks Composer completely.
And since 8.1 and 8.2 don't have the vendor/ directory anymore, we now have a critical issue.

So let's just remove the Exception for now and then work on a 'better' verbose logging improvement..
(So just #19)

The last RTBC patch is fine. We don't need to care about supporting Composer older than beta1.

Okay - I've committed #19 because that solves the critical problem. I'll now reroll #38 and use the BC approach the merge plugin does to logging.

Here's a reroll and using a similar approach to the merge plugin...

Here's a screenshot whilst running composer -vv update on core:

For those following this issue on 8.0.x, you have to apply the patch in #19, followed by the patch in #52 to get things running again.

@Charlotte17
The patch from #19 is already present in 8.0.x-dev.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

I guess we still need #52 on the 8.0.x branch?

There won't be any new 8.0.x releases, so there's no point in tweaking this further.

@bojanz so how do I update 8.0 to 8.1 with composer?
When I run composer update on 8.0.3 - I get this crappy The directory 'tests' in package 'behat/mink' does not exist. error.

When I edit my composer.json file to use drupal/core 8.1 I can't update because composer complains that installed 8.0.x requires symfony/yml 2.7 and 8.1.x requires symfony/yml 2.8

@SiliconMind
Drupal 8.0.6 contains the fix to this bug. Upgrade to that and you'll be fine.

The rest of your question is a generic Composer support request which doesn't belong in this issue.

@bojanz
But I can't update to 8.0.6 from 8.0.3 with composer. If I just do composer update it fails with error:

The directory 'tests' in package 'behat/mink' does not exist.

Am I missing something?

8.0.3 is Composer-broken, you can either update core without Composer (by manually downloading the tarball), or apply the patch from this issue and then update via Composer.