Add a composer script method to Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup

+++ b/composer.json
@@ -50,8 +50,8 @@
-        "post-package-install": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",
-        "post-package-update": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",
+        "post-install-cmd": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",
+        "post-update-cmd": "Drupal\\Core\\Composer\\Composer::vendorTestCodeCleanup",

I think we should leave these as is, and not switch these to commands in drupal/drupal.

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -187,57 +188,84 @@ public static function upgradePHPUnitCheck($phpunit_version) {
+      return static::doTestCodeCleanup(
+        $event->getComposer()->getConfig()->get('vendor-dir'),
+        $package_key,
+        static::$packageToCleanup[$package_key],
+        $event->getIO()
+      );

We can't return the result of doTestCodeCleanup, because it doesnt return anything.
I dont think we need to add a return value as the original function doesnt have a return value.

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -187,57 +188,84 @@ public static function upgradePHPUnitCheck($phpunit_version) {
-    $package_key = static::findPackageKey($package->getName());

+    if ($package_key = static::findPackageKey($package->getName())) {
...
-    if ($package_key) {

Do we need to collapse variables like this? I find code that has been 'made more efficient' like this is harder to parse, harder to debug (with xdebug). Also, we should name this var $package_path to be consistent with the function call and usage in the other method.

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -187,57 +188,84 @@ public static function upgradePHPUnitCheck($phpunit_version) {
-    $vendor_dir = $event->getComposer()->getConfig()->get('vendor-dir');
-    $io = $event->getIO();
...
+      return static::doTestCodeCleanup(
+        $event->getComposer()->getConfig()->get('vendor-dir'),
+        $package_key,
+        static::$packageToCleanup[$package_key],
+        $event->getIO()
+      );

This call is hard to understand without looking at what we're passing to doTestCodeCleanup. Can we leave in the shorthand variables ($vendor_dir/$io) and add one more for static::$packageToCleanup[$package_key], (like $cleanup_paths).

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -187,57 +188,84 @@ public static function upgradePHPUnitCheck($phpunit_version) {
-      if ($io->isVeryVerbose()) {
-        // Add a new line to separate this output from the next package.
-        $io->write("");

Seems like we should preserve this behavior ?.

Otherwise I think this should work.

I think that might be the wrong patch, but everything else looks great, except theres still some returning the return of a void function

+++ b/core/lib/Drupal/Core/Composer/Composer.php
@@ -187,57 +188,89 @@ public static function upgradePHPUnitCheck($phpunit_version) {
+      return static::doTestCodeCleanup($vendor_dir, $package_name, $cleanup_paths, $io);

Here's a patch addressing #9, #11-1, and #11-3.

#11-2 is already handled by the check surrounding the call to $packageToCleanup:

    $package_name = static::findPackageKey($package->getName());
    if ($package_name) {
      $cleanup_paths = static::$packageToCleanup[$package_name];
      static::doTestCodeCleanup($vendor_dir, $package_name, $cleanup_paths, $io);
    }

I have this patch applied to a 'fork' of drupal/core over at https://github.com/ryanaslett/core/blob/8.6.x/lib/Drupal/Core/Composer/C..., and I am using it in the build process for a travis build that utilizes it.

It works exactly how we'd like it to, at least for

https://travis-ci.org/drupal/drupal-project-legacy/builds/443325811#L558...

So. one problem I see is that if we have the post-create-project-cmd as a script, then this runs fine the first time we do a composer create-project, but we also want to ensure that those test dirs are stripped out whenever we update to newer versions of those vendor libs.

So, if we add the post-package-install and post-package-update scripts to our template's composer.json, then we end up with a bunch of

Package operations: 47 installs, 0 updates, 0 removals
  - Installing composer/installers (v1.5.0): Downloading (100%)
Class Drupal\Core\Composer\Composer is not autoloadable, can not call post-package-install script
  - Installing drupal/drupal-scaffold (8.6.x-dev 7bd18ee): Downloading (100%)
Class Drupal\Core\Composer\Composer is not autoloadable, can not call post-package-install script
  - Installing wikimedia/composer-merge-plugin (v1.4.1): Downloading (100%)
Class Drupal\Core\Composer\Composer is not autoloadable, can not call post-package-install script
  - Installing doctrine/cache (v1.6.2): Downloading (100%)
Class Drupal\Core\Composer\Composer is not autoloadable, can not call post-package-install script
  - Installing doctrine/collections (v1.3.0): Downloading (100%)
Class Drupal\Core\Composer\Composer is not autoloadable, can not call post-package-install script

type of error messages on our create project, because that script does not exist until drupal/core gets installed.

Which is making me start to think that this shouldnt be in core's composer.php class at all, and instead should be part of the drupal-scaffold plugin, and be part of the composer plugin framework, instead of being executed as a script. Nonwithstanding the fact that drupal-scaffold's responsibility is to 'add files to the filesystem to setup the product' - i.e. the corrollary of "remove files from the filesystem that we dont want" fits right into its role.

I think I might see about migrating this cleanup code to the drupal scaffold plugin to see if that works better.

As an experiment I moved vendorTestCodeCleanup and vendorTestCodeCleanupCommand to drupal/drupal-scaffold, and it works great. We no longer get the post-package-install script errors.

In fact, the post-create-project-cmd ends up doing essentially nothing now that the post-package-install hook works again. So, Im wondering if maybe we don't need to introduce the "make it work on both command and package events" thing.

I also got rid of the script execution in composer.json and moved the functionality into the plugin.

Im working on incorporating all the feedback from https://www.drupal.org/project/drupal/issues/2982684#comment-12769425 and https://www.drupal.org/project/drupal/issues/2982684#comment-12769839, and I think I'll post on that thread.