Skip to content

feat(grid): add option field type for category products grid#154

Open
Ibochkarev wants to merge 2 commits intobetafrom
feat/grid-option-type
Open

feat(grid): add option field type for category products grid#154
Ibochkarev wants to merge 2 commits intobetafrom
feat/grid-option-type

Conversation

@Ibochkarev
Copy link
Member

@Ibochkarev Ibochkarev commented Mar 16, 2026

Описание

Добавлен новый тип поля option для конфигурации грида товаров в категории. Позволяет отображать и сортировать/фильтровать колонки из msProductOption (опции товара).

Изменения:

  • GridConfigService: validateOptionConfig(), extractOptionFields() — валидация и извлечение полей типа option для построения JOIN
  • CategoryProductsController: динамические LEFT JOIN к msProductOption по ключам опций из конфига грида
  • GridFieldsConfig.vue: UI для настройки полей типа option (ключ опции)
  • Лексиконы: field_type_option (ru/en)

Тип изменений

  • Новая функциональность (non-breaking change)
  • Исправление бага (non-breaking change)
  • Breaking change (изменение, ломающее обратную совместимость)
  • Рефакторинг (без изменения функциональности)
  • Документация
  • Другое (опишите):

Связанные Issues

#140

Как это было протестировано?

  • Ручное тестирование
  • Автоматические тесты (PHPStan, ESLint)
  • Тестирование на разных версиях PHP/MODX

Конфигурация тестирования:

  • MiniShop3:
  • MODX:
  • PHP:

Скриншоты (если применимо)

До После

Чеклист

  • Код соответствует стилю проекта
  • Добавлены/обновлены комментарии в сложных местах
  • Изменения не ломают существующую функциональность
  • Лексиконы добавлены на двух языках (ru/en)
  • PHPStan проходит без новых ошибок
  • ESLint проходит без ошибок (для JS/Vue изменений)
  • Обновлён CHANGELOG.md (для значимых изменений)

Дополнительные заметки

Поля типа option требуют указания option.key — ключ опции из msProductOption. При настройке грида category-products колонки с типом option автоматически подключаются через LEFT JOIN.

- GridConfigService: validateOptionConfig, extractOptionFields
- CategoryProductsController: JOIN msProductOption for option columns
- GridFieldsConfig.vue: UI for option type configuration
- Lexicons: field_type_option (ru/en)
@Ibochkarev Ibochkarev marked this pull request as ready for review March 16, 2026 08:24
Copy link
Member

@biz87 biz87 left a comment

Choose a reason for hiding this comment

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

Code Review

Хорошая фича — опции товара как колонки в гриде. Но есть критичный баг и несколько проблем.


Критичное

1. Дубликаты строк при multi-value опциях

msProductOption хранит одну строку на product+key+value. Если у товара опция color имеет несколько значений (Red, Blue), LEFT JOIN создаст 2 строки для одного товара. В запросе нет GROUP BY — товар появится в гриде дважды.

COUNT(DISTINCT msProduct.id) в подсчёте корректен, но выборка данных — нет.

Варианты:

  • GROUP BY msProduct.id + GROUP_CONCAT(DISTINCT ...) для option values
  • Или документировать что поддерживаются только single-value опции и добавить валидацию

2. $this->modx->escape() для значения фильтра — баг

$filterValue = $this->modx->escape($params[$paramKey]);
$c->where(["`{$opt['alias']}`.value:LIKE" => "%{$filterValue}%"]);

xPDO::escape() оборачивает строку в бэктики (`). Это для идентификаторов (имён таблиц/колонок), не для значений. Результат: фильтр ищет буквальные бэктики в данных.

xPDO where с :LIKE использует prepared statements — значение параметризуется автоматически. escape() не нужен. Правильно:

$c->where(["`{$opt['alias']}`.value:LIKE" => "%{$params[$paramKey]}%"]);

Это паттерн из существующего кода (строки 104-106, 112-114 в текущем контроллере).


Замечания

3. clone xPDOQuery для COUNT

$countQuery = clone $c;

clone в PHP делает shallow copy. xPDOQuery содержит вложенные массивы и объекты ($query['where'], $query['from']). Shallow clone разделяет ссылки — модификация $countQuery может затронуть $c. Нужно проверить, что select() на $countQuery не мутирует $c.

4. Нет re-валидации option key при чтении

Ключ валидируется при сохранении (preg_match('/^[a-z0-9_]+$/i')), но при чтении из конфига и подстановке в SQL — нет. Если кто-то изменит конфиг напрямую в БД, это потенциальная SQL injection через alias/key в JOIN условии. Defense in depth — стоит повторить валидацию в extractOptionFields().

5. formatProduct — хороший рефакторинг

Переход от msProduct->get() к raw array — правильно. Убирает overhead гидратации xPDO объектов. Но добавление ВСЕХ неизвестных ключей из row:

foreach ($row as $key => $value) {
    if (!array_key_exists($key, $data)) {
        $data[$key] = $value;
    }
}

Может протечь внутренние поля xPDO/MySQL (например, class_key, context_key, content и др.). Лучше добавлять только option_* поля:

foreach ($row as $key => $value) {
    if (str_starts_with($key, 'option_') && !array_key_exists($key, $data)) {
        $data[$key] = $value;
    }
}

Резюме

Основное перед мержем:

  1. Решить проблему дублей при multi-value опциях (GROUP BY или ограничение)
  2. Убрать $this->modx->escape() из фильтра значений
  3. Фильтровать протекающие поля в formatProduct
  4. Re-валидация option key в extractOptionFields()

…field handling

- Consolidated product list query logic into a new method, buildProductListQuery, for better readability and maintainability.
- Updated option field handling to use GROUP_CONCAT for compliance with MySQL ONLY_FULL_GROUP_BY.
- Enhanced formatProduct method to prevent leaking internal columns by validating option field names.
- Added validation for option keys in GridConfigService to ensure data integrity.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature] Поддержка колонок с опциями товара в таблице "Список товаров"

2 participants