From f4c844a19e2374dfc7020df77d0db2415e492543 Mon Sep 17 00:00:00 2001 From: Imanuel Bertrand Date: Fri, 11 Sep 2026 10:38:44 +0000 Subject: [PATCH 1/2] Stop ThemeConfig::normalizeValue() casting arrays to string Saving Content > Design > Configuration posts the whole design config form, and the imageUploader fields in it (head_shortcut_icon, header_logo_src, email_logo, the watermark images) post a list of file descriptors, not a scalar -- e.g. [['name' => 'logo.png', 'url' => '...', 'size' => 1234]]. normalizeValue() cast whatever it got with (string), so those fields raised "Array to string conversion". With swissup/module-ignition installed that warning is promoted to an ErrorException, SaveAfter aborts and AbstractActivityObserver swallows it -- the design config is saved but no activity is logged for the change. Flatten arrays instead, the way the sibling Activity\SystemConfig::flattenValue() already does for system config values, and prefer the descriptor's file name so the logged value still compares against what core_config_data holds. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01R1c3CaPXcg4sxvfv9icv69 --- Model/Activity/ThemeConfig.php | 38 +++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/Model/Activity/ThemeConfig.php b/Model/Activity/ThemeConfig.php index 2536987..e5c144a 100644 --- a/Model/Activity/ThemeConfig.php +++ b/Model/Activity/ThemeConfig.php @@ -119,7 +119,7 @@ public function collectAdditionalData(array $oldData, array $newData, array $fie } /** - * Normalize a scalar value to a string for comparison + * Normalize a value to a string for comparison * * @param mixed $value Raw value * @return string Normalized string @@ -130,6 +130,42 @@ private function normalizeValue(mixed $value): string return ''; } + if (is_array($value)) { + return $this->flattenValue($value); + } + return (string)$value; } + + /** + * Flatten an array posted by the design config form to a comparable string + * + * Image uploader fields (favicon, logos) post a list of file descriptors + * rather than a scalar; only the file name of those ends up in + * core_config_data, so compare on that. + * + * @param array $value Raw value + * @return string Normalized string + */ + private function flattenValue(array $value): string + { + if (!array_is_list($value)) { + return (string)json_encode($value, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); + } + + $parts = []; + foreach ($value as $item) { + if (!is_array($item)) { + $parts[] = (string)$item; + continue; + } + + $file = $item['file'] ?? $item['name'] ?? null; + $parts[] = $file === null + ? (string)json_encode($item, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE) + : (string)$file; + } + + return implode(',', $parts); + } } From 3911e465fa705257ade81a51d09f02bbf02940e8 Mon Sep 17 00:00:00 2001 From: Imanuel Bertrand Date: Fri, 11 Sep 2026 10:44:42 +0000 Subject: [PATCH 2/2] Cover and harden the theme config array flattening Follow-up to f4c844a, which left three gaps: Descriptor values were still cast unguarded, so a hand-crafted POST nesting one level deeper (head_shortcut_icon[0][file][]=x) re-raised the very "Array to string conversion" that commit set out to remove. Guard the cast with is_scalar() and fall back to encoding the whole descriptor. json_encode() returns false on malformed UTF-8, and (string)false is '', so a real change could compare equal to an empty old value and never be logged. Encode through a single encodeValue() that falls back to '[unserializable]', the placeholder FieldTracker::truncateValue() already uses, and apply the same fallback to the sibling SystemConfig::flattenValue(). Add the unit tests the branch had none of: the regression itself (re-posting an untouched image descriptor is not a change), the name fallback, replaced and cleared images, scalar lists, associative arrays, and both hardening cases above. The nested-array test asserts through a scoped error handler that no PHP warning fires, rather than relying on convertWarningsToExceptions, which PHPUnit 10 dropped. Rename ThemeConfig::flattenValue() to flattenPostedValue(): it shares a name but not semantics with SystemConfig::flattenValue(), and unifying the two would break the comparison against core_config_data. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01ByaeLVN4rdcm6PbfzVBUJK --- Model/Activity/SystemConfig.php | 9 +- Model/Activity/ThemeConfig.php | 39 +++-- Test/Unit/Model/Activity/SystemConfigTest.php | 20 +++ Test/Unit/Model/Activity/ThemeConfigTest.php | 139 ++++++++++++++++++ 4 files changed, 197 insertions(+), 10 deletions(-) diff --git a/Model/Activity/SystemConfig.php b/Model/Activity/SystemConfig.php index f5b81f1..3e33c92 100644 --- a/Model/Activity/SystemConfig.php +++ b/Model/Activity/SystemConfig.php @@ -28,6 +28,11 @@ class SystemConfig implements ModelInterface { public const MODULE_SYSTEM_CONFIGURATION = 'system_configuration'; + /** + * Placeholder for values that cannot be JSON encoded + */ + private const UNSERIALIZABLE_VALUE = '[unserializable]'; + public function __construct( protected readonly DataObject $dataObject, protected readonly ValueFactory $valueFactory, @@ -155,6 +160,8 @@ private function flattenValue(mixed $value): string return implode(',', $value); } - return (string)json_encode($value, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); + $encoded = json_encode($value, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); + + return $encoded === false ? self::UNSERIALIZABLE_VALUE : $encoded; } } diff --git a/Model/Activity/ThemeConfig.php b/Model/Activity/ThemeConfig.php index e5c144a..d3cb7d0 100644 --- a/Model/Activity/ThemeConfig.php +++ b/Model/Activity/ThemeConfig.php @@ -25,6 +25,11 @@ */ class ThemeConfig implements ModelInterface { + /** + * Placeholder for values that cannot be JSON encoded + */ + private const UNSERIALIZABLE_VALUE = '[unserializable]'; + public function __construct( protected readonly DataObject $dataObject, protected readonly ConfigCollectionFactory $configCollectionFactory, @@ -131,7 +136,7 @@ private function normalizeValue(mixed $value): string } if (is_array($value)) { - return $this->flattenValue($value); + return $this->flattenPostedValue($value); } return (string)$value; @@ -141,31 +146,47 @@ private function normalizeValue(mixed $value): string * Flatten an array posted by the design config form to a comparable string * * Image uploader fields (favicon, logos) post a list of file descriptors - * rather than a scalar; only the file name of those ends up in - * core_config_data, so compare on that. + * rather than a scalar; Theme\Model\Design\Backend\File::afterLoad() puts the + * stored config value in the descriptor's "file" key and its basename in + * "name", so prefer "file" to compare against core_config_data. + * + * Deliberately not the same as the sibling SystemConfig::flattenValue(): + * that one sees backend model values rather than posted form data and has + * no file descriptors to unwrap. * * @param array $value Raw value * @return string Normalized string */ - private function flattenValue(array $value): string + private function flattenPostedValue(array $value): string { if (!array_is_list($value)) { - return (string)json_encode($value, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); + return $this->encodeValue($value); } $parts = []; foreach ($value as $item) { if (!is_array($item)) { - $parts[] = (string)$item; + $parts[] = is_scalar($item) ? (string)$item : $this->encodeValue($item); continue; } $file = $item['file'] ?? $item['name'] ?? null; - $parts[] = $file === null - ? (string)json_encode($item, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE) - : (string)$file; + $parts[] = is_scalar($file) ? (string)$file : $this->encodeValue($item); } return implode(',', $parts); } + + /** + * Encode a value that has no meaningful scalar representation + * + * @param mixed $value Raw value + * @return string Encoded value, or a placeholder if it cannot be encoded + */ + private function encodeValue(mixed $value): string + { + $encoded = json_encode($value, JSON_UNESCAPED_SLASHES | JSON_UNESCAPED_UNICODE); + + return $encoded === false ? self::UNSERIALIZABLE_VALUE : $encoded; + } } diff --git a/Test/Unit/Model/Activity/SystemConfigTest.php b/Test/Unit/Model/Activity/SystemConfigTest.php index 5a000bf..04e9b7e 100644 --- a/Test/Unit/Model/Activity/SystemConfigTest.php +++ b/Test/Unit/Model/Activity/SystemConfigTest.php @@ -215,4 +215,24 @@ public function testGetEditDataFlattensNestedArrayToJson(): void $this->assertArrayHasKey('general/country/allow', $result); $this->assertSame('{"key":"value"}', $result['general/country/allow']['new_value']); } + + public function testGetEditDataUsesPlaceholderForUnserializableValue(): void + { + $model = new DataObject([ + 'path' => 'general/country/allow', + 'value' => ['broken' => "\xB1\x31"], + ]); + $model->setOrigData([ + 'country' => [ + 'fields' => [ + 'allow' => ['value' => 'old'], + ], + ], + ]); + + $result = $this->systemConfig->getEditData($model, []); + + $this->assertArrayHasKey('general/country/allow', $result); + $this->assertSame('[unserializable]', $result['general/country/allow']['new_value']); + } } diff --git a/Test/Unit/Model/Activity/ThemeConfigTest.php b/Test/Unit/Model/Activity/ThemeConfigTest.php index 684c694..12ffc8c 100644 --- a/Test/Unit/Model/Activity/ThemeConfigTest.php +++ b/Test/Unit/Model/Activity/ThemeConfigTest.php @@ -124,4 +124,143 @@ public function testCollectAdditionalDataIgnoresBothEmpty(): void $this->assertEmpty($result); } + + // --- collectAdditionalData: image uploader fields posting file descriptors --- + + public function testCollectAdditionalDataIgnoresUntouchedImageUploaderField(): void + { + $oldData = ['header_logo_src' => 'stores/1/logo.png']; + $newData = ['header_logo_src' => [ + [ + 'url' => 'https://example.com/media/logo/stores/1/logo.png', + 'file' => 'stores/1/logo.png', + 'name' => 'logo.png', + 'size' => 1234, + ] + ]]; + + $result = $this->themeConfig->collectAdditionalData($oldData, $newData, []); + + $this->assertEmpty($result, 'Re-posting an unchanged image descriptor is not a change'); + } + + public function testCollectAdditionalDataFallsBackToDescriptorNameWhenFileMissing(): void + { + $oldData = ['head_shortcut_icon' => 'favicon.png']; + $newData = ['head_shortcut_icon' => [ + ['name' => 'favicon.png', 'url' => 'https://example.com/media/favicon.png'] + ]]; + + $result = $this->themeConfig->collectAdditionalData($oldData, $newData, []); + + $this->assertEmpty($result); + } + + public function testCollectAdditionalDataDetectsReplacedImage(): void + { + $oldData = ['header_logo_src' => 'old-logo.png']; + $newData = ['header_logo_src' => [ + ['file' => 'new-logo.png', 'name' => 'new-logo.png', 'size' => 99] + ]]; + + $result = $this->themeConfig->collectAdditionalData($oldData, $newData, []); + + $this->assertArrayHasKey('design/header/logo_src', $result); + $this->assertSame('old-logo.png', $result['design/header/logo_src']['old_value']); + $this->assertSame('new-logo.png', $result['design/header/logo_src']['new_value']); + } + + public function testCollectAdditionalDataDetectsClearedImage(): void + { + $oldData = ['header_logo_src' => 'logo.png']; + $newData = ['header_logo_src' => []]; + + $result = $this->themeConfig->collectAdditionalData($oldData, $newData, []); + + $this->assertArrayHasKey('design/header/logo_src', $result); + $this->assertSame('logo.png', $result['design/header/logo_src']['old_value']); + $this->assertSame('', $result['design/header/logo_src']['new_value']); + } + + public function testCollectAdditionalDataJoinsListOfScalars(): void + { + $oldData = ['watermark_image_size' => 'a,b']; + $newData = ['watermark_image_size' => ['a', 'b']]; + + $result = $this->themeConfig->collectAdditionalData($oldData, $newData, []); + + $this->assertEmpty($result); + } + + public function testCollectAdditionalDataEncodesAssociativeArray(): void + { + $oldData = ['header_logo_src' => 'logo.png']; + $newData = ['header_logo_src' => ['delete' => '1', 'value' => 'logo.png']]; + + $result = $this->themeConfig->collectAdditionalData($oldData, $newData, []); + + $this->assertArrayHasKey('design/header/logo_src', $result); + $this->assertSame( + '{"delete":"1","value":"logo.png"}', + $result['design/header/logo_src']['new_value'] + ); + } + + /** + * A hand-crafted POST can nest arrays arbitrarily deep. The cast must not + * raise "Array to string conversion": with swissup/module-ignition + * installed that warning becomes an ErrorException, SaveAfter aborts and + * the activity is never logged. + */ + public function testCollectAdditionalDataHandlesNestedArrayInDescriptor(): void + { + $oldData = ['header_logo_src' => 'logo.png']; + $newData = ['header_logo_src' => [['file' => ['nested'], 'name' => ['nested']]]]; + + $warnings = []; + set_error_handler( + static function (int $errno, string $errstr) use (&$warnings): bool { + $warnings[] = $errstr; + return true; + }, + E_WARNING | E_NOTICE + ); + + try { + $result = $this->themeConfig->collectAdditionalData($oldData, $newData, []); + } finally { + restore_error_handler(); + } + + $this->assertSame([], $warnings, 'Flattening must not raise a PHP warning'); + $this->assertArrayHasKey('design/header/logo_src', $result); + $this->assertSame( + '{"file":["nested"],"name":["nested"]}', + $result['design/header/logo_src']['new_value'] + ); + } + + public function testCollectAdditionalDataUsesPlaceholderForUnserializableValue(): void + { + $oldData = ['header_logo_src' => 'logo.png']; + $newData = ['header_logo_src' => ['broken' => "\xB1\x31"]]; + + $result = $this->themeConfig->collectAdditionalData($oldData, $newData, []); + + $this->assertArrayHasKey('design/header/logo_src', $result); + $this->assertSame('[unserializable]', $result['design/header/logo_src']['new_value']); + } + + public function testCollectAdditionalDataDoesNotEscapeSlashesInEncodedValue(): void + { + $oldData = []; + $newData = ['header_logo_src' => ['path' => 'stores/1/logo.png']]; + + $result = $this->themeConfig->collectAdditionalData($oldData, $newData, []); + + $this->assertSame( + '{"path":"stores/1/logo.png"}', + $result['design/header/logo_src']['new_value'] + ); + } }