From 7d2debd0e3a43c0cd31ce6973dc22c464ecf4d65 Mon Sep 17 00:00:00 2001 From: Simon Schmidt Date: Fri, 28 Aug 2026 16:50:46 +0200 Subject: [PATCH 1/4] [BUGFIX] detect and surface site settings write failures Adds a SiteSettingsWriteGuard service that provides two pre/post-write checks for SiteSettingsService::writeSettings() callers: - assertDirectoryWritable(): pre-flight check before any API call, preventing orphaned remote instances when the config directory is not writable. - assertSettingsPersisted(): post-write verification that detects the silent write failure caused by the TYPO3 core bug tracked in https://forge.typo3.org/issues/110550. All three write sites (InstanceRegistrationService, ApiKeyService, AnalyticsStatusService) are updated accordingly. InstanceRegistration throws on failure; the other two log and return silently, as they run on every dashboard load or background sync. Resolves: https://github.com/TYPO3GmbH/analytics/issues/19 --- Classes/Service/AnalyticsStatusService.php | 23 ++- Classes/Service/ApiKeyService.php | 28 +++- .../Service/InstanceRegistrationService.php | 26 +++- Classes/Service/SiteSettingsWriteGuard.php | 52 +++++++ .../SiteSettingsWriteGuardInterface.php | 27 ++++ Configuration/Services.yaml | 3 + .../BackendModuleControllerTest.php | 3 + .../Service/AnalyticsStatusServiceTest.php | 1 + .../InstanceRegistrationServiceTest.php | 1 + .../BackendModuleControllerTest.php | 3 + .../Service/AnalyticsStatusServiceTest.php | 38 +++++ Tests/Unit/Service/ApiKeyServiceTest.php | 36 +++++ .../InstanceRegistrationServiceTest.php | 47 ++++++ Tests/Unit/Service/SiteDataProviderTest.php | 1 + .../Service/SiteSettingsWriteGuardTest.php | 143 ++++++++++++++++++ 15 files changed, 422 insertions(+), 10 deletions(-) create mode 100644 Classes/Service/SiteSettingsWriteGuard.php create mode 100644 Classes/Service/SiteSettingsWriteGuardInterface.php create mode 100644 Tests/Unit/Service/SiteSettingsWriteGuardTest.php diff --git a/Classes/Service/AnalyticsStatusService.php b/Classes/Service/AnalyticsStatusService.php index b3379dd2..0b6fd415 100644 --- a/Classes/Service/AnalyticsStatusService.php +++ b/Classes/Service/AnalyticsStatusService.php @@ -9,6 +9,7 @@ use Psr\Log\LoggerInterface; use T3G\Analytics\Exception\AnalyticsApiException; use TYPO3\CMS\Core\Cache\Frontend\FrontendInterface; +use TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException; use TYPO3\CMS\Core\Site\Entity\Site; use TYPO3\CMS\Core\Site\SiteSettingsFactory; use TYPO3\CMS\Core\Site\SiteSettingsService; @@ -25,6 +26,7 @@ public function __construct( private LoggerInterface $logger, private SiteSettingsService $siteSettingsService, private SiteSettingsFactory $siteSettingsFactory, + private SiteSettingsWriteGuardInterface $writeGuard, ) { } @@ -110,7 +112,26 @@ public function syncSiteSettingsFromStatus(Site $site, array $data): void } $existing = $this->siteSettingsFactory->loadLocalSettings($site->getIdentifier()) ?? []; - $this->siteSettingsService->writeSettings($site, array_merge($existing, $update)); + try { + $this->siteSettingsService->writeSettings($site, array_merge($existing, $update)); + } catch (SiteConfigurationWriteException $e) { + $this->logger->warning( + 'syncSiteSettingsFromStatus: writeSettings threw. Check file system permissions.', + ['siteIdentifier' => $site->getIdentifier(), 'exception' => $e->getMessage()] + ); + return; + } + + try { + $this->writeGuard->assertSettingsPersisted($site, $update); + } catch (AnalyticsApiException) { + $this->logger->warning( + 'syncSiteSettingsFromStatus: settings could not be persisted. Check file system permissions.', + ['siteIdentifier' => $site->getIdentifier()] + ); + return; + } + $this->logger->info( 'Site settings updated from status response.', ['siteIdentifier' => $site->getIdentifier(), 'update' => $update] diff --git a/Classes/Service/ApiKeyService.php b/Classes/Service/ApiKeyService.php index af4f036e..6bb3f2f4 100644 --- a/Classes/Service/ApiKeyService.php +++ b/Classes/Service/ApiKeyService.php @@ -8,6 +8,7 @@ use Psr\Container\NotFoundExceptionInterface; use Psr\Log\LoggerInterface; use T3G\Analytics\Exception\AnalyticsApiException; +use TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException; use TYPO3\CMS\Core\Site\Entity\Site; use TYPO3\CMS\Core\Site\SiteSettingsFactory; use TYPO3\CMS\Core\Site\SiteSettingsService; @@ -19,6 +20,7 @@ public function __construct( private CipherServiceInterface $cipherService, private SiteSettingsService $siteSettingsService, private SiteSettingsFactory $siteSettingsFactory, + private SiteSettingsWriteGuardInterface $writeGuard, private LoggerInterface $logger, ) { } @@ -102,10 +104,28 @@ public function provisionIfNeeded(Site $site, array $currentStatus): void } $existing = $this->siteSettingsFactory->loadLocalSettings($siteIdentifier) ?? []; - $this->siteSettingsService->writeSettings($site, array_merge($existing, [ - 'apiKeyId' => $result['apiKeyId'], - 'apiKey' => $encryptedApiKey, - ])); + try { + $this->siteSettingsService->writeSettings($site, array_merge($existing, [ + 'apiKeyId' => $result['apiKeyId'], + 'apiKey' => $encryptedApiKey, + ])); + } catch (SiteConfigurationWriteException $e) { + $this->logger->error('ApiKeyService: writeSettings threw.', [ + 'siteIdentifier' => $siteIdentifier, + 'exception' => $e->getMessage(), + ]); + return; + } + + try { + $this->writeGuard->assertSettingsPersisted($site, ['apiKeyId' => $result['apiKeyId']]); + } catch (AnalyticsApiException $e) { + $this->logger->error('ApiKeyService: API key could not be persisted.', [ + 'siteIdentifier' => $siteIdentifier, + 'apiKeyId' => $result['apiKeyId'], + ]); + return; + } $this->logger->info('ApiKeyService: API key provisioned.', [ 'siteIdentifier' => $siteIdentifier, diff --git a/Classes/Service/InstanceRegistrationService.php b/Classes/Service/InstanceRegistrationService.php index 394eaf8a..aa77da2e 100644 --- a/Classes/Service/InstanceRegistrationService.php +++ b/Classes/Service/InstanceRegistrationService.php @@ -6,6 +6,7 @@ use Psr\Log\LoggerInterface; use T3G\Analytics\Exception\AnalyticsApiException; +use TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException; use TYPO3\CMS\Core\Site\Entity\Site; use TYPO3\CMS\Core\Site\SiteSettingsFactory; use TYPO3\CMS\Core\Site\SiteSettingsService; @@ -17,6 +18,7 @@ public function __construct( private CipherServiceInterface $cipherService, private SiteSettingsService $siteSettingsService, private SiteSettingsFactory $siteSettingsFactory, + private SiteSettingsWriteGuardInterface $writeGuard, private LoggerInterface $logger, ) { } @@ -31,6 +33,8 @@ public function register(Site $site, string $email): void { $siteIdentifier = $site->getIdentifier(); + $this->writeGuard->assertDirectoryWritable($site); + try { $data = $this->apiClient->registerInstance($site, $email); } catch (AnalyticsApiException $e) { @@ -51,11 +55,23 @@ public function register(Site $site, string $email): void $encryptedSecret = $instanceSecret !== '' ? $this->cipherService->encrypt($instanceSecret) : ''; $existing = $this->siteSettingsFactory->loadLocalSettings($siteIdentifier) ?? []; - $this->siteSettingsService->writeSettings($site, array_merge($existing, [ - 'websiteId' => $websiteId, - 'instanceId' => $instanceId, - 'instanceSecret' => $encryptedSecret, - ])); + try { + $this->siteSettingsService->writeSettings($site, array_merge($existing, [ + 'websiteId' => $websiteId, + 'instanceId' => $instanceId, + 'instanceSecret' => $encryptedSecret, + ])); + } catch (SiteConfigurationWriteException $e) { + $this->logger->error('Registration: writeSettings threw.', ['siteIdentifier' => $siteIdentifier, 'exception' => $e->getMessage()]); + throw new AnalyticsApiException('Settings could not be written to config/sites/' . $siteIdentifier . '/settings.yaml. Check file system permissions for this directory.', 0); + } + + try { + $this->writeGuard->assertSettingsPersisted($site, ['websiteId' => $websiteId]); + } catch (AnalyticsApiException $e) { + $this->logger->error('Registration: settings could not be persisted.', ['siteIdentifier' => $siteIdentifier]); + throw $e; + } $this->logger->info('Site successfully registered.', ['siteIdentifier' => $siteIdentifier, 'websiteId' => $websiteId]); } diff --git a/Classes/Service/SiteSettingsWriteGuard.php b/Classes/Service/SiteSettingsWriteGuard.php new file mode 100644 index 00000000..31cce6d3 --- /dev/null +++ b/Classes/Service/SiteSettingsWriteGuard.php @@ -0,0 +1,52 @@ +getIdentifier(); + $configDir = $this->resolveSitesConfigPath() . '/' . $identifier; + + if (!is_dir($configDir) || !is_writable($configDir)) { + throw new AnalyticsApiException( + 'Cannot write to config/sites/' . $identifier . '/. Check file system permissions for this directory.', + 0 + ); + } + } + + public function assertSettingsPersisted(Site $site, array $expected): void + { + $identifier = $site->getIdentifier(); + $persisted = $this->siteSettingsFactory->loadLocalSettings($identifier) ?? []; + + foreach ($expected as $key => $value) { + if (($persisted[$key] ?? '') !== $value) { + throw new AnalyticsApiException( + 'Settings could not be written to config/sites/' . $identifier . '/settings.yaml. Check file system permissions for this directory.', + 0 + ); + } + } + } + + private function resolveSitesConfigPath(): string + { + return $this->sitesConfigPath !== '' ? $this->sitesConfigPath : Environment::getConfigPath() . '/sites'; + } +} diff --git a/Classes/Service/SiteSettingsWriteGuardInterface.php b/Classes/Service/SiteSettingsWriteGuardInterface.php new file mode 100644 index 00000000..8f318438 --- /dev/null +++ b/Classes/Service/SiteSettingsWriteGuardInterface.php @@ -0,0 +1,27 @@ + $expected key-value pairs that must be present after the write + * @throws AnalyticsApiException when any value is absent or does not match + */ + public function assertSettingsPersisted(Site $site, array $expected): void; +} diff --git a/Configuration/Services.yaml b/Configuration/Services.yaml index fa2d07ab..6790fcfa 100644 --- a/Configuration/Services.yaml +++ b/Configuration/Services.yaml @@ -12,6 +12,9 @@ services: - '../Classes/Dashboard/Widget/TrafficGraphWidgetV14.php' - '../Classes/Dashboard/Widget/TrafficSourcesWidgetV14.php' + T3G\Analytics\Service\SiteSettingsWriteGuardInterface: + alias: T3G\Analytics\Service\SiteSettingsWriteGuard + T3G\Analytics\Service\TopPagesServiceInterface: alias: T3G\Analytics\Service\TopPagesService diff --git a/Tests/Functional/Controller/BackendModuleControllerTest.php b/Tests/Functional/Controller/BackendModuleControllerTest.php index 15184192..c789dcc7 100644 --- a/Tests/Functional/Controller/BackendModuleControllerTest.php +++ b/Tests/Functional/Controller/BackendModuleControllerTest.php @@ -415,6 +415,7 @@ private function buildController( new NullLogger(), $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), + $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), ); $siteDataProvider = new SiteDataProvider( @@ -436,6 +437,7 @@ private function buildController( $cipherService, $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), + $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), new NullLogger(), ); @@ -444,6 +446,7 @@ private function buildController( $cipherService, $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), + $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), new NullLogger(), ); diff --git a/Tests/Functional/Service/AnalyticsStatusServiceTest.php b/Tests/Functional/Service/AnalyticsStatusServiceTest.php index aec684df..5d50741e 100644 --- a/Tests/Functional/Service/AnalyticsStatusServiceTest.php +++ b/Tests/Functional/Service/AnalyticsStatusServiceTest.php @@ -73,6 +73,7 @@ protected function setUp(): void new \Psr\Log\NullLogger(), $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), + $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), ); } diff --git a/Tests/Functional/Service/InstanceRegistrationServiceTest.php b/Tests/Functional/Service/InstanceRegistrationServiceTest.php index edc4a770..be7a7cb5 100644 --- a/Tests/Functional/Service/InstanceRegistrationServiceTest.php +++ b/Tests/Functional/Service/InstanceRegistrationServiceTest.php @@ -75,6 +75,7 @@ protected function setUp(): void $this->cipherService, $this->siteSettingsService, $this->siteSettingsFactory, + $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), new \Psr\Log\NullLogger(), ); } diff --git a/Tests/Unit/Controller/BackendModuleControllerTest.php b/Tests/Unit/Controller/BackendModuleControllerTest.php index 2fa4e1e2..ef5d1452 100644 --- a/Tests/Unit/Controller/BackendModuleControllerTest.php +++ b/Tests/Unit/Controller/BackendModuleControllerTest.php @@ -136,6 +136,7 @@ protected function setUp(): void new NullLogger(), $this->siteSettingsService, $this->siteSettingsFactory, + $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), ); $registrationService = new InstanceRegistrationService( @@ -143,6 +144,7 @@ protected function setUp(): void $cipherService, $this->siteSettingsService, $this->siteSettingsFactory, + $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), new NullLogger(), ); @@ -151,6 +153,7 @@ protected function setUp(): void $cipherService, $this->siteSettingsService, $this->siteSettingsFactory, + $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), new NullLogger(), ); diff --git a/Tests/Unit/Service/AnalyticsStatusServiceTest.php b/Tests/Unit/Service/AnalyticsStatusServiceTest.php index 86e2ca80..7d3b4a5e 100644 --- a/Tests/Unit/Service/AnalyticsStatusServiceTest.php +++ b/Tests/Unit/Service/AnalyticsStatusServiceTest.php @@ -12,11 +12,13 @@ use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\NullLogger; use T3G\Analytics\Configuration\ApiConfiguration; +use T3G\Analytics\Exception\AnalyticsApiException; use T3G\Analytics\Service\AnalyticsApiClient; use T3G\Analytics\Service\AnalyticsStatusService; use T3G\Analytics\Service\ApiExceptionExtractor; use T3G\Analytics\Service\CipherService; use T3G\Analytics\Service\HmacSigner; +use T3G\Analytics\Service\SiteSettingsWriteGuardInterface; use TYPO3\CMS\Core\Cache\Backend\TransientMemoryBackend; use TYPO3\CMS\Core\Cache\Frontend\VariableFrontend; use TYPO3\CMS\Core\Information\Typo3Version; @@ -38,6 +40,7 @@ final class AnalyticsStatusServiceTest extends UnitTestCase private array $httpHistory = []; private SiteSettingsService&MockObject $siteSettingsService; private SiteSettingsFactory&MockObject $siteSettingsFactory; + private SiteSettingsWriteGuardInterface&MockObject $writeGuard; private VariableFrontend $cache; private string $encryptedTestSecret; @@ -58,6 +61,7 @@ protected function setUp(): void $this->siteSettingsService = $this->createMock(SiteSettingsService::class); $this->siteSettingsFactory = $this->createMock(SiteSettingsFactory::class); + $this->writeGuard = $this->createMock(SiteSettingsWriteGuardInterface::class); // TransientMemoryBackend dropped the $context parameter in TYPO3 v14. $backend = (new Typo3Version())->getMajorVersion() >= 14 ? new TransientMemoryBackend() // @phpstan-ignore argument.count @@ -84,6 +88,7 @@ protected function setUp(): void new NullLogger(), $this->siteSettingsService, $this->siteSettingsFactory, + $this->writeGuard, ); } @@ -231,6 +236,39 @@ public function skipsApiCallOnCacheHit(): void self::assertEmpty($this->httpHistory); } + #[Test] + public function syncSiteSettingsFromStatusLogsWarningWhenWriteSettingsThrows(): void + { + $site = $this->buildSite('main', 'w-123', 'i-456'); + $this->mockHandler->append(new Response(200, [], '{"status":"active","maxPrivacyModeTrackingCode":"tc-abc"}')); + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([]); + $this->siteSettingsService + ->method('writeSettings') + ->willThrowException(new \TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException('disk full', 1590487411)); + + $status = $this->subject->getStatus($site, forceRefresh: true); + $this->subject->syncSiteSettingsFromStatus($site, $status ?? []); + + $this->expectNotToPerformAssertions(); + } + + #[Test] + public function syncSiteSettingsFromStatusLogsWarningWhenSettingsCouldNotBePersisted(): void + { + $site = $this->buildSite('main', 'w-123', 'i-456'); + $this->mockHandler->append(new Response(200, [], '{"status":"active","maxPrivacyModeTrackingCode":"tc-abc"}')); + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([]); + $this->writeGuard + ->method('assertSettingsPersisted') + ->willThrowException(new AnalyticsApiException('Credentials could not be written.', 0)); + + $status = $this->subject->getStatus($site, forceRefresh: true); + $this->subject->syncSiteSettingsFromStatus($site, $status ?? []); + + // No exception propagated — the service warns and returns. + $this->expectNotToPerformAssertions(); + } + /** Helpers */ private function buildSite(string $identifier, string $websiteId, string $instanceId, array $extraSettings = []): Site diff --git a/Tests/Unit/Service/ApiKeyServiceTest.php b/Tests/Unit/Service/ApiKeyServiceTest.php index dcbb7620..71fa439e 100644 --- a/Tests/Unit/Service/ApiKeyServiceTest.php +++ b/Tests/Unit/Service/ApiKeyServiceTest.php @@ -12,11 +12,13 @@ use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\NullLogger; use T3G\Analytics\Configuration\ApiConfiguration; +use T3G\Analytics\Exception\AnalyticsApiException; use T3G\Analytics\Service\AnalyticsApiClient; use T3G\Analytics\Service\ApiExceptionExtractor; use T3G\Analytics\Service\ApiKeyService; use T3G\Analytics\Service\CipherService; use T3G\Analytics\Service\HmacSigner; +use T3G\Analytics\Service\SiteSettingsWriteGuardInterface; use TYPO3\CMS\Core\Http\Client\GuzzleClientFactory; use TYPO3\CMS\Core\Http\RequestFactory; use TYPO3\CMS\Core\Settings\Settings; @@ -34,6 +36,7 @@ final class ApiKeyServiceTest extends UnitTestCase private array $httpHistory = []; private SiteSettingsService&MockObject $siteSettingsService; private SiteSettingsFactory&MockObject $siteSettingsFactory; + private SiteSettingsWriteGuardInterface&MockObject $writeGuard; private CipherService $cipherService; private ApiKeyService $subject; @@ -56,6 +59,7 @@ protected function setUp(): void $this->siteSettingsService = $this->createMock(SiteSettingsService::class); $this->siteSettingsFactory = $this->createMock(SiteSettingsFactory::class); + $this->writeGuard = $this->createMock(SiteSettingsWriteGuardInterface::class); $GLOBALS['TYPO3_CONF_VARS']['EXTENSIONS']['analytics']['apiBaseUrl'] = ''; $GLOBALS['TYPO3_CONF_VARS']['EXTENSIONS']['analytics']['verifySsl'] = '0'; @@ -71,6 +75,7 @@ protected function setUp(): void $this->cipherService, $this->siteSettingsService, $this->siteSettingsFactory, + $this->writeGuard, new NullLogger(), ); } @@ -197,6 +202,37 @@ public function provisionIfNeededDoesNotWriteWhenResponseIsIncomplete(): void $this->subject->provisionIfNeeded($site, ['status' => 'active']); } + #[Test] + public function provisionIfNeededReturnsWhenWriteSettingsThrows(): void + { + $this->mockHandler->append(new Response(200, [], '{"apiKeyId":"new-key-uuid","apiKey":"new-api-key"}')); + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([]); + $this->siteSettingsService + ->method('writeSettings') + ->willThrowException(new \TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException('disk full', 1590487411)); + + $site = $this->buildSite('w-123', 'i-456', $this->encryptedSecret); + $this->subject->provisionIfNeeded($site, ['status' => 'active']); + + $this->expectNotToPerformAssertions(); + } + + #[Test] + public function provisionIfNeededLogsErrorWhenApiKeyCouldNotBePersisted(): void + { + $this->mockHandler->append(new Response(200, [], '{"apiKeyId":"new-key-uuid","apiKey":"new-api-key"}')); + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([]); + $this->writeGuard + ->method('assertSettingsPersisted') + ->willThrowException(new AnalyticsApiException('Credentials could not be written.', 0)); + + $site = $this->buildSite('w-123', 'i-456', $this->encryptedSecret); + $this->subject->provisionIfNeeded($site, ['status' => 'active']); + + // No exception thrown — the service returns silently and logs an error. + $this->expectNotToPerformAssertions(); + } + /** Helpers */ private function buildSite( diff --git a/Tests/Unit/Service/InstanceRegistrationServiceTest.php b/Tests/Unit/Service/InstanceRegistrationServiceTest.php index 803b5894..a9ec45cb 100644 --- a/Tests/Unit/Service/InstanceRegistrationServiceTest.php +++ b/Tests/Unit/Service/InstanceRegistrationServiceTest.php @@ -18,6 +18,7 @@ use T3G\Analytics\Service\CipherService; use T3G\Analytics\Service\HmacSigner; use T3G\Analytics\Service\InstanceRegistrationService; +use T3G\Analytics\Service\SiteSettingsWriteGuardInterface; use TYPO3\CMS\Core\Http\Client\GuzzleClientFactory; use TYPO3\CMS\Core\Http\RequestFactory; use TYPO3\CMS\Core\Http\Uri; @@ -36,6 +37,7 @@ final class InstanceRegistrationServiceTest extends UnitTestCase private array $httpHistory = []; private SiteSettingsService&MockObject $siteSettingsService; private SiteSettingsFactory&MockObject $siteSettingsFactory; + private SiteSettingsWriteGuardInterface&MockObject $writeGuard; private CipherService $cipherService; private InstanceRegistrationService $subject; @@ -54,6 +56,7 @@ protected function setUp(): void $this->siteSettingsService = $this->createMock(SiteSettingsService::class); $this->siteSettingsFactory = $this->createMock(SiteSettingsFactory::class); + $this->writeGuard = $this->createMock(SiteSettingsWriteGuardInterface::class); $this->cipherService = new CipherService(); $GLOBALS['TYPO3_CONF_VARS']['EXTENSIONS']['analytics']['apiBaseUrl'] = ''; @@ -70,6 +73,7 @@ protected function setUp(): void $this->cipherService, $this->siteSettingsService, $this->siteSettingsFactory, + $this->writeGuard, new NullLogger(), ); } @@ -168,6 +172,49 @@ public function registerStoresEmptySecretWhenApiResponseOmitsIt(): void $this->subject->register($this->buildSite(), 'user@example.com'); } + #[Test] + public function registerThrowsBeforeApiCallWhenDirectoryIsNotWritable(): void + { + $this->writeGuard + ->method('assertDirectoryWritable') + ->willThrowException(new AnalyticsApiException('Cannot write to config/sites/main/.', 0)); + + $this->expectException(AnalyticsApiException::class); + $this->expectExceptionMessage('Cannot write to config/sites/main/.'); + + $this->subject->register($this->buildSite(), 'user@example.com'); + + self::assertCount(0, $this->httpHistory, 'API must not be called when directory is not writable.'); + } + + #[Test] + public function registerThrowsAnalyticsApiExceptionWhenWriteSettingsThrows(): void + { + $this->mockHandler->append(new Response(200, [], '{"websiteId":"w-123","instanceId":"i-456","instanceSecret":"s3cr3t"}')); + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([]); + $this->siteSettingsService + ->method('writeSettings') + ->willThrowException(new \TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException('disk full', 1590487411)); + + $this->expectException(AnalyticsApiException::class); + + $this->subject->register($this->buildSite(), 'user@example.com'); + } + + #[Test] + public function registerThrowsWhenSettingsCouldNotBePersisted(): void + { + $this->mockHandler->append(new Response(200, [], '{"websiteId":"w-123","instanceId":"i-456","instanceSecret":"s3cr3t"}')); + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([]); + $this->writeGuard + ->method('assertSettingsPersisted') + ->willThrowException(new AnalyticsApiException('Credentials could not be written.', 0)); + + $this->expectException(AnalyticsApiException::class); + + $this->subject->register($this->buildSite(), 'user@example.com'); + } + /** Helpers */ private function buildSite(): Site diff --git a/Tests/Unit/Service/SiteDataProviderTest.php b/Tests/Unit/Service/SiteDataProviderTest.php index 987ced9b..b05fc1d5 100644 --- a/Tests/Unit/Service/SiteDataProviderTest.php +++ b/Tests/Unit/Service/SiteDataProviderTest.php @@ -103,6 +103,7 @@ protected function setUp(): void new NullLogger(), $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), + $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), ); $this->pageAccessChecker = $this->createMock(BackendPageAccessCheckerInterface::class); diff --git a/Tests/Unit/Service/SiteSettingsWriteGuardTest.php b/Tests/Unit/Service/SiteSettingsWriteGuardTest.php new file mode 100644 index 00000000..7bda5d5e --- /dev/null +++ b/Tests/Unit/Service/SiteSettingsWriteGuardTest.php @@ -0,0 +1,143 @@ +siteSettingsFactory = $this->createMock(SiteSettingsFactory::class); + $this->tempDir = sys_get_temp_dir() . '/analytics-guard-test-' . uniqid(); + mkdir($this->tempDir . '/main', 0755, recursive: true); + } + + protected function tearDown(): void + { + chmod($this->tempDir . '/main', 0755); + rmdir($this->tempDir . '/main'); + rmdir($this->tempDir); + parent::tearDown(); + } + + #[Test] + public function assertDirectoryWritableDoesNotThrowWhenDirectoryIsWritable(): void + { + $this->expectNotToPerformAssertions(); + + $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); + $guard->assertDirectoryWritable($this->buildSite()); + } + + #[Test] + public function assertDirectoryWritableThrowsWhenDirectoryIsNotWritable(): void + { + if (posix_getuid() === 0) { + self::markTestSkipped('chmod-based permission tests do not apply when running as root.'); + } + + chmod($this->tempDir . '/main', 0555); + + $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); + + $this->expectException(AnalyticsApiException::class); + $this->expectExceptionMessageMatches('/config\/sites\/main/'); + + $guard->assertDirectoryWritable($this->buildSite()); + } + + #[Test] + public function assertDirectoryWritableThrowsWhenDirectoryDoesNotExist(): void + { + $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, '/nonexistent/path'); + + $this->expectException(AnalyticsApiException::class); + + $guard->assertDirectoryWritable($this->buildSite()); + } + + #[Test] + public function assertSettingsPersistedDoesNotThrowWhenAllValuesMatch(): void + { + $this->expectNotToPerformAssertions(); + + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([ + 'websiteId' => 'w-123', + 'instanceId' => 'i-456', + ]); + $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); + + $guard->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123', 'instanceId' => 'i-456']); + } + + #[Test] + public function assertSettingsPersistedThrowsWhenKeyIsAbsent(): void + { + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([]); + $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); + + $this->expectException(AnalyticsApiException::class); + $this->expectExceptionMessageMatches('/config\/sites\/main\/settings\.yaml/'); + + $guard->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123']); + } + + #[Test] + public function assertSettingsPersistedThrowsWhenValueDoesNotMatch(): void + { + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn(['websiteId' => 'other-id']); + $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); + + $this->expectException(AnalyticsApiException::class); + + $guard->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123']); + } + + #[Test] + public function assertSettingsPersistedThrowsOnFirstFailingKey(): void + { + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn(['websiteId' => 'w-123']); + $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); + + $this->expectException(AnalyticsApiException::class); + + $guard->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123', 'instanceId' => 'i-456']); + } + + #[Test] + public function assertSettingsPersistedReadsSettingsOnlyOnce(): void + { + $this->siteSettingsFactory + ->expects(self::once()) + ->method('loadLocalSettings') + ->willReturn(['websiteId' => 'w-123', 'instanceId' => 'i-456']); + + $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); + $guard->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123', 'instanceId' => 'i-456']); + } + + private function buildSite(): Site + { + $site = $this->createMock(Site::class); + $site->method('getIdentifier')->willReturn('main'); + $site->method('getBase')->willReturn(new Uri('https://example.com')); + $site->method('getSettings')->willReturn(new SiteSettings(new Settings([]), [], [])); + return $site; + } +} From d911b1b6d8cc4e32ac64c952e92217d449e8f7ea Mon Sep 17 00:00:00 2001 From: Simon Schmidt Date: Tue, 1 Sep 2026 10:40:32 +0200 Subject: [PATCH 2/4] [TASK] update after review --- Classes/Service/ApiKeyService.php | 2 +- Classes/Service/SiteSettingsWriteGuard.php | 2 +- .../Controller/BackendModuleControllerTest.php | 7 ++++--- .../Service/SiteSettingsWriteGuardTest.php | 9 ++++----- 4 files changed, 10 insertions(+), 10 deletions(-) rename Tests/{Unit => Functional}/Service/SiteSettingsWriteGuardTest.php (94%) diff --git a/Classes/Service/ApiKeyService.php b/Classes/Service/ApiKeyService.php index 6bb3f2f4..1ee0fc4c 100644 --- a/Classes/Service/ApiKeyService.php +++ b/Classes/Service/ApiKeyService.php @@ -110,7 +110,7 @@ public function provisionIfNeeded(Site $site, array $currentStatus): void 'apiKey' => $encryptedApiKey, ])); } catch (SiteConfigurationWriteException $e) { - $this->logger->error('ApiKeyService: writeSettings threw.', [ + $this->logger->error('ApiKeyService: Failed to write API key settings to site configuration.', [ 'siteIdentifier' => $siteIdentifier, 'exception' => $e->getMessage(), ]); diff --git a/Classes/Service/SiteSettingsWriteGuard.php b/Classes/Service/SiteSettingsWriteGuard.php index 31cce6d3..034e1fbd 100644 --- a/Classes/Service/SiteSettingsWriteGuard.php +++ b/Classes/Service/SiteSettingsWriteGuard.php @@ -24,7 +24,7 @@ public function assertDirectoryWritable(Site $site): void if (!is_dir($configDir) || !is_writable($configDir)) { throw new AnalyticsApiException( - 'Cannot write to config/sites/' . $identifier . '/. Check file system permissions for this directory.', + 'Cannot write to ' . $configDir . '/. Check file system permissions for this directory.', 0 ); } diff --git a/Tests/Functional/Controller/BackendModuleControllerTest.php b/Tests/Functional/Controller/BackendModuleControllerTest.php index c789dcc7..d339da56 100644 --- a/Tests/Functional/Controller/BackendModuleControllerTest.php +++ b/Tests/Functional/Controller/BackendModuleControllerTest.php @@ -22,6 +22,7 @@ use T3G\Analytics\Service\ApiKeyService; use T3G\Analytics\Service\InstanceRegistrationService; use T3G\Analytics\Service\SiteDataProvider; +use T3G\Analytics\Service\SiteSettingsWriteGuardInterface; use T3G\Analytics\Service\BackendPageAccessCheckerInterface; use T3G\Analytics\Tests\Functional\Bootstrap\FunctionalTestCase; use TYPO3\CMS\Backend\Routing\Route; @@ -415,7 +416,7 @@ private function buildController( new NullLogger(), $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), - $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), + $this->createMock(SiteSettingsWriteGuardInterface::class), ); $siteDataProvider = new SiteDataProvider( @@ -437,7 +438,7 @@ private function buildController( $cipherService, $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), - $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), + $this->createMock(SiteSettingsWriteGuardInterface::class), new NullLogger(), ); @@ -446,7 +447,7 @@ private function buildController( $cipherService, $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), - $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), + $this->createMock(SiteSettingsWriteGuardInterface::class), new NullLogger(), ); diff --git a/Tests/Unit/Service/SiteSettingsWriteGuardTest.php b/Tests/Functional/Service/SiteSettingsWriteGuardTest.php similarity index 94% rename from Tests/Unit/Service/SiteSettingsWriteGuardTest.php rename to Tests/Functional/Service/SiteSettingsWriteGuardTest.php index 7bda5d5e..b50d51a9 100644 --- a/Tests/Unit/Service/SiteSettingsWriteGuardTest.php +++ b/Tests/Functional/Service/SiteSettingsWriteGuardTest.php @@ -2,20 +2,20 @@ declare(strict_types=1); -namespace T3G\Analytics\Tests\Unit\Service; +namespace T3G\Analytics\Tests\Functional\Service; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; use T3G\Analytics\Exception\AnalyticsApiException; use T3G\Analytics\Service\SiteSettingsWriteGuard; +use T3G\Analytics\Tests\Functional\Bootstrap\FunctionalTestCase; use TYPO3\CMS\Core\Http\Uri; use TYPO3\CMS\Core\Settings\Settings; use TYPO3\CMS\Core\Site\Entity\Site; use TYPO3\CMS\Core\Site\Entity\SiteSettings; use TYPO3\CMS\Core\Site\SiteSettingsFactory; -use TYPO3\TestingFramework\Core\Unit\UnitTestCase; -final class SiteSettingsWriteGuardTest extends UnitTestCase +final class SiteSettingsWriteGuardTest extends FunctionalTestCase { private SiteSettingsFactory&MockObject $siteSettingsFactory; private string $tempDir; @@ -24,7 +24,7 @@ protected function setUp(): void { parent::setUp(); $this->siteSettingsFactory = $this->createMock(SiteSettingsFactory::class); - $this->tempDir = sys_get_temp_dir() . '/analytics-guard-test-' . uniqid(); + $this->tempDir = $this->getInstancePath() . '/analytics-guard-test'; mkdir($this->tempDir . '/main', 0755, recursive: true); } @@ -57,7 +57,6 @@ public function assertDirectoryWritableThrowsWhenDirectoryIsNotWritable(): void $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); $this->expectException(AnalyticsApiException::class); - $this->expectExceptionMessageMatches('/config\/sites\/main/'); $guard->assertDirectoryWritable($this->buildSite()); } From 2b6176cb1793911d44ffdfe029e674a580bba1fb Mon Sep 17 00:00:00 2001 From: Simon Schmidt Date: Tue, 1 Sep 2026 15:58:57 +0200 Subject: [PATCH 3/4] [TASK] address review feedback on SiteSettingsWriteGuard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - riname SiteSettingsWriteGuard → SiteSettingsWriteVerifier (incl. interface) - make SiteSettingsWriteVerifier final - remove $sitesConfigPath test-seam; use Environment::getConfigPath() directly - simplify exception messages to only include the site identifier - improve ApiKeyService log message for writeSettings failure - add `use` statements for SiteSettingsWriteVerifierInterface in all test files - move SiteSettingsWriteVerifierTest to Functional, drop assertDirectoryWritable tests - drop posix_getuid() root check from tests --- Classes/Service/AnalyticsStatusService.php | 2 +- Classes/Service/ApiKeyService.php | 2 +- .../Service/InstanceRegistrationService.php | 2 +- .../SiteSettingsWriteGuardInterface.php | 27 ---- ...uard.php => SiteSettingsWriteVerifier.php} | 13 +- .../SiteSettingsWriteVerifierInterface.php | 26 ++++ Configuration/Services.yaml | 4 +- .../BackendModuleControllerTest.php | 8 +- .../Service/AnalyticsStatusServiceTest.php | 3 +- .../InstanceRegistrationServiceTest.php | 3 +- .../Service/SiteSettingsWriteGuardTest.php | 142 ------------------ .../Service/SiteSettingsWriteVerifierTest.php | 96 ++++++++++++ .../BackendModuleControllerTest.php | 7 +- .../Service/AnalyticsStatusServiceTest.php | 6 +- Tests/Unit/Service/ApiKeyServiceTest.php | 6 +- .../InstanceRegistrationServiceTest.php | 6 +- Tests/Unit/Service/SiteDataProviderTest.php | 3 +- 17 files changed, 154 insertions(+), 202 deletions(-) delete mode 100644 Classes/Service/SiteSettingsWriteGuardInterface.php rename Classes/Service/{SiteSettingsWriteGuard.php => SiteSettingsWriteVerifier.php} (62%) create mode 100644 Classes/Service/SiteSettingsWriteVerifierInterface.php delete mode 100644 Tests/Functional/Service/SiteSettingsWriteGuardTest.php create mode 100644 Tests/Functional/Service/SiteSettingsWriteVerifierTest.php diff --git a/Classes/Service/AnalyticsStatusService.php b/Classes/Service/AnalyticsStatusService.php index 0b6fd415..4ee03abe 100644 --- a/Classes/Service/AnalyticsStatusService.php +++ b/Classes/Service/AnalyticsStatusService.php @@ -26,7 +26,7 @@ public function __construct( private LoggerInterface $logger, private SiteSettingsService $siteSettingsService, private SiteSettingsFactory $siteSettingsFactory, - private SiteSettingsWriteGuardInterface $writeGuard, + private SiteSettingsWriteVerifierInterface $writeGuard, ) { } diff --git a/Classes/Service/ApiKeyService.php b/Classes/Service/ApiKeyService.php index 1ee0fc4c..a0e8f1af 100644 --- a/Classes/Service/ApiKeyService.php +++ b/Classes/Service/ApiKeyService.php @@ -20,7 +20,7 @@ public function __construct( private CipherServiceInterface $cipherService, private SiteSettingsService $siteSettingsService, private SiteSettingsFactory $siteSettingsFactory, - private SiteSettingsWriteGuardInterface $writeGuard, + private SiteSettingsWriteVerifierInterface $writeGuard, private LoggerInterface $logger, ) { } diff --git a/Classes/Service/InstanceRegistrationService.php b/Classes/Service/InstanceRegistrationService.php index aa77da2e..e3cab375 100644 --- a/Classes/Service/InstanceRegistrationService.php +++ b/Classes/Service/InstanceRegistrationService.php @@ -18,7 +18,7 @@ public function __construct( private CipherServiceInterface $cipherService, private SiteSettingsService $siteSettingsService, private SiteSettingsFactory $siteSettingsFactory, - private SiteSettingsWriteGuardInterface $writeGuard, + private SiteSettingsWriteVerifierInterface $writeGuard, private LoggerInterface $logger, ) { } diff --git a/Classes/Service/SiteSettingsWriteGuardInterface.php b/Classes/Service/SiteSettingsWriteGuardInterface.php deleted file mode 100644 index 8f318438..00000000 --- a/Classes/Service/SiteSettingsWriteGuardInterface.php +++ /dev/null @@ -1,27 +0,0 @@ - $expected key-value pairs that must be present after the write - * @throws AnalyticsApiException when any value is absent or does not match - */ - public function assertSettingsPersisted(Site $site, array $expected): void; -} diff --git a/Classes/Service/SiteSettingsWriteGuard.php b/Classes/Service/SiteSettingsWriteVerifier.php similarity index 62% rename from Classes/Service/SiteSettingsWriteGuard.php rename to Classes/Service/SiteSettingsWriteVerifier.php index 034e1fbd..22486cb7 100644 --- a/Classes/Service/SiteSettingsWriteGuard.php +++ b/Classes/Service/SiteSettingsWriteVerifier.php @@ -9,22 +9,21 @@ use TYPO3\CMS\Core\Site\Entity\Site; use TYPO3\CMS\Core\Site\SiteSettingsFactory; -readonly class SiteSettingsWriteGuard implements SiteSettingsWriteGuardInterface +final readonly class SiteSettingsWriteVerifier implements SiteSettingsWriteVerifierInterface { public function __construct( private SiteSettingsFactory $siteSettingsFactory, - private string $sitesConfigPath = '', ) { } public function assertDirectoryWritable(Site $site): void { $identifier = $site->getIdentifier(); - $configDir = $this->resolveSitesConfigPath() . '/' . $identifier; + $configDir = Environment::getConfigPath() . '/sites/' . $identifier; if (!is_dir($configDir) || !is_writable($configDir)) { throw new AnalyticsApiException( - 'Cannot write to ' . $configDir . '/. Check file system permissions for this directory.', + 'Cannot write to site configuration directory for "' . $identifier . '". Check file system permissions.', 0 ); } @@ -38,15 +37,11 @@ public function assertSettingsPersisted(Site $site, array $expected): void foreach ($expected as $key => $value) { if (($persisted[$key] ?? '') !== $value) { throw new AnalyticsApiException( - 'Settings could not be written to config/sites/' . $identifier . '/settings.yaml. Check file system permissions for this directory.', + 'Settings for site "' . $identifier . '" could not be persisted. Check file system permissions.', 0 ); } } } - private function resolveSitesConfigPath(): string - { - return $this->sitesConfigPath !== '' ? $this->sitesConfigPath : Environment::getConfigPath() . '/sites'; - } } diff --git a/Classes/Service/SiteSettingsWriteVerifierInterface.php b/Classes/Service/SiteSettingsWriteVerifierInterface.php new file mode 100644 index 00000000..551dc78f --- /dev/null +++ b/Classes/Service/SiteSettingsWriteVerifierInterface.php @@ -0,0 +1,26 @@ + $expected key-value pairs that must appear in the persisted settings + * @throws \T3G\Analytics\Exception\AnalyticsApiException + */ + public function assertSettingsPersisted(Site $site, array $expected): void; +} diff --git a/Configuration/Services.yaml b/Configuration/Services.yaml index 6790fcfa..b9d16de4 100644 --- a/Configuration/Services.yaml +++ b/Configuration/Services.yaml @@ -12,8 +12,8 @@ services: - '../Classes/Dashboard/Widget/TrafficGraphWidgetV14.php' - '../Classes/Dashboard/Widget/TrafficSourcesWidgetV14.php' - T3G\Analytics\Service\SiteSettingsWriteGuardInterface: - alias: T3G\Analytics\Service\SiteSettingsWriteGuard + T3G\Analytics\Service\SiteSettingsWriteVerifierInterface: + alias: T3G\Analytics\Service\SiteSettingsWriteVerifier T3G\Analytics\Service\TopPagesServiceInterface: alias: T3G\Analytics\Service\TopPagesService diff --git a/Tests/Functional/Controller/BackendModuleControllerTest.php b/Tests/Functional/Controller/BackendModuleControllerTest.php index d339da56..39ba3dbf 100644 --- a/Tests/Functional/Controller/BackendModuleControllerTest.php +++ b/Tests/Functional/Controller/BackendModuleControllerTest.php @@ -22,7 +22,7 @@ use T3G\Analytics\Service\ApiKeyService; use T3G\Analytics\Service\InstanceRegistrationService; use T3G\Analytics\Service\SiteDataProvider; -use T3G\Analytics\Service\SiteSettingsWriteGuardInterface; +use T3G\Analytics\Service\SiteSettingsWriteVerifierInterface; use T3G\Analytics\Service\BackendPageAccessCheckerInterface; use T3G\Analytics\Tests\Functional\Bootstrap\FunctionalTestCase; use TYPO3\CMS\Backend\Routing\Route; @@ -416,7 +416,7 @@ private function buildController( new NullLogger(), $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), - $this->createMock(SiteSettingsWriteGuardInterface::class), + $this->createMock(SiteSettingsWriteVerifierInterface::class), ); $siteDataProvider = new SiteDataProvider( @@ -438,7 +438,7 @@ private function buildController( $cipherService, $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), - $this->createMock(SiteSettingsWriteGuardInterface::class), + $this->createMock(SiteSettingsWriteVerifierInterface::class), new NullLogger(), ); @@ -447,7 +447,7 @@ private function buildController( $cipherService, $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), - $this->createMock(SiteSettingsWriteGuardInterface::class), + $this->createMock(SiteSettingsWriteVerifierInterface::class), new NullLogger(), ); diff --git a/Tests/Functional/Service/AnalyticsStatusServiceTest.php b/Tests/Functional/Service/AnalyticsStatusServiceTest.php index 5d50741e..44ddf0f6 100644 --- a/Tests/Functional/Service/AnalyticsStatusServiceTest.php +++ b/Tests/Functional/Service/AnalyticsStatusServiceTest.php @@ -15,6 +15,7 @@ use T3G\Analytics\Service\ApiExceptionExtractor; use T3G\Analytics\Service\CipherService; use T3G\Analytics\Service\HmacSigner; +use T3G\Analytics\Service\SiteSettingsWriteVerifierInterface; use T3G\Analytics\Tests\Functional\Bootstrap\FunctionalTestCase; use TYPO3\CMS\Core\Cache\CacheManager; use TYPO3\CMS\Core\Cache\Frontend\FrontendInterface; @@ -73,7 +74,7 @@ protected function setUp(): void new \Psr\Log\NullLogger(), $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), - $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), + $this->createMock(SiteSettingsWriteVerifierInterface::class), ); } diff --git a/Tests/Functional/Service/InstanceRegistrationServiceTest.php b/Tests/Functional/Service/InstanceRegistrationServiceTest.php index be7a7cb5..33f3deb1 100644 --- a/Tests/Functional/Service/InstanceRegistrationServiceTest.php +++ b/Tests/Functional/Service/InstanceRegistrationServiceTest.php @@ -16,6 +16,7 @@ use T3G\Analytics\Service\CipherService; use T3G\Analytics\Service\HmacSigner; use T3G\Analytics\Service\InstanceRegistrationService; +use T3G\Analytics\Service\SiteSettingsWriteVerifierInterface; use T3G\Analytics\Tests\Functional\Bootstrap\FunctionalTestCase; use TYPO3\CMS\Core\Http\Client\GuzzleClientFactory; use TYPO3\CMS\Core\Http\RequestFactory; @@ -75,7 +76,7 @@ protected function setUp(): void $this->cipherService, $this->siteSettingsService, $this->siteSettingsFactory, - $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), + $this->createMock(SiteSettingsWriteVerifierInterface::class), new \Psr\Log\NullLogger(), ); } diff --git a/Tests/Functional/Service/SiteSettingsWriteGuardTest.php b/Tests/Functional/Service/SiteSettingsWriteGuardTest.php deleted file mode 100644 index b50d51a9..00000000 --- a/Tests/Functional/Service/SiteSettingsWriteGuardTest.php +++ /dev/null @@ -1,142 +0,0 @@ -siteSettingsFactory = $this->createMock(SiteSettingsFactory::class); - $this->tempDir = $this->getInstancePath() . '/analytics-guard-test'; - mkdir($this->tempDir . '/main', 0755, recursive: true); - } - - protected function tearDown(): void - { - chmod($this->tempDir . '/main', 0755); - rmdir($this->tempDir . '/main'); - rmdir($this->tempDir); - parent::tearDown(); - } - - #[Test] - public function assertDirectoryWritableDoesNotThrowWhenDirectoryIsWritable(): void - { - $this->expectNotToPerformAssertions(); - - $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); - $guard->assertDirectoryWritable($this->buildSite()); - } - - #[Test] - public function assertDirectoryWritableThrowsWhenDirectoryIsNotWritable(): void - { - if (posix_getuid() === 0) { - self::markTestSkipped('chmod-based permission tests do not apply when running as root.'); - } - - chmod($this->tempDir . '/main', 0555); - - $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); - - $this->expectException(AnalyticsApiException::class); - - $guard->assertDirectoryWritable($this->buildSite()); - } - - #[Test] - public function assertDirectoryWritableThrowsWhenDirectoryDoesNotExist(): void - { - $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, '/nonexistent/path'); - - $this->expectException(AnalyticsApiException::class); - - $guard->assertDirectoryWritable($this->buildSite()); - } - - #[Test] - public function assertSettingsPersistedDoesNotThrowWhenAllValuesMatch(): void - { - $this->expectNotToPerformAssertions(); - - $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([ - 'websiteId' => 'w-123', - 'instanceId' => 'i-456', - ]); - $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); - - $guard->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123', 'instanceId' => 'i-456']); - } - - #[Test] - public function assertSettingsPersistedThrowsWhenKeyIsAbsent(): void - { - $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([]); - $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); - - $this->expectException(AnalyticsApiException::class); - $this->expectExceptionMessageMatches('/config\/sites\/main\/settings\.yaml/'); - - $guard->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123']); - } - - #[Test] - public function assertSettingsPersistedThrowsWhenValueDoesNotMatch(): void - { - $this->siteSettingsFactory->method('loadLocalSettings')->willReturn(['websiteId' => 'other-id']); - $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); - - $this->expectException(AnalyticsApiException::class); - - $guard->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123']); - } - - #[Test] - public function assertSettingsPersistedThrowsOnFirstFailingKey(): void - { - $this->siteSettingsFactory->method('loadLocalSettings')->willReturn(['websiteId' => 'w-123']); - $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); - - $this->expectException(AnalyticsApiException::class); - - $guard->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123', 'instanceId' => 'i-456']); - } - - #[Test] - public function assertSettingsPersistedReadsSettingsOnlyOnce(): void - { - $this->siteSettingsFactory - ->expects(self::once()) - ->method('loadLocalSettings') - ->willReturn(['websiteId' => 'w-123', 'instanceId' => 'i-456']); - - $guard = new SiteSettingsWriteGuard($this->siteSettingsFactory, $this->tempDir); - $guard->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123', 'instanceId' => 'i-456']); - } - - private function buildSite(): Site - { - $site = $this->createMock(Site::class); - $site->method('getIdentifier')->willReturn('main'); - $site->method('getBase')->willReturn(new Uri('https://example.com')); - $site->method('getSettings')->willReturn(new SiteSettings(new Settings([]), [], [])); - return $site; - } -} diff --git a/Tests/Functional/Service/SiteSettingsWriteVerifierTest.php b/Tests/Functional/Service/SiteSettingsWriteVerifierTest.php new file mode 100644 index 00000000..0a71a969 --- /dev/null +++ b/Tests/Functional/Service/SiteSettingsWriteVerifierTest.php @@ -0,0 +1,96 @@ +siteSettingsFactory = $this->createMock(SiteSettingsFactory::class); + } + + #[Test] + public function assertSettingsPersistedDoesNotThrowWhenAllValuesMatch(): void + { + $this->expectNotToPerformAssertions(); + + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([ + 'websiteId' => 'w-123', + 'instanceId' => 'i-456', + ]); + $verifier = new SiteSettingsWriteVerifier($this->siteSettingsFactory); + + $verifier->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123', 'instanceId' => 'i-456']); + } + + #[Test] + public function assertSettingsPersistedThrowsWhenKeyIsAbsent(): void + { + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([]); + $verifier = new SiteSettingsWriteVerifier($this->siteSettingsFactory); + + $this->expectException(AnalyticsApiException::class); + $this->expectExceptionMessageMatches('/"main"/'); + + $verifier->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123']); + } + + #[Test] + public function assertSettingsPersistedThrowsWhenValueDoesNotMatch(): void + { + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn(['websiteId' => 'other-id']); + $verifier = new SiteSettingsWriteVerifier($this->siteSettingsFactory); + + $this->expectException(AnalyticsApiException::class); + + $verifier->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123']); + } + + #[Test] + public function assertSettingsPersistedThrowsOnFirstFailingKey(): void + { + $this->siteSettingsFactory->method('loadLocalSettings')->willReturn(['websiteId' => 'w-123']); + $verifier = new SiteSettingsWriteVerifier($this->siteSettingsFactory); + + $this->expectException(AnalyticsApiException::class); + + $verifier->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123', 'instanceId' => 'i-456']); + } + + #[Test] + public function assertSettingsPersistedReadsSettingsOnlyOnce(): void + { + $this->siteSettingsFactory + ->expects(self::once()) + ->method('loadLocalSettings') + ->willReturn(['websiteId' => 'w-123', 'instanceId' => 'i-456']); + + $verifier = new SiteSettingsWriteVerifier($this->siteSettingsFactory); + $verifier->assertSettingsPersisted($this->buildSite(), ['websiteId' => 'w-123', 'instanceId' => 'i-456']); + } + + private function buildSite(): Site + { + $site = $this->createMock(Site::class); + $site->method('getIdentifier')->willReturn('main'); + $site->method('getBase')->willReturn(new Uri('https://example.com')); + $site->method('getSettings')->willReturn(new SiteSettings(new Settings([]), [], [])); + return $site; + } +} diff --git a/Tests/Unit/Controller/BackendModuleControllerTest.php b/Tests/Unit/Controller/BackendModuleControllerTest.php index ef5d1452..ac49de25 100644 --- a/Tests/Unit/Controller/BackendModuleControllerTest.php +++ b/Tests/Unit/Controller/BackendModuleControllerTest.php @@ -24,6 +24,7 @@ use T3G\Analytics\Service\HmacSigner; use T3G\Analytics\Service\InstanceRegistrationService; use T3G\Analytics\Service\SiteDataProvider; +use T3G\Analytics\Service\SiteSettingsWriteVerifierInterface; use T3G\Analytics\Service\BackendPageAccessCheckerInterface; use TYPO3\CMS\Core\Database\ConnectionPool; use TYPO3\CMS\Core\Database\Query\QueryBuilder; @@ -136,7 +137,7 @@ protected function setUp(): void new NullLogger(), $this->siteSettingsService, $this->siteSettingsFactory, - $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), + $this->createMock(SiteSettingsWriteVerifierInterface::class), ); $registrationService = new InstanceRegistrationService( @@ -144,7 +145,7 @@ protected function setUp(): void $cipherService, $this->siteSettingsService, $this->siteSettingsFactory, - $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), + $this->createMock(SiteSettingsWriteVerifierInterface::class), new NullLogger(), ); @@ -153,7 +154,7 @@ protected function setUp(): void $cipherService, $this->siteSettingsService, $this->siteSettingsFactory, - $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), + $this->createMock(SiteSettingsWriteVerifierInterface::class), new NullLogger(), ); diff --git a/Tests/Unit/Service/AnalyticsStatusServiceTest.php b/Tests/Unit/Service/AnalyticsStatusServiceTest.php index 7d3b4a5e..17e81afd 100644 --- a/Tests/Unit/Service/AnalyticsStatusServiceTest.php +++ b/Tests/Unit/Service/AnalyticsStatusServiceTest.php @@ -18,7 +18,7 @@ use T3G\Analytics\Service\ApiExceptionExtractor; use T3G\Analytics\Service\CipherService; use T3G\Analytics\Service\HmacSigner; -use T3G\Analytics\Service\SiteSettingsWriteGuardInterface; +use T3G\Analytics\Service\SiteSettingsWriteVerifierInterface; use TYPO3\CMS\Core\Cache\Backend\TransientMemoryBackend; use TYPO3\CMS\Core\Cache\Frontend\VariableFrontend; use TYPO3\CMS\Core\Information\Typo3Version; @@ -40,7 +40,7 @@ final class AnalyticsStatusServiceTest extends UnitTestCase private array $httpHistory = []; private SiteSettingsService&MockObject $siteSettingsService; private SiteSettingsFactory&MockObject $siteSettingsFactory; - private SiteSettingsWriteGuardInterface&MockObject $writeGuard; + private SiteSettingsWriteVerifierInterface&MockObject $writeGuard; private VariableFrontend $cache; private string $encryptedTestSecret; @@ -61,7 +61,7 @@ protected function setUp(): void $this->siteSettingsService = $this->createMock(SiteSettingsService::class); $this->siteSettingsFactory = $this->createMock(SiteSettingsFactory::class); - $this->writeGuard = $this->createMock(SiteSettingsWriteGuardInterface::class); + $this->writeGuard = $this->createMock(SiteSettingsWriteVerifierInterface::class); // TransientMemoryBackend dropped the $context parameter in TYPO3 v14. $backend = (new Typo3Version())->getMajorVersion() >= 14 ? new TransientMemoryBackend() // @phpstan-ignore argument.count diff --git a/Tests/Unit/Service/ApiKeyServiceTest.php b/Tests/Unit/Service/ApiKeyServiceTest.php index 71fa439e..65441ec8 100644 --- a/Tests/Unit/Service/ApiKeyServiceTest.php +++ b/Tests/Unit/Service/ApiKeyServiceTest.php @@ -18,7 +18,7 @@ use T3G\Analytics\Service\ApiKeyService; use T3G\Analytics\Service\CipherService; use T3G\Analytics\Service\HmacSigner; -use T3G\Analytics\Service\SiteSettingsWriteGuardInterface; +use T3G\Analytics\Service\SiteSettingsWriteVerifierInterface; use TYPO3\CMS\Core\Http\Client\GuzzleClientFactory; use TYPO3\CMS\Core\Http\RequestFactory; use TYPO3\CMS\Core\Settings\Settings; @@ -36,7 +36,7 @@ final class ApiKeyServiceTest extends UnitTestCase private array $httpHistory = []; private SiteSettingsService&MockObject $siteSettingsService; private SiteSettingsFactory&MockObject $siteSettingsFactory; - private SiteSettingsWriteGuardInterface&MockObject $writeGuard; + private SiteSettingsWriteVerifierInterface&MockObject $writeGuard; private CipherService $cipherService; private ApiKeyService $subject; @@ -59,7 +59,7 @@ protected function setUp(): void $this->siteSettingsService = $this->createMock(SiteSettingsService::class); $this->siteSettingsFactory = $this->createMock(SiteSettingsFactory::class); - $this->writeGuard = $this->createMock(SiteSettingsWriteGuardInterface::class); + $this->writeGuard = $this->createMock(SiteSettingsWriteVerifierInterface::class); $GLOBALS['TYPO3_CONF_VARS']['EXTENSIONS']['analytics']['apiBaseUrl'] = ''; $GLOBALS['TYPO3_CONF_VARS']['EXTENSIONS']['analytics']['verifySsl'] = '0'; diff --git a/Tests/Unit/Service/InstanceRegistrationServiceTest.php b/Tests/Unit/Service/InstanceRegistrationServiceTest.php index a9ec45cb..27eeb12f 100644 --- a/Tests/Unit/Service/InstanceRegistrationServiceTest.php +++ b/Tests/Unit/Service/InstanceRegistrationServiceTest.php @@ -18,7 +18,7 @@ use T3G\Analytics\Service\CipherService; use T3G\Analytics\Service\HmacSigner; use T3G\Analytics\Service\InstanceRegistrationService; -use T3G\Analytics\Service\SiteSettingsWriteGuardInterface; +use T3G\Analytics\Service\SiteSettingsWriteVerifierInterface; use TYPO3\CMS\Core\Http\Client\GuzzleClientFactory; use TYPO3\CMS\Core\Http\RequestFactory; use TYPO3\CMS\Core\Http\Uri; @@ -37,7 +37,7 @@ final class InstanceRegistrationServiceTest extends UnitTestCase private array $httpHistory = []; private SiteSettingsService&MockObject $siteSettingsService; private SiteSettingsFactory&MockObject $siteSettingsFactory; - private SiteSettingsWriteGuardInterface&MockObject $writeGuard; + private SiteSettingsWriteVerifierInterface&MockObject $writeGuard; private CipherService $cipherService; private InstanceRegistrationService $subject; @@ -56,7 +56,7 @@ protected function setUp(): void $this->siteSettingsService = $this->createMock(SiteSettingsService::class); $this->siteSettingsFactory = $this->createMock(SiteSettingsFactory::class); - $this->writeGuard = $this->createMock(SiteSettingsWriteGuardInterface::class); + $this->writeGuard = $this->createMock(SiteSettingsWriteVerifierInterface::class); $this->cipherService = new CipherService(); $GLOBALS['TYPO3_CONF_VARS']['EXTENSIONS']['analytics']['apiBaseUrl'] = ''; diff --git a/Tests/Unit/Service/SiteDataProviderTest.php b/Tests/Unit/Service/SiteDataProviderTest.php index b05fc1d5..c6116c90 100644 --- a/Tests/Unit/Service/SiteDataProviderTest.php +++ b/Tests/Unit/Service/SiteDataProviderTest.php @@ -19,6 +19,7 @@ use T3G\Analytics\Service\CipherService; use T3G\Analytics\Service\HmacSigner; use T3G\Analytics\Service\BackendPageAccessCheckerInterface; +use T3G\Analytics\Service\SiteSettingsWriteVerifierInterface; use T3G\Analytics\Service\SiteDataProvider; use TYPO3\CMS\Backend\Routing\UriBuilder; use TYPO3\CMS\Core\Cache\Backend\TransientMemoryBackend; @@ -103,7 +104,7 @@ protected function setUp(): void new NullLogger(), $this->createMock(SiteSettingsService::class), $this->createMock(SiteSettingsFactory::class), - $this->createMock(\T3G\Analytics\Service\SiteSettingsWriteGuardInterface::class), + $this->createMock(SiteSettingsWriteVerifierInterface::class), ); $this->pageAccessChecker = $this->createMock(BackendPageAccessCheckerInterface::class); From ba328ca0182f419f4f28c8bd4a3fe56063be8abf Mon Sep 17 00:00:00 2001 From: Simon Schmidt Date: Tue, 8 Sep 2026 08:54:56 +0200 Subject: [PATCH 4/4] [TASK] update FQCNs --- Classes/Service/SiteSettingsWriteVerifierInterface.php | 5 +++-- Tests/Unit/Service/AnalyticsStatusServiceTest.php | 3 ++- Tests/Unit/Service/ApiKeyServiceTest.php | 3 ++- Tests/Unit/Service/InstanceRegistrationServiceTest.php | 3 ++- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/Classes/Service/SiteSettingsWriteVerifierInterface.php b/Classes/Service/SiteSettingsWriteVerifierInterface.php index 551dc78f..4eea638d 100644 --- a/Classes/Service/SiteSettingsWriteVerifierInterface.php +++ b/Classes/Service/SiteSettingsWriteVerifierInterface.php @@ -4,6 +4,7 @@ namespace T3G\Analytics\Service; +use T3G\Analytics\Exception\AnalyticsApiException; use TYPO3\CMS\Core\Site\Entity\Site; interface SiteSettingsWriteVerifierInterface @@ -11,7 +12,7 @@ interface SiteSettingsWriteVerifierInterface /** * Asserts that the site configuration directory is writable. * - * @throws \T3G\Analytics\Exception\AnalyticsApiException + * @throws AnalyticsApiException */ public function assertDirectoryWritable(Site $site): void; @@ -20,7 +21,7 @@ public function assertDirectoryWritable(Site $site): void; * Call this after writeSettings() to detect silent write failures. * * @param array $expected key-value pairs that must appear in the persisted settings - * @throws \T3G\Analytics\Exception\AnalyticsApiException + * @throws AnalyticsApiException */ public function assertSettingsPersisted(Site $site, array $expected): void; } diff --git a/Tests/Unit/Service/AnalyticsStatusServiceTest.php b/Tests/Unit/Service/AnalyticsStatusServiceTest.php index 17e81afd..a3abb061 100644 --- a/Tests/Unit/Service/AnalyticsStatusServiceTest.php +++ b/Tests/Unit/Service/AnalyticsStatusServiceTest.php @@ -21,6 +21,7 @@ use T3G\Analytics\Service\SiteSettingsWriteVerifierInterface; use TYPO3\CMS\Core\Cache\Backend\TransientMemoryBackend; use TYPO3\CMS\Core\Cache\Frontend\VariableFrontend; +use TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException; use TYPO3\CMS\Core\Information\Typo3Version; use TYPO3\CMS\Core\Http\Client\GuzzleClientFactory; use TYPO3\CMS\Core\Http\RequestFactory; @@ -244,7 +245,7 @@ public function syncSiteSettingsFromStatusLogsWarningWhenWriteSettingsThrows(): $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([]); $this->siteSettingsService ->method('writeSettings') - ->willThrowException(new \TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException('disk full', 1590487411)); + ->willThrowException(new SiteConfigurationWriteException('disk full', 1590487411)); $status = $this->subject->getStatus($site, forceRefresh: true); $this->subject->syncSiteSettingsFromStatus($site, $status ?? []); diff --git a/Tests/Unit/Service/ApiKeyServiceTest.php b/Tests/Unit/Service/ApiKeyServiceTest.php index 65441ec8..1c4d15a2 100644 --- a/Tests/Unit/Service/ApiKeyServiceTest.php +++ b/Tests/Unit/Service/ApiKeyServiceTest.php @@ -19,6 +19,7 @@ use T3G\Analytics\Service\CipherService; use T3G\Analytics\Service\HmacSigner; use T3G\Analytics\Service\SiteSettingsWriteVerifierInterface; +use TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException; use TYPO3\CMS\Core\Http\Client\GuzzleClientFactory; use TYPO3\CMS\Core\Http\RequestFactory; use TYPO3\CMS\Core\Settings\Settings; @@ -209,7 +210,7 @@ public function provisionIfNeededReturnsWhenWriteSettingsThrows(): void $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([]); $this->siteSettingsService ->method('writeSettings') - ->willThrowException(new \TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException('disk full', 1590487411)); + ->willThrowException(new SiteConfigurationWriteException('disk full', 1590487411)); $site = $this->buildSite('w-123', 'i-456', $this->encryptedSecret); $this->subject->provisionIfNeeded($site, ['status' => 'active']); diff --git a/Tests/Unit/Service/InstanceRegistrationServiceTest.php b/Tests/Unit/Service/InstanceRegistrationServiceTest.php index 27eeb12f..91ca6929 100644 --- a/Tests/Unit/Service/InstanceRegistrationServiceTest.php +++ b/Tests/Unit/Service/InstanceRegistrationServiceTest.php @@ -19,6 +19,7 @@ use T3G\Analytics\Service\HmacSigner; use T3G\Analytics\Service\InstanceRegistrationService; use T3G\Analytics\Service\SiteSettingsWriteVerifierInterface; +use TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException; use TYPO3\CMS\Core\Http\Client\GuzzleClientFactory; use TYPO3\CMS\Core\Http\RequestFactory; use TYPO3\CMS\Core\Http\Uri; @@ -194,7 +195,7 @@ public function registerThrowsAnalyticsApiExceptionWhenWriteSettingsThrows(): vo $this->siteSettingsFactory->method('loadLocalSettings')->willReturn([]); $this->siteSettingsService ->method('writeSettings') - ->willThrowException(new \TYPO3\CMS\Core\Configuration\Exception\SiteConfigurationWriteException('disk full', 1590487411)); + ->willThrowException(new SiteConfigurationWriteException('disk full', 1590487411)); $this->expectException(AnalyticsApiException::class);