From 0c78beee656a2c5912b04eb02a091812490f2cb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20R?= Date: Tue, 11 Dec 2018 14:03:15 +0100 Subject: [PATCH] EZP-29823: Allow to filter Location on translations to not throw when lazy loading Content (#2480) * Created integration test for lazy-loading Location's not avail. Content This happens for the use case where Location has been loaded via SiteAccessAware Repository, but the underlying Content is not available for the languages specified by the SiteAccess prioritized languages list. * EZP-29823: [SPI] Add translation filtering on location load, & expose multi get SPI method while at it * Add useAlwaysAvailable flag on load methods * [API] Expose useAlwaysAvailable on Location load methods * Update eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php Co-Authored-By: andrerom * Apply suggestions from code review Co-Authored-By: andrerom * CS * Cleanup use of generateLanguageMask() * Unused import * CS * CS * TMP Bulk loading API for loadLocationList * Revert "Cleanup use of generateLanguageMask()" This reverts commit dfb56052639684a0a143cb3492ac699ac32e0446. * Revert "TMP Bulk loading API for loadLocationList" This reverts commit a9be8aaff3e3efa4fff9e9bfdbe65b2fa12616f7. * Remove bulk loading locations (to be openened in seperate Pr after this) * CS * Pass useAlwaysAvailable argument to buildLocation() as well Push again to get CI to run. * Fix own review notes --- eZ/Publish/API/Repository/LocationService.php | 10 +- eZ/Publish/API/Repository/Tests/BaseTest.php | 24 +++- .../Repository/Tests/LanguageServiceTest.php | 20 --- .../Repository/Tests/LocationServiceTest.php | 26 +++- .../Persistence/Cache/LocationHandler.php | 84 ++++++----- .../Cache/Tests/LocationHandlerTest.php | 10 +- .../Legacy/Content/Language/MaskGenerator.php | 6 + .../Legacy/Content/Location/Gateway.php | 15 +- .../Location/Gateway/DoctrineDatabase.php | 135 ++++++++++++------ .../Location/Gateway/ExceptionConversion.php | 27 +--- .../Legacy/Content/Location/Handler.php | 22 +-- .../Legacy/Content/TreeHandler.php | 16 ++- .../Location/Gateway/DoctrineDatabaseTest.php | 13 +- .../Gateway/DoctrineDatabaseTrashTest.php | 13 +- .../Content/UrlAlias/UrlAliasHandlerTest.php | 3 +- .../Core/Repository/LocationService.php | 12 +- .../SiteAccessAware/LocationService.php | 10 +- .../Tests/AbstractServiceTest.php | 22 ++- .../Tests/LocationServiceTest.php | 4 +- .../Core/SignalSlot/LocationService.php | 8 +- .../SignalSlot/Tests/LocationServiceTest.php | 4 +- .../storage_engines/legacy/location.yml | 1 + .../Persistence/Content/Location/Handler.php | 8 +- 23 files changed, 292 insertions(+), 201 deletions(-) diff --git a/eZ/Publish/API/Repository/LocationService.php b/eZ/Publish/API/Repository/LocationService.php index 762537eeb4d..e4790605350 100644 --- a/eZ/Publish/API/Repository/LocationService.php +++ b/eZ/Publish/API/Repository/LocationService.php @@ -44,11 +44,12 @@ public function copySubtree(Location $subtree, Location $targetParentLocation); * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException If the specified location is not found * * @param mixed $locationId - * @param string[]|null $prioritizedLanguages Used as prioritized language code on translated properties of returned object. + * @param string[]|null $prioritizedLanguages Filter on and use as prioritized language code on translated properties of returned object. + * @param bool|null $useAlwaysAvailable Respect always available flag on content when filtering on $prioritizedLanguages. * * @return \eZ\Publish\API\Repository\Values\Content\Location */ - public function loadLocation($locationId, array $prioritizedLanguages = null); + public function loadLocation($locationId, array $prioritizedLanguages = null, bool $useAlwaysAvailable = null); /** * Loads a location object from its $remoteId. @@ -57,11 +58,12 @@ public function loadLocation($locationId, array $prioritizedLanguages = null); * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException If the specified location is not found * * @param string $remoteId - * @param string[]|null $prioritizedLanguages Used as prioritized language code on translated properties of returned object. + * @param string[]|null $prioritizedLanguages Filter on and use as prioritized language code on translated properties of returned object. + * @param bool|null $useAlwaysAvailable Respect always available flag on content when filtering on $prioritizedLanguages. * * @return \eZ\Publish\API\Repository\Values\Content\Location */ - public function loadLocationByRemoteId($remoteId, array $prioritizedLanguages = null); + public function loadLocationByRemoteId($remoteId, array $prioritizedLanguages = null, bool $useAlwaysAvailable = null); /** * Loads the locations for the given content object. diff --git a/eZ/Publish/API/Repository/Tests/BaseTest.php b/eZ/Publish/API/Repository/Tests/BaseTest.php index edd9ae61547..9d5b231640e 100644 --- a/eZ/Publish/API/Repository/Tests/BaseTest.php +++ b/eZ/Publish/API/Repository/Tests/BaseTest.php @@ -11,7 +11,7 @@ use Doctrine\DBAL\Connection; use eZ\Publish\API\Repository\Exceptions\ContentFieldValidationException; use eZ\Publish\API\Repository\Tests\PHPUnitConstraint\ValidationErrorOccurs as PHPUnitConstraintValidationErrorOccurs; -use eZ\Publish\API\Repository\Values\Content\Location; +use eZ\Publish\API\Repository\Values\Content\Language; use EzSystems\EzPlatformSolrSearchEngine\Tests\SetupFactory\LegacySetupFactory as LegacySolrSetupFactory; use PHPUnit\Framework\TestCase; use eZ\Publish\API\Repository\Repository; @@ -716,4 +716,26 @@ protected function createFolder(array $names, $parentLocationId) return $contentService->publishVersion($contentDraft->versionInfo); } + + /** + * Add new Language to the Repository. + * + * @param string $languageCode + * @param string $name + * @param bool $enabled + * + * @return \eZ\Publish\API\Repository\Values\Content\Language + */ + protected function createLanguage(string $languageCode, string $name, bool $enabled = true): Language + { + $repository = $this->getRepository(false); + + $languageService = $repository->getContentLanguageService(); + $languageStruct = $languageService->newLanguageCreateStruct(); + $languageStruct->name = $name; + $languageStruct->languageCode = $languageCode; + $languageStruct->enabled = $enabled; + + return $languageService->createLanguage($languageStruct); + } } diff --git a/eZ/Publish/API/Repository/Tests/LanguageServiceTest.php b/eZ/Publish/API/Repository/Tests/LanguageServiceTest.php index 84ad00f6bc8..76d5524a584 100644 --- a/eZ/Publish/API/Repository/Tests/LanguageServiceTest.php +++ b/eZ/Publish/API/Repository/Tests/LanguageServiceTest.php @@ -548,26 +548,6 @@ public function testGetDefaultLanguageCode() ); } - /** - * Helper method that creates a new language test fixture in the - * API implementation under test. - * - * @return \eZ\Publish\API\Repository\Values\Content\Language - */ - private function createLanguage() - { - $repository = $this->getRepository(); - - $languageService = $repository->getContentLanguageService(); - $languageCreate = $languageService->newLanguageCreateStruct(); - - $languageCreate->enabled = false; - $languageCreate->name = 'English'; - $languageCreate->languageCode = 'eng-US'; - - return $languageService->createLanguage($languageCreate); - } - /** * Test for the createLanguage() method. * diff --git a/eZ/Publish/API/Repository/Tests/LocationServiceTest.php b/eZ/Publish/API/Repository/Tests/LocationServiceTest.php index c26d93a5548..c0fc552b1ef 100644 --- a/eZ/Publish/API/Repository/Tests/LocationServiceTest.php +++ b/eZ/Publish/API/Repository/Tests/LocationServiceTest.php @@ -478,11 +478,7 @@ public function testLoadLocationPrioritizedLanguagesFallback() $repository = $this->getRepository(); // Add a language - $languageService = $repository->getContentLanguageService(); - $languageStruct = $languageService->newLanguageCreateStruct(); - $languageStruct->name = 'Norsk'; - $languageStruct->languageCode = 'nor-NO'; - $languageService->createLanguage($languageStruct); + $this->createLanguage('nor-NO', 'Norsk'); $locationService = $repository->getLocationService(); $contentService = $repository->getContentService(); @@ -495,7 +491,7 @@ public function testLoadLocationPrioritizedLanguagesFallback() $draft = $contentService->updateContent($draft->getVersionInfo(), $struct); $contentService->publishVersion($draft->getVersionInfo()); - // Load with prioritc language (fallback will be the old one) + // Load with priority language (fallback will be the old one) $location = $locationService->loadLocation(5, ['nor-NO']); $this->assertInstanceOf( @@ -513,6 +509,24 @@ public function testLoadLocationPrioritizedLanguagesFallback() $this->assertEquals($content->getVersionInfo()->getName('eng-US'), 'Users'); } + /** + * Test that accessing lazy-loaded Content without a translation in the specific + * not available language throws NotFoundException. + */ + public function testLoadLocationThrowsNotFoundExceptionForNotAvailableContent(): void + { + $repository = $this->getRepository(); + + $locationService = $repository->getLocationService(); + + $this->createLanguage('pol-PL', 'Polski'); + + $this->expectException(NotFoundException::class); + + // Note: relying on existing database fixtures to make test case more readable + $locationService->loadLocation(60, ['pol-PL']); + } + /** * Test for the loadLocation() method. * diff --git a/eZ/Publish/Core/Persistence/Cache/LocationHandler.php b/eZ/Publish/Core/Persistence/Cache/LocationHandler.php index 711b84be958..6bfd7882372 100644 --- a/eZ/Publish/Core/Persistence/Cache/LocationHandler.php +++ b/eZ/Publish/Core/Persistence/Cache/LocationHandler.php @@ -21,15 +21,19 @@ class LocationHandler extends AbstractHandler implements LocationHandlerInterfac /** * {@inheritdoc} */ - public function load($locationId) + public function load($locationId, array $translations = null, bool $useAlwaysAvailable = true) { - $cacheItem = $this->cache->getItem("ez-location-${locationId}"); + $translationsKey = $this->getCacheTranslationKey($translations, $useAlwaysAvailable); + $cacheItem = $this->cache->getItem("ez-location-${locationId}-${translationsKey}"); if ($cacheItem->isHit()) { return $cacheItem->get(); } - $this->logger->logCall(__METHOD__, array('location' => $locationId)); - $location = $this->persistenceHandler->locationHandler()->load($locationId); + $this->logger->logCall( + __METHOD__, + ['location' => $locationId, 'translations' => $translations, 'always-available' => $useAlwaysAvailable] + ); + $location = $this->persistenceHandler->locationHandler()->load($locationId, $translations, $useAlwaysAvailable); $cacheItem->set($location); $cacheItem->tag($this->getCacheTags($location)); @@ -53,9 +57,9 @@ public function loadSubtreeIds($locationId) $cacheItem->set($locationIds); $cacheTags = ['location-' . $locationId, 'location-path-' . $locationId]; - foreach ($locationIds as $locationId) { - $cacheTags[] = 'location-' . $locationId; - $cacheTags[] = 'location-path-' . $locationId; + foreach ($locationIds as $id) { + $cacheTags[] = 'location-' . $id; + $cacheTags[] = 'location-path-' . $id; } $cacheItem->tag($cacheTags); $this->cache->save($cacheItem); @@ -120,15 +124,19 @@ public function loadParentLocationsForDraftContent($contentId) /** * {@inheritdoc} */ - public function loadByRemoteId($remoteId) + public function loadByRemoteId($remoteId, array $translations = null, bool $useAlwaysAvailable = true) { - $cacheItem = $this->cache->getItem("ez-location-${remoteId}-by-remoteid"); + $translationsKey = $this->getCacheTranslationKey($translations, $useAlwaysAvailable); + $cacheItem = $this->cache->getItem("ez-location-remoteid-${remoteId}-${translationsKey}"); if ($cacheItem->isHit()) { return $cacheItem->get(); } - $this->logger->logCall(__METHOD__, array('location' => $remoteId)); - $location = $this->persistenceHandler->locationHandler()->loadByRemoteId($remoteId); + $this->logger->logCall( + __METHOD__, + ['location' => $remoteId, 'translations' => $translations, 'always-available' => $useAlwaysAvailable] + ); + $location = $this->persistenceHandler->locationHandler()->loadByRemoteId($remoteId, $translations, $useAlwaysAvailable); $cacheItem->set($location); $cacheItem->tag($this->getCacheTags($location)); @@ -280,27 +288,6 @@ public function changeMainLocation($contentId, $locationId) $this->cache->invalidateTags(['content-' . $contentId]); } - /** - * Return relevant content and location tags so cache can be purged reliably. - * - * @param Location $location - * @param array $tags Optional, can be used to specify additional tags. - * - * @return array - */ - private function getCacheTags(Location $location, $tags = []) - { - $tags[] = 'content-' . $location->contentId; - $tags[] = 'location-' . $location->id; - $tags[] = 'location-data-' . $location->id; - foreach (explode('/', trim($location->pathString, '/')) as $pathId) { - $tags[] = 'location-path-' . $pathId; - $tags[] = 'location-path-data-' . $pathId; - } - - return $tags; - } - /** * Get the total number of all existing Locations. Can be combined with loadAllLocations. * @@ -327,4 +314,37 @@ public function loadAllLocations($offset, $limit) return $this->persistenceHandler->locationHandler()->loadAllLocations($offset, $limit); } + + /** + * Return relevant content and location tags so cache can be purged reliably. + * + * @param \eZ\Publish\SPI\Persistence\Content\Location $location + * @param array $tags Optional, can be used to specify additional tags. + * + * @return array + */ + private function getCacheTags(Location $location, $tags = []) + { + $tags[] = 'content-' . $location->contentId; + $tags[] = 'location-' . $location->id; + $tags[] = 'location-data-' . $location->id; + foreach (explode('/', trim($location->pathString, '/')) as $pathId) { + $tags[] = 'location-path-' . $pathId; + $tags[] = 'location-path-data-' . $pathId; + } + + return $tags; + } + + private function getCacheTranslationKey(array $translations = null, bool $useAlwaysAvailable = true): string + { + if (empty($translations)) { + return (int)$useAlwaysAvailable; + } + + // Sort array as we don't care about order in location handler usage & want to optimize for cache hits. + sort($translations); + + return implode('|', $translations) . '|' . (int)$useAlwaysAvailable; + } } diff --git a/eZ/Publish/Core/Persistence/Cache/Tests/LocationHandlerTest.php b/eZ/Publish/Core/Persistence/Cache/Tests/LocationHandlerTest.php index 340daba0d5f..309687527bf 100644 --- a/eZ/Publish/Core/Persistence/Cache/Tests/LocationHandlerTest.php +++ b/eZ/Publish/Core/Persistence/Cache/Tests/LocationHandlerTest.php @@ -30,7 +30,7 @@ public function getHandlerClassName(): string public function providerForUnCachedMethods(): array { - // string $method, array $arguments, array? $tags, string? $key + // string $method, array $arguments, array? $tags, string? $key, mixed? $returnValue return [ ['copySubtree', [12, 45]], ['move', [12, 45], ['location-path-12']], @@ -51,14 +51,16 @@ public function providerForCachedLoadMethods(): array { $location = new Location(['id' => 12]); - // string $method, array $arguments, string $key, mixed? $data + // string $method, array $arguments, string $key, mixed? $data, bool? $multi = false return [ - ['load', [12], 'ez-location-12', $location], + ['load', [12], 'ez-location-12-1', $location], + ['load', [12, ['eng-GB', 'bra-PG'], false], 'ez-location-12-bra-PG|eng-GB|0', $location], ['loadSubtreeIds', [12], 'ez-location-subtree-12', [33, 44]], ['loadLocationsByContent', [4, 12], 'ez-content-locations-4-root-12', [$location]], ['loadLocationsByContent', [4], 'ez-content-locations-4', [$location]], ['loadParentLocationsForDraftContent', [4], 'ez-content-locations-4-parentForDraft', [$location]], - ['loadByRemoteId', ['34fe5y4'], 'ez-location-34fe5y4-by-remoteid', $location], + ['loadByRemoteId', ['34fe5y4'], 'ez-location-remoteid-34fe5y4-1', $location], + ['loadByRemoteId', ['34fe5y4', ['eng-GB', 'arg-ES']], 'ez-location-remoteid-34fe5y4-arg-ES|eng-GB|1', $location], ]; } } diff --git a/eZ/Publish/Core/Persistence/Legacy/Content/Language/MaskGenerator.php b/eZ/Publish/Core/Persistence/Legacy/Content/Language/MaskGenerator.php index a2431c5fdd1..fb1bbea3b01 100644 --- a/eZ/Publish/Core/Persistence/Legacy/Content/Language/MaskGenerator.php +++ b/eZ/Publish/Core/Persistence/Legacy/Content/Language/MaskGenerator.php @@ -9,6 +9,7 @@ namespace eZ\Publish\Core\Persistence\Legacy\Content\Language; use eZ\Publish\SPI\Persistence\Content\Language\Handler as LanguageHandler; +use RuntimeException; /** * Language MaskGenerator. @@ -48,6 +49,11 @@ public function generateLanguageMask(array $languages) } foreach ($languages as $language => $value) { + if (is_int($language)) { + throw new RuntimeException( + "Expected flipped array with language codes as keys, got int key: $language" + ); + } $mask |= $this->languageHandler->loadByLanguageCode($language)->id; } diff --git a/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway.php b/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway.php index 4d0069ff20d..0464cd9031c 100644 --- a/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway.php +++ b/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway.php @@ -33,27 +33,28 @@ abstract class Gateway /** * Returns an array with basic node data. * - * We might want to cache this, since this method is used by about every - * method in the location handler. - * - * @todo optimize + * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException * * @param mixed $nodeId + * @param string[]|null $translations + * @param bool $useAlwaysAvailable Respect always available flag on content when filtering on $translations. * * @return array */ - abstract public function getBasicNodeData($nodeId); + abstract public function getBasicNodeData($nodeId, array $translations = null, bool $useAlwaysAvailable = true); /** * Returns an array with basic node data for the node with $remoteId. * - * @todo optimize + * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException * * @param mixed $remoteId + * @param string[]|null $translations + * @param bool $useAlwaysAvailable Respect always available flag on content when filtering on $translations. * * @return array */ - abstract public function getBasicNodeDataByRemoteId($remoteId); + abstract public function getBasicNodeDataByRemoteId($remoteId, array $translations = null, bool $useAlwaysAvailable = true); /** * Loads data for all Locations for $contentId, optionally only in the diff --git a/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php b/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php index 26fe4e0651e..e64ed36877f 100644 --- a/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php +++ b/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php @@ -8,6 +8,9 @@ */ namespace eZ\Publish\Core\Persistence\Legacy\Content\Location\Gateway; +use Doctrine\DBAL\FetchMode; +use Doctrine\DBAL\Query\QueryBuilder; +use eZ\Publish\Core\Persistence\Legacy\Content\Language\MaskGenerator; use eZ\Publish\Core\Persistence\Legacy\Content\Location\Gateway; use eZ\Publish\Core\Persistence\Database\DatabaseHandler; use eZ\Publish\Core\Persistence\Database\SelectQuery; @@ -45,45 +48,37 @@ class DoctrineDatabase extends Gateway */ protected $connection; + /** + * Language mask generator. + * + * @var \eZ\Publish\Core\Persistence\Legacy\Content\Language\MaskGenerator + */ + protected $languageMaskGenerator; + /** * Construct from database handler. * * @param \eZ\Publish\Core\Persistence\Database\DatabaseHandler $handler + * @param \eZ\Publish\Core\Persistence\Legacy\Content\Language\MaskGenerator $languageMaskGenerator */ - public function __construct(DatabaseHandler $handler) + public function __construct(DatabaseHandler $handler, MaskGenerator $languageMaskGenerator) { $this->handler = $handler; $this->connection = $handler->getConnection(); + $this->languageMaskGenerator = $languageMaskGenerator; } /** - * Returns an array with basic node data. - * - * We might want to cache this, since this method is used by about every - * method in the location handler. - * - * @todo optimize - * - * @param mixed $nodeId - * - * @return array + * {@inheritdoc} */ - public function getBasicNodeData($nodeId) + public function getBasicNodeData($nodeId, array $translations = null, bool $useAlwaysAvailable = true) { - $query = $this->handler->createSelectQuery(); - $query - ->select('*') - ->from($this->handler->quoteTable('ezcontentobject_tree')) - ->where( - $query->expr->eq( - $this->handler->quoteColumn('node_id'), - $query->bindValue($nodeId) - ) - ); - $statement = $query->prepare(); - $statement->execute(); + $q = $this->createNodeQueryBuilder($translations, $useAlwaysAvailable); + $q->where( + $q->expr()->eq('t.node_id', $q->createNamedParameter($nodeId, PDO::PARAM_INT)) + ); - if ($row = $statement->fetch(\PDO::FETCH_ASSOC)) { + if ($row = $q->execute()->fetch(FetchMode::ASSOCIATIVE)) { return $row; } @@ -91,30 +86,16 @@ public function getBasicNodeData($nodeId) } /** - * Returns an array with basic node data. - * - * @todo optimize - * - * @param mixed $remoteId - * - * @return array + * {@inheritdoc} */ - public function getBasicNodeDataByRemoteId($remoteId) + public function getBasicNodeDataByRemoteId($remoteId, array $translations = null, bool $useAlwaysAvailable = true) { - $query = $this->handler->createSelectQuery(); - $query - ->select('*') - ->from($this->handler->quoteTable('ezcontentobject_tree')) - ->where( - $query->expr->eq( - $this->handler->quoteColumn('remote_id'), - $query->bindValue($remoteId) - ) - ); - $statement = $query->prepare(); - $statement->execute(); + $q = $this->createNodeQueryBuilder($translations, $useAlwaysAvailable); + $q->where( + $q->expr()->eq('t.remote_id', $q->createNamedParameter($remoteId, PDO::PARAM_STR)) + ); - if ($row = $statement->fetch(\PDO::FETCH_ASSOC)) { + if ($row = $q->execute()->fetch(FetchMode::ASSOCIATIVE)) { return $row; } @@ -1546,6 +1527,8 @@ public function loadAllLocationsData($offset, $limit) /** * Get Query Builder for fetching data of all Locations except the Root node. * + * @todo Align with createNodeQueryBuilder, removing the need for both(?) + * * @param array $columns list of columns to fetch * * @return \Doctrine\DBAL\Query\QueryBuilder @@ -1561,4 +1544,64 @@ private function getAllLocationsQueryBuilder(array $columns) return $query; } + + /** + * Create QueryBuilder for selecting node data. + * + * @param array|null $translations Filters on language mask of content if provided. + * @param bool $useAlwaysAvailable Respect always available flag on content when filtering on $translations. + * + * @return \Doctrine\DBAL\Query\QueryBuilder + */ + private function createNodeQueryBuilder(array $translations = null, bool $useAlwaysAvailable = true): QueryBuilder + { + $queryBuilder = $this->connection->createQueryBuilder(); + $queryBuilder + ->select('t.*') + ->from('ezcontentobject_tree', 't'); + + if (!empty($translations)) { + $dbPlatform = $this->connection->getDatabasePlatform(); + $expr = $queryBuilder->expr(); + $mask = $this->generateLanguageMaskFromLanguageCodes($translations, $useAlwaysAvailable); + + $queryBuilder->innerJoin( + 't', + 'ezcontentobject', + 'c', + $expr->andX( + $expr->eq('t.contentobject_id', 'c.id'), + $expr->gt( + $dbPlatform->getBitAndComparisonExpression('c.language_mask', $mask), + 0 + ) + ) + ); + } + + return $queryBuilder; + } + + /** + * Generates a language mask for $translations argument. + * + * @todo Move logic to languageMaskGenerator in master. + */ + private function generateLanguageMaskFromLanguageCodes(array $translations, bool $useAlwaysAvailable = true): int + { + $languages = []; + foreach ($translations as $translation) { + if (isset($languages[$translation])) { + continue; + } + + $languages[$translation] = true; + } + + if ($useAlwaysAvailable) { + $languages['always-available'] = true; + } + + return $this->languageMaskGenerator->generateLanguageMask($languages); + } } diff --git a/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php b/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php index 1815383a7e1..9a044cd4f2d 100644 --- a/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php +++ b/eZ/Publish/Core/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php @@ -40,21 +40,12 @@ public function __construct(Gateway $innerGateway) } /** - * Returns an array with basic node data. - * - * We might want to cache this, since this method is used by about every - * method in the location handler. - * - * @todo optimize - * - * @param mixed $nodeId - * - * @return array + * {@inheritdoc} */ - public function getBasicNodeData($nodeId) + public function getBasicNodeData($nodeId, array $translations = null, bool $useAlwaysAvailable = true) { try { - return $this->innerGateway->getBasicNodeData($nodeId); + return $this->innerGateway->getBasicNodeData($nodeId, $translations, $useAlwaysAvailable); } catch (DBALException $e) { throw new RuntimeException('Database error', 0, $e); } catch (PDOException $e) { @@ -63,18 +54,12 @@ public function getBasicNodeData($nodeId) } /** - * Returns an array with basic node data for the node with $remoteId. - * - * @todo optimize - * - * @param mixed $remoteId - * - * @return array + * {@inheritdoc} */ - public function getBasicNodeDataByRemoteId($remoteId) + public function getBasicNodeDataByRemoteId($remoteId, array $translations = null, bool $useAlwaysAvailable = true) { try { - return $this->innerGateway->getBasicNodeDataByRemoteId($remoteId); + return $this->innerGateway->getBasicNodeDataByRemoteId($remoteId, $translations, $useAlwaysAvailable); } catch (DBALException $e) { throw new RuntimeException('Database error', 0, $e); } catch (PDOException $e) { diff --git a/eZ/Publish/Core/Persistence/Legacy/Content/Location/Handler.php b/eZ/Publish/Core/Persistence/Legacy/Content/Location/Handler.php index becbf3168a7..82810dd7352 100644 --- a/eZ/Publish/Core/Persistence/Legacy/Content/Location/Handler.php +++ b/eZ/Publish/Core/Persistence/Legacy/Content/Location/Handler.php @@ -98,15 +98,11 @@ protected function getParentPathString($pathString) } /** - * Loads the data for the location identified by $locationId. - * - * @param int $locationId - * - * @return \eZ\Publish\SPI\Persistence\Content\Location + * {@inheritdoc} */ - public function load($locationId) + public function load($locationId, array $translations = null, bool $useAlwaysAvailable = true) { - return $this->treeHandler->loadLocation($locationId); + return $this->treeHandler->loadLocation($locationId, $translations, $useAlwaysAvailable); } /** @@ -122,17 +118,11 @@ public function loadSubtreeIds($locationId) } /** - * Loads the data for the location identified by $remoteId. - * - * @param string $remoteId - * - * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException - * - * @return \eZ\Publish\SPI\Persistence\Content\Location + * {@inheritdoc} */ - public function loadByRemoteId($remoteId) + public function loadByRemoteId($remoteId, array $translations = null, bool $useAlwaysAvailable = true) { - $data = $this->locationGateway->getBasicNodeDataByRemoteId($remoteId); + $data = $this->locationGateway->getBasicNodeDataByRemoteId($remoteId, $translations, $useAlwaysAvailable); return $this->locationMapper->createLocationFromRow($data); } diff --git a/eZ/Publish/Core/Persistence/Legacy/Content/TreeHandler.php b/eZ/Publish/Core/Persistence/Legacy/Content/TreeHandler.php index 45f047b372b..8666560378a 100644 --- a/eZ/Publish/Core/Persistence/Legacy/Content/TreeHandler.php +++ b/eZ/Publish/Core/Persistence/Legacy/Content/TreeHandler.php @@ -80,6 +80,8 @@ public function __construct( * * @param int|string $contentId * + * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException + * * @return \eZ\Publish\SPI\Persistence\Content\ContentInfo */ public function loadContentInfo($contentId) @@ -150,12 +152,16 @@ function ($row) use ($contentId) { * Loads the data for the location identified by $locationId. * * @param int $locationId + * @param string[]|null $translations If set, NotFound is thrown if content is not in given translation. + * @param bool $useAlwaysAvailable Respect always available flag on content, where main language is valid translation fallback. + * + * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException * * @return \eZ\Publish\SPI\Persistence\Content\Location */ - public function loadLocation($locationId) + public function loadLocation($locationId, array $translations = null, bool $useAlwaysAvailable = true) { - $data = $this->locationGateway->getBasicNodeData($locationId); + $data = $this->locationGateway->getBasicNodeData($locationId, $translations, $useAlwaysAvailable); return $this->locationMapper->createLocationFromRow($data); } @@ -171,6 +177,8 @@ public function loadLocation($locationId) * * @param mixed $locationId * + * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException + * * @return bool */ public function removeSubtree($locationId) @@ -209,6 +217,8 @@ public function removeSubtree($locationId) * * @param mixed $locationId * @param mixed $sectionId + * + * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException */ public function setSectionForSubtree($locationId, $sectionId) { @@ -224,6 +234,8 @@ public function setSectionForSubtree($locationId, $sectionId) * * @param mixed $contentId * @param mixed $locationId + * + * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException */ public function changeMainLocation($contentId, $locationId) { diff --git a/eZ/Publish/Core/Persistence/Legacy/Tests/Content/Location/Gateway/DoctrineDatabaseTest.php b/eZ/Publish/Core/Persistence/Legacy/Tests/Content/Location/Gateway/DoctrineDatabaseTest.php index 65b5e35b632..1cd732f73a5 100644 --- a/eZ/Publish/Core/Persistence/Legacy/Tests/Content/Location/Gateway/DoctrineDatabaseTest.php +++ b/eZ/Publish/Core/Persistence/Legacy/Tests/Content/Location/Gateway/DoctrineDatabaseTest.php @@ -8,7 +8,7 @@ */ namespace eZ\Publish\Core\Persistence\Legacy\Tests\Content\Location\Gateway; -use eZ\Publish\Core\Persistence\Legacy\Tests\TestCase; +use eZ\Publish\Core\Persistence\Legacy\Tests\Content\LanguageAwareTestCase; use eZ\Publish\SPI\Persistence\Content\Location; use eZ\Publish\SPI\Persistence\Content\Location\CreateStruct; use eZ\Publish\Core\Persistence\Legacy\Content\Location\Gateway\DoctrineDatabase; @@ -17,7 +17,7 @@ /** * Test case for eZ\Publish\Core\Persistence\Legacy\Content\Location\Gateway\DoctrineDatabase. */ -class DoctrineDatabaseTest extends TestCase +class DoctrineDatabaseTest extends LanguageAwareTestCase { protected function getLocationGateway() { @@ -25,14 +25,7 @@ protected function getLocationGateway() return new DoctrineDatabase( $dbHandler, - $this - ->getMockBuilder('eZ\\Publish\\Core\\Persistence\\Legacy\\Content\\Location\\Gateway\\CriteriaConverter') - ->disableOriginalConstructor() - ->getMock(), - $this - ->getMockBuilder('eZ\\Publish\\Core\\Persistence\\Legacy\\Content\\Location\\Gateway\\SortClauseConverter') - ->disableOriginalConstructor() - ->getMock() + $this->getLanguageMaskGenerator() ); } diff --git a/eZ/Publish/Core/Persistence/Legacy/Tests/Content/Location/Gateway/DoctrineDatabaseTrashTest.php b/eZ/Publish/Core/Persistence/Legacy/Tests/Content/Location/Gateway/DoctrineDatabaseTrashTest.php index 151ab100d8f..758123b587d 100644 --- a/eZ/Publish/Core/Persistence/Legacy/Tests/Content/Location/Gateway/DoctrineDatabaseTrashTest.php +++ b/eZ/Publish/Core/Persistence/Legacy/Tests/Content/Location/Gateway/DoctrineDatabaseTrashTest.php @@ -8,7 +8,7 @@ */ namespace eZ\Publish\Core\Persistence\Legacy\Tests\Content\Location\Gateway; -use eZ\Publish\Core\Persistence\Legacy\Tests\TestCase; +use eZ\Publish\Core\Persistence\Legacy\Tests\Content\LanguageAwareTestCase; use eZ\Publish\Core\Persistence\Legacy\Content\Location\Gateway\DoctrineDatabase; use eZ\Publish\API\Repository\Values\Content\Query\SortClause; use eZ\Publish\API\Repository\Values\Content\Query; @@ -16,7 +16,7 @@ /** * Test case for eZ\Publish\Core\Persistence\Legacy\Content\Location\Gateway\DoctrineDatabase. */ -class DoctrineDatabaseTrashTest extends TestCase +class DoctrineDatabaseTrashTest extends LanguageAwareTestCase { protected function getLocationGateway() { @@ -24,14 +24,7 @@ protected function getLocationGateway() return new DoctrineDatabase( $dbHandler, - $this - ->getMockBuilder('eZ\\Publish\\Core\\Persistence\\Legacy\\Content\\Location\\Gateway\\CriteriaConverter') - ->disableOriginalConstructor() - ->getMock(), - $this - ->getMockBuilder('eZ\\Publish\\Core\\Persistence\\Legacy\\Content\\Location\\Gateway\\SortClauseConverter') - ->disableOriginalConstructor() - ->getMock() + $this->getLanguageMaskGenerator() ); } diff --git a/eZ/Publish/Core/Persistence/Legacy/Tests/Content/UrlAlias/UrlAliasHandlerTest.php b/eZ/Publish/Core/Persistence/Legacy/Tests/Content/UrlAlias/UrlAliasHandlerTest.php index 9c6947a9262..46d115e81d1 100644 --- a/eZ/Publish/Core/Persistence/Legacy/Tests/Content/UrlAlias/UrlAliasHandlerTest.php +++ b/eZ/Publish/Core/Persistence/Legacy/Tests/Content/UrlAlias/UrlAliasHandlerTest.php @@ -5400,7 +5400,8 @@ protected function getLocationGateway() { if (!isset($this->locationGateway)) { $this->locationGateway = new DoctrineDatabaseLocation( - $this->getDatabaseHandler() + $this->getDatabaseHandler(), + $this->getLanguageMaskGenerator() ); } diff --git a/eZ/Publish/Core/Repository/LocationService.php b/eZ/Publish/Core/Repository/LocationService.php index 1eabe64930c..e643fa72ec2 100644 --- a/eZ/Publish/Core/Repository/LocationService.php +++ b/eZ/Publish/Core/Repository/LocationService.php @@ -202,10 +202,10 @@ public function copySubtree(APILocation $subtree, APILocation $targetParentLocat /** * {@inheritdoc} */ - public function loadLocation($locationId, array $prioritizedLanguages = null) + public function loadLocation($locationId, array $prioritizedLanguages = null, bool $useAlwaysAvailable = null) { - $spiLocation = $this->persistenceHandler->locationHandler()->load($locationId); - $location = $this->domainMapper->buildLocation($spiLocation, $prioritizedLanguages ?: []); + $spiLocation = $this->persistenceHandler->locationHandler()->load($locationId, $prioritizedLanguages, $useAlwaysAvailable ?? true); + $location = $this->domainMapper->buildLocation($spiLocation, $prioritizedLanguages ?: [], $useAlwaysAvailable ?? true); if (!$this->repository->canUser('content', 'read', $location->getContentInfo(), $location)) { throw new UnauthorizedException('content', 'read', ['locationId' => $location->id]); } @@ -216,14 +216,14 @@ public function loadLocation($locationId, array $prioritizedLanguages = null) /** * {@inheritdoc} */ - public function loadLocationByRemoteId($remoteId, array $prioritizedLanguages = null) + public function loadLocationByRemoteId($remoteId, array $prioritizedLanguages = null, bool $useAlwaysAvailable = null) { if (!is_string($remoteId)) { throw new InvalidArgumentValue('remoteId', $remoteId); } - $spiLocation = $this->persistenceHandler->locationHandler()->loadByRemoteId($remoteId); - $location = $this->domainMapper->buildLocation($spiLocation, $prioritizedLanguages ?: []); + $spiLocation = $this->persistenceHandler->locationHandler()->loadByRemoteId($remoteId, $prioritizedLanguages, $useAlwaysAvailable ?? true); + $location = $this->domainMapper->buildLocation($spiLocation, $prioritizedLanguages ?: [], $useAlwaysAvailable ?? true); if (!$this->repository->canUser('content', 'read', $location->getContentInfo(), $location)) { throw new UnauthorizedException('content', 'read', ['locationId' => $location->id]); } diff --git a/eZ/Publish/Core/Repository/SiteAccessAware/LocationService.php b/eZ/Publish/Core/Repository/SiteAccessAware/LocationService.php index 33eade14b05..fb94ce1374a 100644 --- a/eZ/Publish/Core/Repository/SiteAccessAware/LocationService.php +++ b/eZ/Publish/Core/Repository/SiteAccessAware/LocationService.php @@ -48,19 +48,21 @@ public function copySubtree(Location $subtree, Location $targetParentLocation) return $this->service->copySubtree($subtree, $targetParentLocation); } - public function loadLocation($locationId, array $prioritizedLanguages = null) + public function loadLocation($locationId, array $prioritizedLanguages = null, bool $useAlwaysAvailable = null) { return $this->service->loadLocation( $locationId, - $this->languageResolver->getPrioritizedLanguages($prioritizedLanguages) + $this->languageResolver->getPrioritizedLanguages($prioritizedLanguages), + $this->languageResolver->getUseAlwaysAvailable($useAlwaysAvailable) ); } - public function loadLocationByRemoteId($remoteId, array $prioritizedLanguages = null) + public function loadLocationByRemoteId($remoteId, array $prioritizedLanguages = null, bool $useAlwaysAvailable = null) { return $this->service->loadLocationByRemoteId( $remoteId, - $this->languageResolver->getPrioritizedLanguages($prioritizedLanguages) + $this->languageResolver->getPrioritizedLanguages($prioritizedLanguages), + $this->languageResolver->getUseAlwaysAvailable($useAlwaysAvailable) ); } diff --git a/eZ/Publish/Core/Repository/SiteAccessAware/Tests/AbstractServiceTest.php b/eZ/Publish/Core/Repository/SiteAccessAware/Tests/AbstractServiceTest.php index 28f16a6d922..ef31fadc079 100644 --- a/eZ/Publish/Core/Repository/SiteAccessAware/Tests/AbstractServiceTest.php +++ b/eZ/Publish/Core/Repository/SiteAccessAware/Tests/AbstractServiceTest.php @@ -146,7 +146,7 @@ protected function setLanguagesLookupArguments(array $arguments, $languageArgume * @param bool $return * @param int $languageArgumentIndex From 0 and up, so the array index on $arguments. */ - final public function testForLanguagesLookup($method, array $arguments, $return, $languageArgumentIndex, callable $callback = null) + final public function testForLanguagesLookup($method, array $arguments, $return, $languageArgumentIndex, callable $callback = null, int $alwaysAvailableArgumentIndex = null) { $languages = ['eng-GB', 'eng-US']; @@ -160,6 +160,16 @@ final public function testForLanguagesLookup($method, array $arguments, $return, ->with([]) ->willReturn($languages); + if ($alwaysAvailableArgumentIndex) { + $arguments[$alwaysAvailableArgumentIndex] = null; + $expectedArguments[$alwaysAvailableArgumentIndex] = true; + $this->languageResolverMock + ->expects($this->once()) + ->method('getUseAlwaysAvailable') + ->with(null) + ->willReturn(true); + } + $this->innerApiServiceMock ->expects($this->once()) ->method($method) @@ -203,7 +213,7 @@ protected function setLanguagesPassTroughArguments(array $arguments, $languageAr * @param bool $return * @param int $languageArgumentIndex From 0 and up, so the array index on $arguments. */ - final public function testForLanguagesPassTrough($method, array $arguments, $return, $languageArgumentIndex, callable $callback = null) + final public function testForLanguagesPassTrough($method, array $arguments, $return, $languageArgumentIndex, callable $callback = null, int $alwaysAvailableArgumentIndex = null) { $languages = ['eng-GB', 'eng-US']; $arguments = $this->setLanguagesPassTroughArguments($arguments, $languageArgumentIndex, $languages); @@ -214,6 +224,14 @@ final public function testForLanguagesPassTrough($method, array $arguments, $ret ->with($languages) ->willReturn($languages); + if ($alwaysAvailableArgumentIndex) { + $this->languageResolverMock + ->expects($this->once()) + ->method('getUseAlwaysAvailable') + ->with($arguments[$alwaysAvailableArgumentIndex]) + ->willReturn($arguments[$alwaysAvailableArgumentIndex]); + } + $this->innerApiServiceMock ->expects($this->once()) ->method($method) diff --git a/eZ/Publish/Core/Repository/SiteAccessAware/Tests/LocationServiceTest.php b/eZ/Publish/Core/Repository/SiteAccessAware/Tests/LocationServiceTest.php index 88dd0c2bcdd..48b158e68c4 100644 --- a/eZ/Publish/Core/Repository/SiteAccessAware/Tests/LocationServiceTest.php +++ b/eZ/Publish/Core/Repository/SiteAccessAware/Tests/LocationServiceTest.php @@ -61,13 +61,15 @@ public function providerForLanguagesLookupMethods() $contentInfo = new ContentInfo(); $versionInfo = new VersionInfo(); - // string $method, array $arguments, bool $return, int $languageArgumentIndex + // string $method, array $arguments, bool $return, int $languageArgumentIndex, ?callable $callback, ?int $alwaysAvailableArgumentIndex return [ ['loadLocation', [55], true, 1], ['loadLocation', [55, self::LANG_ARG], true, 1], + ['loadLocation', [55, self::LANG_ARG, true], true, 1, null, 2], ['loadLocationByRemoteId', ['ergemiotregf'], true, 1], ['loadLocationByRemoteId', ['ergemiotregf', self::LANG_ARG], true, 1], + ['loadLocationByRemoteId', ['ergemiotregf', self::LANG_ARG, true], true, 1, null, 2], ['loadLocations', [$contentInfo, null], true, 2], ['loadLocations', [$contentInfo, $location, self::LANG_ARG], true, 2], diff --git a/eZ/Publish/Core/SignalSlot/LocationService.php b/eZ/Publish/Core/SignalSlot/LocationService.php index 8a484a26180..2af556ad90d 100644 --- a/eZ/Publish/Core/SignalSlot/LocationService.php +++ b/eZ/Publish/Core/SignalSlot/LocationService.php @@ -91,17 +91,17 @@ public function copySubtree(Location $subtree, Location $targetParentLocation) /** * {@inheritdoc} */ - public function loadLocation($locationId, array $prioritizedLanguages = null) + public function loadLocation($locationId, array $prioritizedLanguages = null, bool $useAlwaysAvailable = null) { - return $this->service->loadLocation($locationId, $prioritizedLanguages); + return $this->service->loadLocation($locationId, $prioritizedLanguages, $useAlwaysAvailable); } /** * {@inheritdoc} */ - public function loadLocationByRemoteId($remoteId, array $prioritizedLanguages = null) + public function loadLocationByRemoteId($remoteId, array $prioritizedLanguages = null, bool $useAlwaysAvailable = null) { - return $this->service->loadLocationByRemoteId($remoteId, $prioritizedLanguages); + return $this->service->loadLocationByRemoteId($remoteId, $prioritizedLanguages, $useAlwaysAvailable); } /** diff --git a/eZ/Publish/Core/SignalSlot/Tests/LocationServiceTest.php b/eZ/Publish/Core/SignalSlot/Tests/LocationServiceTest.php index 5f72074b3b6..d9bfbdb0a8d 100644 --- a/eZ/Publish/Core/SignalSlot/Tests/LocationServiceTest.php +++ b/eZ/Publish/Core/SignalSlot/Tests/LocationServiceTest.php @@ -87,13 +87,13 @@ public function serviceProvider() ), array( 'loadLocation', - array($rootId, []), + array($rootId, [], true), $root, 0, ), array( 'loadLocationByRemoteId', - array($rootRemoteId, []), + array($rootRemoteId, [], true), $root, 0, ), diff --git a/eZ/Publish/Core/settings/storage_engines/legacy/location.yml b/eZ/Publish/Core/settings/storage_engines/legacy/location.yml index 9ac807056b7..b91ce7b205e 100644 --- a/eZ/Publish/Core/settings/storage_engines/legacy/location.yml +++ b/eZ/Publish/Core/settings/storage_engines/legacy/location.yml @@ -9,6 +9,7 @@ services: class: "%ezpublish.persistence.legacy.location.gateway.class%" arguments: - "@ezpublish.api.storage_engine.legacy.dbhandler" + - "@ezpublish.persistence.legacy.language.mask_generator" ezpublish.persistence.legacy.location.gateway.exception_conversion: class: "%ezpublish.persistence.legacy.location.gateway.exception_conversion.class%" diff --git a/eZ/Publish/SPI/Persistence/Content/Location/Handler.php b/eZ/Publish/SPI/Persistence/Content/Location/Handler.php index 7142af11cb6..28455c139fa 100644 --- a/eZ/Publish/SPI/Persistence/Content/Location/Handler.php +++ b/eZ/Publish/SPI/Persistence/Content/Location/Handler.php @@ -19,12 +19,14 @@ interface Handler * Loads the data for the location identified by $locationId. * * @param int $locationId + * @param string[]|null $translations If set, NotFound is thrown if content is not in given translation. + * @param bool $useAlwaysAvailable Respect always available flag on content, where main language is valid translation fallback. * * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException * * @return \eZ\Publish\SPI\Persistence\Content\Location */ - public function load($locationId); + public function load($locationId, array $translations = null, bool $useAlwaysAvailable = true); /** * Loads the subtree ids of the location identified by $locationId. @@ -41,12 +43,14 @@ public function loadSubtreeIds($locationId); * Loads the data for the location identified by $remoteId. * * @param string $remoteId + * @param string[]|null $translations If set, NotFound is thrown if content is not in given translation. + * @param bool $useAlwaysAvailable Respect always available flag on content, where main language is valid translation fallback. * * @throws \eZ\Publish\API\Repository\Exceptions\NotFoundException * * @return \eZ\Publish\SPI\Persistence\Content\Location */ - public function loadByRemoteId($remoteId); + public function loadByRemoteId($remoteId, array $translations = null, bool $useAlwaysAvailable = true); /** * Loads all locations for $contentId, optionally limited to a sub tree