Code patch updates #201

mikeymike · 2021-02-01T23:27:19Z

Patches internal solution
- Internal solution is patched in temp
- Solution is ran from temp
- Eliminates chance of internal solution becoming "broken"
Users solution is patched in place
- Error handler catches app errors and patches are reverted

- Patches internal solution - Internal solution is patched in temp - Solution is ran from temp - Eliminates chance of internal solution becoming "broken" - Users solution is patched in place - Error handler catches app errors and patches are reverted

src/Application.php

mikeymike · 2021-02-01T23:33:25Z

src/Exercise/AbstractExercise.php

-                    '%s/../../exercises/%s/solution/solution.php',
-                    dirname((string) (new ReflectionClass(static::class))->getFileName()),
-                    self::normaliseName($this->getName())
+        if (null === $this->solution) {


so this was required and probs also for dir solution... the problem being that a patched solution in temp was being overwritten due to the solution being copied on construct and we're constructing fresh on each call to getSolution.

Probably a better fix here, maybe moving the copy to temp dir to somewhere that isn't on solution construct.

@AydinHassan I need your input on this one bro, can you think of a cleaner approach that for this?

Hmm not really off the top of my head. I think it's fine as you have it.

I guess the only problem I foresee here is the implementation of a solution inside the exercise instead of using the Abstract. e.g if you need a dir solution and you use the static factory 🤔

src/ExerciseDispatcher.php

src/Listener/CodePatchListener.php

src/Listener/InMemorySolutionListener.php

src/Application.php

src/Listener/CodePatchListener.php

src/Listener/InMemorySolutionListener.php

src/Solution/InMemorySolution.php

src/ExerciseDispatcher.php

app/config.php

mikeymike · 2021-02-22T22:38:10Z

note to self, a current failure that needs a failing test case writing up

this happens when an exercise is not selected and a run or verify is attempted

php bin/php8appreciate run exercises/a-match-made-in-heaven/solution/solution.php                                                                                                                                                                         ()
PHP Fatal error:  Uncaught RuntimeException: Can only revert previously patched code in /Users/michael/Projects/PHPSchool/php8-appreciate/vendor/php-school/php-workshop/src/Listener/CodePatchListener.php:66
Stack trace:
#0 /Users/michael/Projects/PHPSchool/php8-appreciate/vendor/php-school/php-workshop/src/Factory/EventDispatcherFactory.php(111): PhpSchool\PhpWorkshop\Listener\CodePatchListener->revert(Object(PhpSchool\PhpWorkshop\Event\Event))
#1 /Users/michael/Projects/PHPSchool/php8-appreciate/vendor/php-school/php-workshop/src/Event/EventDispatcher.php(43): PhpSchool\PhpWorkshop\Factory\EventDispatcherFactory->PhpSchool\PhpWorkshop\Factory\{closure}(Object(PhpSchool\PhpWorkshop\Event\Event))
#2 /Users/michael/Projects/PHPSchool/php8-appreciate/vendor/php-school/php-workshop/src/Application.php(242): PhpSchool\PhpWorkshop\Event\EventDispatcher->dispatch(Object(PhpSchool\PhpWorkshop\Event\Event))
#3 /Users/michael/Projects/PHPSchool/php8-appreciate/bin/php8appreciate(4): PhpSchool\PhpWorkshop\Application->run()
#4 {main}
  thrown in /Users/michael/Projects/PHPSchool/php8-appreciate/vendor/php-school/php-workshop/src/Listener/CodePatchListener.php on line 66

Fatal error: Uncaught RuntimeException: Can only revert previously patched code in /Users/michael/Projects/PHPSchool/php8-appreciate/vendor/php-school/php-workshop/src/Listener/CodePatchListener.php on line 66

mikeymike · 2021-03-10T21:41:48Z

src/Application.php

@@ -227,6 +237,10 @@ public function run(): int
                $message = str_replace($basePath, '', $message);
            }

+            $container
+                ->get(EventDispatcher::class)
+                ->dispatch(new Event('application.tear-down'));


So I was thinking, if an exception ever happened in a listener then it would bubble up and mask the original exception.

It could be the initial exception is something we WANT to show the user, and thus an exception covering this up during some failed cleanup process is not something we want the user to see really.

@AydinHassan thoughts? We could move this dispatch into a separate method that tries and catches and either ignores or logs and lets the application carry on the closing

@mikeymike I think logging sounds like a good idea

AydinHassan · 2021-03-11T09:16:21Z

src/Application.php

@@ -227,6 +237,10 @@ public function run(): int
                $message = str_replace($basePath, '', $message);
            }

+            $container
+                ->get(EventDispatcher::class)
+                ->dispatch(new Event('application.tear-down'));


@mikeymike I think logging sounds like a good idea

AydinHassan · 2021-03-11T09:22:06Z

src/Solution/InTempSolution.php

+        $intersection = array_intersect($currentPath, $solutionPath);
+
+        if (count($intersection) <= 1) {
+            $intersection = explode('/', $tempDir);
+        }
+
+        $basename = implode('/', array_diff($solutionPath, $intersection));
+        $entrypoint = implode('/', array_diff($entryPointPath, $intersection));


I actually have no idea what this chuck is doing - can you extract it to a method and maybe add a comment or just give it a good name?

AydinHassan · 2021-03-11T09:27:07Z

src/TestUtils/SolutionPathTransformer.php

+        $tmpDir = realpath(sys_get_temp_dir());
+
+        if (!$tmpDir) {
+            throw new \RuntimeException();


Let's add a message to the exception - also we do have our own RuntimeException you could use

AydinHassan · 2021-03-11T09:27:34Z

src/Utils/System.php

+        $realpath = realpath($path);
+
+        if (false === $realpath) {
+            throw new \RuntimeException(sprintf('Failed to get realpath of "%s"', $path));


Same here, let's use our own RuntimeException

AydinHassan · 2021-03-11T09:31:19Z

src/Solution/InTempSolution.php

@@ -0,0 +1,119 @@
+<?php


Looks like there are no tests for this

AydinHassan · 2021-03-11T09:33:32Z

@mikeymike Might be worth splitting up and getting some of this work in? Probably the events stuff can go separately, the patches, and the FS utils ?

mikeymike · 2021-05-21T15:45:30Z

Superseded by multiple PRs

Code patch updates

60e154c

- Patches internal solution - Internal solution is patched in temp - Solution is ran from temp - Eliminates chance of internal solution becoming "broken" - Users solution is patched in place - Error handler catches app errors and patches are reverted