diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 5a659e9d..8231e8e2 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -16,11 +16,11 @@ include: file: 'github-release.yml' variables: - MAGENTO_PHP_VERSION: '8.1' + MAGENTO_PHP_VERSION: '8.2' magento-module/test/functional: variables: - MIN_PHP_VERSION: '8.1' + MIN_PHP_VERSION: '8.2' DISABLE_DEPENDENCY_AUDIT: "true" release: diff --git a/composer.json b/composer.json index cf1af61d..9d1c073d 100644 --- a/composer.json +++ b/composer.json @@ -3,7 +3,7 @@ "license": "OSL-3.0", "description": "Magento 2 module for Tweakwise integration", "require": { - "php": "^8.1", + "php": "^8.2", "ext-json": "*", "ext-pcre": "*", "ext-simplexml": "*", diff --git a/src/Model/AttributeSlugRepository.php b/src/Model/AttributeSlugRepository.php index a0f06b0a..bbde0dc5 100644 --- a/src/Model/AttributeSlugRepository.php +++ b/src/Model/AttributeSlugRepository.php @@ -81,6 +81,21 @@ public function save(AttributeSlugInterface $attributeSlug): AttributeSlugInterf $baseSlug = $attributeSlug->getSlug(); $storeId = $attributeSlug->getStoreId(); + // If a row for this (attribute, store_id) already exists, reuse its + // primary key so resource->save() issues an UPDATE instead of an INSERT. + try { + $existing = $this->findByAttributeAndStore((string)$attributeSlug->getAttribute(), $storeId); + $attributeSlug->setData('id', (int)$existing->getData('id')); // @phpstan-ignore-line + + // The slug is already persisted correctly; nothing more to do. + if ($existing->getSlug() === $baseSlug) { + return $attributeSlug; + } + } catch (NoSuchEntityException $e) { + // No existing row — we will INSERT below. + } + + // Resolve slug collisions within the same store scope. $newSlug = $baseSlug; $counter = 0; @@ -90,12 +105,14 @@ public function save(AttributeSlugInterface $attributeSlug): AttributeSlugInterf $existingSlug = $this->findBySlug($newSlug, $storeId); if ($existingSlug->getAttribute() === $attributeSlug->getAttribute()) { - return $attributeSlug; + // Same attribute already owns this slug in this store; done. + break; } $counter++; $newSlug = sprintf('%s-%s', $baseSlug, $counter); } catch (NoSuchEntityException $e) { + // Slug is free; use it. break; } } @@ -114,6 +131,27 @@ public function save(AttributeSlugInterface $attributeSlug): AttributeSlugInterf } } + /** + * @param string $attribute + * @param int $storeId + * @return AttributeSlugInterface + * @throws NoSuchEntityException + */ + private function findByAttributeAndStore(string $attribute, int $storeId): AttributeSlugInterface + { + $collection = $this->collectionFactory->create() + ->addFieldToFilter('attribute', $attribute) + ->addFieldToFilter('store_id', (string)$storeId); + + if (!$collection->getSize()) { + throw new NoSuchEntityException(__('No slug found for attribute "%1".', $attribute)); + } + + /** @var AttributeSlug $attributeSlug */ + $attributeSlug = $collection->getFirstItem(); + return $attributeSlug; + } + /** * {@inheritdoc} */ diff --git a/src/Model/Catalog/Layer/Url/Strategy/FilterSlugManager.php b/src/Model/Catalog/Layer/Url/Strategy/FilterSlugManager.php index 0417856f..97a75d8e 100644 --- a/src/Model/Catalog/Layer/Url/Strategy/FilterSlugManager.php +++ b/src/Model/Catalog/Layer/Url/Strategy/FilterSlugManager.php @@ -48,7 +48,7 @@ class FilterSlugManager protected $cache; /** - * @var array + * @var array|null */ protected $lookupTable; @@ -108,6 +108,12 @@ public function getSlugForFilterItem(Item $filterItem): string $attributeSlugEntity->setStoreId($storeId); $savedSlug = $this->attributeSlugRepository->save($attributeSlugEntity); + + // Update the in-memory lookup table immediately so subsequent calls in + // the same request return the cached value without hitting save() again. + $this->lookupTable[$storeId][$attribute] = $savedSlug->getSlug(); + + // Invalidate the shared cache so other processes pick up the new slug. $this->cache->remove(self::CACHE_KEY); return $savedSlug->getSlug(); @@ -211,7 +217,12 @@ private function saveAttributeSlugForOptionLabel( $attributeSlugEntity->setSlug($slug); $attributeSlugEntity->setData('attribute_code', $attributeCode); // @phpstan-ignore-line - $this->attributeSlugRepository->save($attributeSlugEntity); + $savedSlug = $this->attributeSlugRepository->save($attributeSlugEntity); + + // Update the in-memory lookup table immediately so subsequent calls in + // the same request return the cached value without hitting save() again. + $this->lookupTable[$storeId][strtolower($optionLabel)] = $savedSlug->getSlug(); + $this->cache->remove(self::CACHE_KEY); } @@ -289,6 +300,7 @@ protected function loadLookupTable(): array public function truncateSlugTable(): void { $this->attributeSlugRepository->truncateSlugTable(); + $this->lookupTable = null; $this->cache->remove(self::CACHE_KEY); } @@ -304,11 +316,21 @@ public function createFilterSlugByOption(Option $option, int $storeId, ?string $ return; } - $attributeSlugEntity = $this->attributeSlugFactory->create(); - $attributeSlugEntity->setAttribute($option['label']); - $attributeSlugEntity->setStoreId((int)$storeId); - $attributeSlugEntity->setSlug($this->translitUrl->filter($option['label'])); - $attributeSlugEntity->setData('attribute_code', $attributeCode ? $attributeCode : null); // @phpstan-ignore-line - $this->attributeSlugRepository->save($attributeSlugEntity); + // Ensure the in-memory table is initialised before we write to it. + $this->getLookupTable(); + + $attributeSlugEntity = $this->attributeSlugFactory->create(); + $attributeSlugEntity->setAttribute($option['label']); + $attributeSlugEntity->setStoreId((int)$storeId); + $attributeSlugEntity->setSlug($this->translitUrl->filter($option['label'])); + $attributeSlugEntity->setData('attribute_code', $attributeCode ? $attributeCode : null); // @phpstan-ignore-line + + $savedSlug = $this->attributeSlugRepository->save($attributeSlugEntity); + + // Update the in-memory lookup table immediately so subsequent calls in + // the same request return the cached value without hitting save() again. + $this->lookupTable[(int)$storeId][strtolower($option['label'])] = $savedSlug->getSlug(); + + $this->cache->remove(self::CACHE_KEY); } } diff --git a/src/Model/Catalog/Layer/Url/Strategy/QueryParameterStrategy.php b/src/Model/Catalog/Layer/Url/Strategy/QueryParameterStrategy.php index ceb0451f..87c8d862 100644 --- a/src/Model/Catalog/Layer/Url/Strategy/QueryParameterStrategy.php +++ b/src/Model/Catalog/Layer/Url/Strategy/QueryParameterStrategy.php @@ -356,7 +356,6 @@ public function getAttributeSelectUrl(MagentoHttpRequest $request, Item $item): $values[] = $value; } - // @phpstan-ignore-next-line foreach ($values as $key => $value) { $queryUrl = str_replace('__VALUE.' . $key . '__', $value, $queryUrl); } diff --git a/src/Setup/Patch/Schema/FixSlugUniqueIndexOnTweakwiseAttributeSlugTable.php b/src/Setup/Patch/Schema/FixSlugUniqueIndexOnTweakwiseAttributeSlugTable.php new file mode 100644 index 00000000..fd613e19 --- /dev/null +++ b/src/Setup/Patch/Schema/FixSlugUniqueIndexOnTweakwiseAttributeSlugTable.php @@ -0,0 +1,102 @@ +schemaSetup; + $setup->startSetup(); + $connection = $setup->getConnection(); + $tableName = $setup->getTable('tweakwise_attribute_slug'); + + if ($connection->isTableExists($tableName)) { + $this->dropGlobalSlugIndex($tableName); + $this->addCompositeSlugStoreIndex($tableName); + } + + $setup->endSetup(); + + return $this; + } + + /** + * Drops the global unique index on `slug` that was created by InstallSchema. + * + * @param string $tableName + * @return void + */ + private function dropGlobalSlugIndex(string $tableName): void + { + $connection = $this->schemaSetup->getConnection(); + + foreach ($connection->getIndexList($tableName) as $indexName => $indexData) { + $columns = array_map('strtolower', $indexData['COLUMNS_LIST'] ?? []); + if ($columns === ['slug'] && strtoupper($indexData['INDEX_TYPE'] ?? '') === 'UNIQUE') { + $connection->dropIndex($tableName, $indexName); + break; + } + } + } + + /** + * Adds a composite unique index on `(slug, store_id)` if it does not exist yet. + * + * @param string $tableName + * @return void + */ + private function addCompositeSlugStoreIndex(string $tableName): void + { + $setup = $this->schemaSetup; + $connection = $setup->getConnection(); + + foreach ($connection->getIndexList($tableName) as $indexData) { + $columns = array_map('strtolower', $indexData['COLUMNS_LIST'] ?? []); + sort($columns); + if ($columns === ['slug', 'store_id'] && strtoupper($indexData['INDEX_TYPE'] ?? '') === 'UNIQUE') { + return; + } + } + + $connection->addIndex( + $tableName, + $setup->getIdxName($tableName, ['slug', 'store_id'], 'unique'), + ['slug', 'store_id'], + 'unique' + ); + } + + /** + * @return class-string[] + */ + public static function getDependencies() + { + return [ + ChangePrimaryKeyTweakwiseAttributeSlugTable::class, + ]; + } + + /** + * @return string[] + */ + public function getAliases() + { + return []; + } +} diff --git a/tests/Unit/Model/AttributeSlugRepositoryTest.php b/tests/Unit/Model/AttributeSlugRepositoryTest.php new file mode 100644 index 00000000..0e1fb559 --- /dev/null +++ b/tests/Unit/Model/AttributeSlugRepositoryTest.php @@ -0,0 +1,141 @@ +resource = Mockery::mock(AttributeSlugResource::class); + $this->entityFactory = Mockery::mock(AttributeSlugInterfaceFactory::class); + $this->collectionFactory = Mockery::mock(CollectionFactory::class); + $this->searchResultsFactory = Mockery::mock(AttributeSlugSearchResultsInterfaceFactory::class); + $this->collectionProcessor = Mockery::mock(CollectionProcessorInterface::class); + + $this->subject = new AttributeSlugRepository( + $this->resource, + $this->entityFactory, + $this->collectionFactory, + $this->searchResultsFactory, + $this->collectionProcessor, + ); + } + + /** + * @return void + */ + public function testSaveReusesExistingPrimaryKeyAndSkipsWriteWhenSlugIsUnchanged(): void + { + $attributeSlug = Mockery::mock(AttributeSlug::class); + $attributeSlug->shouldReceive('getSlug')->once()->andReturn('color'); + $attributeSlug->shouldReceive('getStoreId')->once()->andReturn(1); + $attributeSlug->shouldReceive('getAttribute')->once()->andReturn('color'); + $attributeSlug->shouldReceive('setData')->once()->with('id', 25); + + $existingByAttributeAndStore = Mockery::mock(AttributeSlug::class); + $existingByAttributeAndStore->shouldReceive('getData')->once()->with('id')->andReturn(25); + $existingByAttributeAndStore->shouldReceive('getSlug')->once()->andReturn('color'); + + $existingCollection = Mockery::mock(Collection::class); + $existingCollection->shouldReceive('addFieldToFilter')->twice()->andReturnSelf(); + $existingCollection->shouldReceive('getSize')->once()->andReturn(1); + $existingCollection->shouldReceive('getFirstItem')->once()->andReturn($existingByAttributeAndStore); + + $this->collectionFactory->shouldReceive('create')->once()->andReturn($existingCollection); + + $this->resource->shouldNotReceive('save'); + + $result = $this->subject->save($attributeSlug); + + $this->assertSame($attributeSlug, $result); + } + + /** + * @return void + */ + public function testSaveReusesExistingPrimaryKeyAndPersistsUpdatedSlug(): void + { + $attributeSlug = Mockery::mock(AttributeSlug::class); + $attributeSlug->shouldReceive('getSlug')->once()->andReturn('color'); + $attributeSlug->shouldReceive('getStoreId')->once()->andReturn(1); + $attributeSlug->shouldReceive('getAttribute')->once()->andReturn('color'); + $attributeSlug->shouldReceive('setData')->once()->with('id', 25); + $attributeSlug->shouldReceive('setSlug')->once()->with('color'); + + $existingByAttributeAndStore = Mockery::mock(AttributeSlug::class); + $existingByAttributeAndStore->shouldReceive('getData')->once()->with('id')->andReturn(25); + $existingByAttributeAndStore->shouldReceive('getSlug')->once()->andReturn('old-color'); + + $existingCollection = Mockery::mock(Collection::class); + $existingCollection->shouldReceive('addFieldToFilter')->twice()->andReturnSelf(); + $existingCollection->shouldReceive('getSize')->once()->andReturn(1); + $existingCollection->shouldReceive('getFirstItem')->once()->andReturn($existingByAttributeAndStore); + + $emptyCollection = Mockery::mock(Collection::class); + $emptyCollection->shouldReceive('addFieldToFilter')->twice()->andReturnSelf(); + $emptyCollection->shouldReceive('getSize')->once()->andReturn(0); + + $this->collectionFactory + ->shouldReceive('create') + ->twice() + ->andReturn($existingCollection, $emptyCollection); + + $this->resource->shouldReceive('save')->once()->with($attributeSlug); + + $result = $this->subject->save($attributeSlug); + + $this->assertSame($attributeSlug, $result); + } +} diff --git a/tests/Unit/Model/Catalog/Layer/Url/Strategy/FilterSlugManagerTest.php b/tests/Unit/Model/Catalog/Layer/Url/Strategy/FilterSlugManagerTest.php new file mode 100644 index 00000000..d4b4163c --- /dev/null +++ b/tests/Unit/Model/Catalog/Layer/Url/Strategy/FilterSlugManagerTest.php @@ -0,0 +1,208 @@ +shouldReceive('getId')->andReturn(1); + + $this->translitUrl = Mockery::mock(TranslitUrl::class); + $this->attributeSlugRepository = Mockery::mock(AttributeSlugRepositoryInterface::class); + $this->attributeSlugFactory = Mockery::mock(AttributeSlugInterfaceFactory::class); + $this->cache = Mockery::mock(CacheInterface::class); + $this->storeManager = Mockery::mock(StoreManagerInterface::class); + $this->storeManager->shouldReceive('getStore')->andReturn($store); + $this->serializer = new Json(); + + $this->subject = new FilterSlugManager( + $this->translitUrl, + $this->attributeSlugRepository, + $this->attributeSlugFactory, + $this->cache, + $this->serializer, + $this->storeManager, + ); + } + + /** + * @return void + */ + public function testGetSlugForFilterItemCachesSavedSlugInMemory(): void + { + $this->cache + ->shouldReceive('load') + ->once() + ->with('tweakwise.slug.lookup') + ->andReturn($this->serializer->serialize([])); + + $this->cache->shouldReceive('remove')->once()->with('tweakwise.slug.lookup'); + + $this->translitUrl->shouldReceive('filter')->once()->with('color')->andReturn('color'); + + $attributeSlugEntity = Mockery::mock(AttributeSlug::class); + $attributeSlugEntity->shouldReceive('setAttribute')->once()->with('color'); + $attributeSlugEntity->shouldReceive('setStoreId')->once()->with(1); + $attributeSlugEntity->shouldReceive('setSlug')->once()->with('color'); + + $this->attributeSlugFactory->shouldReceive('create')->once()->andReturn($attributeSlugEntity); + + $savedSlug = Mockery::mock(AttributeSlugInterface::class); + $savedSlug->shouldReceive('getSlug')->twice()->andReturn('color'); + + $this->attributeSlugRepository + ->shouldReceive('save') + ->once() + ->with($attributeSlugEntity) + ->andReturn($savedSlug); + + $filterAttribute = new DataObject(['title' => 'Color']); + $filterItem = Mockery::mock(Item::class); + $filterItem->shouldReceive('getAttribute')->twice()->andReturn($filterAttribute); + + $this->assertSame('color', $this->subject->getSlugForFilterItem($filterItem)); + $this->assertSame('color', $this->subject->getSlugForFilterItem($filterItem)); + } + + /** + * @return void + */ + public function testCreateFilterSlugByOptionInitializesLookupTableAndUpdatesInMemoryMapping(): void + { + $this->cache + ->shouldReceive('load') + ->once() + ->with('tweakwise.slug.lookup') + ->andReturn($this->serializer->serialize([])); + + $this->cache->shouldReceive('remove')->once()->with('tweakwise.slug.lookup'); + + $this->translitUrl->shouldReceive('filter')->times(2)->with('Blue')->andReturn('blue'); + + $attributeSlugEntity = Mockery::mock(AttributeSlug::class); + $attributeSlugEntity->shouldReceive('setAttribute')->once()->with('Blue'); + $attributeSlugEntity->shouldReceive('setStoreId')->once()->with(1); + $attributeSlugEntity->shouldReceive('setSlug')->once()->with('blue'); + $attributeSlugEntity->shouldReceive('setData')->once()->with('attribute_code', null); + + $this->attributeSlugFactory->shouldReceive('create')->once()->andReturn($attributeSlugEntity); + + $savedSlug = Mockery::mock(AttributeSlugInterface::class); + $savedSlug->shouldReceive('getSlug')->once()->andReturn('blue'); + + $this->attributeSlugRepository + ->shouldReceive('save') + ->once() + ->with($attributeSlugEntity) + ->andReturn($savedSlug); + + $option = Mockery::mock(Option::class); + $option->shouldReceive('offsetGet')->times(5)->with('label')->andReturn('Blue'); + + $this->subject->createFilterSlugByOption($option, 1); + + $this->assertSame('blue', $this->subject->getAttributeBySlug('blue')); + } + + /** + * @return void + */ + public function testTruncateSlugTableReloadsLookupTableFromDatabase(): void + { + $this->cache + ->shouldReceive('load') + ->twice() + ->with('tweakwise.slug.lookup') + ->andReturn( + $this->serializer->serialize([1 => ['color' => 'old-slug']]), + false, + ); + + $this->cache->shouldReceive('remove')->once()->with('tweakwise.slug.lookup'); + + $this->cache + ->shouldReceive('save') + ->once() + ->with($this->serializer->serialize([1 => ['color' => 'new-slug']]), 'tweakwise.slug.lookup'); + + $this->attributeSlugRepository->shouldReceive('truncateSlugTable')->once(); + + $attributeSlug = Mockery::mock(AttributeSlugInterface::class); + $attributeSlug->shouldReceive('getStoreId')->once()->andReturn(1); + $attributeSlug->shouldReceive('getAttribute')->once()->andReturn('color'); + $attributeSlug->shouldReceive('getSlug')->once()->andReturn('new-slug'); + + $searchResults = Mockery::mock(AttributeSlugSearchResultsInterface::class); + $searchResults->shouldReceive('getItems')->once()->andReturn([$attributeSlug]); + + $this->attributeSlugRepository->shouldReceive('getList')->once()->andReturn($searchResults); + + $this->assertSame([1 => ['color' => 'old-slug']], $this->subject->getLookupTable()); + + $this->subject->truncateSlugTable(); + + $this->assertSame([1 => ['color' => 'new-slug']], $this->subject->getLookupTable()); + } +}