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 all commits
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
1 change: 1 addition & 0 deletions .github/workflows/php-workshop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ jobs:
with:
php-version: ${{ matrix.php }}
tools: composer:v2
ini-values: opcache.enable=0

- name: Install Dependencies
run: composer update --prefer-dist
Expand Down
19 changes: 5 additions & 14 deletions app/config.php
Original file line number Diff line number Diff line change
Expand Up @@ -384,28 +384,19 @@ function (CgiResult $result) use ($c) {
],
],
'code-patcher' => [
'cli.verify.start' => [
containerListener(CodePatchListener::class, 'patch'),
],
'cli.verify.finish' => [
'application.tear-down' => [
containerListener(CodePatchListener::class, 'revert'),
],
'cli.run.start' => [
'verify.pre.execute' => [
containerListener(CodePatchListener::class, 'patch'),
],
'cli.run.finish' => [
'verify.post.execute' => [
containerListener(CodePatchListener::class, 'revert'),
],
'cgi.verify.start' => [
containerListener(CodePatchListener::class, 'patch'),
],
'cgi.verify.finish' => [
containerListener(CodePatchListener::class, 'revert'),
],
'cgi.run.start' => [
'run.start' => [
containerListener(CodePatchListener::class, 'patch'),
],
'cgi.run.finish' => [
'run.finish' => [
containerListener(CodePatchListener::class, 'revert'),
],
],
Expand Down
22 changes: 22 additions & 0 deletions src/Application.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@
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;
use PhpSchool\PhpWorkshop\Output\OutputInterface;
use Psr\Container\ContainerInterface;
use Psr\Log\LoggerInterface;
use RuntimeException;

use function class_exists;
Expand Down Expand Up @@ -192,6 +195,12 @@ public function configure(): ContainerInterface
}
}

set_error_handler(function () use ($container): bool {
$this->tearDown($container);

return false; // Use default error handler
});

return $container;
}

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

$this->tearDown($container);

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

return $containerBuilder->build();
}

private function tearDown(ContainerInterface $container): void
{
try {
$container
->get(EventDispatcher::class)
->dispatch(new Event('application.tear-down'));
} catch (\Throwable $t) {
$container->get(LoggerInterface::class)->error($t->getMessage(), ['exception' => $t]);
}
}
}
24 changes: 16 additions & 8 deletions src/Exercise/AbstractExercise.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@
*/
abstract class AbstractExercise
{
/**
* @var SolutionInterface|null
*/
protected $solution;

/**
* Get the name of the exercise, like `Hello World!`.
Expand All @@ -35,15 +39,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
46 changes: 25 additions & 21 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,36 @@ 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 (array_filter($files) as $fileName) {
$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) {
if (null === $this->originalCode || empty($this->originalCode)) {
return;
}

file_put_contents($fileName, $this->originalCode);
foreach ($this->originalCode as $fileName => $contents) {
file_put_contents($fileName, $contents);
}
}
}
17 changes: 11 additions & 6 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 @@ -81,15 +81,20 @@ public function __construct(string $directory, string $entryPoint, array $exclus
* @param string $directory The directory to search for files.
* @param array<string> $exclusions An array of file names to exclude from the folder.
* @param string $entryPoint The relative path from the directory of the entry point file.
* @return self
* @return SolutionInterface
*/
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 InTempSolution::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
119 changes: 119 additions & 0 deletions src/Solution/InTempSolution.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

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

Looks like there are no tests for this


declare(strict_types=1);

namespace PhpSchool\PhpWorkshop\Solution;

use PhpSchool\PhpWorkshop\Utils\System;
use Symfony\Component\Filesystem\Filesystem;

class InTempSolution implements SolutionInterface
{
/**
* @var string
*/
private $baseDirectory;

/**
* @var string
*/
private $entryPoint;

/**
* @var SolutionFile[]
*/
private $files;

private function __construct(SolutionInterface $solution)
{
$fileSystem = new Filesystem();

$tempDir = System::tempDir();
$currentPath = explode('/', System::realpath(__DIR__));
$solutionPath = explode('/', System::realpath($solution->getBaseDirectory()));
$entryPointPath = explode('/', System::realpath($solution->getEntryPoint()));

$intersection = array_intersect($currentPath, $solutionPath);

if (count($intersection) <= 1) {
$intersection = explode('/', $tempDir);
}

$basename = implode('/', array_diff($solutionPath, $intersection));
$entrypoint = implode('/', array_diff($entryPointPath, $intersection));
Comment on lines +36 to +43
Copy link
Member

Choose a reason for hiding this comment

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

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?


$this->baseDirectory = sprintf('%s/php-school/%s', $tempDir, $basename);
$this->entryPoint = sprintf('%s/php-school/%s', $tempDir, $entrypoint);

if ($fileSystem->exists($this->baseDirectory)) {
$fileSystem->remove($this->baseDirectory);
}

$fileSystem->mkdir($this->baseDirectory);

$dirIterator = new \RecursiveDirectoryIterator(
$solution->getBaseDirectory(),
\RecursiveDirectoryIterator::SKIP_DOTS
);
$iterator = new \RecursiveIteratorIterator($dirIterator, \RecursiveIteratorIterator::SELF_FIRST);

foreach ($iterator as $file) {
$target = sprintf('%s/%s', $this->baseDirectory, $iterator->getSubPathName());
$file->isDir()
? $fileSystem->mkdir($target)
: $fileSystem->copy($file->getPathname(), $target);
}

$this->files = array_map(function (SolutionFile $solutionFile) use ($intersection, $tempDir) {
$filePath = explode('/', System::realpath($solutionFile->__toString()));
$file = implode('/', array_diff($filePath, $intersection));
return SolutionFile::fromFile(sprintf('%s/php-school/%s', $tempDir, $file));
}, $solution->getFiles());
}

public static function fromSolution(SolutionInterface $solution): SolutionInterface
{
return new self($solution);
}

/**
* 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
*/
public function getEntryPoint(): string
{
return $this->entryPoint;
}

/**
* Get all the files which are contained with the solution.
*
* @return array<SolutionFile>
*/
public function getFiles(): array
{
return $this->files;
}

/**
* Get the absolute path to the directory containing the solution.
*
* @return string
*/
public function getBaseDirectory(): string
{
return $this->baseDirectory;
}

/**
* Check whether there is a `composer.lock` file in the base directory.
*
* @return bool
*/
public function hasComposerFile(): bool
{
return file_exists(sprintf('%s/composer.lock', $this->baseDirectory));
}
}
8 changes: 4 additions & 4 deletions src/Solution/SingleFileSolution.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class SingleFileSolution implements SolutionInterface
* @param string $file The absolute path of the reference solution.
* @throws InvalidArgumentException If the file does not exist.
*/
public function __construct(string $file)
private function __construct(string $file)
{
if (!file_exists($file)) {
throw new InvalidArgumentException(sprintf('File: "%s" does not exist', $file));
Expand All @@ -33,12 +33,12 @@ public function __construct(string $file)
* Static constructor to build an instance from an absolute file path.
*
* @param string $file The absolute path of the reference solution.
* @return self
* @return SolutionInterface
* @throws InvalidArgumentException If the file does not exist.
*/
public static function fromFile(string $file): self
public static function fromFile(string $file): SolutionInterface
{
return new self($file);
return InTempSolution::fromSolution(new self($file));
}

/**
Expand Down
Loading