From 3e3e2387a14a3c296b78cd23b7cd159fa711cf4d Mon Sep 17 00:00:00 2001 From: Dawid Parafinski Date: Wed, 16 Sep 2026 14:11:28 +0200 Subject: [PATCH] [Tests] Fixed stale @depends targets and outdated assertions in ContentService integration tests Several @depends annotations still targeted testLoadContentDrafts and loadRelationList, method names that no longer exist since the testLoadContentDrafts -> testLoadContentDraftList rename (aba61e9098). PHPUnit 9 silently skips tests whose @depends target is unresolvable, so these dependents had not actually run since that rename. Retargeted: - ContentServiceAuthorizationTest::testLoadContentDraftListReturnsEmptyListForUserWithoutVersionReadAccess(WithUser) (was testLoadContentDraftsThrowsUnauthorizedException(WithUser)): testLoadContentDrafts -> testLoadContentDraftList - ContentServiceTest::testLoadRelationsSkipsArchivedContent/SkipsDraftContent: loadRelationList -> testLoadRelationList - ContentServiceTest::testDeleteVersionInTransactionWithCommit: testLoadContentDrafts -> testLoadContentDraftList A full reflection-based sweep of tests/ found no other stale @depends targets. Running the previously-skipped tests uncovered that several no longer test what their names claim, because loadRelationList()/loadContentDraftList() were redesigned (already true on 4.6) to never throw UnauthorizedException - they return an empty/partial list instead: - testLoadRelationsThrowsUnauthorizedException, testLoadRelationsForDraftVersionThrowsUnauthorizedException, testLoadContentDraftsThrowsUnauthorizedException(WithUser): renamed and rewritten to assert the actual current behaviour (empty list back). - testLoadRelationsWithUnauthorizedRelations: loadRelationList() keeps one list item per relation, exposing unreadable ones as UnauthorizedRelationListItem placeholders rather than omitting them, so the test now asserts 4 items (2 authorized + 2 unauthorized) instead of 2. ContentServiceTest::testLoadRelationsSkipsArchivedContent also had its own fixture bug: it created its throwaway draft under the very Location it then trashed, so trashing purged the draft (and its relations) itself, instead of just archiving the "Demo Design" relation target. Moved the draft under the unrelated Home location instead. No src/ changes. - testLoadContentWithinGracePeriod overrode the shared ContentService's grace period via reflection and never restored it. With the @depends above resolvable again PHPUnit reorders execution so this test now runs before ContentServiceAuthorizationTest::testLoadContentThrowsUnauthorizedExceptionsOnArchives, which then read the archived version inside the leaked grace period. The original value is now restored in a finally block. - The four renamed authorization tests declare `: void` instead of keeping their phpstan-baseline "no return type" entries. --- phpstan-baseline.neon | 24 ------ .../ContentServiceAuthorizationTest.php | 85 +++++++++++-------- .../Core/Repository/ContentServiceTest.php | 61 +++++++++---- 3 files changed, 92 insertions(+), 78 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index ef862d0f57..6a3c904267 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -23196,18 +23196,6 @@ parameters: count: 1 path: tests/integration/Core/Repository/ContentServiceAuthorizationTest.php - - - message: '#^Method Ibexa\\Tests\\Integration\\Core\\Repository\\ContentServiceAuthorizationTest\:\:testLoadContentDraftsThrowsUnauthorizedException\(\) has no return type specified\.$#' - identifier: missingType.return - count: 1 - path: tests/integration/Core/Repository/ContentServiceAuthorizationTest.php - - - - message: '#^Method Ibexa\\Tests\\Integration\\Core\\Repository\\ContentServiceAuthorizationTest\:\:testLoadContentDraftsThrowsUnauthorizedExceptionWithUser\(\) has no return type specified\.$#' - identifier: missingType.return - count: 1 - path: tests/integration/Core/Repository/ContentServiceAuthorizationTest.php - - message: '#^Method Ibexa\\Tests\\Integration\\Core\\Repository\\ContentServiceAuthorizationTest\:\:testLoadContentInfoByRemoteIdThrowsUnauthorizedException\(\) has no return type specified\.$#' identifier: missingType.return @@ -23256,18 +23244,6 @@ parameters: count: 1 path: tests/integration/Core/Repository/ContentServiceAuthorizationTest.php - - - message: '#^Method Ibexa\\Tests\\Integration\\Core\\Repository\\ContentServiceAuthorizationTest\:\:testLoadRelationsForDraftVersionThrowsUnauthorizedException\(\) has no return type specified\.$#' - identifier: missingType.return - count: 1 - path: tests/integration/Core/Repository/ContentServiceAuthorizationTest.php - - - - message: '#^Method Ibexa\\Tests\\Integration\\Core\\Repository\\ContentServiceAuthorizationTest\:\:testLoadRelationsThrowsUnauthorizedException\(\) has no return type specified\.$#' - identifier: missingType.return - count: 1 - path: tests/integration/Core/Repository/ContentServiceAuthorizationTest.php - - message: '#^Method Ibexa\\Tests\\Integration\\Core\\Repository\\ContentServiceAuthorizationTest\:\:testLoadRelationsWithUnauthorizedRelations\(\) has no return type specified\.$#' identifier: missingType.return diff --git a/tests/integration/Core/Repository/ContentServiceAuthorizationTest.php b/tests/integration/Core/Repository/ContentServiceAuthorizationTest.php index b6ed19fc8c..35f0522783 100644 --- a/tests/integration/Core/Repository/ContentServiceAuthorizationTest.php +++ b/tests/integration/Core/Repository/ContentServiceAuthorizationTest.php @@ -723,32 +723,31 @@ public function testCountContentDraftsReturnZero() /** * @covers \Ibexa\Contracts\Core\Repository\ContentService::loadContentDraftList() * - * @depends Ibexa\Tests\Integration\Core\Repository\ContentServiceTest::testLoadContentDrafts - * @depends Ibexa\Tests\Integration\Core\Repository\ContentServiceTest::testLoadContentDrafts + * @depends Ibexa\Tests\Integration\Core\Repository\ContentServiceTest::testLoadContentDraftList */ - public function testLoadContentDraftsThrowsUnauthorizedException() + public function testLoadContentDraftListReturnsEmptyListForUserWithoutVersionReadAccess(): void { $this->permissionResolver->setCurrentUserReference($this->anonymousUser); - $this->expectException(UnauthorizedException::class); - $this->expectExceptionMessageMatches('/\'versionread\' \'content\'/'); + $draftList = $this->contentService->loadContentDraftList(); - $this->contentService->loadContentDraftList(); + self::assertSame(0, $draftList->totalCount); + self::assertEmpty($draftList->items); } /** * @covers \Ibexa\Contracts\Core\Repository\ContentService::loadContentDraftList($user) * - * @depends Ibexa\Tests\Integration\Core\Repository\ContentServiceTest::testLoadContentDrafts + * @depends Ibexa\Tests\Integration\Core\Repository\ContentServiceTest::testLoadContentDraftList */ - public function testLoadContentDraftsThrowsUnauthorizedExceptionWithUser() + public function testLoadContentDraftListReturnsEmptyListForUserWithoutVersionReadAccessWithUser(): void { $this->permissionResolver->setCurrentUserReference($this->anonymousUser); - $this->expectException(UnauthorizedException::class); - $this->expectExceptionMessageMatches('/\'versionread\' \'content\'/'); + $draftList = $this->contentService->loadContentDraftList($this->administratorUser); - $this->contentService->loadContentDraftList($this->administratorUser); + self::assertSame(0, $draftList->totalCount); + self::assertEmpty($draftList->items); } /** @@ -911,13 +910,11 @@ public function testCopyContentThrowsUnauthorizedExceptionWithGivenVersion() } /** - * Test for the loadRelations() method. - * * @covers \Ibexa\Contracts\Core\Repository\ContentService::loadRelationList() * * @depends Ibexa\Tests\Integration\Core\Repository\ContentServiceTest::testLoadRelationList */ - public function testLoadRelationsThrowsUnauthorizedException() + public function testLoadRelationsReturnsEmptyListForUserWithoutReadAccess(): void { $mediaEditor = $this->createMediaUserVersion1(); @@ -931,29 +928,27 @@ public function testLoadRelationsThrowsUnauthorizedException() $this->permissionResolver->setCurrentUserReference($mediaEditor); - $this->expectException(UnauthorizedException::class); - $this->expectExceptionMessageMatches('/\'read\' \'content\'/'); + $relationList = $this->contentService->loadRelationList($versionInfo); - $this->contentService->loadRelationList($versionInfo); + self::assertSame(0, $relationList->totalCount); + self::assertEmpty($relationList->items); } /** - * Test for the loadRelations() method. - * * @covers \Ibexa\Contracts\Core\Repository\ContentService::loadRelationList() * * @depends Ibexa\Tests\Integration\Core\Repository\ContentServiceTest::testLoadRelationList */ - public function testLoadRelationsForDraftVersionThrowsUnauthorizedException() + public function testLoadRelationsForDraftVersionReturnsEmptyListForUserWithoutVersionReadAccess(): void { $draft = $this->createContentDraftVersion1(); $this->permissionResolver->setCurrentUserReference($this->anonymousUser); - $this->expectException(UnauthorizedException::class); - $this->expectExceptionMessageMatches('/\'versionread\' \'content\'/'); + $relationList = $this->contentService->loadRelationList($draft->versionInfo); - $this->contentService->loadRelationList($draft->versionInfo); + self::assertSame(0, $relationList->totalCount); + self::assertEmpty($relationList->items); } /** @@ -1066,9 +1061,9 @@ private function createAnonymousWithEditorRole() } /** - * Test that for an user that doesn't have access (read permissions) to an - * related object, executing loadRelations() would not throw any exception, - * only that the non-readable related object(s) won't be loaded. + * Test that for a user that doesn't have access (read permissions) to a + * related object, executing loadRelationList() would not throw any exception, and + * would instead expose the non-readable related object(s) as unauthorized list items. * * @covers \Ibexa\Contracts\Core\Repository\ContentService::loadRelationList() * @@ -1180,31 +1175,42 @@ public function testLoadRelationsWithUnauthorizedRelations() $actualRelations = $this->contentService->loadRelationList($testFolder->getVersionInfo()); // assert results - // verify that the only expected relations are from the 2 readable objects + // one item per relation, non-readable targets come back as UnauthorizedRelationListItem + self::assertCount( + 4, + $actualRelations->items, + 'Expected one list item per relation, including the unauthorized ones' + ); + + // verify that the only readable relations are from the 2 readable objects // Main Folder and Available Folder $expectedRelations = [ $mainRelation->destinationContentInfo->id => $mainRelation, $availableRelation->destinationContentInfo->id => $availableRelation, ]; - // assert there are as many expected relations as actual ones - self::assertEquals( - count($expectedRelations), - count($actualRelations->items), - "Expected '" . count($expectedRelations) - . "' relations found '" . count($actualRelations->items) . "'" - ); + $unauthorizedItemsCount = 0; // assert each relation /** * @var \Ibexa\Contracts\Core\Repository\Values\Content\RelationList\RelationListItemInterface $relationListItem */ foreach ($actualRelations as $relationListItem) { + if (!$relationListItem->hasRelation()) { + // non-readable target + ++$unauthorizedItemsCount; + continue; + } + /** @var \Ibexa\Contracts\Core\Repository\Values\Content\Relation $relation */ $relation = $relationListItem->getRelation(); $destination = $relation->destinationContentInfo; + self::assertArrayHasKey( + $destination->id, + $expectedRelations, + "Non expected relation with '{$destination->id}' id found" + ); $expected = $expectedRelations[$destination->id]->destinationContentInfo; - self::assertNotEmpty($expected, "Non expected relation with '{$destination->id}' id found"); self::assertEquals( $expected->id, $destination->id, @@ -1220,13 +1226,20 @@ public function testLoadRelationsWithUnauthorizedRelations() unset($expectedRelations[$destination->id]); } - // verify all expected relations were found + // verify all expected (readable) relations were found self::assertCount( 0, $expectedRelations, "Expected to find '" . (count($expectedRelations) + count($actualRelations->items)) . "' relations found '" . count($actualRelations->items) . "'" ); + + // verify the 2 non-readable relations came back as unauthorized placeholders + self::assertSame( + 2, + $unauthorizedItemsCount, + 'Expected the 2 relations towards non-readable content to be reported as unauthorized' + ); } /** diff --git a/tests/integration/Core/Repository/ContentServiceTest.php b/tests/integration/Core/Repository/ContentServiceTest.php index b7b14e3812..04022d0aaf 100644 --- a/tests/integration/Core/Repository/ContentServiceTest.php +++ b/tests/integration/Core/Repository/ContentServiceTest.php @@ -38,6 +38,7 @@ use Ibexa\Core\Repository\Values\Content\ContentUpdateStruct; use InvalidArgumentException; use ReflectionClass; +use ReflectionProperty; use Symfony\Bridge\PhpUnit\ClockMock; /** @@ -3673,13 +3674,14 @@ public function testAddRelationThrowsBadStateException() * @covers \Ibexa\Contracts\Core\Repository\ContentService::loadRelationList() * * @depends testAddRelation - * @depends loadRelationList + * @depends testLoadRelationList */ public function testLoadRelationsSkipsArchivedContent() { $trashService = $this->getRepository()->getTrashService(); - $draft = $this->createContentDraftVersion1(); + // trashing location 56 below would purge a draft created under it + $draft = $this->createContentDraftVersion1(2); // Load other content objects $media = $this->contentService->loadContentInfoByRemoteId(self::MEDIA_REMOTE_ID); @@ -3736,7 +3738,7 @@ public function testLoadRelationsSkipsArchivedContent() * @covers \Ibexa\Contracts\Core\Repository\ContentService::loadRelationList() * * @depends testAddRelation - * @depends loadRelationList + * @depends testLoadRelationList */ public function testLoadRelationsSkipsDraftContent() { @@ -5075,7 +5077,7 @@ public function testDeleteVersionInTransactionWithRollback() * * @depends testCreateContent * @depends testLoadContentInfo - * @depends testLoadContentDrafts + * @depends testLoadContentDraftList */ public function testDeleteVersionInTransactionWithCommit() { @@ -7172,20 +7174,43 @@ public function testLoadContentWithinGracePeriod(): void $anonymousUserId = $this->generateId('user', 10); $repository->getPermissionResolver()->setCurrentUserReference($repository->getUserService()->loadUser($anonymousUserId)); + // the ContentService instance is shared across the test run, restore the setting afterwards + $originalGracePeriod = $this->getGracePeriod(); $this->setGracePeriod(10); - //Reset clock, to make sure that upfront operations did not exceed grace period. - ClockMock::withClockMock(strtotime('2025-04-01 14:00:02')); - $this->contentService->loadContent($unPublishedVersionOneContent->getId(), null, $unPublishedVersionOneContent->getVersionInfo()->versionNo); - - ClockMock::sleep(20); - $this->expectException(CoreUnauthorizedException::class); - $this->contentService->loadContent($unPublishedVersionOneContent->getId(), null, $unPublishedVersionOneContent->getVersionInfo()->versionNo); + try { + //Reset clock, to make sure that upfront operations did not exceed grace period. + ClockMock::withClockMock(strtotime('2025-04-01 14:00:02')); + $this->contentService->loadContent($unPublishedVersionOneContent->getId(), null, $unPublishedVersionOneContent->getVersionInfo()->versionNo); + + ClockMock::sleep(20); + $this->expectException(CoreUnauthorizedException::class); + $this->contentService->loadContent($unPublishedVersionOneContent->getId(), null, $unPublishedVersionOneContent->getVersionInfo()->versionNo); + } finally { + ClockMock::withClockMock(false); + $this->setGracePeriod($originalGracePeriod); + } + } - ClockMock::withClockMock(false); + private function getGracePeriod(): int + { + return $this->getInnerContentServiceSettingsProperty()->getValue( + $this->getInnerContentService() + )['grace_period_in_seconds']; } private function setGracePeriod(int $value): void + { + $innerService = $this->getInnerContentService(); + $settingsProperty = $this->getInnerContentServiceSettingsProperty(); + + $settings = $settingsProperty->getValue($innerService); + $settings['grace_period_in_seconds'] = $value; + + $settingsProperty->setValue($innerService, $settings); + } + + private function getInnerContentService(): object { $reflection = new ReflectionClass($this->contentService); $serviceProperty = $reflection->getProperty('service'); @@ -7197,15 +7222,15 @@ private function setGracePeriod(int $value): void $innerServiceProperty = $serviceReflection->getProperty('innerService'); $innerServiceProperty->setAccessible(true); - $innerService = $innerServiceProperty->getValue($service); + return $innerServiceProperty->getValue($service); + } - $innerServiceReflection = new ReflectionClass($innerService); + private function getInnerContentServiceSettingsProperty(): ReflectionProperty + { + $innerServiceReflection = new ReflectionClass($this->getInnerContentService()); $settingsProperty = $innerServiceReflection->getProperty('settings'); $settingsProperty->setAccessible(true); - $settings = $settingsProperty->getValue($innerService); - $settings['grace_period_in_seconds'] = $value; - - $settingsProperty->setValue($innerService, $settings); + return $settingsProperty; } }