From 60935e4c4fc2be74b9e10d4c147e28bc8a799b5e Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Sun, 30 Aug 2026 17:28:58 +0600 Subject: [PATCH 1/2] fix(extra-fields): host extras only on own-table models (#645) Reject msProduct/msCategory (modResource STI), resolve unprefixed Phinx tables from mysql metaMap, and validate field keys as SQL identifiers. --- .../minishop3/lexicon/en/default.inc.php | 2 + .../minishop3/lexicon/en/vue.inc.php | 1 + .../minishop3/lexicon/ru/default.inc.php | 2 + .../minishop3/lexicon/ru/vue.inc.php | 1 + .../src/Services/ExtraFieldsService.php | 69 +++++++++---- .../src/Services/MigrationGenerator.php | 77 +++++++++------ .../MigrationGeneratorTableResolveTest.php | 96 +++++++++++++++++++ .../Services/ExtraFieldsValidationTest.php | 63 ++++++++++++ .../src/components/ExtraFieldsManager.vue | 22 ++++- 9 files changed, 280 insertions(+), 53 deletions(-) create mode 100644 core/components/minishop3/tests/MigrationGeneratorTableResolveTest.php create mode 100644 core/components/minishop3/tests/Unit/Services/ExtraFieldsValidationTest.php diff --git a/core/components/minishop3/lexicon/en/default.inc.php b/core/components/minishop3/lexicon/en/default.inc.php index 163378ff0..f94517f40 100644 --- a/core/components/minishop3/lexicon/en/default.inc.php +++ b/core/components/minishop3/lexicon/en/default.inc.php @@ -169,6 +169,8 @@ $_lang['ms3_err_ns'] = 'This field is required'; $_lang['ms3_err_product_key_required'] = 'Product key is required'; $_lang['ms3_err_field_key_required'] = 'Field key is required'; +$_lang['ms3_err_extra_field_class_unsupported'] = 'This model class cannot host extra fields. Use a model with its own database table (e.g. msProductData).'; +$_lang['ms3_err_extra_field_key_invalid'] = 'Field key must contain only Latin letters, digits, and underscores.'; $_lang['ms3_err_fields_required'] = 'Fields array is required'; $_lang['ms3_err_field_nf'] = 'Field not found'; $_lang['ms3_err_ae'] = 'This field must be unique'; diff --git a/core/components/minishop3/lexicon/en/vue.inc.php b/core/components/minishop3/lexicon/en/vue.inc.php index 3e59cca7d..d93550177 100644 --- a/core/components/minishop3/lexicon/en/vue.inc.php +++ b/core/components/minishop3/lexicon/en/vue.inc.php @@ -297,6 +297,7 @@ $_lang['ms3_vue_error_loading_fields'] = 'Failed to load fields list'; $_lang['ms3_vue_validation'] = 'Validation'; $_lang['ms3_vue_validation_key_required'] = 'Please specify field name (key)'; +$_lang['ms3_vue_validation_key_invalid'] = 'Field key must contain only Latin letters, digits, and underscores'; $_lang['ms3_vue_validation_dbtype_required'] = 'Please specify database type (dbtype)'; $_lang['ms3_vue_field_created'] = 'created'; $_lang['ms3_vue_error_creating'] = 'Creation Error'; diff --git a/core/components/minishop3/lexicon/ru/default.inc.php b/core/components/minishop3/lexicon/ru/default.inc.php index 266ae656c..e6b665503 100644 --- a/core/components/minishop3/lexicon/ru/default.inc.php +++ b/core/components/minishop3/lexicon/ru/default.inc.php @@ -169,6 +169,8 @@ $_lang['ms3_err_ns'] = 'Это поле обязательно'; $_lang['ms3_err_product_key_required'] = 'Не указан ключ товара'; $_lang['ms3_err_field_key_required'] = 'Не указан ключ поля'; +$_lang['ms3_err_extra_field_class_unsupported'] = 'Этот класс модели не может содержать дополнительные поля. Используйте модель с собственной таблицей БД (например, msProductData).'; +$_lang['ms3_err_extra_field_key_invalid'] = 'Ключ поля может содержать только латинские буквы, цифры и подчёркивание.'; $_lang['ms3_err_fields_required'] = 'Требуется массив полей'; $_lang['ms3_err_field_nf'] = 'Поле не найдено'; $_lang['ms3_err_ae'] = 'Это поле должно быть уникально'; diff --git a/core/components/minishop3/lexicon/ru/vue.inc.php b/core/components/minishop3/lexicon/ru/vue.inc.php index 64d600371..01da74b1b 100644 --- a/core/components/minishop3/lexicon/ru/vue.inc.php +++ b/core/components/minishop3/lexicon/ru/vue.inc.php @@ -297,6 +297,7 @@ $_lang['ms3_vue_error_loading_fields'] = 'Не удалось загрузить список полей'; $_lang['ms3_vue_validation'] = 'Валидация'; $_lang['ms3_vue_validation_key_required'] = 'Укажите имя поля (key)'; +$_lang['ms3_vue_validation_key_invalid'] = 'Ключ поля может содержать только латинские буквы, цифры и подчёркивание'; $_lang['ms3_vue_validation_dbtype_required'] = 'Укажите тип данных БД (dbtype)'; $_lang['ms3_vue_field_created'] = 'создано'; $_lang['ms3_vue_error_creating'] = 'Ошибка создания'; diff --git a/core/components/minishop3/src/Services/ExtraFieldsService.php b/core/components/minishop3/src/Services/ExtraFieldsService.php index 395dbc6ea..56648c3be 100644 --- a/core/components/minishop3/src/Services/ExtraFieldsService.php +++ b/core/components/minishop3/src/Services/ExtraFieldsService.php @@ -6,6 +6,7 @@ use MiniShop3\Model\msProductField; use MiniShop3\Services\ExtraFields\KeyValueFieldService; use MiniShop3\Services\ExtraFields\RepeaterFieldService; +use MiniShop3\Services\Grid\GridColumnRules; use MiniShop3\Utils\ExtraFields; use MODX\Revolution\modX; use Phinx\Config\Config; @@ -103,27 +104,41 @@ public function deleteField(int $id): array return ['success' => false, 'message' => 'Field not found']; } - try { - $migrationFile = $this->migrationGenerator->generateDropColumnMigration($field); - } catch (\Exception $e) { - return ['success' => false, 'message' => 'Migration generation error: ' . $e->getMessage()]; - } + $class = (string) $field->get('class'); + $migrationName = null; + $migrationOutput = ''; - $migrationResult = $this->runMigrations(); + // STI / unsupported hosts (e.g. legacy msProduct): metadata only — never ALTER site_content. + if ($this->migrationGenerator->canHostExtraField($class)) { + try { + $migrationFile = $this->migrationGenerator->generateDropColumnMigration($field); + } catch (\Exception $e) { + return ['success' => false, 'message' => 'Migration generation error: ' . $e->getMessage()]; + } - if (!$migrationResult['success']) { + $migrationResult = $this->runMigrations(); + + if (!$migrationResult['success']) { + @unlink($migrationFile); + return $migrationResult; + } + + $migrationName = basename($migrationFile); + $migrationOutput = $migrationResult['output'] ?? ''; @unlink($migrationFile); - return $migrationResult; + $this->modx->log( + modX::LOG_LEVEL_INFO, + "[ExtraFieldsService] Migration file deleted: {$migrationName}" + ); + } else { + $this->modx->log( + modX::LOG_LEVEL_INFO, + "[ExtraFieldsService] Skipping drop migration for unsupported class {$class}; removing metadata only" + ); } - @unlink($migrationFile); - $this->modx->log( - modX::LOG_LEVEL_INFO, - "[ExtraFieldsService] Migration file deleted: " . basename($migrationFile) - ); - // Only delete msProductField for product-related models - if ($field->get('class') === 'MiniShop3\\Model\\msProductData') { + if ($class === 'MiniShop3\\Model\\msProductData') { $this->deleteProductFieldsByName($field->get('key')); } @@ -134,8 +149,8 @@ public function deleteField(int $id): array return [ 'success' => true, 'message' => 'Field deleted successfully', - 'migration' => basename($migrationFile), - 'output' => $migrationResult['output'] ?? '' + 'migration' => $migrationName, + 'output' => $migrationOutput, ]; } @@ -199,18 +214,36 @@ private function formatField(msExtraField $field): array */ private function validateFieldData(array $data): array { + $this->modx->lexicon->load('minishop3:default'); + $required = ['class', 'key', 'dbtype', 'phptype']; foreach ($required as $fieldName) { if (empty($data[$fieldName])) { return [ 'success' => false, - 'message' => "Field '{$fieldName}' is required", + 'message' => $this->modx->lexicon('ms3_err_ns'), 'field' => $fieldName ]; } } + if (!$this->migrationGenerator->canHostExtraField((string) $data['class'])) { + return [ + 'success' => false, + 'message' => $this->modx->lexicon('ms3_err_extra_field_class_unsupported'), + 'field' => 'class', + ]; + } + + if (!GridColumnRules::isValidSqlIdentifier((string) $data['key'])) { + return [ + 'success' => false, + 'message' => $this->modx->lexicon('ms3_err_extra_field_key_invalid'), + 'field' => 'key', + ]; + } + $exists = $this->modx->getObject(msExtraField::class, [ 'class' => $data['class'], 'key' => $data['key'] diff --git a/core/components/minishop3/src/Services/MigrationGenerator.php b/core/components/minishop3/src/Services/MigrationGenerator.php index 382bf23fc..b3c12cb6c 100644 --- a/core/components/minishop3/src/Services/MigrationGenerator.php +++ b/core/components/minishop3/src/Services/MigrationGenerator.php @@ -13,7 +13,8 @@ class MigrationGenerator public function __construct(modX $modx) { $this->modx = $modx; - $this->migrationsPath = MODX_CORE_PATH . 'components/minishop3/migrations/'; + $corePath = defined('MODX_CORE_PATH') ? \MODX_CORE_PATH : (dirname(__DIR__, 2) . '/'); + $this->migrationsPath = $corePath . 'components/minishop3/migrations/'; } /** @@ -59,7 +60,7 @@ public function generateDropColumnMigration(msExtraField $field): string */ private function renderAddColumnTemplate(string $className, msExtraField $field): string { - $tableName = $this->getTableName($field->get('class')); + $tableName = $this->resolveTableName($field->get('class')); $columnName = $field->get('key'); $dbtype = $this->mapDbTypeToPhinx($field->get('dbtype')); $precision = $this->parsePrecision($field); @@ -118,7 +119,7 @@ public function change(): void */ private function renderDropColumnTemplate(string $className, msExtraField $field): string { - $tableName = $this->getTableName($field->get('class')); + $tableName = $this->resolveTableName($field->get('class')); $columnName = $field->get('key'); $indexName = $field->hasIndex() ? $field->getIndexName() : ''; @@ -270,31 +271,52 @@ private function parseAttributes(msExtraField $field): string } /** - * Get table name from model class + * Whether the model class has its own DB table (not STI / modResource inheritance). */ - private function getTableName(string $class): string + public function canHostExtraField(string $class): bool { - // MiniShop3\Model\msProductData → ms3_products - $tableMap = [ - 'MiniShop3\\Model\\msProductData' => 'ms3_products', - 'MiniShop3\\Model\\msVendor' => 'ms3_vendors', - 'MiniShop3\\Model\\msOrder' => 'ms3_orders', - 'MiniShop3\\Model\\msCategory' => 'ms3_categories', - 'MiniShop3\\Model\\msOrderProduct' => 'ms3_order_products', - 'MiniShop3\\Model\\msOrderAddress' => 'ms3_order_addresses', - ]; + return $this->ownTableName($class) !== null; + } + + /** + * Unprefixed, unquoted table name for Phinx (table_prefix is applied in phinx.php). + */ + public function resolveTableName(string $class): string + { + $table = $this->ownTableName($class); + if ($table === null) { + throw new \InvalidArgumentException("Model class cannot host extra fields: {$class}"); + } + + return $table; + } - if (isset($tableMap[$class])) { - return $tableMap[$class]; + /** + * Own logical table from mysql metaMap, or null when the class inherits another table. + */ + private function ownTableName(string $class): ?string + { + $mysqlClass = str_replace('\\Model\\', '\\Model\\mysql\\', $class); + if (!str_starts_with($mysqlClass, 'MiniShop3\\Model\\mysql\\') || !class_exists($mysqlClass)) { + return null; + } + + $table = $mysqlClass::$metaMap['table'] ?? null; + if (!is_string($table) || $table === '') { + return null; } - // If class not in map, try to get from xPDO - $object = $this->modx->newObject($class); - if ($object) { - return $this->modx->getTableName($class); + $table = trim($table, '`'); + if (str_contains($table, '.')) { + $parts = explode('.', $table); + $table = (string) end($parts); } - throw new \Exception("Cannot determine table name for class: {$class}"); + if ($table === '' || str_contains($table, '`')) { + return null; + } + + return $table; } /** @@ -302,17 +324,10 @@ private function getTableName(string $class): string */ private function getTableShortName(string $class): string { - // MiniShop3\Model\msProductData → Products - $map = [ - 'MiniShop3\\Model\\msProductData' => 'Products', - 'MiniShop3\\Model\\msVendor' => 'Vendors', - 'MiniShop3\\Model\\msOrder' => 'Orders', - 'MiniShop3\\Model\\msCategory' => 'Categories', - 'MiniShop3\\Model\\msOrderProduct' => 'OrderProducts', - 'MiniShop3\\Model\\msOrderAddress' => 'OrderAddresses', - ]; + $table = $this->resolveTableName($class); + $base = preg_replace('/^ms3_/', '', $table) ?? $table; - return $map[$class] ?? 'Table'; + return $this->toCamelCase($base); } private function toCamelCase(string $str): string diff --git a/core/components/minishop3/tests/MigrationGeneratorTableResolveTest.php b/core/components/minishop3/tests/MigrationGeneratorTableResolveTest.php new file mode 100644 index 000000000..41f9277f4 --- /dev/null +++ b/core/components/minishop3/tests/MigrationGeneratorTableResolveTest.php @@ -0,0 +1,96 @@ +resolveTableName(msProductData::class); +$assertSame('ms3_products', $productDataTable, 'msProductData → ms3_products'); +$assertFalse(str_contains($productDataTable, '`'), 'table name must not contain backticks'); +$assertFalse(str_contains($productDataTable, 'site_content'), 'must not resolve to modResource table'); + +$assertSame('ms3_vendors', $generator->resolveTableName(msVendor::class), 'msVendor → ms3_vendors'); + +$assertTrue($generator->canHostExtraField(msProductData::class), 'msProductData can host extra fields'); +$assertFalse($generator->canHostExtraField(msProduct::class), 'msProduct must be rejected (modResource STI)'); +$assertFalse($generator->canHostExtraField(msCategory::class), 'msCategory must be rejected (modResource STI)'); + +foreach ([msProduct::class, msCategory::class] as $unsupportedClass) { + try { + $generator->resolveTableName($unsupportedClass); + $fail('resolveTableName must throw for ' . $unsupportedClass); + } catch (\InvalidArgumentException) { + // expected + } +} + +echo "OK MigrationGeneratorTableResolveTest\n"; diff --git a/core/components/minishop3/tests/Unit/Services/ExtraFieldsValidationTest.php b/core/components/minishop3/tests/Unit/Services/ExtraFieldsValidationTest.php new file mode 100644 index 000000000..4d9187b4b --- /dev/null +++ b/core/components/minishop3/tests/Unit/Services/ExtraFieldsValidationTest.php @@ -0,0 +1,63 @@ +assertFalse(GridColumnRules::isValidSqlIdentifier('гарантия')); + $this->assertFalse(GridColumnRules::isValidSqlIdentifier('warranty months')); + $this->assertTrue(GridColumnRules::isValidSqlIdentifier('warranty_months')); + } + + public function testMigrationGeneratorRejectsResourceStiClasses(): void + { + $generator = new MigrationGenerator($this->createModxStub()); + + $this->assertTrue($generator->canHostExtraField(msProductData::class)); + $this->assertFalse($generator->canHostExtraField(msProduct::class)); + $this->assertFalse($generator->canHostExtraField(msCategory::class)); + $this->assertSame('ms3_products', $generator->resolveTableName(msProductData::class)); + } + + private function createModxStub(): modX + { + return new class extends modX { + public function getOption($key, $options = null, $default = null) + { + if ($key === 'dbtype') { + return 'mysql'; + } + + return $default; + } + + public function log($level, $msg, $target = '', $def = '', $file = '', $line = ''): void + { + } + }; + } +} diff --git a/vueManager/src/components/ExtraFieldsManager.vue b/vueManager/src/components/ExtraFieldsManager.vue index 545ba44f5..05d3d92ec 100644 --- a/vueManager/src/components/ExtraFieldsManager.vue +++ b/vueManager/src/components/ExtraFieldsManager.vue @@ -69,13 +69,10 @@ const fieldForm = ref({ }) /** - * Available model classes + * Available model classes (only models with their own ms3_* table; no modResource STI). */ const classOptions = computed(() => [ - // Товары - { label: _('ms3_vue_class_product'), value: 'MiniShop3\\Model\\msProduct' }, { label: _('ms3_vue_class_product_data'), value: 'MiniShop3\\Model\\msProductData' }, - { label: _('ms3_vue_class_category'), value: 'MiniShop3\\Model\\msCategory' }, { label: _('ms3_vue_class_vendor'), value: 'MiniShop3\\Model\\msVendor' }, { label: _('ms3_vue_class_option'), value: 'MiniShop3\\Model\\msOption' }, { label: _('ms3_vue_class_link'), value: 'MiniShop3\\Model\\msLink' }, @@ -158,6 +155,13 @@ const indexTypeOptions = computed(() => [ const isRepeaterField = computed(() => fieldForm.value.xtype === REPEATER_XTYPE) const isKeyValueField = computed(() => fieldForm.value.xtype === KEY_VALUE_XTYPE) +/** Same rule as GridColumnRules::SQL_IDENTIFIER_PATTERN on the server. */ +const SQL_IDENTIFIER_PATTERN = /^[a-z0-9_]+$/i + +function isValidFieldKey(key) { + return typeof key === 'string' && key !== '' && SQL_IDENTIFIER_PATTERN.test(key) +} + watch( () => fieldForm.value.xtype, xtype => { @@ -310,6 +314,16 @@ async function createField() { return } + if (!isValidFieldKey(fieldForm.value.key)) { + toast.add({ + severity: 'warn', + summary: _('ms3_vue_validation'), + detail: _('ms3_vue_validation_key_invalid'), + life: 3000, + }) + return + } + if (!fieldForm.value.dbtype) { toast.add({ severity: 'warn', From 7b276f43e2021e0c3ec390b80e06ee51419fb278 Mon Sep 17 00:00:00 2001 From: Ivan Bochkarev Date: Sun, 30 Aug 2026 17:33:34 +0600 Subject: [PATCH 2/2] test(extra-fields): cover createField gates and fix migrations fallback (#645) Assert ExtraFieldsService rejects STI classes and invalid keys before save, and point the MODX_CORE_PATH-less migrations path at the package root. --- .../src/Services/MigrationGenerator.php | 6 +- .../Services/ExtraFieldsValidationTest.php | 102 +++++++++++++++++- .../minishop3/tests/support/xpdo_stub.php | 1 + 3 files changed, 105 insertions(+), 4 deletions(-) diff --git a/core/components/minishop3/src/Services/MigrationGenerator.php b/core/components/minishop3/src/Services/MigrationGenerator.php index b3c12cb6c..ea33e7bee 100644 --- a/core/components/minishop3/src/Services/MigrationGenerator.php +++ b/core/components/minishop3/src/Services/MigrationGenerator.php @@ -13,8 +13,10 @@ class MigrationGenerator public function __construct(modX $modx) { $this->modx = $modx; - $corePath = defined('MODX_CORE_PATH') ? \MODX_CORE_PATH : (dirname(__DIR__, 2) . '/'); - $this->migrationsPath = $corePath . 'components/minishop3/migrations/'; + // MODX_CORE_PATH is the site core/; fallback is this package root (…/components/minishop3). + $this->migrationsPath = defined('MODX_CORE_PATH') + ? rtrim((string) \MODX_CORE_PATH, '/\\') . '/components/minishop3/migrations/' + : dirname(__DIR__, 2) . '/migrations/'; } /** diff --git a/core/components/minishop3/tests/Unit/Services/ExtraFieldsValidationTest.php b/core/components/minishop3/tests/Unit/Services/ExtraFieldsValidationTest.php index 4d9187b4b..16940139c 100644 --- a/core/components/minishop3/tests/Unit/Services/ExtraFieldsValidationTest.php +++ b/core/components/minishop3/tests/Unit/Services/ExtraFieldsValidationTest.php @@ -7,6 +7,7 @@ use MiniShop3\Model\msCategory; use MiniShop3\Model\msProduct; use MiniShop3\Model\msProductData; +use MiniShop3\Services\ExtraFieldsService; use MiniShop3\Services\Grid\GridColumnRules; use MiniShop3\Services\MigrationGenerator; use MODX\Revolution\modX; @@ -14,12 +15,14 @@ /** * Field-key and host-class rules for Extra Fields (#645). - * Full ExtraFieldsService needs xPDO cache constants; covered via MigrationGenerator + GridColumnRules. */ final class ExtraFieldsValidationTest extends TestCase { + private bool $savedCalled = false; + protected function setUp(): void { + $this->savedCalled = false; if (!class_exists(modX::class, false)) { require_once dirname(__DIR__, 2) . '/stubs/ModxStub.php'; } @@ -43,9 +46,63 @@ public function testMigrationGeneratorRejectsResourceStiClasses(): void $this->assertSame('ms3_products', $generator->resolveTableName(msProductData::class)); } + public function testCreateFieldRejectsUnsupportedClassBeforeSave(): void + { + $service = new ExtraFieldsService($this->createModxStub()); + $result = $service->createField([ + 'class' => msProduct::class, + 'key' => 'warranty_months', + 'dbtype' => 'varchar', + 'phptype' => 'string', + ]); + + $this->assertFalse($result['success']); + $this->assertSame('class', $result['field'] ?? null); + $this->assertSame('ms3_err_extra_field_class_unsupported', $result['message'] ?? null); + $this->assertFalse($this->savedCalled); + } + + public function testCreateFieldRejectsCyrillicKeyBeforeSave(): void + { + $service = new ExtraFieldsService($this->createModxStub()); + $result = $service->createField([ + 'class' => msProductData::class, + 'key' => 'гарантия', + 'dbtype' => 'varchar', + 'phptype' => 'string', + ]); + + $this->assertFalse($result['success']); + $this->assertSame('key', $result['field'] ?? null); + $this->assertSame('ms3_err_extra_field_key_invalid', $result['message'] ?? null); + $this->assertFalse($this->savedCalled); + } + + public function testCreateFieldRejectsCategoryStiClassBeforeSave(): void + { + $service = new ExtraFieldsService($this->createModxStub()); + $result = $service->createField([ + 'class' => msCategory::class, + 'key' => 'warranty_months', + 'dbtype' => 'varchar', + 'phptype' => 'string', + ]); + + $this->assertFalse($result['success']); + $this->assertSame('class', $result['field'] ?? null); + $this->assertFalse($this->savedCalled); + } + private function createModxStub(): modX { - return new class extends modX { + $test = $this; + + return new class ($test) extends modX { + public function __construct(private ExtraFieldsValidationTest $test) + { + parent::__construct(); + } + public function getOption($key, $options = null, $default = null) { if ($key === 'dbtype') { @@ -58,6 +115,47 @@ public function getOption($key, $options = null, $default = null) public function log($level, $msg, $target = '', $def = '', $file = '', $line = ''): void { } + + public function newObject($className, $fields = []) + { + $this->test->markSavedCalled(); + + return new class { + public function fromArray($data): void + { + } + + public function save(): bool + { + return true; + } + + public function remove(): bool + { + return true; + } + + public function get($key) + { + return null; + } + + public function toArray(): array + { + return []; + } + }; + } + + public function getObject($className, $criteria = null) + { + return null; + } }; } + + public function markSavedCalled(): void + { + $this->savedCalled = true; + } } diff --git a/core/components/minishop3/tests/support/xpdo_stub.php b/core/components/minishop3/tests/support/xpdo_stub.php index 29be3651f..328016307 100644 --- a/core/components/minishop3/tests/support/xpdo_stub.php +++ b/core/components/minishop3/tests/support/xpdo_stub.php @@ -6,4 +6,5 @@ class xPDO { + public const OPT_CACHE_KEY = 'cache_key'; }