From cc6f8dbbb71a6f6d7350720984f80566e2ef4464 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 2 Sep 2026 08:43:11 +0600 Subject: [PATCH 1/3] feat(fenom): Name the source in compile errors Fenom logged a content hash as the template name, so a syntax error could not be traced to a chunk, file, or resource. Keep the same cache keys and add a readable label, excerpt, and cache path. Fixes #21 --- core/components/pdotools/docs/changelog.txt | 1 + core/components/pdotools/src/CoreTools.php | 84 +++++++ .../pdotools/src/Parsing/Fenom/ErrorLog.php | 141 ++++++++++++ .../pdotools/src/Parsing/Fenom/Fenom.php | 205 +++++++++++++++++- .../pdotools/src/Parsing/Parser.php | 37 +++- 5 files changed, 459 insertions(+), 9 deletions(-) create mode 100644 core/components/pdotools/src/Parsing/Fenom/ErrorLog.php diff --git a/core/components/pdotools/docs/changelog.txt b/core/components/pdotools/docs/changelog.txt index b62d9f3..0f4bff4 100644 --- a/core/components/pdotools/docs/changelog.txt +++ b/core/components/pdotools/docs/changelog.txt @@ -15,6 +15,7 @@ Versioning: [Semantic Versioning](https://semver.org/spec/v2.0.0.html) - [#358] [pdoTools] cachePath can be set via the "pdotools_cache_path" option. - [#392] [pdoPage] Snippet properties are available as placeholders in pagination templates. - Added GitHub Actions release workflow for 3.x transport packages. +- [pdoTools3#21] [Fenom] Compile and runtime errors log the element name, resource, source excerpt, and cache path instead of a bare content hash. ### Changed diff --git a/core/components/pdotools/src/CoreTools.php b/core/components/pdotools/src/CoreTools.php index 7bf5236..62e720b 100644 --- a/core/components/pdotools/src/CoreTools.php +++ b/core/components/pdotools/src/CoreTools.php @@ -723,6 +723,7 @@ protected function _loadElement($name, $type, $row = []) } $properties = []; + $sourceFile = ''; /** @var modElement $element */ switch ($binding) { case 'CODE': @@ -765,6 +766,7 @@ protected function _loadElement($name, $type, $row = []) return false; } + $sourceFile = $path; if ($content = file_get_contents($path)) { $element = $this->modx->newObject($type, ['name' => $cache_name]); $element->setContent($content); @@ -820,6 +822,16 @@ protected function _loadElement($name, $type, $row = []) return false; } + if ($sourceFile === '' && $element instanceof modElement && method_exists($element, 'isStatic') && $element->isStatic()) { + $file = $element->getSourceFile(); + if (!empty($file)) { + $sourceFile = $file; + } + } + $elementName = $type === 'modTemplate' + ? (string)$element->get('templatename') + : (string)$element->get('name'); + $data = [ 'object' => $element, 'content' => $content, @@ -828,12 +840,84 @@ protected function _loadElement($name, $type, $row = []) 'id' => (int)$element->get('id'), 'binding' => strtolower($type), 'cacheable' => $cacheable, + 'elementName' => $elementName, + 'sourceFile' => $sourceFile, + 'sourceLabel' => $this->buildElementSourceLabel($type, $binding, $element, $elementName, $sourceFile), ]; $this->setStore($cache_key, $data, $type); return $data; } + /** + * Label for Fenom error logs. Not used as a cache key. + * + * @param string $type + * @param string $binding + * @param modElement $element + * @param string $elementName + * @param string $sourceFile + * @return string + */ + protected function buildElementSourceLabel($type, $binding, $element, $elementName, $sourceFile) + { + $kinds = [ + 'modChunk' => 'chunk', + 'modTemplate' => 'template', + 'modSnippet' => 'snippet', + ]; + $kind = isset($kinds[$type]) ? $kinds[$type] : 'element'; + $id = is_object($element) ? (int)$element->get('id') : 0; + $file = $this->toLogPath($sourceFile); + $isHash = is_string($elementName) && (bool)preg_match('/^[a-f0-9]{32}$/i', $elementName); + + if ($binding === 'FILE' && $file !== '') { + return 'file:' . $file; + } + if ($binding === 'INLINE' || $binding === 'CODE') { + return 'inline'; + } + if ($elementName !== '' && !$isHash) { + $label = $kind . ':' . $elementName; + if ($id > 0) { + $label .= ' (#' . $id . ')'; + } + if ($file !== '') { + $label .= ' file:' . $file; + } + + return $label; + } + if ($id > 0) { + return $kind . ':#' . $id; + } + if ($file !== '') { + return 'file:' . $file; + } + + return $kind; + } + + /** + * @param string $path + * @return string + */ + protected function toLogPath($path) + { + if (!is_string($path) || $path === '') { + return ''; + } + $path = str_replace('\\', '/', $path); + if (defined('MODX_CORE_PATH') && MODX_CORE_PATH !== '' && strpos($path, str_replace('\\', '/', MODX_CORE_PATH)) === 0) { + return 'core/' . ltrim(substr($path, strlen(str_replace('\\', '/', MODX_CORE_PATH))), '/'); + } + if (defined('MODX_BASE_PATH') && MODX_BASE_PATH !== '' && strpos($path, str_replace('\\', '/', MODX_BASE_PATH)) === 0) { + return ltrim(substr($path, strlen(str_replace('\\', '/', MODX_BASE_PATH))), '/'); + } + + return $path; + } + /** * Builds a hierarchical tree from given array * diff --git a/core/components/pdotools/src/Parsing/Fenom/ErrorLog.php b/core/components/pdotools/src/Parsing/Fenom/ErrorLog.php new file mode 100644 index 0000000..e2c14a0 --- /dev/null +++ b/core/components/pdotools/src/Parsing/Fenom/ErrorLog.php @@ -0,0 +1,141 @@ +getMessage(), $m)) { + return (int)$m[1]; + } + if ($e instanceof ErrorException) { + $file = str_replace('\\', '/', (string)$e->getFile()); + if ($file !== '' && substr($file, -4) !== '.php' && strpos($file, '/') === false) { + return (int)$e->getLine(); + } + } + + return 0; + } + + /** + * @param string $message + * @return string + */ + public static function extractNear($message) + { + if (!is_string($message) || !preg_match("/near '([^']*)'/s", $message, $m)) { + return ''; + } + + return $m[1]; + } + + /** + * @param string $text + * @return bool + */ + public static function hasUnprocessedModx($text) + { + return is_string($text) && (bool)preg_match('/\[\[(?:\+|\*|\$|%|~|#|&)?/', $text); + } + + /** + * @param string $text + * @return string + */ + public static function modxHint($text) + { + if (!self::hasUnprocessedModx($text)) { + return ''; + } + if (preg_match('/\[\[\+([a-zA-Z0-9._-]+)/', $text, $m)) { + return 'Unprocessed MODX tag inside Fenom. Use {$' . $m[1] . '} or parse MODX before Fenom.'; + } + + return 'Unprocessed MODX tag inside Fenom. Use {$placeholder} or parse MODX before Fenom.'; + } + + /** + * @param string $content + * @param int $line + * @param int $radius + * @return string + */ + public static function excerpt($content, $line, $radius = 2) + { + if (!is_string($content) || $content === '' || $line < 1) { + return ''; + } + $lines = preg_split("/\r\n|\n|\r/", $content); + $index = $line - 1; + if (!isset($lines[$index])) { + return ''; + } + $start = max(0, $index - $radius); + $end = min(count($lines) - 1, $index + $radius); + $out = []; + for ($i = $start; $i <= $end; $i++) { + $mark = ($i === $index) ? '>' : ' '; + $out[] = sprintf('%s %d: %s', $mark, $i + 1, $lines[$i]); + } + + return implode("\n", $out); + } + + /** + * @param string $message + * @param string $name + * @param string $label + * @return string + */ + public static function replaceTemplateName($message, $name, $label) + { + if (!is_string($message) || $name === '' || $label === '' || $name === $label) { + return $message; + } + + return str_replace(' in ' . $name . ' ', ' in ' . $label . ' ', $message); + } +} diff --git a/core/components/pdotools/src/Parsing/Fenom/Fenom.php b/core/components/pdotools/src/Parsing/Fenom/Fenom.php index adb5885..515eeae 100644 --- a/core/components/pdotools/src/Parsing/Fenom/Fenom.php +++ b/core/components/pdotools/src/Parsing/Fenom/Fenom.php @@ -103,11 +103,12 @@ public function process($chunk, array $properties = []) $name = md5($content); } /** @var \Fenom\Template $tpl */ + $label = $this->resolveSourceLabel($chunk, $name, $content); if (!$tpl = $this->pdoTools->getStore($name, 'fenom')) { if (!empty($this->pdoTools->config('useFenomCache'))) { $compileKey = 'pdotools/' . $name; if (!$cache = $this->pdoTools->getExactCache($compileKey)) { - if ($tpl = $this->_compileChunk($content, $name)) { + if ($tpl = $this->_compileChunk($content, $name, $label)) { $this->pdoTools->setExactCache($compileKey, $tpl->getTemplateCode()); } } else { @@ -115,7 +116,7 @@ public function process($chunk, array $properties = []) $tpl = eval($cache); } } else { - $tpl = $this->_compileChunk($content, $name); + $tpl = $this->_compileChunk($content, $name, $label); } if ($tpl) { $this->pdoTools->setStore($name, $tpl, 'fenom'); @@ -134,8 +135,7 @@ public function process($chunk, array $properties = []) try { $content = $tpl->fetch($properties); } catch (Exception $e) { - $this->modx->log(modX::LOG_LEVEL_ERROR, $e->getMessage()); - $this->modx->log(modX::LOG_LEVEL_INFO, $tpl->getTemplateCode()); + $this->logFenomError($e, $name, $content, $label, 'runtime'); } } @@ -173,30 +173,219 @@ public function setCompileDir($dir) * * @param $content * @param string $name + * @param string $label * * @return \Fenom\Template */ - protected function _compileChunk($content, $name = '') + protected function _compileChunk($content, $name = '', $label = '') { if (empty($name)) { $name = md5($content); } + if ($label === '') { + $label = $name; + } try { $tpl = $this->getRawTemplate()->source($name, $content, true); $this->pdoTools->addTime('Compiled Fenom chunk with name "' . $name . '"'); } catch (Exception $e) { - $this->modx->log(modX::LOG_LEVEL_ERROR, $e->getMessage()); - $this->modx->log(modX::LOG_LEVEL_INFO, $content); if ($this->modx->getOption('pdotools_fenom_save_on_errors')) { $this->pdoTools->setExactCache('error/' . $name, $content); } + $this->logFenomError($e, $name, $content, $label, 'compile'); $tpl = $this->getRawTemplate()->source($name, '', false); - $this->pdoTools->addTime('Can`t compile Fenom chunk with name "' . $name . '": ' . $e->getMessage()); } return $tpl; } + /** + * Human-readable source for logs. Cache keys still use $name. + * + * @param array|string $chunk + * @param string $name + * @param string $content + * @return string + */ + protected function resolveSourceLabel($chunk, $name, $content) + { + if (is_array($chunk) && !empty($chunk['sourceLabel'])) { + return (string)$chunk['sourceLabel']; + } + + $type = ''; + $id = 0; + $elementName = ''; + $sourceFile = ''; + if (is_array($chunk)) { + $type = !empty($chunk['binding']) ? (string)$chunk['binding'] : ''; + $id = !empty($chunk['id']) ? (int)$chunk['id'] : 0; + if (!empty($chunk['sourceFile'])) { + $sourceFile = ErrorLog::relativePath($chunk['sourceFile']); + } + if (!empty($chunk['elementName'])) { + $elementName = (string)$chunk['elementName']; + } elseif (!empty($chunk['name']) && !ErrorLog::looksLikeHash($chunk['name'])) { + $elementName = (string)$chunk['name']; + } + } + + $kind = $this->elementKind($type); + if ($sourceFile !== '' && ($elementName === '' || ErrorLog::looksLikeHash($elementName))) { + return trim($kind . ' file:' . $sourceFile); + } + if ($elementName !== '') { + $label = $kind !== '' ? $kind . ':' . $elementName : $elementName; + if ($id > 0) { + $label .= ' (#' . $id . ')'; + } + if ($sourceFile !== '') { + $label .= ' file:' . $sourceFile; + } + + return $label; + } + if ($id > 0) { + return ($kind !== '' ? $kind : 'element') . ':#' . $id; + } + + $page = $this->formatResourceContext(); + if ($page !== '') { + return $page; + } + + return $name !== '' ? $name : md5($content); + } + + /** + * @param string $binding + * @return string + */ + protected function elementKind($binding) + { + switch ($binding) { + case 'modchunk': + case 'modChunk': + return 'chunk'; + case 'modtemplate': + case 'modTemplate': + return 'template'; + case 'modsnippet': + case 'modSnippet': + return 'snippet'; + default: + return ''; + } + } + + /** + * @return string + */ + protected function formatResourceContext() + { + $resource = $this->modx->resource; + if (!is_object($resource) || !method_exists($resource, 'get')) { + return ''; + } + $id = (int)$resource->get('id'); + if ($id <= 0) { + return ''; + } + $ctx = ''; + if (is_object($this->modx->context) && method_exists($this->modx->context, 'get')) { + $ctx = (string)$this->modx->context->get('key'); + } + $uri = (string)$resource->get('uri'); + if ($uri === '') { + $uri = (string)$resource->get('alias'); + } + $label = 'resource:#' . $id; + if ($ctx !== '' || $uri !== '') { + $label .= ' (' . $ctx . ':' . $uri . ')'; + } + + return $label; + } + + /** + * @param Exception $e + * @param string $name + * @param string $content + * @param string $label + * @param string $phase + */ + protected function logFenomError(Exception $e, $name, $content, $label, $phase) + { + $message = $this->formatFenomError($e, $name, $content, $label, $phase); + $this->modx->log(modX::LOG_LEVEL_ERROR, $message); + $this->pdoTools->addTime($message); + } + + /** + * @param Exception $e + * @param string $name + * @param string $content + * @param string $label + * @param string $phase + * @return string + */ + protected function formatFenomError(Exception $e, $name, $content, $label, $phase) + { + if ($label === '') { + $label = $name; + } + $raw = ErrorLog::replaceTemplateName($e->getMessage(), $name, $label); + $line = ErrorLog::extractLine($e); + $near = ErrorLog::extractNear($e->getMessage()); + $excerpt = ErrorLog::excerpt($content, $line); + $hint = ErrorLog::modxHint($near . "\n" . $excerpt); + + $lines = [ + '[pdoTools][Fenom] ' . $phase . ' error in ' . $label, + ]; + $context = $this->formatResourceContext(); + if ($context !== '' && strpos($label, 'resource:') !== 0) { + $lines[] = $context; + } + if ($name !== '' && $name !== $label) { + $lines[] = 'cache name: ' . $name; + } + $lines[] = $raw; + if ($excerpt !== '') { + $lines[] = $excerpt; + } + if ($hint !== '') { + $lines[] = $hint; + } + $compiled = $this->compiledPathIfExists($name); + if ($compiled !== '') { + $lines[] = 'compiled: ' . $compiled; + } + if ($phase === 'compile' && $this->modx->getOption('pdotools_fenom_save_on_errors')) { + $errorPath = rtrim((string)$this->pdoTools->config('cachePath'), '/'); + $lines[] = 'source dump: ' . ErrorLog::relativePath($errorPath . '/error/' . $name); + } + + return implode("\n", $lines); + } + + /** + * @param string $name + * @return string + */ + protected function compiledPathIfExists($name) + { + if ($name === '' || empty($this->_compile_dir)) { + return ''; + } + $file = rtrim((string)$this->_compile_dir, '/\\') . '/' . $this->getCompileName($name); + if (!is_file($file)) { + return ''; + } + + return ErrorLog::relativePath($file); + } + /** * Add default modifiers diff --git a/core/components/pdotools/src/Parsing/Parser.php b/core/components/pdotools/src/Parsing/Parser.php index e4422af..ee953bd 100644 --- a/core/components/pdotools/src/Parsing/Parser.php +++ b/core/components/pdotools/src/Parsing/Parser.php @@ -66,7 +66,10 @@ public function processElementTags( } $_processingUncacheable = $this->_processingUncacheable; $this->_processingUncacheable = true; - $content = $this->pdoTools->getFenom()->process($content, $this->modx->placeholders); + $content = $this->pdoTools->getFenom()->process([ + 'content' => $content, + 'sourceLabel' => $this->fenomSourceLabel(), + ], $this->modx->placeholders); $this->_processingUncacheable = $_processingUncacheable; } @@ -279,4 +282,36 @@ public function processTag($tag, $processUncacheable = true) return $output; } + /** + * Label for Fenom page-parser errors. Does not become a cache key. + * + * @return string + */ + protected function fenomSourceLabel() + { + $resource = $this->modx->resource; + if (!is_object($resource) || !method_exists($resource, 'get')) { + return 'resource'; + } + $id = (int)$resource->get('id'); + $ctx = ''; + if (is_object($this->modx->context) && method_exists($this->modx->context, 'get')) { + $ctx = (string)$this->modx->context->get('key'); + } + $uri = (string)$resource->get('uri'); + if ($uri === '') { + $uri = (string)$resource->get('alias'); + } + $label = 'resource:#' . $id; + if ($ctx !== '' || $uri !== '') { + $label .= ' (' . $ctx . ':' . $uri . ')'; + } + $tplId = (int)$resource->get('template'); + if ($tplId > 0) { + $label .= ', template:#' . $tplId; + } + + return $label; + } + } From f972045f47013c18696e371b2dfc62a14aa78efb Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 2 Sep 2026 12:25:48 +0600 Subject: [PATCH 2/3] test(fenom): Cover named compile error logs Assert labels, excerpts, MODX-tag hints, and that the Fenom cache name stays a hash or binding/id. --- .../pdotools/tests/Stubs/ModxStub.php | 11 ++ .../tests/Support/CoreToolsHarness.php | 12 ++ .../pdotools/tests/Support/FenomHarness.php | 29 ++++ .../tests/Unit/CoreTools/SourceLabelTest.php | 60 +++++++++ .../tests/Unit/Parsing/ErrorLogTest.php | 71 ++++++++-- .../tests/Unit/Parsing/FenomErrorLogTest.php | 127 ++++++++++++++++++ 6 files changed, 301 insertions(+), 9 deletions(-) create mode 100644 core/components/pdotools/tests/Support/FenomHarness.php create mode 100644 core/components/pdotools/tests/Unit/CoreTools/SourceLabelTest.php create mode 100644 core/components/pdotools/tests/Unit/Parsing/FenomErrorLogTest.php diff --git a/core/components/pdotools/tests/Stubs/ModxStub.php b/core/components/pdotools/tests/Stubs/ModxStub.php index 10ceba1..1f59a81 100644 --- a/core/components/pdotools/tests/Stubs/ModxStub.php +++ b/core/components/pdotools/tests/Stubs/ModxStub.php @@ -51,6 +51,11 @@ public function offsetUnset($offset): void if (!class_exists(modX::class, false)) { class modX { + public const LOG_LEVEL_ERROR = 0; + public const LOG_LEVEL_WARN = 1; + public const LOG_LEVEL_INFO = 2; + public const LOG_LEVEL_DEBUG = 3; + public $user; public $context; public $resource; @@ -92,6 +97,12 @@ public function getOption($key, $options = null, $default = null, $skipEmpty = f return $value; } } + if (array_key_exists($key, $this->config)) { + $value = $this->config[$key]; + if (!$skipEmpty || ($value !== '' && $value !== null)) { + return $value; + } + } return $default; } diff --git a/core/components/pdotools/tests/Support/CoreToolsHarness.php b/core/components/pdotools/tests/Support/CoreToolsHarness.php index ef5393f..c6660fe 100644 --- a/core/components/pdotools/tests/Support/CoreToolsHarness.php +++ b/core/components/pdotools/tests/Support/CoreToolsHarness.php @@ -25,4 +25,16 @@ public function publicGetCacheKey($options = []) { return $this->getCacheKey($options); } + + /** + * @param string $type + * @param string $binding + * @param object $element + * @param string $elementName + * @param string $sourceFile + */ + public function publicBuildElementSourceLabel($type, $binding, $element, $elementName, $sourceFile): string + { + return $this->buildElementSourceLabel($type, $binding, $element, $elementName, $sourceFile); + } } diff --git a/core/components/pdotools/tests/Support/FenomHarness.php b/core/components/pdotools/tests/Support/FenomHarness.php new file mode 100644 index 0000000..d4674ac --- /dev/null +++ b/core/components/pdotools/tests/Support/FenomHarness.php @@ -0,0 +1,29 @@ +|string $chunk + */ + public function publicResolveSourceLabel($chunk, string $name, string $content): string + { + return $this->resolveSourceLabel($chunk, $name, $content); + } + + public function publicFormatFenomError( + Exception $e, + string $name, + string $content, + string $label, + string $phase + ): string { + return $this->formatFenomError($e, $name, $content, $label, $phase); + } +} diff --git a/core/components/pdotools/tests/Unit/CoreTools/SourceLabelTest.php b/core/components/pdotools/tests/Unit/CoreTools/SourceLabelTest.php new file mode 100644 index 0000000..85ab9fb --- /dev/null +++ b/core/components/pdotools/tests/Unit/CoreTools/SourceLabelTest.php @@ -0,0 +1,60 @@ +modx); + $this->assertSame( + 'chunk:tpl.product.row (#12)', + $tools->publicBuildElementSourceLabel('modChunk', '', $this->element(12), 'tpl.product.row', '') + ); + } + + public function testFileBindingUsesCoreRelativePath(): void + { + $tools = new CoreToolsHarness($this->modx); + $file = rtrim(str_replace('\\', '/', MODX_CORE_PATH), '/') . '/elements/chunks/item.tpl'; + $this->assertSame( + 'file:core/elements/chunks/item.tpl', + $tools->publicBuildElementSourceLabel('modChunk', 'FILE', $this->element(0), '', $file) + ); + } + + public function testInlineBinding(): void + { + $tools = new CoreToolsHarness($this->modx); + $this->assertSame( + 'inline', + $tools->publicBuildElementSourceLabel('modChunk', 'INLINE', $this->element(0), '', '') + ); + } + + /** + * @return object + */ + private function element(int $id) + { + return new class ($id) { + /** @var int */ + private $id; + + public function __construct(int $id) + { + $this->id = $id; + } + + public function get($key) + { + return $key === 'id' ? $this->id : null; + } + }; + } +} diff --git a/core/components/pdotools/tests/Unit/Parsing/ErrorLogTest.php b/core/components/pdotools/tests/Unit/Parsing/ErrorLogTest.php index 3d438a9..4028288 100644 --- a/core/components/pdotools/tests/Unit/Parsing/ErrorLogTest.php +++ b/core/components/pdotools/tests/Unit/Parsing/ErrorLogTest.php @@ -4,23 +4,18 @@ namespace ModxPro\PdoTools\Tests\Unit\Parsing; +use ErrorException; +use Exception; use ModxPro\PdoTools\Parsing\Fenom\ErrorLog; use PHPUnit\Framework\TestCase; class ErrorLogTest extends TestCase { - protected function setUp(): void - { - parent::setUp(); - if (!class_exists(ErrorLog::class)) { - $this->markTestSkipped('ErrorLog is not on this branch yet (pdoTools3#21).'); - } - } - public function testLooksLikeHash(): void { $this->assertTrue(ErrorLog::looksLikeHash(md5('chunk'))); $this->assertFalse(ErrorLog::looksLikeHash('my-chunk')); + $this->assertFalse(ErrorLog::looksLikeHash('modchunk/12')); } public function testExcerptMarksTheLine(): void @@ -28,8 +23,16 @@ public function testExcerptMarksTheLine(): void $content = "one\ntwo\n{var \$x = [[+limit]]}\nfour"; $excerpt = ErrorLog::excerpt($content, 3, 1); - $this->assertStringContainsString('>', $excerpt); + $this->assertStringContainsString('> 3:', $excerpt); $this->assertStringContainsString('[[+limit]]', $excerpt); + $this->assertStringContainsString(' 2: two', $excerpt); + } + + public function testExcerptRejectsMissingLine(): void + { + $this->assertSame('', ErrorLog::excerpt("one\ntwo", 9)); + $this->assertSame('', ErrorLog::excerpt('', 1)); + $this->assertSame('', ErrorLog::excerpt('one', 0)); } public function testModxHintNamesThePlaceholder(): void @@ -38,9 +41,59 @@ public function testModxHintNamesThePlaceholder(): void $this->assertStringContainsString('{$limit}', $hint); } + public function testModxHintGenericWhenTagHasNoName(): void + { + $hint = ErrorLog::modxHint('[[$other]]'); + $this->assertStringContainsString('{$placeholder}', $hint); + } + public function testHasUnprocessedModx(): void { $this->assertTrue(ErrorLog::hasUnprocessedModx('[[+limit]]')); + $this->assertTrue(ErrorLog::hasUnprocessedModx('[[*pagetitle]]')); $this->assertFalse(ErrorLog::hasUnprocessedModx('{$limit}')); } + + public function testReplaceTemplateNameKeepsHashOutOfTheMessage(): void + { + $hash = 'ee058690d9fd7413748b95b0960e006b'; + $message = "Unexpected token '+' in expression in {$hash} line 6, near '{var \$limit = [[+' <- there"; + $replaced = ErrorLog::replaceTemplateName($message, $hash, 'chunk:tpl.product.row (#12)'); + + $this->assertStringContainsString('chunk:tpl.product.row (#12)', $replaced); + $this->assertStringNotContainsString($hash, $replaced); + } + + public function testReplaceTemplateNameLeavesMessageWhenLabelMatchesName(): void + { + $message = 'error in inline line 1'; + $this->assertSame($message, ErrorLog::replaceTemplateName($message, 'inline', 'inline')); + } + + public function testExtractLineFromMessage(): void + { + $e = new Exception("Unexpected token '+' in expression in chunk:row line 6, near '{var'"); + $this->assertSame(6, ErrorLog::extractLine($e)); + } + + public function testExtractNear(): void + { + $this->assertSame( + '{var $limit = [[+', + ErrorLog::extractNear("near '{var \$limit = [[+' <- there") + ); + $this->assertSame('', ErrorLog::extractNear('no near clause')); + } + + public function testRelativePathUsesCorePrefix(): void + { + $path = rtrim(str_replace('\\', '/', MODX_CORE_PATH), '/') . '/cache/pdotools/error/foo'; + $this->assertSame('core/cache/pdotools/error/foo', ErrorLog::relativePath($path)); + } + + public function testExtractLineFromErrorExceptionWithoutPhpFile(): void + { + $e = new ErrorException('boom', 0, E_ERROR, 'inline-template', 4); + $this->assertSame(4, ErrorLog::extractLine($e)); + } } diff --git a/core/components/pdotools/tests/Unit/Parsing/FenomErrorLogTest.php b/core/components/pdotools/tests/Unit/Parsing/FenomErrorLogTest.php new file mode 100644 index 0000000..a01f77d --- /dev/null +++ b/core/components/pdotools/tests/Unit/Parsing/FenomErrorLogTest.php @@ -0,0 +1,127 @@ +harness(); + $this->assertSame( + 'chunk:tpl.product.row (#12)', + $fenom->publicResolveSourceLabel( + ['sourceLabel' => 'chunk:tpl.product.row (#12)', 'name' => md5('x')], + md5('x'), + '{var $x = 1}' + ) + ); + } + + public function testSourceLabelFromChunkBinding(): void + { + $fenom = $this->harness(); + $this->assertSame( + 'chunk:tpl.product.row (#12)', + $fenom->publicResolveSourceLabel( + [ + 'binding' => 'modchunk', + 'id' => 12, + 'name' => 'tpl.product.row', + 'elementName' => 'tpl.product.row', + ], + 'modchunk/12', + '{var $x = 1}' + ) + ); + } + + public function testSourceLabelForFileAndInline(): void + { + $fenom = $this->harness(); + $file = rtrim(str_replace('\\', '/', MODX_CORE_PATH), '/') . '/elements/chunks/item.tpl'; + $this->assertSame( + 'chunk file:core/elements/chunks/item.tpl', + $fenom->publicResolveSourceLabel( + ['binding' => 'modchunk', 'sourceFile' => $file], + 'modchunk/' . md5('file'), + '{var $x = 1}' + ) + ); + $this->assertSame( + 'inline', + $fenom->publicResolveSourceLabel(['sourceLabel' => 'inline'], 'inline', '{var $x = 1}') + ); + } + + public function testFormatReplacesHashAndAddsHint(): void + { + $hash = 'ee058690d9fd7413748b95b0960e006b'; + $content = "one\ntwo\nthree\nfour\nfive\n{var \$limit = [[+limit]]}\nseven"; + $e = new Exception( + "Unexpected token '+' in expression in {$hash} line 6, near '{var \$limit = [[+' <- there" + ); + $message = $this->harness()->publicFormatFenomError( + $e, + $hash, + $content, + 'chunk:tpl.product.row (#12)', + 'compile' + ); + + $this->assertStringContainsString('[pdoTools][Fenom] compile error in chunk:tpl.product.row (#12)', $message); + $this->assertStringContainsString('cache name: ' . $hash, $message); + $this->assertStringContainsString('chunk:tpl.product.row (#12) line 6', $message); + $this->assertStringNotContainsString(' in ' . $hash . ' ', $message); + $this->assertStringContainsString('> 6:', $message); + $this->assertStringContainsString('{$limit}', $message); + } + + public function testProcessLogsCompileErrorWithoutChangingCacheName(): void + { + $chunk = [ + 'content' => '{var $limit = [[+limit]]}', + 'binding' => 'modchunk', + 'id' => 12, + 'name' => 'tpl.product.row', + 'elementName' => 'tpl.product.row', + 'sourceLabel' => 'chunk:tpl.product.row (#12)', + ]; + $this->fenom()->process($chunk); + + $this->assertNotEmpty($this->modx->logs); + $logged = (string)$this->modx->logs[0]['message']; + $this->assertStringContainsString('chunk:tpl.product.row (#12)', $logged); + $this->assertStringContainsString('cache name: modchunk/12', $logged); + $this->assertStringContainsString('{$limit}', $logged); + $this->assertNotNull($this->pdoTools->getStore('modchunk/12', 'fenom')); + } + + public function testSaveOnErrorsListsSourceDump(): void + { + $this->modx->config['pdotools_fenom_save_on_errors'] = true; + $this->modx->cacheManager = $this->modx->getCacheManager(); + $hash = md5('broken'); + $e = new Exception("Unexpected token '+' in expression in {$hash} line 1, near '{var'"); + $message = $this->harness()->publicFormatFenomError( + $e, + $hash, + '{var $limit = [[+limit]]}', + 'inline', + 'compile' + ); + + $this->assertStringContainsString('source dump:', $message); + $this->assertStringContainsString('error/' . $hash, $message); + } + + private function harness(): FenomHarness + { + return new FenomHarness($this->modx, $this->pdoTools); + } +} From 1fe9d1475fc3d27cfc2a8f0fd1af3886fb7f2c8e Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Wed, 2 Sep 2026 12:54:53 +0600 Subject: [PATCH 3/3] refactor(fenom): Centralize error labels in ErrorLog One label/format owner; CoreTools and Parser pass origin and resource facts only. Drop duplicate builders and happy-path label work. --- core/components/pdotools/src/CoreTools.php | 71 +------ .../pdotools/src/Parsing/Fenom/ErrorLog.php | 183 +++++++++++++++-- .../pdotools/src/Parsing/Fenom/Fenom.php | 192 ++++-------------- .../pdotools/src/Parsing/Parser.php | 37 ++-- .../tests/Support/CoreToolsHarness.php | 12 -- .../pdotools/tests/Support/FenomHarness.php | 29 --- .../tests/Unit/CoreTools/SourceLabelTest.php | 60 ------ .../tests/Unit/Parsing/ErrorLogTest.php | 164 +++++++++------ .../tests/Unit/Parsing/FenomErrorLogTest.php | 118 +++-------- 9 files changed, 351 insertions(+), 515 deletions(-) delete mode 100644 core/components/pdotools/tests/Support/FenomHarness.php delete mode 100644 core/components/pdotools/tests/Unit/CoreTools/SourceLabelTest.php diff --git a/core/components/pdotools/src/CoreTools.php b/core/components/pdotools/src/CoreTools.php index 62e720b..52a7b74 100644 --- a/core/components/pdotools/src/CoreTools.php +++ b/core/components/pdotools/src/CoreTools.php @@ -839,85 +839,16 @@ protected function _loadElement($name, $type, $row = []) 'name' => $cache_name, 'id' => (int)$element->get('id'), 'binding' => strtolower($type), + 'origin' => $binding, 'cacheable' => $cacheable, 'elementName' => $elementName, 'sourceFile' => $sourceFile, - 'sourceLabel' => $this->buildElementSourceLabel($type, $binding, $element, $elementName, $sourceFile), ]; $this->setStore($cache_key, $data, $type); return $data; } - /** - * Label for Fenom error logs. Not used as a cache key. - * - * @param string $type - * @param string $binding - * @param modElement $element - * @param string $elementName - * @param string $sourceFile - * @return string - */ - protected function buildElementSourceLabel($type, $binding, $element, $elementName, $sourceFile) - { - $kinds = [ - 'modChunk' => 'chunk', - 'modTemplate' => 'template', - 'modSnippet' => 'snippet', - ]; - $kind = isset($kinds[$type]) ? $kinds[$type] : 'element'; - $id = is_object($element) ? (int)$element->get('id') : 0; - $file = $this->toLogPath($sourceFile); - $isHash = is_string($elementName) && (bool)preg_match('/^[a-f0-9]{32}$/i', $elementName); - - if ($binding === 'FILE' && $file !== '') { - return 'file:' . $file; - } - if ($binding === 'INLINE' || $binding === 'CODE') { - return 'inline'; - } - if ($elementName !== '' && !$isHash) { - $label = $kind . ':' . $elementName; - if ($id > 0) { - $label .= ' (#' . $id . ')'; - } - if ($file !== '') { - $label .= ' file:' . $file; - } - - return $label; - } - if ($id > 0) { - return $kind . ':#' . $id; - } - if ($file !== '') { - return 'file:' . $file; - } - - return $kind; - } - - /** - * @param string $path - * @return string - */ - protected function toLogPath($path) - { - if (!is_string($path) || $path === '') { - return ''; - } - $path = str_replace('\\', '/', $path); - if (defined('MODX_CORE_PATH') && MODX_CORE_PATH !== '' && strpos($path, str_replace('\\', '/', MODX_CORE_PATH)) === 0) { - return 'core/' . ltrim(substr($path, strlen(str_replace('\\', '/', MODX_CORE_PATH))), '/'); - } - if (defined('MODX_BASE_PATH') && MODX_BASE_PATH !== '' && strpos($path, str_replace('\\', '/', MODX_BASE_PATH)) === 0) { - return ltrim(substr($path, strlen(str_replace('\\', '/', MODX_BASE_PATH))), '/'); - } - - return $path; - } - /** * Builds a hierarchical tree from given array * diff --git a/core/components/pdotools/src/Parsing/Fenom/ErrorLog.php b/core/components/pdotools/src/Parsing/Fenom/ErrorLog.php index e2c14a0..197fcfd 100644 --- a/core/components/pdotools/src/Parsing/Fenom/ErrorLog.php +++ b/core/components/pdotools/src/Parsing/Fenom/ErrorLog.php @@ -2,20 +2,175 @@ namespace ModxPro\PdoTools\Parsing\Fenom; -use ErrorException; use Throwable; /** - * Formats Fenom compile/runtime errors for the MODX log. - * Does not change template names or cache keys. + * Builds Fenom error labels and log messages. Does not change cache keys. */ class ErrorLog { + /** + * Human-readable source from element/resource facts. + * + * Expected keys: binding (modchunk|…), origin (FILE|INLINE|CODE|''), + * elementName, sourceFile, id, name, resourceId, resourceUri, + * resourceContext, templateId. + * + * @param array $source + * @param string $cacheName Fenom template / store name + * @return string + */ + public static function label(array $source, $cacheName = '') + { + $origin = isset($source['origin']) ? strtoupper((string)$source['origin']) : ''; + $binding = isset($source['binding']) ? (string)$source['binding'] : ''; + $id = !empty($source['id']) ? (int)$source['id'] : 0; + $elementName = isset($source['elementName']) ? (string)$source['elementName'] : ''; + if ( + $elementName === '' + && !empty($source['name']) + && !self::looksLikeHash((string)$source['name']) + ) { + $elementName = (string)$source['name']; + } + $file = self::relativePath(isset($source['sourceFile']) ? (string)$source['sourceFile'] : ''); + $kind = self::elementKind($binding); + + if ($origin === 'FILE' && $file !== '') { + return 'file:' . $file; + } + if ($origin === 'INLINE' || $origin === 'CODE') { + return 'inline'; + } + if ($elementName !== '' && !self::looksLikeHash($elementName)) { + $label = $kind !== '' ? $kind . ':' . $elementName : $elementName; + if ($id > 0) { + $label .= ' (#' . $id . ')'; + } + if ($file !== '') { + $label .= ' file:' . $file; + } + + return $label; + } + if ($id > 0) { + return ($kind !== '' ? $kind : 'element') . ':#' . $id; + } + if ($file !== '') { + return 'file:' . $file; + } + + $resource = self::formatResource($source); + if ($resource !== '') { + return $resource; + } + + return $cacheName !== '' ? $cacheName : 'unknown'; + } + + /** + * Full MODX / &showLog error block. + * + * @param Throwable $e + * @param string $cacheName + * @param string $content + * @param string $label + * @param string $phase compile|runtime + * @param array $extra compiled, sourceDump (paths), resource (facts for a second line) + * @return string + */ + public static function format(Throwable $e, $cacheName, $content, $label, $phase, array $extra = []) + { + if ($label === '') { + $label = $cacheName !== '' ? $cacheName : 'unknown'; + } + $raw = self::replaceTemplateName($e->getMessage(), $cacheName, $label); + $line = self::extractLine($e); + $near = self::extractNear($e->getMessage()); + $excerpt = self::excerpt($content, $line); + $hint = self::modxHint($near . "\n" . $excerpt); + + $lines = [ + '[pdoTools][Fenom] ' . $phase . ' error in ' . $label, + ]; + if (!empty($extra['resource']) && is_array($extra['resource']) && strpos($label, 'resource:') !== 0) { + $resourceLine = self::formatResource($extra['resource']); + if ($resourceLine !== '') { + $lines[] = $resourceLine; + } + } + if ($cacheName !== '' && $cacheName !== $label) { + $lines[] = 'cache name: ' . $cacheName; + } + $lines[] = $raw; + if ($excerpt !== '') { + $lines[] = $excerpt; + } + if ($hint !== '') { + $lines[] = $hint; + } + if (!empty($extra['compiled'])) { + $compiled = self::relativePath((string)$extra['compiled']); + if ($compiled !== '') { + $lines[] = 'compiled: ' . $compiled; + } + } + if (!empty($extra['sourceDump'])) { + $dump = self::relativePath((string)$extra['sourceDump']); + if ($dump !== '') { + $lines[] = 'source dump: ' . $dump; + } + } + + return implode("\n", $lines); + } + + /** + * @param string $binding + * @return string + */ + private static function elementKind($binding) + { + switch (strtolower((string)$binding)) { + case 'modchunk': + return 'chunk'; + case 'modtemplate': + return 'template'; + case 'modsnippet': + return 'snippet'; + default: + return ''; + } + } + + /** + * @param array $source + * @return string + */ + private static function formatResource(array $source) + { + if (!array_key_exists('resourceId', $source)) { + return ''; + } + $id = (int)$source['resourceId']; + $ctx = isset($source['resourceContext']) ? (string)$source['resourceContext'] : ''; + $uri = isset($source['resourceUri']) ? (string)$source['resourceUri'] : ''; + $label = 'resource:#' . $id; + if ($ctx !== '' || $uri !== '') { + $label .= ' (' . $ctx . ':' . $uri . ')'; + } + if (!empty($source['templateId'])) { + $label .= ', template:#' . (int)$source['templateId']; + } + + return $label; + } + /** * @param string $value * @return bool */ - public static function looksLikeHash($value) + private static function looksLikeHash($value) { return is_string($value) && (bool)preg_match('/^[a-f0-9]{32}$/i', $value); } @@ -24,7 +179,7 @@ public static function looksLikeHash($value) * @param string $path * @return string */ - public static function relativePath($path) + private static function relativePath($path) { if (!is_string($path) || $path === '') { return ''; @@ -44,17 +199,11 @@ public static function relativePath($path) * @param Throwable $e * @return int */ - public static function extractLine(Throwable $e) + private static function extractLine(Throwable $e) { if (preg_match('/\bline\s+(\d+)/i', $e->getMessage(), $m)) { return (int)$m[1]; } - if ($e instanceof ErrorException) { - $file = str_replace('\\', '/', (string)$e->getFile()); - if ($file !== '' && substr($file, -4) !== '.php' && strpos($file, '/') === false) { - return (int)$e->getLine(); - } - } return 0; } @@ -63,7 +212,7 @@ public static function extractLine(Throwable $e) * @param string $message * @return string */ - public static function extractNear($message) + private static function extractNear($message) { if (!is_string($message) || !preg_match("/near '([^']*)'/s", $message, $m)) { return ''; @@ -76,7 +225,7 @@ public static function extractNear($message) * @param string $text * @return bool */ - public static function hasUnprocessedModx($text) + private static function hasUnprocessedModx($text) { return is_string($text) && (bool)preg_match('/\[\[(?:\+|\*|\$|%|~|#|&)?/', $text); } @@ -85,7 +234,7 @@ public static function hasUnprocessedModx($text) * @param string $text * @return string */ - public static function modxHint($text) + private static function modxHint($text) { if (!self::hasUnprocessedModx($text)) { return ''; @@ -103,7 +252,7 @@ public static function modxHint($text) * @param int $radius * @return string */ - public static function excerpt($content, $line, $radius = 2) + private static function excerpt($content, $line, $radius = 2) { if (!is_string($content) || $content === '' || $line < 1) { return ''; @@ -130,7 +279,7 @@ public static function excerpt($content, $line, $radius = 2) * @param string $label * @return string */ - public static function replaceTemplateName($message, $name, $label) + private static function replaceTemplateName($message, $name, $label) { if (!is_string($message) || $name === '' || $label === '' || $name === $label) { return $message; diff --git a/core/components/pdotools/src/Parsing/Fenom/Fenom.php b/core/components/pdotools/src/Parsing/Fenom/Fenom.php index 515eeae..b9bc895 100644 --- a/core/components/pdotools/src/Parsing/Fenom/Fenom.php +++ b/core/components/pdotools/src/Parsing/Fenom/Fenom.php @@ -103,12 +103,12 @@ public function process($chunk, array $properties = []) $name = md5($content); } /** @var \Fenom\Template $tpl */ - $label = $this->resolveSourceLabel($chunk, $name, $content); + $source = is_array($chunk) ? $chunk : []; if (!$tpl = $this->pdoTools->getStore($name, 'fenom')) { if (!empty($this->pdoTools->config('useFenomCache'))) { $compileKey = 'pdotools/' . $name; if (!$cache = $this->pdoTools->getExactCache($compileKey)) { - if ($tpl = $this->_compileChunk($content, $name, $label)) { + if ($tpl = $this->_compileChunk($content, $name, $source)) { $this->pdoTools->setExactCache($compileKey, $tpl->getTemplateCode()); } } else { @@ -116,7 +116,7 @@ public function process($chunk, array $properties = []) $tpl = eval($cache); } } else { - $tpl = $this->_compileChunk($content, $name, $label); + $tpl = $this->_compileChunk($content, $name, $source); } if ($tpl) { $this->pdoTools->setStore($name, $tpl, 'fenom'); @@ -135,7 +135,7 @@ public function process($chunk, array $properties = []) try { $content = $tpl->fetch($properties); } catch (Exception $e) { - $this->logFenomError($e, $name, $content, $label, 'runtime'); + $this->logFenomError($e, $name, $content, $source, 'runtime'); } } @@ -173,18 +173,15 @@ public function setCompileDir($dir) * * @param $content * @param string $name - * @param string $label + * @param array $source Element/resource facts for error labels * * @return \Fenom\Template */ - protected function _compileChunk($content, $name = '', $label = '') + protected function _compileChunk($content, $name = '', array $source = []) { if (empty($name)) { $name = md5($content); } - if ($label === '') { - $label = $name; - } try { $tpl = $this->getRawTemplate()->source($name, $content, true); $this->pdoTools->addTime('Compiled Fenom chunk with name "' . $name . '"'); @@ -192,7 +189,7 @@ protected function _compileChunk($content, $name = '', $label = '') if ($this->modx->getOption('pdotools_fenom_save_on_errors')) { $this->pdoTools->setExactCache('error/' . $name, $content); } - $this->logFenomError($e, $name, $content, $label, 'compile'); + $this->logFenomError($e, $name, $content, $source, 'compile'); $tpl = $this->getRawTemplate()->source($name, '', false); } @@ -200,176 +197,63 @@ protected function _compileChunk($content, $name = '', $label = '') } /** - * Human-readable source for logs. Cache keys still use $name. - * - * @param array|string $chunk + * @param Exception $e * @param string $name * @param string $content - * @return string + * @param array $source + * @param string $phase */ - protected function resolveSourceLabel($chunk, $name, $content) + protected function logFenomError(Exception $e, $name, $content, array $source, $phase) { - if (is_array($chunk) && !empty($chunk['sourceLabel'])) { - return (string)$chunk['sourceLabel']; - } - - $type = ''; - $id = 0; - $elementName = ''; - $sourceFile = ''; - if (is_array($chunk)) { - $type = !empty($chunk['binding']) ? (string)$chunk['binding'] : ''; - $id = !empty($chunk['id']) ? (int)$chunk['id'] : 0; - if (!empty($chunk['sourceFile'])) { - $sourceFile = ErrorLog::relativePath($chunk['sourceFile']); - } - if (!empty($chunk['elementName'])) { - $elementName = (string)$chunk['elementName']; - } elseif (!empty($chunk['name']) && !ErrorLog::looksLikeHash($chunk['name'])) { - $elementName = (string)$chunk['name']; - } - } - - $kind = $this->elementKind($type); - if ($sourceFile !== '' && ($elementName === '' || ErrorLog::looksLikeHash($elementName))) { - return trim($kind . ' file:' . $sourceFile); - } - if ($elementName !== '') { - $label = $kind !== '' ? $kind . ':' . $elementName : $elementName; - if ($id > 0) { - $label .= ' (#' . $id . ')'; - } - if ($sourceFile !== '') { - $label .= ' file:' . $sourceFile; - } - - return $label; - } - if ($id > 0) { - return ($kind !== '' ? $kind : 'element') . ':#' . $id; - } - - $page = $this->formatResourceContext(); - if ($page !== '') { - return $page; + $label = ErrorLog::label($source, $name); + $extra = [ + 'resource' => $this->currentResourceFacts(), + ]; + $compiled = $this->compiledPathIfExists($name); + if ($compiled !== '') { + $extra['compiled'] = $compiled; } - - return $name !== '' ? $name : md5($content); - } - - /** - * @param string $binding - * @return string - */ - protected function elementKind($binding) - { - switch ($binding) { - case 'modchunk': - case 'modChunk': - return 'chunk'; - case 'modtemplate': - case 'modTemplate': - return 'template'; - case 'modsnippet': - case 'modSnippet': - return 'snippet'; - default: - return ''; + if ($phase === 'compile' && $this->modx->getOption('pdotools_fenom_save_on_errors')) { + $extra['sourceDump'] = rtrim((string)$this->pdoTools->config('cachePath'), '/') . '/error/' . $name; } + $message = ErrorLog::format($e, $name, $content, $label, $phase, $extra); + $this->modx->log(modX::LOG_LEVEL_ERROR, $message); + $this->pdoTools->addTime($message); } /** - * @return string + * Current page facts for a secondary log line (no template id). + * + * @return array */ - protected function formatResourceContext() + protected function currentResourceFacts() { $resource = $this->modx->resource; if (!is_object($resource) || !method_exists($resource, 'get')) { - return ''; + return []; } $id = (int)$resource->get('id'); if ($id <= 0) { - return ''; - } - $ctx = ''; - if (is_object($this->modx->context) && method_exists($this->modx->context, 'get')) { - $ctx = (string)$this->modx->context->get('key'); + return []; } $uri = (string)$resource->get('uri'); if ($uri === '') { $uri = (string)$resource->get('alias'); } - $label = 'resource:#' . $id; - if ($ctx !== '' || $uri !== '') { - $label .= ' (' . $ctx . ':' . $uri . ')'; - } - - return $label; - } - - /** - * @param Exception $e - * @param string $name - * @param string $content - * @param string $label - * @param string $phase - */ - protected function logFenomError(Exception $e, $name, $content, $label, $phase) - { - $message = $this->formatFenomError($e, $name, $content, $label, $phase); - $this->modx->log(modX::LOG_LEVEL_ERROR, $message); - $this->pdoTools->addTime($message); - } - - /** - * @param Exception $e - * @param string $name - * @param string $content - * @param string $label - * @param string $phase - * @return string - */ - protected function formatFenomError(Exception $e, $name, $content, $label, $phase) - { - if ($label === '') { - $label = $name; - } - $raw = ErrorLog::replaceTemplateName($e->getMessage(), $name, $label); - $line = ErrorLog::extractLine($e); - $near = ErrorLog::extractNear($e->getMessage()); - $excerpt = ErrorLog::excerpt($content, $line); - $hint = ErrorLog::modxHint($near . "\n" . $excerpt); - - $lines = [ - '[pdoTools][Fenom] ' . $phase . ' error in ' . $label, + $facts = [ + 'resourceId' => $id, + 'resourceUri' => $uri, ]; - $context = $this->formatResourceContext(); - if ($context !== '' && strpos($label, 'resource:') !== 0) { - $lines[] = $context; - } - if ($name !== '' && $name !== $label) { - $lines[] = 'cache name: ' . $name; - } - $lines[] = $raw; - if ($excerpt !== '') { - $lines[] = $excerpt; - } - if ($hint !== '') { - $lines[] = $hint; - } - $compiled = $this->compiledPathIfExists($name); - if ($compiled !== '') { - $lines[] = 'compiled: ' . $compiled; - } - if ($phase === 'compile' && $this->modx->getOption('pdotools_fenom_save_on_errors')) { - $errorPath = rtrim((string)$this->pdoTools->config('cachePath'), '/'); - $lines[] = 'source dump: ' . ErrorLog::relativePath($errorPath . '/error/' . $name); + if (is_object($this->modx->context) && method_exists($this->modx->context, 'get')) { + $facts['resourceContext'] = (string)$this->modx->context->get('key'); } - return implode("\n", $lines); + return $facts; } /** + * Absolute path to a compiled template file, if it exists. + * * @param string $name * @return string */ @@ -383,7 +267,7 @@ protected function compiledPathIfExists($name) return ''; } - return ErrorLog::relativePath($file); + return $file; } diff --git a/core/components/pdotools/src/Parsing/Parser.php b/core/components/pdotools/src/Parsing/Parser.php index ee953bd..d2346a3 100644 --- a/core/components/pdotools/src/Parsing/Parser.php +++ b/core/components/pdotools/src/Parsing/Parser.php @@ -66,10 +66,10 @@ public function processElementTags( } $_processingUncacheable = $this->_processingUncacheable; $this->_processingUncacheable = true; - $content = $this->pdoTools->getFenom()->process([ - 'content' => $content, - 'sourceLabel' => $this->fenomSourceLabel(), - ], $this->modx->placeholders); + $content = $this->pdoTools->getFenom()->process( + $this->fenomSourcePayload($content), + $this->modx->placeholders + ); $this->_processingUncacheable = $_processingUncacheable; } @@ -283,35 +283,30 @@ public function processTag($tag, $processUncacheable = true) } /** - * Label for Fenom page-parser errors. Does not become a cache key. + * Facts for Fenom page-parser error labels. Not a cache key. * - * @return string + * @param string $content + * @return array */ - protected function fenomSourceLabel() + protected function fenomSourcePayload($content) { + $payload = ['content' => $content]; $resource = $this->modx->resource; if (!is_object($resource) || !method_exists($resource, 'get')) { - return 'resource'; - } - $id = (int)$resource->get('id'); - $ctx = ''; - if (is_object($this->modx->context) && method_exists($this->modx->context, 'get')) { - $ctx = (string)$this->modx->context->get('key'); + return $payload; } + $payload['resourceId'] = (int)$resource->get('id'); $uri = (string)$resource->get('uri'); if ($uri === '') { $uri = (string)$resource->get('alias'); } - $label = 'resource:#' . $id; - if ($ctx !== '' || $uri !== '') { - $label .= ' (' . $ctx . ':' . $uri . ')'; - } - $tplId = (int)$resource->get('template'); - if ($tplId > 0) { - $label .= ', template:#' . $tplId; + $payload['resourceUri'] = $uri; + $payload['templateId'] = (int)$resource->get('template'); + if (is_object($this->modx->context) && method_exists($this->modx->context, 'get')) { + $payload['resourceContext'] = (string)$this->modx->context->get('key'); } - return $label; + return $payload; } } diff --git a/core/components/pdotools/tests/Support/CoreToolsHarness.php b/core/components/pdotools/tests/Support/CoreToolsHarness.php index c6660fe..ef5393f 100644 --- a/core/components/pdotools/tests/Support/CoreToolsHarness.php +++ b/core/components/pdotools/tests/Support/CoreToolsHarness.php @@ -25,16 +25,4 @@ public function publicGetCacheKey($options = []) { return $this->getCacheKey($options); } - - /** - * @param string $type - * @param string $binding - * @param object $element - * @param string $elementName - * @param string $sourceFile - */ - public function publicBuildElementSourceLabel($type, $binding, $element, $elementName, $sourceFile): string - { - return $this->buildElementSourceLabel($type, $binding, $element, $elementName, $sourceFile); - } } diff --git a/core/components/pdotools/tests/Support/FenomHarness.php b/core/components/pdotools/tests/Support/FenomHarness.php deleted file mode 100644 index d4674ac..0000000 --- a/core/components/pdotools/tests/Support/FenomHarness.php +++ /dev/null @@ -1,29 +0,0 @@ -|string $chunk - */ - public function publicResolveSourceLabel($chunk, string $name, string $content): string - { - return $this->resolveSourceLabel($chunk, $name, $content); - } - - public function publicFormatFenomError( - Exception $e, - string $name, - string $content, - string $label, - string $phase - ): string { - return $this->formatFenomError($e, $name, $content, $label, $phase); - } -} diff --git a/core/components/pdotools/tests/Unit/CoreTools/SourceLabelTest.php b/core/components/pdotools/tests/Unit/CoreTools/SourceLabelTest.php deleted file mode 100644 index 85ab9fb..0000000 --- a/core/components/pdotools/tests/Unit/CoreTools/SourceLabelTest.php +++ /dev/null @@ -1,60 +0,0 @@ -modx); - $this->assertSame( - 'chunk:tpl.product.row (#12)', - $tools->publicBuildElementSourceLabel('modChunk', '', $this->element(12), 'tpl.product.row', '') - ); - } - - public function testFileBindingUsesCoreRelativePath(): void - { - $tools = new CoreToolsHarness($this->modx); - $file = rtrim(str_replace('\\', '/', MODX_CORE_PATH), '/') . '/elements/chunks/item.tpl'; - $this->assertSame( - 'file:core/elements/chunks/item.tpl', - $tools->publicBuildElementSourceLabel('modChunk', 'FILE', $this->element(0), '', $file) - ); - } - - public function testInlineBinding(): void - { - $tools = new CoreToolsHarness($this->modx); - $this->assertSame( - 'inline', - $tools->publicBuildElementSourceLabel('modChunk', 'INLINE', $this->element(0), '', '') - ); - } - - /** - * @return object - */ - private function element(int $id) - { - return new class ($id) { - /** @var int */ - private $id; - - public function __construct(int $id) - { - $this->id = $id; - } - - public function get($key) - { - return $key === 'id' ? $this->id : null; - } - }; - } -} diff --git a/core/components/pdotools/tests/Unit/Parsing/ErrorLogTest.php b/core/components/pdotools/tests/Unit/Parsing/ErrorLogTest.php index 4028288..75495c3 100644 --- a/core/components/pdotools/tests/Unit/Parsing/ErrorLogTest.php +++ b/core/components/pdotools/tests/Unit/Parsing/ErrorLogTest.php @@ -4,96 +4,138 @@ namespace ModxPro\PdoTools\Tests\Unit\Parsing; -use ErrorException; use Exception; use ModxPro\PdoTools\Parsing\Fenom\ErrorLog; use PHPUnit\Framework\TestCase; class ErrorLogTest extends TestCase { - public function testLooksLikeHash(): void + public function testLabelForNamedChunk(): void { - $this->assertTrue(ErrorLog::looksLikeHash(md5('chunk'))); - $this->assertFalse(ErrorLog::looksLikeHash('my-chunk')); - $this->assertFalse(ErrorLog::looksLikeHash('modchunk/12')); - } - - public function testExcerptMarksTheLine(): void - { - $content = "one\ntwo\n{var \$x = [[+limit]]}\nfour"; - $excerpt = ErrorLog::excerpt($content, 3, 1); - - $this->assertStringContainsString('> 3:', $excerpt); - $this->assertStringContainsString('[[+limit]]', $excerpt); - $this->assertStringContainsString(' 2: two', $excerpt); - } - - public function testExcerptRejectsMissingLine(): void - { - $this->assertSame('', ErrorLog::excerpt("one\ntwo", 9)); - $this->assertSame('', ErrorLog::excerpt('', 1)); - $this->assertSame('', ErrorLog::excerpt('one', 0)); + $this->assertSame( + 'chunk:tpl.product.row (#12)', + ErrorLog::label([ + 'binding' => 'modchunk', + 'id' => 12, + 'elementName' => 'tpl.product.row', + ], 'modchunk/12') + ); } - public function testModxHintNamesThePlaceholder(): void + public function testLabelForFileAndInlineOrigins(): void { - $hint = ErrorLog::modxHint('{var $limit = [[+limit]]}'); - $this->assertStringContainsString('{$limit}', $hint); + $file = rtrim(str_replace('\\', '/', MODX_CORE_PATH), '/') . '/elements/chunks/item.tpl'; + $this->assertSame( + 'file:core/elements/chunks/item.tpl', + ErrorLog::label([ + 'binding' => 'modchunk', + 'origin' => 'FILE', + 'sourceFile' => $file, + ], 'modchunk/' . md5('file')) + ); + $this->assertSame( + 'inline', + ErrorLog::label(['binding' => 'modchunk', 'origin' => 'INLINE'], 'inline') + ); } - public function testModxHintGenericWhenTagHasNoName(): void + public function testLabelFallsBackToFileWithoutOrigin(): void { - $hint = ErrorLog::modxHint('[[$other]]'); - $this->assertStringContainsString('{$placeholder}', $hint); + $file = rtrim(str_replace('\\', '/', MODX_CORE_PATH), '/') . '/elements/chunks/item.tpl'; + $this->assertSame( + 'file:core/elements/chunks/item.tpl', + ErrorLog::label([ + 'binding' => 'modchunk', + 'sourceFile' => $file, + ], 'modchunk/' . md5('file')) + ); } - public function testHasUnprocessedModx(): void + public function testLabelForResourceWithTemplate(): void { - $this->assertTrue(ErrorLog::hasUnprocessedModx('[[+limit]]')); - $this->assertTrue(ErrorLog::hasUnprocessedModx('[[*pagetitle]]')); - $this->assertFalse(ErrorLog::hasUnprocessedModx('{$limit}')); + $this->assertSame( + 'resource:#42 (web:catalog/item), template:#5', + ErrorLog::label([ + 'resourceId' => 42, + 'resourceContext' => 'web', + 'resourceUri' => 'catalog/item', + 'templateId' => 5, + ], md5('page')) + ); } - public function testReplaceTemplateNameKeepsHashOutOfTheMessage(): void + public function testFormatReplacesHashAndAddsHint(): void { $hash = 'ee058690d9fd7413748b95b0960e006b'; - $message = "Unexpected token '+' in expression in {$hash} line 6, near '{var \$limit = [[+' <- there"; - $replaced = ErrorLog::replaceTemplateName($message, $hash, 'chunk:tpl.product.row (#12)'); + $content = "one\ntwo\nthree\nfour\nfive\n{var \$limit = [[+limit]]}\nseven"; + $e = new Exception( + "Unexpected token '+' in expression in {$hash} line 6, near '{var \$limit = [[+' <- there" + ); + $message = ErrorLog::format( + $e, + $hash, + $content, + 'chunk:tpl.product.row (#12)', + 'compile' + ); - $this->assertStringContainsString('chunk:tpl.product.row (#12)', $replaced); - $this->assertStringNotContainsString($hash, $replaced); + $this->assertStringContainsString('[pdoTools][Fenom] compile error in chunk:tpl.product.row (#12)', $message); + $this->assertStringContainsString('cache name: ' . $hash, $message); + $this->assertStringContainsString('chunk:tpl.product.row (#12) line 6', $message); + $this->assertStringNotContainsString(' in ' . $hash . ' ', $message); + $this->assertStringContainsString('> 6:', $message); + $this->assertStringContainsString('{$limit}', $message); } - public function testReplaceTemplateNameLeavesMessageWhenLabelMatchesName(): void + public function testFormatAddsResourceLineAndSourceDump(): void { - $message = 'error in inline line 1'; - $this->assertSame($message, ErrorLog::replaceTemplateName($message, 'inline', 'inline')); - } + $hash = md5('broken'); + $e = new Exception("Unexpected token '+' in expression in {$hash} line 1, near '{var'"); + $dump = rtrim(str_replace('\\', '/', MODX_CORE_PATH), '/') . '/cache/pdotools/error/' . $hash; + $message = ErrorLog::format( + $e, + $hash, + '{var $limit = [[+limit]]}', + 'inline', + 'compile', + [ + 'resource' => [ + 'resourceId' => 42, + 'resourceContext' => 'web', + 'resourceUri' => 'catalog/item', + ], + 'sourceDump' => $dump, + ] + ); - public function testExtractLineFromMessage(): void - { - $e = new Exception("Unexpected token '+' in expression in chunk:row line 6, near '{var'"); - $this->assertSame(6, ErrorLog::extractLine($e)); + $this->assertStringContainsString('resource:#42 (web:catalog/item)', $message); + $this->assertStringContainsString('source dump: core/cache/pdotools/error/' . $hash, $message); + $this->assertStringContainsString('{$limit}', $message); } - public function testExtractNear(): void + public function testFormatSkipsResourceLineWhenLabelIsResource(): void { - $this->assertSame( - '{var $limit = [[+', - ErrorLog::extractNear("near '{var \$limit = [[+' <- there") + $e = new Exception('syntax error near token'); + $message = ErrorLog::format( + $e, + md5('x'), + '{var $x = 1}', + 'resource:#1 (web:home), template:#2', + 'compile', + [ + 'resource' => [ + 'resourceId' => 1, + 'resourceContext' => 'web', + 'resourceUri' => 'home', + ], + ] ); - $this->assertSame('', ErrorLog::extractNear('no near clause')); - } - public function testRelativePathUsesCorePrefix(): void - { - $path = rtrim(str_replace('\\', '/', MODX_CORE_PATH), '/') . '/cache/pdotools/error/foo'; - $this->assertSame('core/cache/pdotools/error/foo', ErrorLog::relativePath($path)); - } - - public function testExtractLineFromErrorExceptionWithoutPhpFile(): void - { - $e = new ErrorException('boom', 0, E_ERROR, 'inline-template', 4); - $this->assertSame(4, ErrorLog::extractLine($e)); + $lines = explode("\n", $message); + $resourceLines = array_filter($lines, static function ($line) { + return strpos($line, 'resource:#') === 0; + }); + $this->assertCount(0, $resourceLines); + $this->assertStringContainsString('compile error in resource:#1 (web:home), template:#2', $message); } } diff --git a/core/components/pdotools/tests/Unit/Parsing/FenomErrorLogTest.php b/core/components/pdotools/tests/Unit/Parsing/FenomErrorLogTest.php index a01f77d..81ae1d8 100644 --- a/core/components/pdotools/tests/Unit/Parsing/FenomErrorLogTest.php +++ b/core/components/pdotools/tests/Unit/Parsing/FenomErrorLogTest.php @@ -4,93 +4,19 @@ namespace ModxPro\PdoTools\Tests\Unit\Parsing; -use Exception; -use ModxPro\PdoTools\Tests\Support\FenomHarness; use ModxPro\PdoTools\Tests\TestCase; class FenomErrorLogTest extends TestCase { - public function testSourceLabelPrefersExplicitField(): void - { - $fenom = $this->harness(); - $this->assertSame( - 'chunk:tpl.product.row (#12)', - $fenom->publicResolveSourceLabel( - ['sourceLabel' => 'chunk:tpl.product.row (#12)', 'name' => md5('x')], - md5('x'), - '{var $x = 1}' - ) - ); - } - - public function testSourceLabelFromChunkBinding(): void - { - $fenom = $this->harness(); - $this->assertSame( - 'chunk:tpl.product.row (#12)', - $fenom->publicResolveSourceLabel( - [ - 'binding' => 'modchunk', - 'id' => 12, - 'name' => 'tpl.product.row', - 'elementName' => 'tpl.product.row', - ], - 'modchunk/12', - '{var $x = 1}' - ) - ); - } - - public function testSourceLabelForFileAndInline(): void - { - $fenom = $this->harness(); - $file = rtrim(str_replace('\\', '/', MODX_CORE_PATH), '/') . '/elements/chunks/item.tpl'; - $this->assertSame( - 'chunk file:core/elements/chunks/item.tpl', - $fenom->publicResolveSourceLabel( - ['binding' => 'modchunk', 'sourceFile' => $file], - 'modchunk/' . md5('file'), - '{var $x = 1}' - ) - ); - $this->assertSame( - 'inline', - $fenom->publicResolveSourceLabel(['sourceLabel' => 'inline'], 'inline', '{var $x = 1}') - ); - } - - public function testFormatReplacesHashAndAddsHint(): void - { - $hash = 'ee058690d9fd7413748b95b0960e006b'; - $content = "one\ntwo\nthree\nfour\nfive\n{var \$limit = [[+limit]]}\nseven"; - $e = new Exception( - "Unexpected token '+' in expression in {$hash} line 6, near '{var \$limit = [[+' <- there" - ); - $message = $this->harness()->publicFormatFenomError( - $e, - $hash, - $content, - 'chunk:tpl.product.row (#12)', - 'compile' - ); - - $this->assertStringContainsString('[pdoTools][Fenom] compile error in chunk:tpl.product.row (#12)', $message); - $this->assertStringContainsString('cache name: ' . $hash, $message); - $this->assertStringContainsString('chunk:tpl.product.row (#12) line 6', $message); - $this->assertStringNotContainsString(' in ' . $hash . ' ', $message); - $this->assertStringContainsString('> 6:', $message); - $this->assertStringContainsString('{$limit}', $message); - } - public function testProcessLogsCompileErrorWithoutChangingCacheName(): void { $chunk = [ 'content' => '{var $limit = [[+limit]]}', 'binding' => 'modchunk', + 'origin' => '', 'id' => 12, 'name' => 'tpl.product.row', 'elementName' => 'tpl.product.row', - 'sourceLabel' => 'chunk:tpl.product.row (#12)', ]; $this->fenom()->process($chunk); @@ -102,26 +28,36 @@ public function testProcessLogsCompileErrorWithoutChangingCacheName(): void $this->assertNotNull($this->pdoTools->getStore('modchunk/12', 'fenom')); } - public function testSaveOnErrorsListsSourceDump(): void + public function testProcessFileOriginUsesCanonicalFileLabel(): void { - $this->modx->config['pdotools_fenom_save_on_errors'] = true; - $this->modx->cacheManager = $this->modx->getCacheManager(); - $hash = md5('broken'); - $e = new Exception("Unexpected token '+' in expression in {$hash} line 1, near '{var'"); - $message = $this->harness()->publicFormatFenomError( - $e, - $hash, - '{var $limit = [[+limit]]}', - 'inline', - 'compile' - ); + $file = rtrim(str_replace('\\', '/', MODX_CORE_PATH), '/') . '/elements/chunks/item.tpl'; + $this->fenom()->process([ + 'content' => '{var $limit = [[+limit]]}', + 'binding' => 'modchunk', + 'origin' => 'FILE', + 'sourceFile' => $file, + 'name' => md5('@FILE item'), + ]); - $this->assertStringContainsString('source dump:', $message); - $this->assertStringContainsString('error/' . $hash, $message); + $logged = (string)$this->modx->logs[0]['message']; + $this->assertStringContainsString('compile error in file:core/elements/chunks/item.tpl', $logged); + $this->assertStringNotContainsString('chunk file:', $logged); } - private function harness(): FenomHarness + public function testSaveOnErrorsListsSourceDump(): void { - return new FenomHarness($this->modx, $this->pdoTools); + $this->modx->config['pdotools_fenom_save_on_errors'] = true; + $this->modx->cacheManager = $this->modx->getCacheManager(); + $this->fenom()->process([ + 'content' => '{var $limit = [[+limit]]}', + 'binding' => 'modchunk', + 'origin' => 'INLINE', + 'name' => 'inline-broken', + ]); + + $logged = (string)$this->modx->logs[0]['message']; + $this->assertStringContainsString('compile error in inline', $logged); + $this->assertStringContainsString('source dump:', $logged); + $this->assertStringContainsString('error/', $logged); } }