fix(product): improve options parsing and empty options handling#149
fix(product): improve options parsing and empty options handling#149
Conversation
- Extract option key parsing to Utils::extractOptionKey() - Use str_starts_with() for options-* field detection (PHP 8.2+) - Always set options property in Update/UpdateFromGrid for form clearing - Remove redundant is_array check in OptionSyncService - Handle empty options array with removeUnusedOptions when removeOther=true
biz87
left a comment
biz87
left a comment
There was a problem hiding this comment.
Code Review
Рефакторинг extractOptionKey() — чисто и правильно. Но основная заявленная цель PR (очистка опций через форму) не работает из-за разрыва в цепочке вызовов.
Критичное: фикс не достигает цели
afterSave() в Update.php блокирует пустой массив опций
PR меняет beforeSet() — теперь setProperty('options', $options) вызывается всегда, даже при $options = []. Но afterSave() (строки 113-114) не изменён:
$options = $this->getProperty('options');
if (!empty($options) && is_array($options)) {
$service->saveOptions($productData, $options, false);
}!empty([]) — false. Пустой массив никогда не дойдёт до saveOptions().
Более того, afterSave() вызывает saveOptions() с removeOther = false. Даже если пустой массив доберётся до OptionSyncService::saveProductOptions(), изменение в сервисе (вызов removeUnusedOptions при empty($options) && $removeOther) не сработает, т.к. removeOther всегда false в этом месте.
Цепочка разорвана в двух местах. Заявленная очистка опций через форму не происходит.
То же самое для UpdateFromGrid — наследует afterSave() от Update.
Замечания по extractOptionKey
Метод корректный для основного кейса, но есть edge case:
return rtrim(substr($key, 8), '[]');rtrim($str, '[]') удаляет символы из набора [, ] с конца строки поочерёдно. Для options-color[] → color — ок. Но для гипотетического options- → пустая строка (не null). Стоит добавить проверку:
$optionKey = rtrim(substr($key, 8), '[]');
return $optionKey !== '' ? $optionKey : null;Итого
- Критично:
afterSave()вUpdate.phpнужно обновить — заменить!empty($options)на$options !== null(илиis_array), и передаватьremoveOther = trueесли нужна очистка. Без этого весь фикс OptionSyncService бесполезен. extractOptionKey— добавить guard на пустую строку- Сам рефакторинг (
str_starts_with, вынос в Utils) — хорошо
- Simplify options handling by removing redundant checks for empty options - Adjust logic to remove other options only when the options array is empty - Update comments for clarity on options processing
Without the guard, every product save without options-* form fields triggers afterSave → saveOptions([], removeOther=true) which deletes all options previously saved by msProductData::save(). The actual fix for #148 is extractOptionKey() with rtrim('[]') — form sends options-color[] but old substr gave key "color[]" instead of "color", so the option was never matched/deleted. - Restore !empty($options) check in beforeSet/afterSave (Update, Create) - Keep removeOther=false: custom form options must not remove JSON options - Restore !empty guard in UpdateFromGrid::beforeSet
2 participants
Описание
Улучшение парсинга опций товара из полей формы и обработки пустого массива опций.
Изменения:
Utils::extractOptionKey()(устранение дублирования)str_starts_with()вместоstrpos()для проверки префиксаoptions-(PHP 8.2+)setProperty('options', $options), включая пустой массив — обеспечивает корректную очистку опций через форму!is_array($options)в OptionSyncService (тип задан в сигнатуре)removeOther=trueтеперь вызываетсяremoveUnusedOptions()Тип изменений
Связанные Issues
#148
Как это было протестировано?
Чеклист