fix(product): сохранение Data и extra fields в процессорах Create/Update

[Ibochkarev]

Описание

Исправляет #297: поля msProductData (включая Object Extension / extra fields, например ext_id) не сохранялись при вызове runProcessor с блоком Data или плоскими ключами (price, …).

Причина (две части):

1. Resource\Create|Update вызывают $object->fromArray($properties) без ignoreInvalid — колонки msProductData не попадают на объект.
2. На Create composite Data требует id ресурса; запись в beforeSave() не гарантирует persist price и extra fields.

Решение:

• Trait ProductDataPayloadTrait: захват Data в beforeSet(), whitelist через getDataFieldsNames(), loadMap() для extra fields.
• Update — applyProductDataPayload() в beforeSave().
• Create — persistProductDataPayload() в afterSave() после insert ресурса.
• Alias-классы msProductCreateProcessor / msProductUpdateProcessor для резолвера Resource\Create|Update::getInstance() (импорт CSV, интеграции).

Поддерживаются плоские ключи в корне payload и явный блок Data (при конфликте побеждает Data).

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

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

Связанные Issues

Closes #297

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

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

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

• MiniShop3: dev / beta
• MODX: 3.x
• PHP: php -l для изменённых PHP-файлов (OK)

Сценарии для ревьюера:

1. MiniShop3\Processors\Product\Create + Data => ['price' => 5200, 'ext_id' => '…'] — значения в ms3_products.
2. MODX\Revolution\Processors\Resource\Create + class_key = msProduct — тот же результат (alias-процессор).
3. Update с блоком Data — поля обновляются.
4. Плоские price / ext_id в корне payload.
5. Регрессия: создание/редактирование товара в менеджере (price, options-*).

Пример runProcessor:

$response = $modx->runProcessor('MiniShop3\\Processors\\Product\\Create', [
    'pagetitle' => 'Товар',
    'parent' => 42,
    'class_key' => \MiniShop3\Model\msProduct::class,
    'Data' => [
        'price' => 1990.00,
        'ext_id' => 'SKU-123',
    ],
]);

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

До	После
price / extra fields не сохранялись через runProcessor на Create	сохраняются через Data и плоские ключи после afterSave()

Чеклист

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

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

Code review (clean-code / pre-merge)

Сильные стороны:

• Trait с одной ответственностью, методы короткие, имена раскрывают intent (capture / apply / persist).
• Whitelist полей через getDataFieldsNames() + array_intersect_key — безопаснее произвольного merge.
• fromArray(..., true, true) с ignoreInvalid — корректный контракт для extra fields.
• Разделение Create (afterSave + explicit save) / Update (beforeSave) задокументировано в docblock trait.
• Alias-процессоры в Model\ — минимальный diff, без дублирования логики.

Замечания (не блокеры, follow-up):

• persistProductDataPayload() возвращает false и при «нет полей для записи», и при ошибке $productData->save() — в Create::afterSave() результат не проверяется; при сбое save клиент может получить success. Имеет смысл различать noop / failure или проверять return при непустом payload.
• Невалидная JSON-строка в Data из connector молча игнорируется — для отладки можно log или validation error (низкий приоритет).
• Автотестов на trait нет — regression только ручной (типично для MODX processors).

Затронутые файлы:

• Processors/Product/ProductDataPayloadTrait.php (новый)
• Processors/Product/Create.php, Update.php
• Model/msProductCreateProcessor.php, msProductUpdateProcessor.php (новые)

[biz87]

Спасибо за PR — закрывает реальный гэп в processor-флоу для msProductData и сделано архитектурно правильно (trait, whitelist, разделение Create/Update, alias-классы). Прошёлся по коду, PHPStan прогнал — есть три вещи, которые лучше доделать до мержа.

Корректность диагноза

Подтверждаю: modResource::fromArray() без ignoreInvalid отбрасывает колонки msProductData; на Create composite Data требует id ресурса, которого нет в beforeSave(), → нужен afterSave(). На Update — beforeSave() достаточно. Закрывает гэп для runProcessor (CSV-импорт, интеграции, Resource\Create с class_key=msProduct).

Архитектура

Делю на сильные стороны и проблемы.

Хорошо

• Trait с одной ответственностью, методы short + self-describing (capture / apply / persist).
• Whitelist через $this->object->getDataFieldsNames() + array_intersect_key — безопаснее raw merge.
• loadMap() через services->get('ms3') гарантирует регистрацию extra fields к моменту whitelist.
• Alias-классы в MiniShop3\Model\msProduct{Create,Update}Processor — корректное место для резолвера Resource\Create::getInstance(), минимальный diff.
• Конфликт Data vs flat keys разрешён в пользу Data через порядок assign(flat) → assign(nested). Документировано.
• unsetProperty('Data') — parent fromArray не споткнётся на лишнем ключе.
• Защита от {"Data": {"id": 999999}} через unset($fields['id']) в getAllowedProductDataFieldNames().

🔴 Блокер 1 — PHPStan

Прогнал по файлам PR (phpstan.neon):

ProductDataPayloadTrait.php:115
Method getAllowedProductDataFieldNames() should return array<string, true>
but returns array<mixed, (int|string)>.

array_flip($this->object->getDataFieldsNames()) — PHPStan не может сузить тип к array<string, true>, потому что docblock getDataFieldsNames() объявляет просто array. Чек-бокс «PHPStan проходит» в PR не отмечен — подтверждается.

Решение: либо уточнить тип в msProduct::getDataFieldsNames() (@return list<string>), либо явный assert/cast в trait. Первый вариант чище, заодно улучшит inference в других вызовах.

🔴 Блокер 2 — discarded return value в Create::afterSave()

В описании PR отмечено как follow-up. Не согласен — это блокирующий класс багов для критического пути (Create продукта).

Сейчас:

$result = parent::afterSave();
$this->persistProductDataPayload();  // bool отброшен

Если $productData->save() упадёт (DB error, валидация, FK constraint) — ресурс создан, запись в ms3_products отсутствует, клиент получает success. Silent partial-state, найти потом по жалобе «у товаров нет цен» — недёшево.

Плюс persistProductDataPayload() возвращает false и для «нет полей для записи», и для «save упал» — два разных исхода под одним boolean. Нужно различать.

Минимальный фикс:

private function assignProductDataPayload(bool $persist): bool
{
    // ...
    if ($flatFields === [] && $nestedFields === []) {
        return true;  // noop = success, нет работы
    }
    // ...
    return $persist ? (bool) $productData->save() : true;
}

И в Create::afterSave():

if (!$this->persistProductDataPayload()) {
    $this->modx->log(
        \MODX\Revolution\modX::LOG_LEVEL_ERROR,
        '[msProduct/Create] failed to persist msProductData for resource id '
        . $this->object->get('id')
    );
}

Решать — ронять процессор через addFieldError или продолжать с логом — на твоё усмотрение.

🟡 Замечание 1 — невалидный JSON в Data молча игнорируется

В captureProductDataPayload():

if (is_string($payload)) {
    $decoded = json_decode($payload, true);
    if (is_array($decoded)) {
        $payload = $decoded;
    } else {
        return;  // payload потерян без следа
    }
}

Connector с опечаткой в JSON ('{"price":1990, malformed}') → блок молча выкинут, success возвращается. Debug-кошмар: пользователь жалуется «не сохранилось», ищется часами, проблема в одной запятой.

Цена фикса — одна строка перед return:

$this->modx->log(
    \MODX\Revolution\modX::LOG_LEVEL_WARN,
    '[msProduct] malformed Data JSON payload: ' . json_last_error_msg()
);

Рекомендую.

🟡 Замечание 2 — Update полагается на каскад

applyProductDataPayload() в Update::beforeSave() модифицирует $productData в памяти, рассчитывает что parent::save() его сохранит каскадно. Если по любой причине каскад не отработает (xPDO config, события прерывают, гонка) — изменения не уйдут в БД.

Не блокер, но единая стратегия (explicit save и для Create, и для Update в afterSave()) убрала бы скрытую зависимость от каскада composite. На усмотрение.

Резюме

Закрыть до мержа:

1. PHPStan типы (5 минут).
2. Discarded return value + лог ошибки на Create (15 минут).
3. Лог при невалидном JSON (1 строка).

После этого — мержим без оговорок.

[Ibochkarev]

@biz87 можно смотреть повторно

[biz87]

Спасибо за фиксы — закрыто всё содержательное из первого ревью. Прогнал PHPStan на актуальной версии, осталось две мелочи перед мержем.

✅ Что закрыто

1. Discarded return value в Create::afterSave()

if (!$this->persistProductDataPayload()) {
    $this->modx->log(modX::LOG_LEVEL_ERROR,
        '[msProduct/Create] failed to persist msProductData for resource id '
        . $this->object->get('id'));
}

Плюс в трейте assignProductDataPayload() теперь возвращает true при noop, а не false — ложноположительный fail устранён. Точно по моему фидбеку.

2. Лог при malformed JSON

$this->modx->log(modX::LOG_LEVEL_WARN,
    '[msProduct] malformed Data JSON payload: ' . json_last_error_msg());

3. PHPStan typing на getDataFieldsNames()

/**
 * @return list<string>
 */
public function getDataFieldsNames(): array

Закрыли в источнике — это правильнее, чем правка в потребителе.

🟡 Осталось — две мелочи

PHPStan ещё ругается на trait

После уточнения getDataFieldsNames(): list<string> PHPStan вычисляет точный тип возврата getAllowedProductDataFieldNames():

ProductDataPayloadTrait.php:121
Method getAllowedProductDataFieldNames() should return array<string, true>
but returns array<string, int<0, max>>.

Docblock в трейте остался от предыдущей итерации (array<string, true>), а array_flip на list<string> теперь даёт array<string, int<0, max>>. Поведенчески безразлично (isset() не смотрит на значение), но PHPStan не пропускает.

Один из вариантов:

a) Поправить docblock на корректный тип:

/**
 * @return array<string, int<0, max>>
 */
private function getAllowedProductDataFieldNames(): array

b) Заменить array_flip() на array_fill_keys() — семантически чище, true явно означает «whitelisted»:

$fields = array_fill_keys($this->object->getDataFieldsNames(), true);
unset($fields['id']);
return $fields;

Я бы выбрал (b) — читается как «список разрешённых ключей», и array<string, true> остаётся корректным.

🔴 Conflicting — нужен rebase

mergeStateStatus: DIRTY. Скорее всего из-за свежих мержей в beta (#306/#315 — class_key в Product/Create + use modDocument;).

Правки концептуально аддитивные:

• fix(product): автоматический class_key msProduct в Product\Create (#305) #306: добавил initialize() и use MODX\Revolution\modDocument;
• fix(product): сохранение Data и extra fields в процессорах Create/Update #298: добавляет use ProductDataPayloadTrait;, use MODX\Revolution\modX; и пара вызовов в beforeSet()/afterSave()

Take-both в обеих секциях после git rebase origin/beta.

Резюме

Содержательно PR готов. До мержа:

1. git rebase origin/beta
2. Одна правка в трейте (вариант a или b выше)

После этого — мержим без оговорок.

[biz87]

Закрыто всё:

• ✅ Rebase на текущий beta — mergeStateStatus: CLEAN. Проверил Product/Create.php — initialize() из fix(product): автоматический class_key msProduct в Product\Create (#305) #306 и use ProductDataPayloadTrait; из этого PR сосуществуют корректно.
• ✅ PHPStan typing на трейт — взят вариант (b), array_fill_keys(..., true) вместо array_flip. Теперь docblock @return array<string, true> соответствует реальному возврату.
• ✅ PHPStan чист по всем 6 файлам PR.

Плюс ранее уже было закрыто:

• ✅ Discarded return value + лог ошибки на Create
• ✅ Лог при malformed JSON в Data
• ✅ Типизация getDataFieldsNames(): list<string> в msProduct

Мержим.