Merge pull request #604 from phel-lang/feat/602-improve-error-message

Improve PHP notices and error messages
phel-lang · Aug 1, 2023 · 69779ea · 69779ea
2 parents d501f91 + 30cab4a
commit 69779ea
Show file tree

Hide file tree

Showing 15 changed files with 176 additions and 59 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,9 @@ All notable changes to this project will be documented in this file.
 ## Unreleased
 
 * Extract building "out" settings into `PhelOutConfig`
+* Improve the error display for PHP Notice messages
+* Save all errors in a temp `error.log` file
+  * You can change the error.log file path with `PhelConfig::setErrorLogFile(str)`
 
 ## 0.10.1 (2023-05-12)
 

diff --git a/phel-config.php b/phel-config.php
@@ -6,6 +6,7 @@
     ->setSrcDirs(['src/phel'])
     ->setTestDirs(['tests/phel'])
     ->setVendorDir('vendor')
+    ->setErrorLogFile('data/error.log')
     ->setOut((new \Phel\Config\PhelOutConfig())
         ->setDestDir('out')
         ->setMainPhelNamespace('phel\core')

diff --git a/src/php/Command/CommandConfig.php b/src/php/Command/CommandConfig.php
@@ -14,11 +14,13 @@ final class CommandConfig extends AbstractConfig
     public const TEST_DIRS = 'test-dirs';
     public const VENDOR_DIR = 'vendor-dir';
     public const OUTPUT = 'out';
+    public const ERROR_LOG_FILE = 'error-log-file';
 
     private const DEFAULT_VENDOR_DIR = 'vendor';
     private const DEFAULT_SRC_DIRS = ['src'];
     private const DEFAULT_TEST_DIRS = ['tests'];
     private const DEFAULT_OUTPUT_DIR = 'out';
+    private const DEFAULT_ERROR_LOG_FILE = 'data/error.log';
 
     public function getCodeDirs(): CodeDirectories
     {
@@ -35,4 +37,9 @@ public function getVendorDir(): string
     {
         return (string)$this->get(self::VENDOR_DIR, self::DEFAULT_VENDOR_DIR);
     }
+
+    public function getErrorLogFile(): string
+    {
+        return (string)$this->get(self::ERROR_LOG_FILE, self::DEFAULT_ERROR_LOG_FILE);
+    }
 }
diff --git a/src/php/Command/CommandFacade.php b/src/php/Command/CommandFacade.php
@@ -5,9 +5,9 @@
 namespace Phel\Command;
 
 use Gacela\Framework\AbstractFacade;
+use Phel\Command\Domain\Handler\ExceptionHandler;
 use Phel\Command\Domain\Shared\Exceptions\ExceptionPrinterInterface;
 use Phel\Compiler\Domain\Exceptions\AbstractLocatedException;
-use Phel\Compiler\Domain\Exceptions\CompilerException;
 use Phel\Compiler\Domain\Parser\ReadModel\CodeSnippet;
 use Symfony\Component\Console\Output\OutputInterface;
 use Throwable;
@@ -50,15 +50,7 @@ public function getStackTraceString(Throwable $e): string
 
     public function registerExceptionHandler(): void
     {
-        $exceptionPrinter = $this->getExceptionPrinter();
-
-        set_exception_handler(static function (Throwable $exception) use ($exceptionPrinter): void {
-            if ($exception instanceof CompilerException) {
-                $exceptionPrinter->printException($exception->getNestedException(), $exception->getCodeSnippet());
-            } else {
-                $exceptionPrinter->printStackTrace($exception);
-            }
-        });
+        $this->createExceptionHandler()->register();
     }
 
     /**
@@ -117,4 +109,11 @@ public function readPhelConfig(string $absolutePath): array
             ->getPhpConfigReader()
             ->read($absolutePath);
     }
+
+    private function createExceptionHandler(): ExceptionHandler
+    {
+        return new ExceptionHandler(
+            $this->getExceptionPrinter(),
+        );
+    }
 }
diff --git a/src/php/Command/CommandFactory.php b/src/php/Command/CommandFactory.php
@@ -12,6 +12,7 @@
 use Phel\Command\Domain\Finder\VendorDirectoriesFinderInterface;
 use Phel\Command\Domain\Shared\CommandExceptionWriter;
 use Phel\Command\Domain\Shared\CommandExceptionWriterInterface;
+use Phel\Command\Domain\Shared\ErrorLog\ErrorLog;
 use Phel\Command\Domain\Shared\Exceptions\ExceptionArgsPrinter;
 use Phel\Command\Domain\Shared\Exceptions\ExceptionPrinterInterface;
 use Phel\Command\Domain\Shared\Exceptions\Extractor\FilePositionExtractor;
@@ -30,6 +31,7 @@ public function createCommandExceptionWriter(): CommandExceptionWriterInterface
     {
         return new CommandExceptionWriter(
             $this->createExceptionPrinter(),
+            new ErrorLog($this->getConfig()->getErrorLogFile()),
         );
     }
 
@@ -40,6 +42,7 @@ public function createExceptionPrinter(): ExceptionPrinterInterface
             ColorStyle::withStyles(),
             new Munge(),
             new FilePositionExtractor(new SourceMapExtractor()),
+            new ErrorLog($this->getConfig()->getErrorLogFile()),
         );
     }
 

diff --git a/src/php/Command/Domain/Handler/ExceptionHandler.php b/src/php/Command/Domain/Handler/ExceptionHandler.php
@@ -0,0 +1,64 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Phel\Command\Domain\Handler;
+
+use Phel\Command\Domain\Shared\Exceptions\ExceptionPrinterInterface;
+use Phel\Compiler\Domain\Exceptions\CompilerException;
+use Throwable;
+
+final class ExceptionHandler
+{
+    private static string $previousNotice = '';
+
+    public function __construct(
+        private ExceptionPrinterInterface $exceptionPrinter,
+    ) {
+    }
+
+    public function register(): void
+    {
+        $this->registerExceptionHandler();
+        $this->registerNoticeHandler();
+    }
+
+    private function registerExceptionHandler(): void
+    {
+        set_exception_handler(function (Throwable $exception): void {
+            if ($exception instanceof CompilerException) {
+                $this->exceptionPrinter->printException($exception->getNestedException(), $exception->getCodeSnippet());
+            } else {
+                $this->exceptionPrinter->printStackTrace($exception);
+            }
+        });
+    }
+
+    private function registerNoticeHandler(): void
+    {
+        set_error_handler(function (
+            int $errno,
+            string $errstr,
+            string $errfile = '',
+            int $errline = 0,
+            array $errcontext = [],
+        ): bool {
+            $text = sprintf(
+                "[%s] NOTICE found!\nmessage: \"%s\"\nfile: %s:%d\ncontext: %s",
+                date(DATE_ATOM),
+                $errstr,
+                $errfile,
+                $errline,
+                json_encode(array_merge($errcontext, ['errno' => $errno])),
+            );
+            if (self::$previousNotice !== $text) {
+                $this->exceptionPrinter->printError($text);
+            } else {
+                $this->exceptionPrinter->printError('and again');
+            }
+
+            self::$previousNotice = $text;
+            return true;
+        }, E_NOTICE);
+    }
+}
diff --git a/src/php/Command/Domain/Shared/CommandExceptionWriter.php b/src/php/Command/Domain/Shared/CommandExceptionWriter.php
@@ -4,6 +4,7 @@
 
 namespace Phel\Command\Domain\Shared;
 
+use Phel\Command\Domain\Shared\ErrorLog\ErrorLogInterface;
 use Phel\Command\Domain\Shared\Exceptions\ExceptionPrinterInterface;
 use Phel\Compiler\Domain\Exceptions\AbstractLocatedException;
 use Phel\Compiler\Domain\Parser\ReadModel\CodeSnippet;
@@ -12,29 +13,27 @@
 
 final class CommandExceptionWriter implements CommandExceptionWriterInterface
 {
-    public function __construct(private ExceptionPrinterInterface $exceptionPrinter)
-    {
+    public function __construct(
+        private ExceptionPrinterInterface $exceptionPrinter,
+        private ErrorLogInterface $errorLog,
+    ) {
     }
 
     public function writeStackTrace(
         OutputInterface $output,
         Throwable $e,
     ): void {
-        $output->writeln($this->exceptionPrinter->getStackTraceString($e));
+        $output->writeln($e->getMessage());
 
-        if ($e->getPrevious()) {
-            $output->writeln('');
-            $output->writeln('Caused by');
-            $this->writeStackTrace($output, $e->getPrevious());
-        }
+        $this->errorLog->writeln($this->getStackTraceString($e));
     }
 
     public function writeLocatedException(
         OutputInterface $output,
         AbstractLocatedException $e,
         CodeSnippet $codeSnippet,
     ): void {
-        $output->writeln($this->exceptionPrinter->getExceptionString($e, $codeSnippet));
+        $output->writeln($this->getExceptionString($e, $codeSnippet));
     }
 
     public function getExceptionString(AbstractLocatedException $e, CodeSnippet $codeSnippet): string

diff --git a/src/php/Command/Domain/Shared/ErrorLog/ErrorLog.php b/src/php/Command/Domain/Shared/ErrorLog/ErrorLog.php
@@ -0,0 +1,22 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Phel\Command\Domain\Shared\ErrorLog;
+
+final class ErrorLog implements ErrorLogInterface
+{
+    public function __construct(
+        private string $filepath,
+    ) {
+    }
+
+    public function writeln(string $text): void
+    {
+        file_put_contents(
+            $this->filepath,
+            $text . PHP_EOL,
+            FILE_APPEND | LOCK_EX,
+        );
+    }
+}
diff --git a/src/php/Command/Domain/Shared/ErrorLog/ErrorLogInterface.php b/src/php/Command/Domain/Shared/ErrorLog/ErrorLogInterface.php
@@ -0,0 +1,10 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Phel\Command\Domain\Shared\ErrorLog;
+
+interface ErrorLogInterface
+{
+    public function writeln(string $text): void;
+}
diff --git a/src/php/Command/Domain/Shared/Exceptions/ExceptionPrinterInterface.php b/src/php/Command/Domain/Shared/Exceptions/ExceptionPrinterInterface.php
@@ -10,6 +10,8 @@
 
 interface ExceptionPrinterInterface
 {
+    public function printError(string $error): void;
+
     public function printException(AbstractLocatedException $e, CodeSnippet $codeSnippet): void;
 
     public function getExceptionString(AbstractLocatedException $e, CodeSnippet $codeSnippet): string;

diff --git a/src/php/Command/Domain/Shared/Exceptions/TextExceptionPrinter.php b/src/php/Command/Domain/Shared/Exceptions/TextExceptionPrinter.php
@@ -4,6 +4,7 @@
 
 namespace Phel\Command\Domain\Shared\Exceptions;
 
+use Phel\Command\Domain\Shared\ErrorLog\ErrorLogInterface;
 use Phel\Command\Domain\Shared\Exceptions\Extractor\FilePositionExtractorInterface;
 use Phel\Compiler\Domain\Emitter\OutputEmitter\MungeInterface;
 use Phel\Compiler\Domain\Exceptions\AbstractLocatedException;
@@ -24,12 +25,20 @@ public function __construct(
         private ColorStyleInterface $style,
         private MungeInterface $munge,
         private FilePositionExtractorInterface $filePositionExtractor,
+        private ErrorLogInterface $errorLog,
     ) {
     }
 
+    public function printError(string $error): void
+    {
+        echo $error . PHP_EOL;
+        $this->errorLog->writeln($error);
+    }
+
     public function printException(AbstractLocatedException $e, CodeSnippet $codeSnippet): void
     {
-        echo $this->getExceptionString($e, $codeSnippet);
+        echo $e->getMessage() . PHP_EOL;
+        $this->errorLog->writeln($this->getExceptionString($e, $codeSnippet));
     }
 
     public function getExceptionString(AbstractLocatedException $e, CodeSnippet $codeSnippet): string
@@ -63,18 +72,13 @@ public function getExceptionString(AbstractLocatedException $e, CodeSnippet $cod
             }
         }
 
-        if ($e->getPrevious()) {
-            $str .= PHP_EOL . PHP_EOL . 'Caused by:' . PHP_EOL;
-            $str .= $e->getPrevious()->getTraceAsString();
-            $str .= PHP_EOL;
-        }
-
         return $str;
     }
 
     public function printStackTrace(Throwable $e): void
     {
-        echo $this->getStackTraceString($e);
+        echo $e->getMessage() . PHP_EOL;
+        $this->errorLog->writeln($this->getStackTraceString($e));
     }
 
     public function getStackTraceString(Throwable $e): string

diff --git a/src/php/Config/PhelConfig.php b/src/php/Config/PhelConfig.php
@@ -19,6 +19,7 @@ final class PhelConfig implements JsonSerializable
     /** @var list<string> */
     private array $testDirs = ['tests/phel'];
     private string $vendorDir = 'vendor';
+    private string $errorLogFile = 'data/error.log';
     private PhelExportConfig $export;
     private PhelOutConfig $out;
 
@@ -89,6 +90,18 @@ public function setExport(PhelExportConfig $export): self
         return $this;
     }
 
+    public function getErrorLogFile(): string
+    {
+        return $this->errorLogFile;
+    }
+
+    public function setErrorLogFile(string $file): self
+    {
+        $this->errorLogFile = $file;
+
+        return $this;
+    }
+
     public function getOut(): PhelOutConfig
     {
         return $this->out;
@@ -149,6 +162,7 @@ public function jsonSerialize(): array
             CommandConfig::SRC_DIRS => $this->getSrcDirs(),
             CommandConfig::TEST_DIRS => $this->getTestDirs(),
             CommandConfig::VENDOR_DIR => $this->getVendorDir(),
+            CommandConfig::ERROR_LOG_FILE => $this->getErrorLogFile(),
             CommandConfig::OUTPUT => $this->getOut()->jsonSerialize(),
             InteropConfig::EXPORT => $this->getExport()->jsonSerialize(),
             BuildConfig::IGNORE_WHEN_BUILDING => $this->getIgnoreWhenBuilding(),

diff --git a/tests/php/Integration/Run/Command/Repl/ReplCommandTest.php b/tests/php/Integration/Run/Command/Repl/ReplCommandTest.php
@@ -8,6 +8,7 @@
 use Gacela\Framework\ClassResolver\GlobalInstance\AnonymousGlobal;
 use Gacela\Framework\Gacela;
 use Generator;
+use Phel\Command\Domain\Shared\ErrorLog\ErrorLogInterface;
 use Phel\Command\Domain\Shared\Exceptions\ExceptionArgsPrinter;
 use Phel\Command\Domain\Shared\Exceptions\Extractor\FilePositionExtractor;
 use Phel\Command\Domain\Shared\Exceptions\Extractor\SourceMapExtractor;
@@ -31,11 +32,10 @@ final class ReplCommandTest extends AbstractCommandTest
 {
     public function setUp(): void
     {
-        $configFn = static function (GacelaConfig $config): void {
+        Gacela::bootstrap(__DIR__, static function (GacelaConfig $config): void {
             $config->resetInMemoryCache();
             $config->addAppConfig('config/*.php', 'config/local.php');
-        };
-        Gacela::bootstrap(__DIR__, $configFn);
+        });
     }
 
     /**
@@ -152,6 +152,7 @@ private function createReplTestIo(): ReplTestIo
             ColorStyle::noStyles(),
             new Munge(),
             new FilePositionExtractor(new SourceMapExtractor()),
+            $this->createStub(ErrorLogInterface::class),
         );
 
         return new ReplTestIo($exceptionPrinter);