From d23fa77179c8536ed0a0417e813289bef4c46d0e Mon Sep 17 00:00:00 2001 From: Jens Schaller Date: Fri, 6 Mar 2026 11:36:30 +0100 Subject: [PATCH 1/3] [BUGFIX] Address Copilot PR review comments - Remove languageUid from findTagStrings() cache key: the query has no language constraint so the key must not vary by language context - Fix git push in release workflow: use explicit branch ref to avoid detached HEAD failures and push tag separately - Fix php-pcov package name to be version-agnostic (php-pcov instead of php8.3-pcov) so CI works with PHP 8.4 images - Add FindTagStringsTest with 7 functional tests covering correctness, category filtering, empty-category edge case, and cache-hit behaviour --- .ddev/config.yaml | 2 +- .github/workflows/release.yml | 3 +- .../Repository/AbstractObjectRepository.php | 4 +- .../Domain/Repository/FindTagStringsTest.php | 177 ++++++++++++++++++ 4 files changed, 181 insertions(+), 5 deletions(-) create mode 100644 Tests/Functional/Domain/Repository/FindTagStringsTest.php diff --git a/.ddev/config.yaml b/.ddev/config.yaml index 3747f03..4f6b3b4 100644 --- a/.ddev/config.yaml +++ b/.ddev/config.yaml @@ -12,7 +12,7 @@ database: type: mariadb version: "10.11" nodejs_version: "20" -webimage_extra_packages: [php8.3-pcov] +webimage_extra_packages: [php-pcov] use_dns_when_possible: true composer_version: "2" web_environment: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 7ba56b1..db288b1 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -124,7 +124,8 @@ jobs: run: | git tag -a "${{ steps.version.outputs.version }}" \ -m "Release ${{ steps.version.outputs.version }}" - git push origin HEAD "${{ steps.version.outputs.version }}" + git push origin "HEAD:${{ github.ref_name }}" + git push origin "${{ steps.version.outputs.version }}" - name: Create GitHub Release if: ${{ !inputs.dry_run }} diff --git a/Classes/Domain/Repository/AbstractObjectRepository.php b/Classes/Domain/Repository/AbstractObjectRepository.php index 7d8b1b5..9897a36 100644 --- a/Classes/Domain/Repository/AbstractObjectRepository.php +++ b/Classes/Domain/Repository/AbstractObjectRepository.php @@ -135,10 +135,8 @@ public function findByUid(mixed $uid, bool $ignoreRestrictions = null): ?DomainO public function findTagStrings(ObjectDemandInterface $demand): array { $categoryUid = $demand->getCategory(); - $languageUid = (int)GeneralUtility::makeInstance(Context::class) - ->getPropertyFromAspect('language', 'id', 0); $cacheKey = 'pagebased_tags_' . md5( - $this->registration->getIdentifier() . '_' . $categoryUid . '_' . $languageUid + $this->registration->getIdentifier() . '_' . $categoryUid ); $cache = $this->getTagsCache(); diff --git a/Tests/Functional/Domain/Repository/FindTagStringsTest.php b/Tests/Functional/Domain/Repository/FindTagStringsTest.php new file mode 100644 index 0000000..2751ed6 --- /dev/null +++ b/Tests/Functional/Domain/Repository/FindTagStringsTest.php @@ -0,0 +1,177 @@ +bootstrapTestRegistration(); + $this->importCSVDataSet(__DIR__ . '/../../Fixtures/Database/pages_many_objects.csv'); + + $this->repository = $this->get(TestObjectRepository::class); + } + + private function bootstrapTestRegistration(): void + { + $objectRegistration = new ObjectRegistration('Test Object'); + $objectRegistration->setClassName(TestObject::class); + $objectRegistration->setRepositoryClass(TestObjectRepository::class); + + $categoryRegistration = new CategoryRegistration('Test Category'); + $categoryRegistration->setClassName(TestCategory::class); + $categoryRegistration->setRepositoryClass(TestCategoryRepository::class); + $categoryRegistration->setDocumentType(199); + + $registration = new Registration('test', 'test_news'); + $registration->setObject($objectRegistration); + $registration->setCategory($categoryRegistration); + + RegistrationService::addRegistration($registration); + } + + // --------------------------------------------------------------------------- + // Return value correctness + // --------------------------------------------------------------------------- + + /** @test */ + public function findTagStringsReturnsArrayOfStrings(): void + { + $demand = $this->repository->initializeDemand(); + $result = $this->repository->findTagStrings($demand); + + self::assertIsArray($result); + self::assertNotEmpty($result, 'Expected at least one tag string in fixture data'); + + foreach ($result as $tagString) { + self::assertIsString($tagString, 'Each element must be a raw tag string'); + self::assertNotSame('', $tagString, 'Empty tag strings must be excluded by the query'); + } + } + + /** @test */ + public function findTagStringsReturnsExpectedCountForKnownDataset(): void + { + $demand = $this->repository->initializeDemand(); + $result = $this->repository->findTagStrings($demand); + + // 3 categories × 20 objects = 60 total; last in each category is hidden → 57 visible + self::assertCount(57, $result, '57 visible objects with non-empty tags expected'); + } + + // --------------------------------------------------------------------------- + // Category filtering + // --------------------------------------------------------------------------- + + /** @test */ + public function findTagStringsWithCategoryFilterReturnsSubsetOfAllResults(): void + { + $allDemand = $this->repository->initializeDemand(); + $allResults = $this->repository->findTagStrings($allDemand); + + $categoryDemand = $this->repository->initializeDemand()->setCategory(400); + $categoryResults = $this->repository->findTagStrings($categoryDemand); + + self::assertNotEmpty($categoryResults, 'Category 400 must have objects with tags'); + self::assertLessThan( + count($allResults), + count($categoryResults), + 'Category-scoped result must be a strict subset of all results' + ); + } + + /** @test */ + public function findTagStringsWithCategoryFilterReturnsOnlyObjectsUnderThatCategory(): void + { + // Category 400 has 20 objects (UIDs 500–519), last one is hidden → 19 visible + $demand = $this->repository->initializeDemand()->setCategory(400); + $result = $this->repository->findTagStrings($demand); + + self::assertCount(19, $result, 'Category 400 must yield exactly 19 visible tag strings'); + } + + /** @test */ + public function findTagStringsReturnsEmptyArrayForCategoryWithNoChildPages(): void + { + // UID 9999 does not exist in the fixture, so collectPagesBelow() returns []. + $demand = $this->repository->initializeDemand()->setCategory(9999); + $result = $this->repository->findTagStrings($demand); + + self::assertSame([], $result, 'Non-existent category must yield an empty array'); + } + + // --------------------------------------------------------------------------- + // Cache-hit behaviour + // --------------------------------------------------------------------------- + + /** @test */ + public function findTagStringsReturnsSameResultOnConsecutiveCalls(): void + { + $demand = $this->repository->initializeDemand(); + + $firstCall = $this->repository->findTagStrings($demand); + $secondCall = $this->repository->findTagStrings($demand); + + self::assertSame( + $firstCall, + $secondCall, + 'Consecutive calls with identical demand must return the same result (cache hit)' + ); + } + + /** @test */ + public function findTagStringsWithCategoryFilterReturnsSameResultOnConsecutiveCalls(): void + { + $demand = $this->repository->initializeDemand()->setCategory(401); + + $firstCall = $this->repository->findTagStrings($demand); + $secondCall = $this->repository->findTagStrings($demand); + + self::assertSame( + $firstCall, + $secondCall, + 'Consecutive category-filtered calls must return the same result (cache hit)' + ); + } +} From 7b48b49b2797cc09d37f8411947973c4d0b902a6 Mon Sep 17 00:00:00 2001 From: Jens Schaller Date: Fri, 6 Mar 2026 14:20:18 +0100 Subject: [PATCH 2/3] [BUGFIX] Address remaining Copilot PR review comments - findTagStrings(): replicate nav_hide, l18n_cfg and child-object exclusion constraints that Extbase applies via createDemandConstraints(), so the raw DBAL path returns the same visible set as findByDemand(). Also include includeChildObjects flag in the cache key. - TagUtility::getTags(): skip findTagStrings() when a specific language uid is requested, falling back to the hydrated Extbase path which correctly applies language overlays. - ddev test command: fix argument forwarding from $* to "$@" so flags with spaces/special chars are preserved when proxying into the container. - RootLineUtilityPerformanceTest: align timing assertion with the documented 5x tolerance instead of a strict less-than check. - RepositoryPerformanceTest: align findByUid() docblock with the actual assertLessThanOrEqual(3) assertion (was "at most 2 queries"). --- .ddev/commands/web/test | 2 +- .../Repository/AbstractObjectRepository.php | 26 ++++++++++++++++++- Classes/Utility/TagUtility.php | 6 +++-- .../Performance/RepositoryPerformanceTest.php | 4 +-- .../RootLineUtilityPerformanceTest.php | 9 ++++--- 5 files changed, 37 insertions(+), 10 deletions(-) diff --git a/.ddev/commands/web/test b/.ddev/commands/web/test index 91e7b93..e0001d6 100755 --- a/.ddev/commands/web/test +++ b/.ddev/commands/web/test @@ -10,7 +10,7 @@ set -Eeuo pipefail # Auto-proxy into DDEV if running outside the container if [ -z "${IS_DDEV_PROJECT:-}" ]; then if command -v ddev >/dev/null 2>&1 && [ -d ".ddev" ]; then - ddev exec bash -lc "/var/www/html/.ddev/commands/web/test $*" + ddev exec bash -lc '/var/www/html/.ddev/commands/web/test "$@"' -- test "$@" exit $? fi fi diff --git a/Classes/Domain/Repository/AbstractObjectRepository.php b/Classes/Domain/Repository/AbstractObjectRepository.php index 9897a36..e395668 100644 --- a/Classes/Domain/Repository/AbstractObjectRepository.php +++ b/Classes/Domain/Repository/AbstractObjectRepository.php @@ -135,8 +135,9 @@ public function findByUid(mixed $uid, bool $ignoreRestrictions = null): ?DomainO public function findTagStrings(ObjectDemandInterface $demand): array { $categoryUid = $demand->getCategory(); + $excludeChildObjects = $demand->getIncludeChildObjects() === false; $cacheKey = 'pagebased_tags_' . md5( - $this->registration->getIdentifier() . '_' . $categoryUid + $this->registration->getIdentifier() . '_' . $categoryUid . '_' . (int)$excludeChildObjects ); $cache = $this->getTagsCache(); @@ -161,6 +162,29 @@ public function findTagStrings(ObjectDemandInterface $demand): array $qb->expr()->neq('pagebased_tags', $qb->createNamedParameter('')) ); + // Replicate nav_hide constraint from AbstractPageRepository::createDemandConstraints() + $qb->andWhere($qb->expr()->eq('nav_hide', $qb->createNamedParameter(0, \Doctrine\DBAL\ParameterType::INTEGER))); + + // Replicate l18n_cfg constraint from AbstractPageRepository::createDemandConstraints() + $qb->andWhere($qb->expr()->or( + $qb->expr()->eq('l18n_cfg', $qb->createNamedParameter(0, \Doctrine\DBAL\ParameterType::INTEGER)), + $qb->expr()->and( + $qb->expr()->gte('l18n_cfg', $qb->createNamedParameter(1, \Doctrine\DBAL\ParameterType::INTEGER)), + $qb->expr()->gte( + $GLOBALS['TCA'][AbstractPage::TABLE_NAME]['ctrl']['languageField'], + $qb->createNamedParameter(1, \Doctrine\DBAL\ParameterType::INTEGER) + ) + ) + )); + + // Replicate child-object exclusion from AbstractObjectRepository::createDemandConstraints() + if ($excludeChildObjects) { + $qb->andWhere($qb->expr()->neq( + DetectionUtility::CHILD_OBJECT_FIELD_NAME, + $qb->createNamedParameter(1, \Doctrine\DBAL\ParameterType::INTEGER) + )); + } + if ($categoryUid > 0) { $pageIds = array_keys(RootLineUtility::collectPagesBelow($categoryUid)); if (!empty($pageIds)) { diff --git a/Classes/Utility/TagUtility.php b/Classes/Utility/TagUtility.php index 4c14dce..ffdeffc 100644 --- a/Classes/Utility/TagUtility.php +++ b/Classes/Utility/TagUtility.php @@ -64,8 +64,10 @@ public static function getTags(ObjectDemandInterface $demand, RepositoryInterfac $repository->setDefaultQuerySettings($querySettings); } - // Use a lightweight tag-only query when supported (avoids full Extbase object hydration) - if ($repository instanceof AbstractObjectRepository) { + // Use a lightweight tag-only query when supported (avoids full Extbase object hydration). + // Skip this path when a specific language is requested: findTagStrings() uses raw DBAL + // and does not apply language overlays, so language-scoped callers must use the hydrated path. + if ($repository instanceof AbstractObjectRepository && $languageUid === null) { $tagDemand = $ignoreTagsFromDemand === true ? $demand->setTags(null) : $demand; $tagStrings = $repository->findTagStrings($tagDemand); diff --git a/Tests/Functional/Performance/RepositoryPerformanceTest.php b/Tests/Functional/Performance/RepositoryPerformanceTest.php index 01eef26..e48a727 100644 --- a/Tests/Functional/Performance/RepositoryPerformanceTest.php +++ b/Tests/Functional/Performance/RepositoryPerformanceTest.php @@ -120,9 +120,9 @@ public function findByDemandReturnsExpectedCountForLargeDataset(): void /** * @test - * findByUid() must use at most 2 queries. Before the optimisation it ran + * findByUid() must use at most 3 queries. Before the optimisation it ran * through the full demand pipeline (site-scope query + registration query). - * After the fix it is a single direct SELECT. + * After the fix it is a single direct SELECT with minimal Extbase overhead. */ public function findByUidIssuesMinimalQueries(): void { diff --git a/Tests/Functional/Performance/RootLineUtilityPerformanceTest.php b/Tests/Functional/Performance/RootLineUtilityPerformanceTest.php index 93991f0..f187d7e 100644 --- a/Tests/Functional/Performance/RootLineUtilityPerformanceTest.php +++ b/Tests/Functional/Performance/RootLineUtilityPerformanceTest.php @@ -159,8 +159,9 @@ public function collectPagesBelowWithDifferentArgumentsBypassesCache(): void /** * @test - * The cached call must be faster than the cold call. - * We allow a generous factor of 5× to avoid flaky behaviour on slow CI. + * The cached call must not be significantly more expensive than the cold call. + * We allow a generous factor of 5× to account for measurement noise on slow CI. + * The zero-query guarantee is verified separately in collectPagesBelowIssuesZeroQueriesOnCacheHit(). */ public function cachedCallIsFasterThanColdCall(): void { @@ -174,8 +175,8 @@ public function cachedCallIsFasterThanColdCall(): void RootLineUtility::collectPagesBelow(200); $warmMs = (microtime(true) - $start) * 1000; - self::assertLessThan($coldMs, $warmMs, sprintf( - 'Cached call (%.3f ms) should be faster than cold call (%.3f ms)', + self::assertLessThan($coldMs * 5, $warmMs, sprintf( + 'Cached call (%.3f ms) should not exceed 5× the cold call duration (%.3f ms)', $warmMs, $coldMs )); From 6ba091e74f829beb1b2e4327b25bca3bfa790e1b Mon Sep 17 00:00:00 2001 From: Jens Schaller Date: Fri, 6 Mar 2026 15:39:03 +0100 Subject: [PATCH 3/3] [BUGFIX] Address Copilot PR #7 review comments - TagUtility::getTags(): also check the TYPO3 Context language id (not just the explicit $languageUid parameter) before taking the fast findTagStrings() path, so implicit callers operating in a non-default site language correctly fall back to the hydrated Extbase path. - AbstractObjectRepository::findTagStrings(): extend docblock to document the default-language-only constraint and the enforcement mechanism. - ddev test proxy: replace placeholder 'test' with neutral '_' so the inner script's $@ receives only the original arguments. - release.yml: guard the branch push with a ref_type == branch check to prevent failure when the workflow is dispatched from a tag ref. - RepositoryPerformanceTest: clarify docblock to distinguish the single data SELECT from the total 3-query budget. - RootLineUtilityPerformanceTest: rename cachedCallIsFasterThanColdCall() to cachedCallIsNotSignificantlySlowerThanColdCall() to match the 5x tolerance assertion. --- .ddev/commands/web/test | 2 +- .github/workflows/release.yml | 4 +++- .../Domain/Repository/AbstractObjectRepository.php | 5 +++++ Classes/Utility/TagUtility.php | 11 ++++++++--- .../Performance/RepositoryPerformanceTest.php | 7 ++++--- .../Performance/RootLineUtilityPerformanceTest.php | 2 +- 6 files changed, 22 insertions(+), 9 deletions(-) diff --git a/.ddev/commands/web/test b/.ddev/commands/web/test index e0001d6..32f80c2 100755 --- a/.ddev/commands/web/test +++ b/.ddev/commands/web/test @@ -10,7 +10,7 @@ set -Eeuo pipefail # Auto-proxy into DDEV if running outside the container if [ -z "${IS_DDEV_PROJECT:-}" ]; then if command -v ddev >/dev/null 2>&1 && [ -d ".ddev" ]; then - ddev exec bash -lc '/var/www/html/.ddev/commands/web/test "$@"' -- test "$@" + ddev exec bash -lc '/var/www/html/.ddev/commands/web/test "$@"' -- _ "$@" exit $? fi fi diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index db288b1..25868cd 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -124,7 +124,9 @@ jobs: run: | git tag -a "${{ steps.version.outputs.version }}" \ -m "Release ${{ steps.version.outputs.version }}" - git push origin "HEAD:${{ github.ref_name }}" + if [ "${{ github.ref_type }}" = "branch" ]; then + git push origin "HEAD:${{ github.ref_name }}" + fi git push origin "${{ steps.version.outputs.version }}" - name: Create GitHub Release diff --git a/Classes/Domain/Repository/AbstractObjectRepository.php b/Classes/Domain/Repository/AbstractObjectRepository.php index e395668..8494568 100644 --- a/Classes/Domain/Repository/AbstractObjectRepository.php +++ b/Classes/Domain/Repository/AbstractObjectRepository.php @@ -129,6 +129,11 @@ public function findByUid(mixed $uid, bool $ignoreRestrictions = null): ?DomainO * bypassing full Extbase object hydration. Results are cached in TYPO3's data * cache and automatically invalidated when pages are modified. * + * This method operates on the default language only (no language overlays are applied). + * Call sites must ensure it is not invoked in a non-default language context. + * TagUtility::getTags() enforces this by checking both the explicit $languageUid + * parameter and the current TYPO3 Context language before choosing this path. + * * @param ObjectDemandInterface $demand Used for optional category-tree filtering. * @return string[] Raw comma-separated tag strings, one entry per page row. */ diff --git a/Classes/Utility/TagUtility.php b/Classes/Utility/TagUtility.php index ffdeffc..4922f35 100644 --- a/Classes/Utility/TagUtility.php +++ b/Classes/Utility/TagUtility.php @@ -4,6 +4,7 @@ namespace Zeroseven\Pagebased\Utility; +use TYPO3\CMS\Core\Context\Context; use TYPO3\CMS\Core\Utility\GeneralUtility; use TYPO3\CMS\Extbase\Persistence\QueryResultInterface; use Zeroseven\Pagebased\Domain\Model\Demand\ObjectDemandInterface; @@ -65,9 +66,13 @@ public static function getTags(ObjectDemandInterface $demand, RepositoryInterfac } // Use a lightweight tag-only query when supported (avoids full Extbase object hydration). - // Skip this path when a specific language is requested: findTagStrings() uses raw DBAL - // and does not apply language overlays, so language-scoped callers must use the hydrated path. - if ($repository instanceof AbstractObjectRepository && $languageUid === null) { + // Skip this path when a non-default language is active: findTagStrings() uses raw DBAL + // and does not apply language overlays. We check both the explicit $languageUid parameter + // and the current TYPO3 Context language so implicit callers (e.g. AbstractObjectController) + // are also protected when operating in a non-default site language. + $contextLanguageUid = (int)GeneralUtility::makeInstance(Context::class) + ->getPropertyFromAspect('language', 'id', 0); + if ($repository instanceof AbstractObjectRepository && $languageUid === null && $contextLanguageUid === 0) { $tagDemand = $ignoreTagsFromDemand === true ? $demand->setTags(null) : $demand; $tagStrings = $repository->findTagStrings($tagDemand); diff --git a/Tests/Functional/Performance/RepositoryPerformanceTest.php b/Tests/Functional/Performance/RepositoryPerformanceTest.php index e48a727..a6ac08f 100644 --- a/Tests/Functional/Performance/RepositoryPerformanceTest.php +++ b/Tests/Functional/Performance/RepositoryPerformanceTest.php @@ -120,9 +120,10 @@ public function findByDemandReturnsExpectedCountForLargeDataset(): void /** * @test - * findByUid() must use at most 3 queries. Before the optimisation it ran - * through the full demand pipeline (site-scope query + registration query). - * After the fix it is a single direct SELECT with minimal Extbase overhead. + * findByUid() must use at most 3 queries. The main data retrieval is a single + * direct SELECT; any additional Extbase/TCA/language overhead must keep the + * total at or below 3 queries. Before the optimisation it ran through the full + * demand pipeline (site-scope query + registration query). */ public function findByUidIssuesMinimalQueries(): void { diff --git a/Tests/Functional/Performance/RootLineUtilityPerformanceTest.php b/Tests/Functional/Performance/RootLineUtilityPerformanceTest.php index f187d7e..8f579b9 100644 --- a/Tests/Functional/Performance/RootLineUtilityPerformanceTest.php +++ b/Tests/Functional/Performance/RootLineUtilityPerformanceTest.php @@ -163,7 +163,7 @@ public function collectPagesBelowWithDifferentArgumentsBypassesCache(): void * We allow a generous factor of 5× to account for measurement noise on slow CI. * The zero-query guarantee is verified separately in collectPagesBelowIssuesZeroQueriesOnCacheHit(). */ - public function cachedCallIsFasterThanColdCall(): void + public function cachedCallIsNotSignificantlySlowerThanColdCall(): void { // Cold call $start = microtime(true);