Skip to content

Code patch updates #201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions app/config.php
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,12 @@ function (CgiResult $result) use ($c) {
],
],
'code-patcher' => [
'application.tear-down' => [
containerListener(CodePatchListener::class, 'revert'),
],
'verify.post.exception' => [
containerListener(CodePatchListener::class, 'revert'),
],
'cli.verify.start' => [
containerListener(CodePatchListener::class, 'patch'),
],
Expand Down
32 changes: 32 additions & 0 deletions src/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
use DI\Container;
use DI\ContainerBuilder;
use PhpSchool\PhpWorkshop\Check\CheckRepository;
use PhpSchool\PhpWorkshop\Event\Event;
use PhpSchool\PhpWorkshop\Event\EventDispatcher;
use PhpSchool\PhpWorkshop\Exception\InvalidArgumentException;
use PhpSchool\PhpWorkshop\Exception\MissingArgumentException;
use PhpSchool\PhpWorkshop\Factory\ResultRendererFactory;
Expand Down Expand Up @@ -84,6 +86,8 @@ public function __construct(string $workshopTitle, string $diConfigFile)

$this->workshopTitle = $workshopTitle;
$this->diConfigFile = $diConfigFile;

set_error_handler([$this, 'handleInternalError']);
}

/**
Expand Down Expand Up @@ -192,6 +196,18 @@ public function configure(): ContainerInterface
}
}

$tearDown = function () use ($container): bool {
// TODO: This works.. but wrong event as PatchListener take ExerciseRunnerEvent
$container
->get(EventDispatcher::class)
->dispatch(new Event('application.tear-down'));

// Fallback to default error handler
return false;
};

set_error_handler($tearDown);

return $container;
}

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

$container
->get(EventDispatcher::class)
->dispatch(new Event('application.tear-down'));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikeymike I think logging sounds like a good idea


$container
->get(OutputInterface::class)
->printError(
Expand Down Expand Up @@ -268,4 +288,16 @@ private function getContainer(): Container

return $containerBuilder->build();
}

private function handleInternalError(
int $errno,
string $errstr,
string $errfile,
int $errline
): bool {



return false;
}
}
21 changes: 13 additions & 8 deletions src/Exercise/AbstractExercise.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
*/
abstract class AbstractExercise
{
protected ?SolutionInterface $solution = null;

/**
* Get the name of the exercise, like `Hello World!`.
Expand All @@ -35,15 +36,19 @@ abstract public function getName(): string;
*/
public function getSolution(): SolutionInterface
{
return SingleFileSolution::fromFile(
(string) realpath(
sprintf(
'%s/../../exercises/%s/solution/solution.php',
dirname((string) (new ReflectionClass(static::class))->getFileName()),
self::normaliseName($this->getName())
if (null === $this->solution) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

$this->solution = SingleFileSolution::fromFile(
(string)realpath(
sprintf(
'%s/../../exercises/%s/solution/solution.php',
dirname((string)(new ReflectionClass(static::class))->getFileName()),
self::normaliseName($this->getName())
)
)
)
);
);
}

return $this->solution;
}

/**
Expand Down
3 changes: 3 additions & 0 deletions src/ExerciseDispatcher.php
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,9 @@ public function verify(ExerciseInterface $exercise, Input $input): ResultAggrega

try {
$this->results->add($runner->verify($input));
} catch (\Throwable $e) {
$this->eventDispatcher->dispatch(new ExerciseRunnerEvent('verify.post.exception', $exercise, $input));
throw $e;
} finally {
$this->eventDispatcher->dispatch(new ExerciseRunnerEvent('verify.post.execute', $exercise, $input));
}
Expand Down
46 changes: 27 additions & 19 deletions src/Listener/CodePatchListener.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@

use PhpSchool\PhpWorkshop\CodePatcher;
use PhpSchool\PhpWorkshop\Event\ExerciseRunnerEvent;
use PhpSchool\PhpWorkshop\Exercise\ProvidesSolution;
use PhpSchool\PhpWorkshop\Solution\SolutionFile;
use RuntimeException;

/**
* Listener which patches student's solutions
* Listener which patches internal and student's solutions
*/
class CodePatchListener
{
Expand All @@ -19,9 +21,9 @@ class CodePatchListener
private $codePatcher;

/**
* @var string
* @var array<string, string>
*/
private $originalCode;
private $originalCode = [];

/**
* @param CodePatcher $codePatcher
Expand All @@ -36,34 +38,40 @@ public function __construct(CodePatcher $codePatcher)
*/
public function patch(ExerciseRunnerEvent $event): void
{
$fileName = $event->getInput()->getArgument('program');
$files = [
$event->getInput()->getArgument('program'),
];

if (null === $fileName) {
return;
$exercise = $event->getExercise();
if ($exercise instanceof ProvidesSolution) {
$files[] = $exercise->getSolution()->getEntryPoint();
}

$this->originalCode = (string) file_get_contents($fileName);
file_put_contents(
$fileName,
$this->codePatcher->patch($event->getExercise(), $this->originalCode)
);
foreach ($files as $fileName) {
if (null === $fileName) {
return;
}

$this->originalCode[$fileName] = (string) file_get_contents($fileName);

file_put_contents(
$fileName,
$this->codePatcher->patch($event->getExercise(), $this->originalCode[$fileName])
);
}
}

/**
* @param ExerciseRunnerEvent $event
* @param \PhpSchool\PhpWorkshop\Event\EventInterface $event
*/
public function revert(ExerciseRunnerEvent $event): void
public function revert(\PhpSchool\PhpWorkshop\Event\EventInterface $event): void
{
if (null === $this->originalCode) {
throw new RuntimeException('Can only revert previously patched code');
}

$fileName = $event->getInput()->getArgument('program');

if (null === $fileName) {
return;
foreach ($this->originalCode as $fileName => $contents) {
file_put_contents($fileName, $contents);
}

file_put_contents($fileName, $this->originalCode);
}
}
66 changes: 66 additions & 0 deletions src/Listener/InMemorySolutionListener.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
<?php

declare(strict_types=1);

namespace PhpSchool\PhpWorkshop\Listener;

use PhpSchool\PhpWorkshop\Event\ExerciseRunnerEvent;
use Symfony\Component\Filesystem\Filesystem;

/**
* Move solution to temp and replace program arg
*/
class InMemorySolutionListener
{
/**
* @param ExerciseRunnerEvent $event
*/
public function __invoke(ExerciseRunnerEvent $event): void
{
if (!$event->getInput()->hasArgument('program')) {
return;
}

$program = $event->getInput()->getRequiredArgument('program');

if (!file_exists($program)) {
return;
}

$filesystem = new Filesystem();
$basedir = basename($program);
$tmp = sprintf('%s/php-school/user-solution', sys_get_temp_dir());

if ($filesystem->exists($tmp)) {
$filesystem->remove($tmp);
}

$filesystem->mkdir($tmp);

// Iterator with exclusions... TODO: Useful?
$dirIterator = new \RecursiveDirectoryIterator($basedir, \RecursiveDirectoryIterator::SKIP_DOTS);
$filter = new class($dirIterator) extends RecursiveFilterIterator {
private const FILTERS = ['vendor', '.idea', ',git'];
public function accept() {
return !in_array($this->current()->getFilename(), self::FILTERS, true);
}
};
$iterator = new \RecursiveIteratorIterator($filter, \RecursiveIteratorIterator::SELF_FIRST);

$composerFile = sprintf('%s/composer.json', $basedir);
if ($filesystem->exists($composerFile)) {
$filesystem->copy($composerFile, $tmp . '/composer.json');
}


// TODO should this event be somewhere we have the exercise
// TODO: We could then check if it's a single file solutino or dir solution
// TODO: Single file solutions can be just copying entrypoint to temp
// TODO: dir solution we can copy the dirname to temp, however we should take care here
// TODO: e.g. what if it's all just in the root home dir... we can't recursively copy that all into temp...
// TODO: how do we prevent such an issue ?!?


$event->getInput()->setArgument('program', (string) realpath($program));
}
}
15 changes: 10 additions & 5 deletions src/Solution/DirectorySolution.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class DirectorySolution implements SolutionInterface
* @param array<string> $exclusions An array of file names to exclude from the folder.
* @throws InvalidArgumentException If the entry point does not exist in the folder.
*/
public function __construct(string $directory, string $entryPoint, array $exclusions = [])
private function __construct(string $directory, string $entryPoint, array $exclusions = [])
{
$directory = (string) realpath(rtrim($directory, '/'));
$entryPoint = ltrim($entryPoint, '/');
Expand Down Expand Up @@ -83,13 +83,18 @@ public function __construct(string $directory, string $entryPoint, array $exclus
* @param string $entryPoint The relative path from the directory of the entry point file.
* @return self
*/
public static function fromDirectory(string $directory, array $exclusions = [], $entryPoint = 'solution.php'): self
{
return new self($directory, $entryPoint, array_merge($exclusions, ['composer.lock', 'vendor']));
public static function fromDirectory(
string $directory,
array $exclusions = [],
$entryPoint = 'solution.php'
): SolutionInterface {
return InMemorySolution::fromSolution(
new self($directory, $entryPoint, array_merge($exclusions, ['composer.lock', 'vendor']))
);
}

/**
* Get the entry point. This is the PHP file that PHO would execute in order to run the
* Get the entry point. This is the PHP file that PHP would execute in order to run the
* program. This should be the absolute path.
*
* @return string
Expand Down
Loading