Merge pull request #395 from Landerstraeten/fix-phpcsfixer2-issues

Fix phpcsfixer2 issues

doc/tasks/phpcsfixer2.md

@@ -22,7 +22,7 @@ parameters:
             config: ~
             rules: []
             using_cache: true
-            path_mode: ~
+            config_contains_finder: true
             verbose: true
             diff: false
             triggered_by: ['php']
@@ -97,12 +97,14 @@ The tool will fix all files if the tool version has changed or the list of fixer
 Cache is supported only for tool downloaded as phar file or installed via composer.
 
 
-**path_mode**
+**config_contains_finder**
 
-*Default: null*
-
-Specify path mode (can be override or intersection).
+*Default: true*
 
+Intersection mode can only be used when you have a configuration file which contains a Finder.
+This mode works best since only files that are being commit and are in your configuration will be checked.
+When there is no Finder in your configuration, you'll have set this parameter to false. 
+Otherwise php-cs-fixer will crash the execution.
 
 **verbose**
 

resources/config/tasks.yml

@@ -242,7 +242,6 @@ services:
         arguments:
           - '@config'
           - '@process_builder'
-          - '@async_process_runner'
           - '@formatter.phpcsfixer'
         tags:
           - {name: grumphp.task, config: phpcsfixer2}

spec/Task/PhpCsFixerV2Spec.php

@@ -6,7 +6,6 @@
 use GrumPHP\Collection\ProcessArgumentsCollection;
 use GrumPHP\Configuration\GrumPHP;
 use GrumPHP\Formatter\PhpCsFixerFormatter;
-use GrumPHP\Process\AsyncProcessRunner;
 use GrumPHP\Process\ProcessBuilder;
 use GrumPHP\Runner\TaskResult;
 use GrumPHP\Runner\TaskResultInterface;
@@ -22,15 +21,15 @@
 
 class PhpCsFixerV2Spec extends ObjectBehavior
 {
-    function let(GrumPHP $grumPHP, ProcessBuilder $processBuilder, AsyncProcessRunner $processRunner, PhpCsFixerFormatter $formatter)
+    function let(GrumPHP $grumPHP, ProcessBuilder $processBuilder, PhpCsFixerFormatter $formatter)
     {
         $grumPHP->getTaskConfiguration('phpcsfixer2')->willReturn([]);
 
         $formatter->format(Argument::any())->willReturn('');
         $formatter->formatSuggestion(Argument::any())->willReturn('');
         $formatter->formatErrorMessage(Argument::cetera())->willReturn('');
 
-        $this->beConstructedWith($grumPHP, $processBuilder, $processRunner, $formatter);
+        $this->beConstructedWith($grumPHP, $processBuilder, $formatter);
     }
 
     function it_is_initializable()
@@ -52,7 +51,7 @@ function it_should_have_configurable_options()
         $options->getDefinedOptions()->shouldContain('config');
         $options->getDefinedOptions()->shouldContain('rules');
         $options->getDefinedOptions()->shouldContain('using_cache');
-        $options->getDefinedOptions()->shouldContain('path_mode');
+        $options->getDefinedOptions()->shouldContain('config_contains_finder');
         $options->getDefinedOptions()->shouldContain('verbose');
         $options->getDefinedOptions()->shouldContain('diff');
         $options->getDefinedOptions()->shouldContain('triggered_by');
@@ -79,14 +78,17 @@ function it_should_run_in_run_context(RunContext $context)
         $this->canRunInContext($context)->shouldReturn(true);
     }
 
-    function it_runs_the_suite_for_all_files(
+    function it_runs_phpcsfixer2_on_finder_in_run_context_with_finder_config(
         GrumPHP $grumPHP,
         ProcessBuilder $processBuilder,
         Process $process,
         RunContext $context,
         PhpCsFixerFormatter $formatter
     ) {
-        $grumPHP->getTaskConfiguration('phpcsfixer2')->willReturn(['config' => '.php_cs']);
+        $grumPHP->getTaskConfiguration('phpcsfixer2')->willReturn([
+            'config' => '.php_cs',
+            'config_contains_finder' => true,
+        ]);
         $formatter->resetCounter()->shouldBeCalled();
 
         $context->getFiles()->willReturn(new FilesCollection([
@@ -96,7 +98,8 @@ function it_runs_the_suite_for_all_files(
 
         $processBuilder->createArgumentsForCommand('php-cs-fixer')->willReturn(new ProcessArgumentsCollection());
         $processBuilder->buildProcess(Argument::that(function (ProcessArgumentsCollection $args) use ($file1, $file2) {
-            return !($args->contains($file1) || $args->contains($file2));
+            return !$args->contains($file1->getPathname())
+                && !$args->contains($file2->getPathname());
         }))->willReturn($process);
 
         $process->run()->shouldBeCalled();
@@ -107,11 +110,43 @@ function it_runs_the_suite_for_all_files(
         $result->isPassed()->shouldBe(true);
     }
 
-    function it_runs_the_suite_for_changed_files(
+    function it_runs_phpcsfixer2_on_all_files_in_run_context_without_finder_config(
+        GrumPHP $grumPHP,
         ProcessBuilder $processBuilder,
-        AsyncProcessRunner $processRunner,
         Process $process,
-        ContextInterface $context,
+        RunContext $context,
+        PhpCsFixerFormatter $formatter
+    ) {
+        $grumPHP->getTaskConfiguration('phpcsfixer2')->willReturn([
+            'config' => '.php_cs',
+            'config_contains_finder' => false,
+        ]);
+        $formatter->resetCounter()->shouldBeCalled();
+
+        $context->getFiles()->willReturn(new FilesCollection([
+            $file1 = new SplFileInfo('file1.php', '.', 'file1.php'),
+            $file2 = new SplFileInfo('file2.php', '.', 'file2.php'),
+        ]));
+
+        $processBuilder->createArgumentsForCommand('php-cs-fixer')->willReturn(new ProcessArgumentsCollection());
+        $processBuilder->buildProcess(Argument::that(function (ProcessArgumentsCollection $args) use ($file1, $file2) {
+            return !$args->contains('--path-mode=intersection')
+                && $args->contains($file1->getPathname())
+                && $args->contains($file2->getPathname());
+        }))->willReturn($process);
+
+        $process->run()->shouldBeCalled();
+        $process->isSuccessful()->willReturn(true);
+
+        $result = $this->run($context);
+        $result->shouldBeAnInstanceOf(TaskResultInterface::class);
+        $result->isPassed()->shouldBe(true);
+    }
+
+    function it_runs_the_suite_for_changed_files_on_pre_commit(
+        ProcessBuilder $processBuilder,
+        Process $process,
+        GitPreCommitContext $context,
         PhpCsFixerFormatter $formatter
     ) {
         $formatter->resetCounter()->shouldBeCalled();
@@ -122,10 +157,10 @@ function it_runs_the_suite_for_changed_files(
 
         $processBuilder->createArgumentsForCommand('php-cs-fixer')->willReturn(new ProcessArgumentsCollection());
         $processBuilder->buildProcess(Argument::that(function (ProcessArgumentsCollection $args) use ($file1, $file2) {
-            return $args->contains($file1) || $args->contains($file2);
+            return $args->contains($file1->getPathname()) && $args->contains($file2->getPathname());
         }))->willReturn($process);
 
-        $processRunner->run(Argument::type('array'))->shouldBeCalled();
+        $process->run()->shouldBeCalled();
         $process->isSuccessful()->willReturn(true);
 
         $result = $this->run($context);
@@ -135,7 +170,6 @@ function it_runs_the_suite_for_changed_files(
 
     function it_throws_exception_if_the_process_fails(
         ProcessBuilder $processBuilder,
-        AsyncProcessRunner $processRunner,
         Process $process,
         ContextInterface $context,
         PhpCsFixerFormatter $formatter
@@ -146,7 +180,7 @@ function it_throws_exception_if_the_process_fails(
         $processBuilder->createArgumentsForCommand('php-cs-fixer')->willReturn($arguments);
         $processBuilder->buildProcess(Argument::type(ProcessArgumentsCollection::class))->willReturn($process);
 
-        $processRunner->run(Argument::type('array'))->shouldBeCalled();
+        $process->run()->shouldBeCalled();
         $process->isSuccessful()->willReturn(false);
 
         $context->getFiles()->willReturn(new FilesCollection([

src/Task/PhpCsFixerV2.php

@@ -4,13 +4,14 @@
 
 use GrumPHP\Runner\TaskResult;
 use GrumPHP\Task\Context\ContextInterface;
+use GrumPHP\Task\Context\GitPreCommitContext;
 use GrumPHP\Task\Context\RunContext;
 use Symfony\Component\OptionsResolver\OptionsResolver;
 
 /**
  * Php-cs-fixer task v2
  */
-class PhpCsFixerV2 extends AbstractPhpCsFixerTask
+class PhpCsFixerV2 extends AbstractExternalTask
 {
     /**
      * @return string
@@ -32,7 +33,7 @@ public function getConfigurableOptions()
             'config' => null,
             'rules' => [],
             'using_cache' => true,
-            'path_mode' => null,
+            'config_contains_finder' => true,
             'verbose' => true,
             'diff' => false,
             'triggered_by' => ['php'],
@@ -43,16 +44,22 @@ public function getConfigurableOptions()
         $resolver->addAllowedTypes('config', ['null', 'string']);
         $resolver->addAllowedTypes('rules', ['array']);
         $resolver->addAllowedTypes('using_cache', ['bool']);
-        $resolver->addAllowedTypes('path_mode', ['null', 'string']);
+        $resolver->addAllowedTypes('config_contains_finder', ['bool']);
         $resolver->addAllowedTypes('verbose', ['bool']);
         $resolver->addAllowedTypes('diff', ['bool']);
         $resolver->addAllowedTypes('triggered_by', ['array']);
 
-        $resolver->setAllowedValues('path_mode', [null, 'override', 'intersection']);
-
         return $resolver;
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function canRunInContext(ContextInterface $context)
+    {
+        return ($context instanceof GitPreCommitContext || $context instanceof RunContext);
+    }
+
     /**
      * {@inheritdoc}
      */
@@ -81,16 +88,29 @@ public function run(ContextInterface $context)
             ));
         }
 
+        $canUseIntersection = !($context instanceof RunContext) && $config['config_contains_finder'];
+
         $arguments->addOptionalArgument('--using-cache=%s', $config['using_cache'] ? 'yes' : 'no');
-        $arguments->addOptionalArgument('--path-mode=%s', $config['path_mode']);
+        $arguments->addOptionalArgument('--path-mode=intersection', $canUseIntersection);
         $arguments->addOptionalArgument('--verbose', $config['verbose']);
         $arguments->addOptionalArgument('--diff', $config['diff']);
         $arguments->add('fix');
 
-        if ($context instanceof RunContext && $config['config'] !== null) {
-            return $this->runOnAllFiles($context, $arguments);
+        if ($context instanceof GitPreCommitContext || !$config['config_contains_finder']) {
+            $arguments->addFiles($files);
+        }
+
+        $process = $this->processBuilder->buildProcess($arguments);
+        $process->run();
+
+        if (!$process->isSuccessful()) {
+            $messages = [$this->formatter->format($process)];
+            $suggestions = [$this->formatter->formatSuggestion($process)];
+            $errorMessage = $this->formatter->formatErrorMessage($messages, $suggestions);
+
+            return TaskResult::createFailed($this, $context, $errorMessage);
         }
 
-        return $this->runOnChangedFiles($context, $arguments, $files);
+        return TaskResult::createPassed($this, $context);
     }
 }