diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e72fea..18d4ee9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +# Version 23.3.0 + +## Features + +* Add state detection to observers for "import:attributes", ensuring changes are only persisted when necessary +* Handle media directory creation on demand to prevent invalid paths during file uploads and enhance error handling consistency + # Version 23.2.0 ## Features diff --git a/etc/configuration/operations.json b/etc/configuration/operations.json index 31d602d..739fa26 100644 --- a/etc/configuration/operations.json +++ b/etc/configuration/operations.json @@ -219,6 +219,7 @@ }, "params": { "copy-images": false, + "override-images": true, "media-directory" : "pub/media/attribute/swatch", "images-file-directory" : "var/importexport/media/attribute/swatch", "clean-up-empty-columns": [] @@ -316,6 +317,7 @@ }, "params": { "copy-images": false, + "override-images": true, "media-directory" : "pub/media/attribute/swatch", "images-file-directory" : "var/importexport/media/attribute/swatch", "clean-up-empty-columns": [] diff --git a/src/Observers/AttributeLabelObserver.php b/src/Observers/AttributeLabelObserver.php index 204acbb..c7b5ed1 100644 --- a/src/Observers/AttributeLabelObserver.php +++ b/src/Observers/AttributeLabelObserver.php @@ -18,6 +18,7 @@ use TechDivision\Import\Attribute\Utils\ColumnKeys; use TechDivision\Import\Attribute\Utils\MemberNames; use TechDivision\Import\Attribute\Services\AttributeBunchProcessorInterface; +use TechDivision\Import\Observers\StateDetectorInterface; /** * Observer that create's the EAV attribute label. @@ -42,10 +43,16 @@ class AttributeLabelObserver extends AbstractAttributeImportObserver * Initializes the observer with the passed subject instance. * * @param \TechDivision\Import\Attribute\Services\AttributeBunchProcessorInterface $attributeBunchProcessor The attribute bunch processor instance + * @param \TechDivision\Import\Observers\StateDetectorInterface|null $stateDetector The state detector instance to use */ - public function __construct(AttributeBunchProcessorInterface $attributeBunchProcessor) - { + public function __construct( + AttributeBunchProcessorInterface $attributeBunchProcessor, + ?StateDetectorInterface $stateDetector = null + ) { $this->attributeBunchProcessor = $attributeBunchProcessor; + + // pass the state detector to the parent method + parent::__construct($stateDetector); } /** @@ -66,8 +73,14 @@ protected function process() // query whether or not an value for the attribute label is available if ($attributeLabel = $this->prepareAttributes()) { - // prepare and persist the attribue label - $this->persistAttributeLabel($this->initializeAttribute($attributeLabel)); + // initialize the attribute label + $attributeLabel = $this->initializeAttribute($attributeLabel); + + // query whether or not the attribute label has changed and has to be persisted + if ($this->hasChanges($attributeLabel)) { + // prepare and persist the attribue label + $this->persistAttributeLabel($attributeLabel); + } } } diff --git a/src/Observers/AttributeObserver.php b/src/Observers/AttributeObserver.php index f216dd6..b285231 100644 --- a/src/Observers/AttributeObserver.php +++ b/src/Observers/AttributeObserver.php @@ -18,6 +18,7 @@ use TechDivision\Import\Attribute\Utils\ColumnKeys; use TechDivision\Import\Attribute\Utils\MemberNames; use TechDivision\Import\Attribute\Services\AttributeBunchProcessorInterface; +use TechDivision\Import\Observers\StateDetectorInterface; /** * Observer that create's the EAV attribute itself. @@ -42,10 +43,16 @@ class AttributeObserver extends AbstractAttributeImportObserver * Initializes the observer with the passed subject instance. * * @param \TechDivision\Import\Attribute\Services\AttributeBunchProcessorInterface $attributeBunchProcessor The attribute bunch processor instance + * @param \TechDivision\Import\Observers\StateDetectorInterface|null $stateDetector The state detector instance to use */ - public function __construct(AttributeBunchProcessorInterface $attributeBunchProcessor) - { + public function __construct( + AttributeBunchProcessorInterface $attributeBunchProcessor, + ?StateDetectorInterface $stateDetector = null + ) { $this->attributeBunchProcessor = $attributeBunchProcessor; + + // pass the state detector to the parent method + parent::__construct($stateDetector); } /** @@ -64,8 +71,14 @@ protected function process() // prepare the attribue values $attribute = $this->initializeAttribute($this->prepareAttributes()); - // insert the entity and set the entity ID - $this->setLastAttributeId($this->persistAttribute($attribute)); + // query whether or not the attribute has changed and has to be persisted + if ($this->hasChanges($attribute)) { + // insert the entity and set the entity ID + $this->setLastAttributeId($this->persistAttribute($attribute)); + } else { + // make the ID available for subsequent observers, even if nothing has been persisted + $this->setLastAttributeId($attribute[MemberNames::ATTRIBUTE_ID]); + } } /** diff --git a/src/Observers/AttributeOptionObserver.php b/src/Observers/AttributeOptionObserver.php index 366e69e..703771a 100644 --- a/src/Observers/AttributeOptionObserver.php +++ b/src/Observers/AttributeOptionObserver.php @@ -108,8 +108,14 @@ protected function process() // prepare the attribue values $attributeOption = $this->initializeAttribute($this->prepareDynamicAttributes()); - // insert the attribute option and set the option ID - $this->setLastOptionId($this->persistAttributeOption($attributeOption)); + // query whether or not the attribute option has changed and has to be persisted + if ($this->hasChanges($attributeOption)) { + // insert the attribute option and set the option ID + $this->setLastOptionId($this->persistAttributeOption($attributeOption)); + } else { + // make the ID available for subsequent observers, even if nothing has been persisted + $this->setLastOptionId($attributeOption[MemberNames::OPTION_ID]); + } } /** @@ -125,11 +131,11 @@ protected function process() */ protected function mergeEntity(array $entity, array $attr, $changeSetName = null) { - return array_merge( - $entity, - $this->entityMerger ? $this->entityMerger->merge($this, $entity, $attr) : $attr, - array(EntityStatus::MEMBER_NAME => $this->detectState($entity, $attr, $changeSetName)) - ); + // merge the entity with the (optionally cleaned-up) attributes first, so the + // state detector compares against the actually persisted values and NOT + // against raw/default values of columns that have not been touched by the CSV + $merged = array_merge($entity, $this->entityMerger ? $this->entityMerger->merge($this, $entity, $attr) : $attr); + return array_merge($merged, array(EntityStatus::MEMBER_NAME => $this->detectState($entity, $merged, $changeSetName))); } /** diff --git a/src/Observers/AttributeOptionSwatchFileUploadObserver.php b/src/Observers/AttributeOptionSwatchFileUploadObserver.php index 48c1960..2dbe003 100644 --- a/src/Observers/AttributeOptionSwatchFileUploadObserver.php +++ b/src/Observers/AttributeOptionSwatchFileUploadObserver.php @@ -10,6 +10,7 @@ namespace TechDivision\Import\Attribute\Observers; +use TechDivision\Import\Attribute\Utils\ColumnKeys; use TechDivision\Import\Attribute\Utils\ConfigurationKeys; use TechDivision\Import\Attribute\Utils\MemberNames; use TechDivision\Import\Attribute\Utils\SwatchTypes; @@ -41,30 +42,45 @@ protected function process() return; } - // initialize the option swatch attribute - $attributeOptionSwatch = $this->initializeAttribute(array( - MemberNames::OPTION_ID => $this->getLastOptionId() - )); + // skip this step for color swatches and text swatches - read the raw CSV filename reference directly from the + // row, independent of whatever the .update observer did (or, for existing image swatches, deliberately did NOT) + // persist for this row + $type = $this->getValue(ColumnKeys::SWATCH_TYPE); - // skip this step for color swatches and text swatches - if (isset($attributeOptionSwatch[MemberNames::TYPE]) && $attributeOptionSwatch[MemberNames::TYPE] === SwatchTypes::IMAGE) { - // upload the file to the configured directory - $imagePath = $this->getSubject()->uploadFile($attributeOptionSwatch[MemberNames::VALUE]); + if ($type === null || (int) $type !== SwatchTypes::IMAGE) { + return; + } + + $rawValue = $this->getValue(ColumnKeys::SWATCH_VALUE); + + // load the current DB state (swatch_id + the last successfully uploaded, stable path) + $attributeOptionSwatch = $this->initializeAttribute([MemberNames::OPTION_ID => $this->getLastOptionId()]); - // inject the new image path and update the attribute option swatch - $attributeOptionSwatch['value'] = $imagePath; - $this->getAttributeBunchProcessor()->persistAttributeOptionSwatch($attributeOptionSwatch); + // upload the file (or resolve the already up-to-date, stable path for it) + $imagePath = $this->getSubject()->uploadFile($rawValue); - // add debug log entry - $this->getSubject() - ->getSystemLogger() - ->debug( - sprintf( - 'Successfully copied image %s for swatch with id %s', - $imagePath, - $attributeOptionSwatch[MemberNames::SWATCH_ID] - ) - ); + // skip the persist if the row already exists and already has this exact path stored - the genuine "unchanged" + // case, only reliably detectable here because "override-images" makes uploadFile()'s result stable across + // separate runs and the .update observer no longer overwrites it with the (never-matching) raw filename + // beforehand + if (isset($attributeOptionSwatch[MemberNames::SWATCH_ID]) + && isset($attributeOptionSwatch[MemberNames::VALUE]) + && $attributeOptionSwatch[MemberNames::VALUE] === $imagePath) { + return; } + + // inject the new image path and type, then persist the attribute option swatch + $attributeOptionSwatch[MemberNames::TYPE] = SwatchTypes::IMAGE; + $attributeOptionSwatch[MemberNames::VALUE] = $imagePath; + $this->getAttributeBunchProcessor()->persistAttributeOptionSwatch($attributeOptionSwatch); + + // add debug log entry + $this->getSubject()->getSystemLogger()->debug( + sprintf( + 'Successfully copied image %s for swatch with id %s', + $imagePath, + $attributeOptionSwatch[MemberNames::SWATCH_ID] ?? 'n/a' + ) + ); } } diff --git a/src/Observers/AttributeOptionSwatchObserver.php b/src/Observers/AttributeOptionSwatchObserver.php index 8042f31..e1516eb 100644 --- a/src/Observers/AttributeOptionSwatchObserver.php +++ b/src/Observers/AttributeOptionSwatchObserver.php @@ -17,7 +17,10 @@ use TechDivision\Import\Utils\StoreViewCodes; use TechDivision\Import\Attribute\Utils\ColumnKeys; use TechDivision\Import\Attribute\Utils\MemberNames; +use TechDivision\Import\Attribute\Utils\SwatchTypes; use TechDivision\Import\Attribute\Services\AttributeBunchProcessorInterface; +use TechDivision\Import\Dbal\Utils\EntityStatus; +use TechDivision\Import\Observers\StateDetectorInterface; /** * Observer that create's the attribute option swatchs found in the additional CSV file. @@ -42,10 +45,16 @@ class AttributeOptionSwatchObserver extends AbstractAttributeImportObserver * Initializes the observer with the passed subject instance. * * @param \TechDivision\Import\Attribute\Services\AttributeBunchProcessorInterface $attributeBunchProcessor The attribute bunch processor instance + * @param \TechDivision\Import\Observers\StateDetectorInterface|null $stateDetector The state detector instance to use */ - public function __construct(AttributeBunchProcessorInterface $attributeBunchProcessor) - { + public function __construct( + AttributeBunchProcessorInterface $attributeBunchProcessor, + ?StateDetectorInterface $stateDetector = null + ) { $this->attributeBunchProcessor = $attributeBunchProcessor; + + // pass the state detector to the parent method + parent::__construct($stateDetector); } /** @@ -61,10 +70,40 @@ protected function process() // prepare and insert the attribute option swatch if ($attr = $this->prepareAttributes()) { - $this->persistAttributeOptionSwatch($this->initializeAttribute($attr)); + // query whether or not the attribute option swatch has changed and has to be persisted + $initialized = $this->initializeAttribute($attr); + if ($this->shouldPersist($initialized)) { + $this->persistAttributeOptionSwatch($initialized); + } } } + /** + * Queries whether or not the swatch has to be persisted. For existing image swatches, the decision is deferred + * entirely to AttributeOptionSwatchFileUploadObserver, since comparing the raw CSV filename reference (which is all + * this observer has access to) against the already-uploaded DB path here would always appear "changed" and defeat + * the diff - the file upload observer compares against the actually uploaded, stable target path instead. + * + * @param array $entity The (merged) entity to query + * + * @return boolean TRUE if the entity has to be persisted here, else FALSE + */ + protected function shouldPersist(array $entity): bool + { + // nothing to do, if nothing has changed at all + if (!$this->hasChanges($entity)) { + return false; + } + + // for existing (= status update) image swatches, defer to the file upload observer + if ($entity[EntityStatus::MEMBER_NAME] === EntityStatus::STATUS_UPDATE && isset($entity[MemberNames::TYPE]) + && (int)$entity[MemberNames::TYPE] === SwatchTypes::IMAGE) { + return false; + } + + return true; + } + /** * Prepare the attributes of the entity that has to be persisted. * diff --git a/src/Observers/AttributeOptionValueObserver.php b/src/Observers/AttributeOptionValueObserver.php index 29ce339..8dee9df 100644 --- a/src/Observers/AttributeOptionValueObserver.php +++ b/src/Observers/AttributeOptionValueObserver.php @@ -19,6 +19,7 @@ use TechDivision\Import\Attribute\Utils\ColumnKeys; use TechDivision\Import\Attribute\Utils\MemberNames; use TechDivision\Import\Attribute\Services\AttributeBunchProcessorInterface; +use TechDivision\Import\Observers\StateDetectorInterface; /** * Observer that create's the attribute option values found in the additional CSV file. @@ -43,10 +44,16 @@ class AttributeOptionValueObserver extends AbstractAttributeImportObserver * Initializes the observer with the passed subject instance. * * @param \TechDivision\Import\Attribute\Services\AttributeBunchProcessorInterface $attributeBunchProcessor The attribute bunch processor instance + * @param \TechDivision\Import\Observers\StateDetectorInterface|null $stateDetector The state detector instance to use */ - public function __construct(AttributeBunchProcessorInterface $attributeBunchProcessor) - { + public function __construct( + AttributeBunchProcessorInterface $attributeBunchProcessor, + ?StateDetectorInterface $stateDetector = null + ) { $this->attributeBunchProcessor = $attributeBunchProcessor; + + // pass the state detector to the parent method + parent::__construct($stateDetector); } /** @@ -62,7 +69,13 @@ protected function process() // prepare and insert the attribute option value try { - $this->persistAttributeOptionValue($this->initializeAttribute($this->prepareAttributes())); + // initialize the attribute option value + $attributeOptionValue = $this->initializeAttribute($this->prepareAttributes()); + + // query whether or not the attribute option value has changed and has to be persisted + if ($this->hasChanges($attributeOptionValue)) { + $this->persistAttributeOptionValue($attributeOptionValue); + } } catch (\Exception $e) { // prepare a log message $message = sprintf( diff --git a/src/Observers/CatalogAttributeObserver.php b/src/Observers/CatalogAttributeObserver.php index e94a1f3..cc606c9 100644 --- a/src/Observers/CatalogAttributeObserver.php +++ b/src/Observers/CatalogAttributeObserver.php @@ -114,8 +114,13 @@ protected function process() return; } - // initialize and persist the EAV catalog attribute - $this->persistCatalogAttribute($this->initializeAttribute($this->prepareAttributes())); + // initialize the EAV catalog attribute + $catalogAttribute = $this->initializeAttribute($this->prepareAttributes()); + + // query whether or not the EAV catalog attribute has changed and has to be persisted + if ($this->hasChanges($catalogAttribute)) { + $this->persistCatalogAttribute($catalogAttribute); + } } /** @@ -131,11 +136,59 @@ protected function process() */ protected function mergeEntity(array $entity, array $attr, $changeSetName = null) { - return array_merge( - $entity, - $this->entityMerger ? $this->entityMerger->merge($this, $entity, $attr) : $attr, - array(EntityStatus::MEMBER_NAME => $this->detectState($entity, $attr, $changeSetName)) - ); + // merge the entity with the (optionally cleaned-up) attributes first, so the + // state detector compares against the actually persisted values and NOT + // against raw/default values of columns that have not been touched by the CSV + $merged = array_merge($entity, $this->entityMerger ? $this->entityMerger->merge($this, $entity, $attr) : $attr); + + // additional_data is the only non-scalar diff column across all ChangeSet tables - + // the generic array_diff_assoc()-based computer can't compare arrays/objects correctly + // (casts them to the literal string "Array", or fatals on stdClass vs. array) - normalize + // BOTH sides to a canonical JSON string for the comparison only, $merged itself keeps the + // array/object form untouched (serializeAdditionalData() still runs on it afterwards) + $entityForDiff = $entity; + $mergedForDiff = $merged; + if (array_key_exists(MemberNames::ADDITIONAL_DATA, $entityForDiff)) { + $entityForDiff[MemberNames::ADDITIONAL_DATA] = $this->normalizeAdditionalDataForDiff($entityForDiff[MemberNames::ADDITIONAL_DATA]); + } + if (array_key_exists(MemberNames::ADDITIONAL_DATA, $mergedForDiff)) { + $mergedForDiff[MemberNames::ADDITIONAL_DATA] = $this->normalizeAdditionalDataForDiff($mergedForDiff[MemberNames::ADDITIONAL_DATA]); + } + + return array_merge($merged, array(EntityStatus::MEMBER_NAME => $this->detectState($entityForDiff, $mergedForDiff, $changeSetName))); + } + + /** + * Normalizes an additional_data value (string|array|stdClass|null) into a canonical JSON + * string with sorted keys, so it can be compared as a plain scalar by the generic + * array_diff_assoc()-based ChangeSet computer. + * + * @param mixed $value The raw additional_data value (array, stdClass, JSON string or null) + * + * @return string|null The canonical JSON representation, or null if the value is null + */ + protected function normalizeAdditionalDataForDiff($value) + { + if ($value === null) { + return null; + } + + if (is_string($value)) { + // already a JSON string (e.g. untouched raw DB value) - decode/re-encode + // as well, so key-order differences don't cause false positives + $decoded = json_decode($value, true); + $value = is_array($decoded) ? $decoded : $value; + } elseif (is_object($value)) { + $value = (array) $value; + } + + if (is_array($value)) { + ksort($value); + return json_encode($value); + } + + // not array/object/JSON-string - leave as-is (defensive fallback) + return $value; } /** diff --git a/src/Observers/EntityAttributeObserver.php b/src/Observers/EntityAttributeObserver.php index 1b7d867..5dc904f 100644 --- a/src/Observers/EntityAttributeObserver.php +++ b/src/Observers/EntityAttributeObserver.php @@ -160,8 +160,11 @@ protected function process() // prepare the EAV entity attribue values $entityAttribute = $this->initializeAttribute($this->prepareAttributes()); - // insert the EAV entity attribute - $this->persistEntityAttribute($entityAttribute); + // query whether or not the EAV entity attribute has changed and has to be persisted + if ($this->hasChanges($entityAttribute)) { + // insert the EAV entity attribute + $this->persistEntityAttribute($entityAttribute); + } } } @@ -179,11 +182,11 @@ protected function process() */ protected function mergeEntity(array $entity, array $attr, $changeSetName = null) { - return array_merge( - $entity, - $this->entityMerger ? $this->entityMerger->merge($this, $entity, $attr) : $attr, - array(EntityStatus::MEMBER_NAME => $this->detectState($entity, $attr, $changeSetName)) - ); + // merge the entity with the (optionally cleaned-up) attributes first, so the + // state detector compares against the actually persisted values and NOT + // against raw/default values of columns that have not been touched by the CSV + $merged = array_merge($entity, $this->entityMerger ? $this->entityMerger->merge($this, $entity, $attr) : $attr); + return array_merge($merged, array(EntityStatus::MEMBER_NAME => $this->detectState($entity, $merged, $changeSetName))); } /** diff --git a/src/Subjects/OptionSubject.php b/src/Subjects/OptionSubject.php index 06555ea..a4ef1f8 100644 --- a/src/Subjects/OptionSubject.php +++ b/src/Subjects/OptionSubject.php @@ -76,13 +76,42 @@ public function setUp($serial) // initialize media directory => can be absolute or relative if ($this->getConfiguration()->hasParam(FileUploadConfigurationKeys::MEDIA_DIRECTORY)) { + $mediaDirectoryConfigValue = $this->getConfiguration()->getParam( + FileUploadConfigurationKeys::MEDIA_DIRECTORY + ); try { - $this->setMediaDir($this->resolvePath($this->getConfiguration() - ->getParam(FileUploadConfigurationKeys::MEDIA_DIRECTORY))); + $this->setMediaDir($this->resolvePath($mediaDirectoryConfigValue)); } catch (\InvalidArgumentException $iae) { // only if we wanna copy images we need directories if ($this->hasCopyImages()) { - $this->getSystemLogger()->debug($iae->getMessage()); + // media-directory is always a WRITE target (unlike images-file-directory, which is a READ source + // and must genuinely pre-exist) - so unlike resolvePath()'s default behaviour, it's safe and + // correct to create it on demand here, instead of silently ending up with a null media dir. + // + // IMPORTANT: only use the absolute, cwd-prefixed form for the mkdir()/isDir() filesystem check + // itself - setMediaDir() must still receive the ORIGINAL (typically relative) config value, exactly + // like resolvePath() itself would have returned on success. FileUploadTrait::uploadFile() + // unconditionally does ltrim($this->getMediaDir(), '/') - passing an absolute, leading-slash path + // here would have that leading slash stripped there, turning it into a bogus cwd-relative path + // (e.g. "pub/..." becomes "Volumes/workspace/.../pub/..." and gets re-prepended with getcwd() a + // second time by sprintf('%s/%s', ...)). + $absoluteMediaDirectory = $this->getFilesystemAdapter()->isDir($mediaDirectoryConfigValue) + ? $mediaDirectoryConfigValue + : getcwd() . DIRECTORY_SEPARATOR . ltrim( + $mediaDirectoryConfigValue, + '/' + ); + // isDir()-check immediately before mkdir() mirrors the existing pattern in + // FileUploadTrait::uploadFile() - reduces (but doesn't fully eliminate) a "mkdir(): File exists" + // warning when parallel bunch processes race to create the same missing directory + if (!$this->getFilesystemAdapter()->isDir($absoluteMediaDirectory)) { + $this->getFilesystemAdapter()->mkdir($absoluteMediaDirectory); + } + + $this->setMediaDir($mediaDirectoryConfigValue); + $this->getSystemLogger()->debug( + sprintf('Created missing media directory "%s"', $absoluteMediaDirectory) + ); } } }