Log code parse errors from patches

app/config.php

@@ -269,12 +269,12 @@
         return $parserFactory->create(ParserFactory::PREFER_PHP7);
     },
     CodePatcher::class  => function (ContainerInterface $c) {
-        $patch = (new Patch)
+        $patch = (new Patch())
             ->withInsertion(new Insertion(Insertion::TYPE_BEFORE, 'ini_set("display_errors", "1");'))
             ->withInsertion(new Insertion(Insertion::TYPE_BEFORE, 'error_reporting(E_ALL);'))
             ->withInsertion(new Insertion(Insertion ::TYPE_BEFORE, 'date_default_timezone_set("Europe/London");'));
 
-        return new CodePatcher($c->get(Parser::class), new Standard, $patch);
+        return new CodePatcher($c->get(Parser::class), new Standard(), $c->get(LoggerInterface::class), $patch);
     },
     FakerGenerator::class => function () {
         return FakerFactory::create();

src/CodePatcher.php

@@ -12,6 +12,7 @@
 use PhpSchool\PhpWorkshop\Exercise\ExerciseInterface;
 use PhpSchool\PhpWorkshop\Exercise\SubmissionPatchable;
 use PhpSchool\PhpWorkshop\Patch\Transformer;
+use Psr\Log\LoggerInterface;
 
 /**
  * Service to apply patches to a student's solution. Accepts a default patch via the constructor.
@@ -30,6 +31,11 @@ class CodePatcher
      */
     private $printer;
 
+    /**
+     * @var LoggerInterface
+     */
+    private $logger;
+
     /**
      * @var Patch|null
      */
@@ -45,12 +51,14 @@ class CodePatcher
      *
      * @param Parser $parser
      * @param Standard $printer
+     * @param LoggerInterface $logger
      * @param Patch|null $defaultPatch
      */
-    public function __construct(Parser $parser, Standard $printer, Patch $defaultPatch = null)
+    public function __construct(Parser $parser, Standard $printer, LoggerInterface $logger, Patch $defaultPatch = null)
     {
         $this->parser = $parser;
         $this->printer = $printer;
+        $this->logger = $logger;
         $this->defaultPatch = $defaultPatch;
     }
 
@@ -141,7 +149,10 @@ private function applyCodeInsertion(CodeInsertion $codeInsertion, array $stateme
             $codeToInsert = sprintf('<?php %s', preg_replace('/^\s*<\?php/', '', $codeToInsert));
             $additionalStatements = $this->parser->parse($codeToInsert) ?? [];
         } catch (Error $e) {
-            //we should probably log this and have a dev mode or something
+            $this->logger->critical(
+                'Code Insertion could not be parsed: ' . $e->getMessage(),
+                ['code' => $codeInsertion->getCode()]
+            );
             return $statements;
         }
 

test/CodePatcherTest.php

@@ -17,6 +17,8 @@
 use PhpSchool\PhpWorkshopTest\Asset\PatchableExercise;
 use PHPUnit\Framework\TestCase;
 use PhpSchool\PhpWorkshop\CodeInsertion as Insertion;
+use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
 
 class CodePatcherTest extends TestCase
 {
@@ -25,7 +27,12 @@ public function testDefaultPatchIsAppliedIfAvailable(): void
         $patch = (new Patch())
             ->withInsertion(new Insertion(Insertion::TYPE_BEFORE, 'ini_set("display_errors", 1);'));
 
-        $patcher = new CodePatcher((new ParserFactory())->create(ParserFactory::PREFER_PHP7), new Standard(), $patch);
+        $patcher = new CodePatcher(
+            (new ParserFactory())->create(ParserFactory::PREFER_PHP7),
+            new Standard(),
+            new NullLogger(),
+            $patch
+        );
         $exercise = $this->createMock(ExerciseInterface::class);
 
         $expected = "<?php\n\nini_set(\"display_errors\", 1);\n\$original = true;";
@@ -34,7 +41,11 @@ public function testDefaultPatchIsAppliedIfAvailable(): void
 
     public function testPatcherDoesNotApplyPatchIfNotPatchableExercise(): void
     {
-        $patcher = new CodePatcher((new ParserFactory())->create(ParserFactory::PREFER_PHP7), new Standard());
+        $patcher = new CodePatcher(
+            (new ParserFactory())->create(ParserFactory::PREFER_PHP7),
+            new Standard(),
+            new NullLogger()
+        );
         $exercise = $this->createMock(ExerciseInterface::class);
 
         $code = '<?php $original = true;';
@@ -46,8 +57,11 @@ public function testPatcherDoesNotApplyPatchIfNotPatchableExercise(): void
      */
     public function testPatcher(string $code, Patch $patch, string $expectedResult): void
     {
-        $patcher = new CodePatcher((new ParserFactory())->create(ParserFactory::PREFER_PHP7), new Standard());
-
+        $patcher = new CodePatcher(
+            (new ParserFactory())->create(ParserFactory::PREFER_PHP7),
+            new Standard(),
+            new NullLogger()
+        );
         $exercise = $this->createMock(PatchableExercise::class);
 
         $exercise
@@ -146,7 +160,11 @@ public function testBeforeInsertionInsertsAfterStrictTypesDeclaration(): void
         $code = '<?php declare(strict_types=1); $original = true;';
         $patch = (new Patch())->withInsertion(new Insertion(Insertion::TYPE_BEFORE, '$before = "here";'));
 
-        $patcher = new CodePatcher((new ParserFactory())->create(ParserFactory::PREFER_PHP7), new Standard());
+        $patcher = new CodePatcher(
+            (new ParserFactory())->create(ParserFactory::PREFER_PHP7),
+            new Standard(),
+            new NullLogger()
+        );
 
         $exercise = $this->createMock(PatchableExercise::class);
 
@@ -174,7 +192,11 @@ public function testTransformerWithStrictTypes(): void
                 ];
             });
 
-        $patcher = new CodePatcher((new ParserFactory())->create(ParserFactory::PREFER_PHP7), new Standard());
+        $patcher = new CodePatcher(
+            (new ParserFactory())->create(ParserFactory::PREFER_PHP7),
+            new Standard(),
+            new NullLogger()
+        );
 
         $exercise = $this->createMock(PatchableExercise::class);
 
@@ -202,7 +224,11 @@ public function testTransformerWhichAddsStrictTypesDoesNotResultInDoubleStrictTy
                 ])];
             });
 
-        $patcher = new CodePatcher((new ParserFactory())->create(ParserFactory::PREFER_PHP7), new Standard());
+        $patcher = new CodePatcher(
+            (new ParserFactory())->create(ParserFactory::PREFER_PHP7),
+            new Standard(),
+            new NullLogger()
+        );
 
         $exercise = $this->createMock(PatchableExercise::class);
 
@@ -231,7 +257,11 @@ public function testAddingStrictTypesDeclareDoesNotBreakBeforeInsertion(): void
             })
             ->withInsertion(new Insertion(Insertion::TYPE_BEFORE, '$before = "here";'));
 
-        $patcher = new CodePatcher((new ParserFactory())->create(ParserFactory::PREFER_PHP7), new Standard());
+        $patcher = new CodePatcher(
+            (new ParserFactory())->create(ParserFactory::PREFER_PHP7),
+            new Standard(),
+            new NullLogger()
+        );
 
         $exercise = $this->createMock(PatchableExercise::class);
 
@@ -245,4 +275,35 @@ public function testAddingStrictTypesDeclareDoesNotBreakBeforeInsertion(): void
             $patcher->patch($exercise, $code)
         );
     }
+
+    public function testExceptionIsLoggedIfCodeIsNotParseable(): void
+    {
+        $patcher = new CodePatcher(
+            (new ParserFactory())->create(ParserFactory::PREFER_PHP7),
+            new Standard(),
+            $logger = $this->createMock(LoggerInterface::class)
+        );
+
+        $exercise = $this->createMock(PatchableExercise::class);
+
+        $patch = (new Patch())->withInsertion(new Insertion(Insertion::TYPE_BEFORE, '$before = "here"'));
+
+        $exercise
+            ->expects($this->once())
+            ->method('getPatch')
+            ->willReturn($patch);
+
+        $logger
+            ->expects($this->once())
+            ->method('critical')
+            ->with(
+                'Code Insertion could not be parsed: Syntax error, unexpected EOF on line 1',
+                ['code' => '$before = "here"']
+            );
+
+        $this->assertEquals(
+            "<?php\n\n\$original = true;",
+            $patcher->patch($exercise, '<?php $original = true;')
+        );
+    }
 }