Fix executing plugin in a namespace (#115)

Closes #62

Re-using the existing application to run the command in the sub-namespace is having another side-effect (besides potentially registering commands which we already are cleaning up since #96) which is that plugins are not re-loaded.

As a result if a plugin is registered in a namespace, it will not be loaded at all hence all the commands it registers will not be available

e2e/scenario8/README.md

@@ -1,3 +1 @@
 Tests that extra arguments and options are not lost when forwarding the command to a bin namespace.
-In this scenario the dependencies are not install in the bin namespace since the parsed (forwarded)
-command is invalid.

e2e/scenario9/.gitignore

@@ -0,0 +1,5 @@
+/actual.txt
+/composer.lock
+/vendor/
+/vendor-bin/*/composer.lock
+/vendor-bin/*/vendor/

e2e/scenario9/README.md

@@ -0,0 +1 @@
+Tests that extra arguments and options are not lost when forwarding the command to a bin namespace.

e2e/scenario9/composer.json

@@ -0,0 +1,21 @@
+{
+    "repositories": [
+        {
+            "type": "path",
+            "url": "../../"
+        }
+    ],
+    "require": {
+        "bamarni/composer-bin-plugin": "dev-master"
+    },
+    "config": {
+        "allow-plugins": {
+            "bamarni/composer-bin-plugin": true
+        }
+    },
+    "extra": {
+        "bamarni-bin": {
+            "forward-command": true
+        }
+    }
+}

e2e/scenario9/expected.txt

@@ -0,0 +1,9 @@
+[bamarni-bin] Checking namespace vendor-bin/composer-unused
+
+Loading packages
+----------------
+
+ ! [NOTE] Found 0 package(s) to be checked.
+
+ [OK] Done. No required packages to scan.
+

e2e/scenario9/script.sh

@@ -0,0 +1,28 @@
+#!/usr/bin/env bash
+
+set -Eeuo pipefail
+
+# Set env envariables in order to experience a behaviour closer to what happens
+# in the CI locally. It should not hurt to set those in the CI as the CI should
+# contain those values.
+export CI=1
+export COMPOSER_NO_INTERACTION=1
+
+readonly ORIGINAL_WORKING_DIR=$(pwd)
+
+trap "cd ${ORIGINAL_WORKING_DIR}" err exit
+
+# Change to script directory
+cd "$(dirname "$0")"
+
+# Ensure we have a clean state
+rm -rf actual.txt || true
+rm -rf composer.lock || true
+rm -rf vendor || true
+rm -rf vendor-bin/*/composer.lock || true
+rm -rf vendor-bin/*/vendor || true
+
+composer update
+
+# Actual command to execute the test itself
+composer bin composer-unused unused --no-progress 2>&1 | tee > actual.txt || true

e2e/scenario9/vendor-bin/composer-unused/.gitignore

@@ -0,0 +1,2 @@
+/composer-unused-dump-*
+

e2e/scenario9/vendor-bin/composer-unused/composer.json

@@ -0,0 +1,11 @@
+{
+    "require": {
+        "composer-runtime-api": "^2.2",
+        "icanhazstring/composer-unused": "~0.7.12"
+    },
+    "config": {
+        "allow-plugins": {
+            "icanhazstring/composer-unused": true
+        }
+    }
+}

src/BinCommand.php

@@ -8,8 +8,6 @@
 use Composer\Factory;
 use Composer\IO\IOInterface;
 use Composer\IO\NullIO;
-use ReflectionClass;
-use ReflectionProperty;
 use Symfony\Component\Console\Application;
 use Symfony\Component\Console\Input\InputArgument;
 use Symfony\Component\Console\Input\InputInterface;
@@ -36,15 +34,23 @@ class BinCommand extends BaseCommand
 
     private const NAMESPACE_ARG = 'namespace';
 
+    /**
+     * @var NamespaceApplicationFactory
+     */
+    private $applicationFactory;
+
     /**
      * @var Logger
      */
     private $logger;
 
-    public function __construct(?Logger $logger = null)
-    {
+    public function __construct(
+        ?NamespaceApplicationFactory $applicationFactory = null,
+        ?Logger $logger = null
+    ) {
         parent::__construct('bin');
 
+        $this->applicationFactory = $applicationFactory ?? new FreshInstanceApplicationFactory();
         $this->logger = $logger ?? new Logger(new NullIO());
     }
 
@@ -106,24 +112,18 @@ public function execute(InputInterface $input, OutputInterface $output): int
             $input
         );
 
-        $applicationReflection = new ReflectionClass(Application::class);
-        $commandsReflection = $applicationReflection->getProperty('commands');
-        $commandsReflection->setAccessible(true);
-
         return (self::ALL_NAMESPACES !== $namespace)
             ? $this->executeInNamespace(
                 $currentWorkingDir,
                 $vendorRoot.'/'.$namespace,
                 $binInput,
-                $output,
-                $commandsReflection
+                $output
             )
             : $this->executeAllNamespaces(
                 $currentWorkingDir,
                 $vendorRoot,
                 $binInput,
-                $output,
-                $commandsReflection
+                $output
             );
     }
 
@@ -139,8 +139,7 @@ private function executeAllNamespaces(
         string $originalWorkingDir,
         string $binVendorRoot,
         InputInterface $input,
-        OutputInterface $output,
-        ReflectionProperty $commandsReflection
+        OutputInterface $output
     ): int {
         $namespaces = self::getBinNamespaces($binVendorRoot);
 
@@ -159,8 +158,7 @@ private function executeAllNamespaces(
                 $originalWorkingDir,
                 $namespace,
                 $input,
-                $output,
-                $commandsReflection
+                $output
             );
         }
 
@@ -171,8 +169,7 @@ private function executeInNamespace(
         string $originalWorkingDir,
         string $namespace,
         InputInterface $input,
-        OutputInterface $output,
-        ReflectionProperty $commandsReflection
+        OutputInterface $output
     ): int {
         $this->logger->logStandard(
             sprintf(
@@ -194,25 +191,21 @@ private function executeInNamespace(
             return self::FAILURE;
         }
 
-        $application = $this->getApplication();
-        $commands = $application->all();
+        // Use a new application: this avoids a variety of issues:
+        // - A command may be added in a namespace which may cause side effects
+        //   when executed in another namespace afterwards (since it is the same
+        //   process).
+        // - Different plugins may be registered in the namespace in which case
+        //   an already executed application will not pick that up.
+        $namespaceApplication = $this->applicationFactory->create(
+            $this->getApplication()
+        );
 
         // It is important to clean up the state either for follow-up plugins
         // or for example the execution in the next namespace.
-        $cleanUp = function () use (
-            $originalWorkingDir,
-            $commandsReflection,
-            $application,
-            $commands
-        ): void {
+        $cleanUp = function () use ($originalWorkingDir): void {
             $this->chdir($originalWorkingDir);
             $this->resetComposers();
-
-            // When executing composer in a namespace, some commands may be
-            // registered.
-            // For example when scripts are registered in the composer.json,
-            // Composer adds them as commands to the application.
-            $commandsReflection->setValue($application, $commands);
         };
 
         $this->chdir($namespace);
@@ -229,7 +222,7 @@ private function executeInNamespace(
         );
 
         try {
-            $exitCode = $application->doRun($namespaceInput, $output);
+            $exitCode = $namespaceApplication->doRun($namespaceInput, $output);
         } catch (Throwable $executionFailed) {
             // Ensure we do the cleanup even in case of failure
             $cleanUp();

src/FreshInstanceApplicationFactory.php

@@ -0,0 +1,15 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Bamarni\Composer\Bin;
+
+use Composer\Console\Application;
+
+final class FreshInstanceApplicationFactory implements NamespaceApplicationFactory
+{
+    public function create(Application $existingApplication): Application
+    {
+        return new Application();
+    }
+}

src/NamespaceApplicationFactory.php

@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Bamarni\Composer\Bin;
+
+use Composer\Console\Application;
+
+interface NamespaceApplicationFactory
+{
+    public function create(Application $existingApplication): Application;
+}

tests/BinCommandTest.php

@@ -6,6 +6,7 @@
 
 use Bamarni\Composer\Bin\BinCommand;
 use Bamarni\Composer\Bin\Tests\Fixtures\MyTestCommand;
+use Bamarni\Composer\Bin\Tests\Fixtures\ReuseApplicationFactory;
 use Composer\Composer;
 use Composer\Console\Application;
 use PHPUnit\Framework\TestCase;
@@ -70,7 +71,7 @@ protected function setUp(): void
 
         $this->application = new Application();
         $this->application->addCommands([
-            new BinCommand(),
+            new BinCommand(new ReuseApplicationFactory()),
             $this->testCommand,
         ]);
     }

tests/EndToEndTest.php

@@ -7,16 +7,20 @@
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\Finder\Finder;
 use Symfony\Component\Process\Process;
+use function array_map;
 use function basename;
 use function dirname;
+use function explode;
 use function file_exists;
 use function file_get_contents;
 use function getcwd;
+use function implode;
 use function preg_replace;
 use function realpath;
 use function sprintf;
 use function str_replace;
 use function trim;
+use const PHP_EOL;
 
 /**
  * @group e2e
@@ -39,7 +43,9 @@ public function test_it_passes_the_e2e_test(string $scenarioPath): void
             10
         );
 
-        $expected = file_get_contents($scenarioPath.'/expected.txt');
+        $expected = self::normalizeTrailingWhitespacesAndLineReturns(
+            file_get_contents($scenarioPath.'/expected.txt')
+        );
 
         $scenarioProcess->run();
 
@@ -167,6 +173,14 @@ private static function retrieveActualOutput(
             $normalizedContent
         );
 
-        return $normalizedContent;
+        return self::normalizeTrailingWhitespacesAndLineReturns($normalizedContent);
+    }
+
+    private static function normalizeTrailingWhitespacesAndLineReturns(string $value): string
+    {
+        return implode(
+            "\n",
+            array_map('rtrim', explode(PHP_EOL, $value))
+        );
     }
 }

tests/Fixtures/ReuseApplicationFactory.php

@@ -0,0 +1,16 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Bamarni\Composer\Bin\Tests\Fixtures;
+
+use Bamarni\Composer\Bin\NamespaceApplicationFactory;
+use Composer\Console\Application;
+
+final class ReuseApplicationFactory implements NamespaceApplicationFactory
+{
+    public function create(Application $existingApplication): Application
+    {
+        return $existingApplication;
+    }
+}