From 17e69c8a1d044982726220ee10d0b7a9f3364ff1 Mon Sep 17 00:00:00 2001 From: Andrew Shell Date: Wed, 5 Aug 2026 17:48:16 -0500 Subject: [PATCH 1/3] refactor(registry): parse subscription state in one place load() and all() each had their own copy of read -> json_decode -> normalise, and the copies had already diverged: load() logged unusable JSON while all() skipped it in silence, and only all() rejected an empty url. The silence was the wrong way round. all() exists to feed a status screen -- the one view whose purpose is showing what is broken -- so a corrupt state file vanished from it entirely, indistinguishable from a subscription that never existed, while the hot path that does not need the warning emitted one. all() cannot simply call load(): load() derives its directory from a resource URL via sha1(), which does not invert, so all() has to recover the URL from the file. That justifies two methods, not two parsers. Both now share readState(). Also constrains `kind` to the two known values in normalise(), so a caller can key off it without validating, and documents that all() yields in hash order and is single-pass. Co-Authored-By: Claude Opus 5 (1M context) --- RssCloud/Registry.php | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/RssCloud/Registry.php b/RssCloud/Registry.php index fae1028..1074f05 100644 --- a/RssCloud/Registry.php +++ b/RssCloud/Registry.php @@ -40,13 +40,32 @@ public function directory(string $resourceUrl): string { * @return RssCloudState|null */ public function load(string $resourceUrl): ?array { - $json = @file_get_contents($this->directory($resourceUrl) . '/!cloud.json'); + return self::readState($this->directory($resourceUrl), $resourceUrl); + } + + /** + * Read and normalise the state file held in one subscription directory. + * + * Shared by {@see self::load()} and {@see self::all()}, which differ only in how they arrive at + * a directory: `load()` derives it from a resource URL, while `all()` walks them and recovers + * the URL from the file, the directory name being a one-way hash. Parsing this in two places + * left the copies free to diverge, and they had. + * + * An absent file is unremarkable — {@see self::save()} creates the directory before writing it, + * and {@see self::addSubscriber()} can create one on its own — so only content that exists and + * cannot be used is worth logging. + * + * @param string $context names the subscription in the log + * @return RssCloudState|null + */ + private static function readState(string $directory, string $context): ?array { + $json = @file_get_contents($directory . '/!cloud.json'); if (!is_string($json) || $json === '') { return null; } $state = json_decode($json, true); - if (!is_array($state) || !is_string($state['url'] ?? null)) { - Minz_Log::warning('rssCloud: invalid state JSON for ' . $resourceUrl, RSSCLOUD_LOG); + if (!is_array($state) || !is_string($state['url'] ?? null) || $state['url'] === '') { + Minz_Log::warning('rssCloud: invalid state JSON for ' . $context, RSSCLOUD_LOG); return null; } return self::normalise($state); @@ -59,7 +78,8 @@ public function load(string $resourceUrl): ?array { private static function normalise(array $state): array { return [ 'url' => is_string($state['url'] ?? null) ? $state['url'] : '', - 'kind' => is_string($state['kind'] ?? null) ? $state['kind'] : self::KIND_FEED, + // Constrained to the two known kinds, so that a caller may key off it without checking. + 'kind' => ($state['kind'] ?? null) === self::KIND_OPML ? self::KIND_OPML : self::KIND_FEED, 'endpoint' => is_string($state['endpoint'] ?? null) ? $state['endpoint'] : '', 'registerProcedure' => is_string($state['registerProcedure'] ?? null) ? $state['registerProcedure'] : '', 'lease_start' => is_numeric($state['lease_start'] ?? null) ? (int)$state['lease_start'] : 0, @@ -145,18 +165,18 @@ public function forget(string $resourceUrl): void { /** * Iterate over every known subscription. * + * Yields in directory order, which is `sha1()` order and so arbitrary: anything displaying + * these has to sort them itself. Being a generator, the result is single-pass and cannot be + * counted without first collecting it. + * * @return iterable */ public function all(): iterable { $directories = @glob($this->basePath . '/resources/*', GLOB_ONLYDIR | GLOB_NOSORT); foreach ($directories ?: [] as $directory) { - $json = @file_get_contents($directory . '/!cloud.json'); - if (!is_string($json) || $json === '') { - continue; - } - $state = json_decode($json, true); - if (is_array($state) && is_string($state['url'] ?? null) && $state['url'] !== '') { - yield self::normalise($state); + $state = self::readState($directory, $directory); + if ($state !== null) { + yield $state; } } } From 86dd93a9ee9185ebbbf0c061b4e11fdf8293b391 Mon Sep 17 00:00:00 2001 From: Andrew Shell Date: Wed, 5 Aug 2026 17:48:28 -0500 Subject: [PATCH 2/3] feat(config): show subscription status on the configuration screen Until now the only way to tell whether a feed used rssCloud, and whether its handshake succeeded, was to read data/users/_/log_rsscloud.txt or cat the JSON under data/rssCloud/resources/ inside the container. Adds a read-only table to the extension's configuration screen: one row per discovered resource, with its type, state, cloud server, last renewal attempt, last notification received, and the users interested in it. The state column distinguishes four cases that the raw file does not make obvious. lease_start is stamped *before* the request goes out, deliberately, so that a crashed call cannot spin -- which means a non-zero lease_start does not imply the handshake worked. Only `error` says that, and it defaults to true when absent. So: pending (discovered, never attempted), failed (with the message), active (registered and inside the renewal window), stale (registered but the lease is older than the window and may have lapsed). The model layer for this already existed and was simply never called. Co-Authored-By: Claude Opus 5 (1M context) --- configure.phtml | 40 +++++++++++++++++++++++++++++++++++++ extension.php | 52 +++++++++++++++++++++++++++++++++++++++++++++++++ i18n/en/ext.php | 17 ++++++++++++++++ 3 files changed, 109 insertions(+) diff --git a/configure.phtml b/configure.phtml index 204278a..c4f50eb 100644 --- a/configure.phtml +++ b/configure.phtml @@ -2,6 +2,7 @@ declare(strict_types=1); /** @var RssCloudExtension $this */ $callback = $this->callbackUrl(); + $subscriptions = $this->subscriptions(); ?>
@@ -87,3 +88,42 @@
+ +

+

+ + +

+ + + + + + + + + + + + + + + + + + + + + + + + + + +
+ + +
+ + 		0 ? timestamptodate($state['lease_start']) : _t('ext.rsscloud.status.never') ?> 0 ? timestamptodate($state['last_notify']) : _t('ext.rsscloud.status.never') ?>
+ diff --git a/extension.php b/extension.php index 9fced78..97af8eb 100644 --- a/extension.php +++ b/extension.php @@ -50,6 +50,18 @@ final class RssCloudExtension extends Minz_Extension { /** Upper bound on `pleaseNotify` calls issued in a single refresh cycle, so cron cannot stall. */ public const MAX_SUBSCRIPTIONS_PER_RUN = 10; + /** A cloud server is registered and believed to be notifying us. */ + public const STATUS_ACTIVE = 'active'; + + /** The last `pleaseNotify` was rejected or could not be made; `error_message` says why. */ + public const STATUS_ERROR = 'error'; + + /** Discovered, but no registration has been attempted yet. */ + public const STATUS_PENDING = 'pending'; + + /** Registered successfully, but the lease is older than the renewal window and may have lapsed. */ + public const STATUS_STALE = 'stale'; + /** Feed attribute holding the resource URL this feed is (or should be) subscribed under. */ private const FEED_ATTRIBUTE = 'rssCloud'; @@ -149,6 +161,46 @@ public function renewSeconds(): int { return min(self::MAX_RENEW_HOURS, max(1, $hours)) * 3600; } + /** + * Every known subscription with the state the configuration screen displays, resource-sorted + * because {@see RssCloud_Registry::all()} yields in hash order. + * + * Collected rather than streamed: the view needs to know whether there is anything at all + * before it commits to drawing a table, which a generator cannot answer. + * + * @return list}> + */ + public function subscriptions(): array { + $registry = $this->registry(); + $renewSeconds = $this->renewSeconds(); + + $rows = []; + foreach ($registry->all() as $state) { + $rows[] = [ + 'state' => $state, + 'status' => self::statusOf($state, $renewSeconds), + 'subscribers' => $registry->subscribers($state['url']), + ]; + } + usort($rows, static fn(array $a, array $b): int => strcmp($a['state']['url'], $b['state']['url'])); + + return $rows; + } + + /** + * @param RssCloudState $state + * @return self::STATUS_* + */ + private static function statusOf(array $state, int $renewSeconds): string { + if ($state['error']) { + return self::STATUS_ERROR; + } + if ($state['lease_start'] <= 0) { + return self::STATUS_PENDING; + } + return RssCloud_Subscriber::isHealthy($state, $renewSeconds) ? self::STATUS_ACTIVE : self::STATUS_STALE; + } + public function maxStalenessSeconds(): int { return max(1, $this->getSystemConfigurationInt('max_staleness_hours') ?? self::DEFAULT_MAX_STALENESS_HOURS) * 3600; } diff --git a/i18n/en/ext.php b/i18n/en/ext.php index 01ca433..325ed4d 100644 --- a/i18n/en/ext.php +++ b/i18n/en/ext.php @@ -20,5 +20,22 @@ 'renew_hours.help' => 'rssCloud does not negotiate a lease duration. Subscriptions expire after 25 hours and are meant to be renewed every 24, so the default of 23 leaves a margin. Anything above 24 would simply lapse, and is capped.', 'skip_polling' => 'Skip polling covered resources', 'skip_polling.help' => 'Stop polling a resource on a timer while its cloud subscription is healthy. It is still polled if it goes stale, or when refreshed individually.', + 'status' => 'Subscriptions', + 'status.active' => 'Active', + 'status.endpoint' => 'Cloud server', + 'status.error' => 'Failed', + 'status.help' => 'One row per resource this instance has discovered a cloud server for. "Renewed" is when registration was last attempted, not when it last succeeded; the state column says whether it did.', + 'status.kind' => 'Type', + 'status.kind.feed' => 'Feed', + 'status.kind.opml' => 'Dynamic OPML', + 'status.never' => 'Never', + 'status.none' => 'No cloud servers have been discovered yet. Resources are registered as they are refreshed, so this fills in once feeds advertising a cloud have been fetched at least once.', + 'status.notified' => 'Last notified', + 'status.pending' => 'Pending', + 'status.renewed' => 'Renewal attempted', + 'status.resource' => 'Resource', + 'status.stale' => 'Stale', + 'status.state' => 'State', + 'status.subscribers' => 'Users', ), ); From a5a38026d9dbe0bbbe0646a197600ee770f55ae2 Mon Sep 17 00:00:00 2001 From: Andrew Shell Date: Wed, 5 Aug 2026 17:57:03 -0500 Subject: [PATCH 3/3] test(registry): add PHPUnit and cover subscription state parsing The repo had no test harness, so the refactor that unified load() and all() was verified out-of-tree and thrown away. This lands it. No second set of dev dependencies: the tests borrow core's PHPUnit and core's autoloader, exactly as phpstan and phpcs already do, so there is no composer.json here to drift out of step. The bootstrap requires core's constants.php and lib_rss.php directly rather than core's own tests/bootstrap.php -- the two lines are identical, and constants.php is far less likely to move. Using the real Minz_Log rather than a stub means the tests assert on the log file itself, which is what the refactor was about: unusable state must be reported, not silently skipped. Removing that one Minz_Log::warning call fails three of these tests, so they are load-bearing rather than decorative. Runs on both matrix legs, and verified against 1.29.0 as well as edge before landing. Also corrects the README, which still claimed CI ran against edge only. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/ci.yml | 9 ++ README.md | 8 +- tests/RssCloud/RegistryTest.php | 215 ++++++++++++++++++++++++++++++++ tests/bootstrap.php | 29 +++++ 4 files changed, 260 insertions(+), 1 deletion(-) create mode 100644 tests/RssCloud/RegistryTest.php create mode 100644 tests/bootstrap.php diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ca9aecb..867072c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -50,6 +50,15 @@ jobs: working-directory: FreshRSS/extensions/xExtension-RssCloud run: find . \( -name '*.php' -o -name '*.phtml' \) -print0 | xargs -0 -n1 php -l > /dev/null + - name: phpunit + working-directory: FreshRSS/extensions/xExtension-RssCloud + # Core's PHPUnit and core's autoloader, so there is no second set of dev dependencies to + # keep in step. Runs against both cores in the matrix, which is the point: the tests + # exercise the extension against whichever FreshRSS is checked out beside it. + run: >- + ../../vendor/bin/phpunit --bootstrap tests/bootstrap.php + --display-notices --display-deprecations tests + - name: phpcs working-directory: FreshRSS run: | diff --git a/README.md b/README.md index e8dc72a..5409735 100644 --- a/README.md +++ b/README.md @@ -195,6 +195,10 @@ class it touches lives there — so clone it into a FreshRSS checkout at ```sh composer install +# phpunit, using core's autoloader as the bootstrap +( cd extensions/xExtension-RssCloud && \ + ../../vendor/bin/phpunit --bootstrap tests/bootstrap.php tests ) + # phpstan, using core's ruleset scoped to this extension ( cd extensions/xExtension-RssCloud && ../../vendor/bin/phpstan analyse -c phpstan.neon ) @@ -203,7 +207,9 @@ sed '/(?-i:extensions)/d' phpcs.xml > phpcs-extensions.xml vendor/bin/phpcs --standard=phpcs-extensions.xml extensions/xExtension-RssCloud -s ``` -CI runs exactly these against FreshRSS `edge` on every push and pull request. +CI runs exactly these on every push and pull request, against **both** ends of the supported range: +FreshRSS `edge` and `1.29.0`. Analysing only the development tip once let a call to an edge-only +core method ship as a fatal error on every released version, so the floor is checked too. ### Commits diff --git a/tests/RssCloud/RegistryTest.php b/tests/RssCloud/RegistryTest.php new file mode 100644 index 0000000..e3de648 --- /dev/null +++ b/tests/RssCloud/RegistryTest.php @@ -0,0 +1,215 @@ +basePath = sys_get_temp_dir() . '/rsscloud-registry-test-' . getmypid() . '-' . uniqid(); + mkdir($this->basePath . '/resources', 0770, true); + $this->registry = new RssCloud_Registry($this->basePath); + @unlink(RSSCLOUD_LOG); + } + + #[\Override] + protected function tearDown(): void { + recursive_unlink($this->basePath); + @rmdir($this->basePath); + @unlink(RSSCLOUD_LOG); + } + + /** Write a state file straight to disk, bypassing save(), so malformed content can be staged. */ + private function writeRaw(string $resourceUrl, string $json): void { + $directory = $this->registry->directory($resourceUrl); + mkdir($directory, 0770, true); + if ($json !== '') { + file_put_contents($directory . '/!cloud.json', $json); + } + } + + private function log(): string { + return (string)@file_get_contents(RSSCLOUD_LOG); + } + + /** @param array $state */ + private function writeState(string $resourceUrl, array $state): void { + $this->writeRaw($resourceUrl, (string)json_encode($state + ['url' => $resourceUrl])); + } + + public function test_load_returnsStoredState(): void { + $this->writeState('https://a.example/feed', [ + 'kind' => 'opml', + 'endpoint' => 'https://rpc.example/pleaseNotify', + 'registerProcedure' => '', + 'lease_start' => 1000, + 'last_notify' => 2000, + 'error' => false, + 'error_message' => '', + ]); + + $state = $this->registry->load('https://a.example/feed'); + + self::assertIsArray($state); + self::assertSame('https://a.example/feed', $state['url']); + self::assertSame('opml', $state['kind']); + self::assertSame('https://rpc.example/pleaseNotify', $state['endpoint']); + self::assertSame(1000, $state['lease_start']); + self::assertSame(2000, $state['last_notify']); + self::assertFalse($state['error']); + } + + public function test_load_reportsMalformedJson(): void { + $this->writeRaw('https://b.example/feed', '{not json'); + + self::assertNull($this->registry->load('https://b.example/feed')); + self::assertStringContainsString('invalid state JSON', $this->log()); + } + + /** An empty url used to be accepted here and rejected in all(); both now reject it. */ + public function test_load_reportsEmptyUrl(): void { + $this->writeRaw('https://c.example/feed', (string)json_encode(['url' => ''])); + + self::assertNull($this->registry->load('https://c.example/feed')); + self::assertStringContainsString('invalid state JSON', $this->log()); + } + + /** + * A directory without a state file is ordinary: save() creates the directory before writing, + * and addSubscriber() can create one on its own. It must not be logged as a fault. + */ + public function test_load_isSilentWhenFileAbsent(): void { + $this->writeRaw('https://d.example/feed', ''); + + self::assertNull($this->registry->load('https://d.example/feed')); + self::assertSame('', $this->log()); + } + + public function test_load_constrainsUnknownKindToFeed(): void { + $this->writeState('https://e.example/feed', ['kind' => 'something-else']); + + $state = $this->registry->load('https://e.example/feed'); + + self::assertIsArray($state); + self::assertSame(RssCloud_Registry::KIND_FEED, $state['kind']); + } + + /** Absence of the flag must read as broken, not as fine, like core's WebSub code. */ + public function test_load_treatsMissingErrorFlagAsError(): void { + $this->writeState('https://f.example/feed', []); + + $state = $this->registry->load('https://f.example/feed'); + + self::assertIsArray($state); + self::assertTrue($state['error']); + } + + public function test_all_yieldsOnlyUsableStateAndReportsTheRest(): void { + $this->writeState('https://a.example/feed', ['endpoint' => 'https://rpc.example/x']); + $this->writeRaw('https://b.example/feed', '{not json'); + $this->writeRaw('https://c.example/feed', (string)json_encode(['url' => ''])); + $this->writeRaw('https://d.example/feed', ''); + + $urls = []; + foreach ($this->registry->all() as $state) { + $urls[] = $state['url']; + } + + self::assertSame(['https://a.example/feed'], $urls); + // The two unusable files are reported; the absent one is not. + self::assertSame(2, substr_count($this->log(), 'invalid state JSON')); + } + + /** Documented as single-pass, so callers know not to count it without collecting it first. */ + public function test_all_isAGenerator(): void { + self::assertInstanceOf(Generator::class, $this->registry->all()); + } + + public function test_all_isEmptyWhenNothingIsStored(): void { + $states = []; + foreach ($this->registry->all() as $state) { + $states[] = $state; + } + + self::assertSame([], $states); + } + + public function test_subscribers_roundTrip(): void { + $this->writeState('https://a.example/feed', []); + + $this->registry->addSubscriber('https://a.example/feed', 'alice'); + $this->registry->addSubscriber('https://a.example/feed', 'bob'); + self::assertSame(['alice', 'bob'], $this->sortedSubscribers('https://a.example/feed')); + + $this->registry->removeSubscriber('https://a.example/feed', 'alice'); + self::assertSame(['bob'], $this->sortedSubscribers('https://a.example/feed')); + } + + /** @return list */ + private function sortedSubscribers(string $resourceUrl): array { + $subscribers = $this->registry->subscribers($resourceUrl); + sort($subscribers); + return $subscribers; + } + + public function test_saveThenLoad_roundTrips(): void { + $this->registry->init(); + $this->registry->save('https://g.example/feed', [ + 'kind' => RssCloud_Registry::KIND_OPML, + 'endpoint' => 'https://rpc.example/pleaseNotify', + 'lease_start' => 42, + 'error' => false, + ]); + + $state = $this->registry->load('https://g.example/feed'); + + self::assertIsArray($state); + self::assertSame('https://g.example/feed', $state['url']); + self::assertSame(RssCloud_Registry::KIND_OPML, $state['kind']); + self::assertSame(42, $state['lease_start']); + self::assertFalse($state['error']); + } + + /** Moving to a different cloud server invalidates the lease rather than carrying it over. */ + public function test_remember_resetsLeaseWhenEndpointChanges(): void { + $this->registry->init(); + $this->registry->save('https://h.example/feed', [ + 'endpoint' => 'https://old.example/pleaseNotify', + 'lease_start' => 999, + 'error' => false, + ]); + + $endpoint = RssCloud_Endpoint::fromUrl('https://new.example/pleaseNotify'); + self::assertInstanceOf(RssCloud_Endpoint::class, $endpoint); + $moved = $this->registry->remember('https://h.example/feed', $endpoint, RssCloud_Registry::KIND_FEED); + + self::assertSame(0, $moved['lease_start']); + self::assertTrue($moved['error']); + } + + public function test_remember_keepsLeaseWhenEndpointIsUnchanged(): void { + $this->registry->init(); + $this->registry->save('https://i.example/feed', [ + 'endpoint' => 'https://same.example/pleaseNotify', + 'lease_start' => 999, + 'error' => false, + ]); + + $endpoint = RssCloud_Endpoint::fromUrl('https://same.example/pleaseNotify'); + self::assertInstanceOf(RssCloud_Endpoint::class, $endpoint); + $kept = $this->registry->remember('https://i.example/feed', $endpoint, RssCloud_Registry::KIND_FEED); + + self::assertSame(999, $kept['lease_start']); + self::assertFalse($kept['error']); + } +} diff --git a/tests/bootstrap.php b/tests/bootstrap.php new file mode 100644 index 0000000..ce42cef --- /dev/null +++ b/tests/bootstrap.php @@ -0,0 +1,29 @@ +