From 46a9781366aacfcf3fc4a0c80babe7e1e2735a3b Mon Sep 17 00:00:00 2001 From: Davide Iadeluca Date: Fri, 6 Nov 2026 09:43:01 +0100 Subject: [PATCH 1/2] feat: add stackable modifyResourceQuery() to the Sitemap extender --- README.md | 32 ++++ src/Extend/Sitemap.php | 34 ++++ src/Generate/Generator.php | 29 ++- src/Providers/Provider.php | 7 +- .../ExcludeDiscussionQueryModifier.php | 29 +++ tests/integration/XmlSitemapTestTrait.php | 23 +++ tests/integration/api/ExtenderTest.php | 179 ++++++++++++++++++ tests/integration/console/CachedModeTest.php | 23 +++ 8 files changed, 353 insertions(+), 3 deletions(-) create mode 100644 tests/integration/ExcludeDiscussionQueryModifier.php diff --git a/README.md b/README.md index 88a4bb5..4022b16 100644 --- a/README.md +++ b/README.md @@ -133,12 +133,14 @@ The recommended way to extend sitemaps uses the unified `Sitemap` extender with ```php use FoF\Sitemap\Extend; +use Illuminate\Database\Eloquent\Builder; return [ (new Extend\Sitemap()) ->addResource(YourCustomResource::class) ->removeResource(\FoF\Sitemap\Resources\Tag::class) ->replaceResource(\FoF\Sitemap\Resources\User::class, YourCustomUserResource::class) + ->modifyResourceQuery(\FoF\Sitemap\Resources\Discussion::class, fn (Builder $query) => $query->where('discussions.is_sticky', false)) ->addStaticUrl('reviews.index') ->forceCached(), ]; @@ -148,9 +150,39 @@ return [ - **`addResource(string $resourceClass)`**: Add a custom resource to the sitemap - **`removeResource(string $resourceClass)`**: Remove an existing resource from the sitemap - **`replaceResource(string $oldResourceClass, string $newResourceClass)`**: Replace an existing resource +- **`modifyResourceQuery(string $resourceClass, callable|string $modifier)`**: Filter or adjust the query of an existing resource (see below) - **`addStaticUrl(string $routeName)`**: Add a static URL by route name - **`forceCached()`**: Force cached mode for managed hosting environments +#### Filtering Existing Resources + +To leave items out of a built-in resource, use `modifyResourceQuery()` rather than `replaceResource()`. Modifiers from all extensions stack: every registered modifier runs, in registration order. `replaceResource()` swaps the class by exact name, so when two extensions replace or remove the same resource, only one of them takes effect. + +```php +use FoF\Sitemap\Extend; +use FoF\Sitemap\Resources; +use Illuminate\Database\Eloquent\Builder; +use Illuminate\Support\Collection; + +return [ + (new Extend\Sitemap()) + // Modify the Builder in place... + ->modifyResourceQuery(Resources\Discussion::class, function (Builder $query) { + $query->whereNull('discussions.redirect_url'); + }) + // ...or pass the ::class of an invokable class, resolved from the container + ->modifyResourceQuery(Resources\Tag::class, ExcludeInternalTags::class) + // Collection resources such as StaticUrls are filtered by returning a new Collection + ->modifyResourceQuery(Resources\StaticUrls::class, fn (Collection $routes) => $routes->reject(fn ($route) => $route === 'tags')), +]; +``` + +The modifier receives the resource's query (an Eloquent `Builder`, or a `Collection` for `StaticUrls`) and the resource instance. If it returns `null`, the query it was given is used. Any other return value replaces the query and must be a `Builder` or a `Collection`. + +Things to keep in mind: +- A modifier applies to the resource class you name **and to its subclasses**, including a `replaceResource()` replacement that extends it. It does not apply to a replacement that extends `Resource` directly. +- Qualify column names (`discussions.id`, not `id`). When **Risky Performance Improvements** is enabled, the Discussion query selects only `id`, `slug`, `created_at` and `last_posted_at`. Adding `where` clauses is fine, but don't rely on other columns being loaded, and avoid joins, which make the unqualified `id` in that select ambiguous. + #### Creating Custom Resources Create a class that extends `FoF\Sitemap\Resources\Resource`: diff --git a/src/Extend/Sitemap.php b/src/Extend/Sitemap.php index 1383ba8..5abb7a2 100644 --- a/src/Extend/Sitemap.php +++ b/src/Extend/Sitemap.php @@ -14,6 +14,7 @@ use Flarum\Extend\ExtenderInterface; use Flarum\Extension\Extension; +use Flarum\Foundation\ContainerUtil; use FoF\Sitemap\Resources\Resource; use FoF\Sitemap\Resources\StaticUrls; use Illuminate\Contracts\Container\Container; @@ -24,6 +25,7 @@ class Sitemap implements ExtenderInterface private array $resourcesToAdd = []; private array $resourcesToRemove = []; private array $resourcesToReplace = []; + private array $queryModifiers = []; private array $staticUrls = []; private bool $forceCached = false; @@ -74,6 +76,28 @@ public function replaceResource(string $oldResource, string $newResource): self return $this; } + /** + * Modify the query of a resource before the sitemap is built. + * Applies to the given resource class and all of its subclasses, including replacements registered + * via replaceResource() that extend it. Modifiers from all extensions stack and run in registration order. + * + * The modifier receives the result of Resource::query() (a Builder, or a Collection for StaticUrls) and the + * resource instance. A Builder can be modified in place; any non-null return value replaces the query. + * The modifier can be a callable or the ::class of an invokable class, which is resolved from the container. + * + * @param class-string $resourceClass + * @param callable|class-string $modifier + * + * @return self + */ + public function modifyResourceQuery(string $resourceClass, callable|string $modifier): self + { + $this->validateResource($resourceClass); + $this->queryModifiers[] = [$resourceClass, $modifier]; + + return $this; + } + /** * Add a static URL to the sitemap. Specify the route name. * @@ -131,6 +155,16 @@ public function extend(Container $container, ?Extension $extension = null): void }); } + if (!empty($this->queryModifiers)) { + $container->extend('fof-sitemaps.queryModifiers', function (array $modifiers) use ($container) { + foreach ($this->queryModifiers as [$resourceClass, $modifier]) { + $modifiers[] = [$resourceClass, ContainerUtil::wrapCallback($modifier, $container)]; + } + + return $modifiers; + }); + } + // Register static URLs foreach ($this->staticUrls as $routeName) { StaticUrls::addRoute($routeName); diff --git a/src/Generate/Generator.php b/src/Generate/Generator.php index 3234d5b..2b59eb7 100644 --- a/src/Generate/Generator.php +++ b/src/Generate/Generator.php @@ -26,13 +26,15 @@ use Illuminate\Support\Collection; use Symfony\Component\Console\Output\NullOutput; use Symfony\Component\Console\Output\OutputInterface; +use UnexpectedValueException; class Generator { public function __construct( protected DeployInterface $deploy, protected array $resources, - protected SettingsRepositoryInterface $settings + protected SettingsRepositoryInterface $settings, + protected array $queryModifiers = [] ) { } @@ -88,7 +90,7 @@ public function loop(?OutputInterface $output = null): array } // Get query once and reuse - $query = $resource->query(); + $query = $this->modifyQuery($resource, $resource->query()); // For Collections, check if empty immediately to avoid unnecessary processing if ($query instanceof Collection && $query->isEmpty()) { @@ -163,4 +165,27 @@ public function loop(?OutputInterface $output = null): array return $remotes; } + + protected function modifyQuery(AbstractResource $resource, Builder|Collection $query): Builder|Collection + { + foreach ($this->queryModifiers as [$resourceClass, $modifier]) { + if (!$resource instanceof $resourceClass) { + continue; + } + + $result = $modifier($query, $resource); + + if ($result === null) { + continue; + } + + if (!$result instanceof Builder && !$result instanceof Collection) { + throw new UnexpectedValueException('Sitemap query modifier for '.$resourceClass.' must return null, '.Builder::class.' or '.Collection::class.', got '.get_debug_type($result)); + } + + $query = $result; + } + + return $query; + } } diff --git a/src/Providers/Provider.php b/src/Providers/Provider.php index 2362ff8..6777f7d 100644 --- a/src/Providers/Provider.php +++ b/src/Providers/Provider.php @@ -41,11 +41,16 @@ public function register() ]; }); + $this->container->singleton('fof-sitemaps.queryModifiers', function () { + return []; + }); + $this->container->singleton(Generator::class, function (Container $container) { return new Generator( $container->make(DeployInterface::class), $container->make('fof-sitemaps.resources'), - $container->make(SettingsRepositoryInterface::class) + $container->make(SettingsRepositoryInterface::class), + $container->make('fof-sitemaps.queryModifiers') ); }); } diff --git a/tests/integration/ExcludeDiscussionQueryModifier.php b/tests/integration/ExcludeDiscussionQueryModifier.php new file mode 100644 index 0000000..599db19 --- /dev/null +++ b/tests/integration/ExcludeDiscussionQueryModifier.php @@ -0,0 +1,29 @@ +where('discussions.id', '!=', (int) $this->settings->get('fof-sitemap-test.excluded_discussion_id')); + } +} diff --git a/tests/integration/XmlSitemapTestTrait.php b/tests/integration/XmlSitemapTestTrait.php index 00a67b5..26af599 100644 --- a/tests/integration/XmlSitemapTestTrait.php +++ b/tests/integration/XmlSitemapTestTrait.php @@ -52,6 +52,29 @@ private function getUrlsFromSitemap(string $sitemapXml): array return $urls; } + /** + * Fetch the sitemap index and every set it lists, and return all URLs found in the sets. + */ + private function getAllSitemapUrls(): array + { + $indexResponse = $this->send($this->request('GET', '/sitemap.xml')); + $this->assertEquals(200, $indexResponse->getStatusCode()); + + $urls = []; + + foreach ($this->getSitemapUrls($indexResponse->getBody()->getContents()) as $sitemapUrl) { + $sitemapResponse = $this->send($this->request('GET', parse_url($sitemapUrl, PHP_URL_PATH))); + $this->assertEquals(200, $sitemapResponse->getStatusCode()); + + $sitemapBody = $sitemapResponse->getBody()->getContents(); + $this->assertValidSitemapXml($sitemapBody); + + $urls = array_merge($urls, $this->getUrlsFromSitemap($sitemapBody)); + } + + return $urls; + } + private function assertValidSitemapIndexXml(string $xml): void { // Check if XML is well-formed diff --git a/tests/integration/api/ExtenderTest.php b/tests/integration/api/ExtenderTest.php index 18973b7..aa7c106 100644 --- a/tests/integration/api/ExtenderTest.php +++ b/tests/integration/api/ExtenderTest.php @@ -18,9 +18,14 @@ use Flarum\Testing\integration\TestCase; use Flarum\User\User; use FoF\Sitemap\Extend\Sitemap; +use FoF\Sitemap\Resources\StaticUrls; +use FoF\Sitemap\Tests\integration\ExcludeDiscussionQueryModifier; use FoF\Sitemap\Tests\integration\TestDiscussionResource; use FoF\Sitemap\Tests\integration\TestResource; use FoF\Sitemap\Tests\integration\XmlSitemapTestTrait; +use Illuminate\Database\Eloquent\Builder; +use Illuminate\Support\Collection; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; class ExtenderTest extends TestCase @@ -45,9 +50,31 @@ public function setUp(): void 'comment_count' => 1, 'is_private' => 0, ], + [ + 'id' => 2, + 'title' => 'Second Discussion', + 'created_at' => Carbon::createFromDate(2023, 1, 2)->toDateTimeString(), + 'last_posted_at' => Carbon::createFromDate(2023, 1, 2)->toDateTimeString(), + 'user_id' => 1, + 'first_post_id' => 2, + 'comment_count' => 1, + 'is_private' => 0, + ], + [ + 'id' => 3, + 'title' => 'Third Discussion', + 'created_at' => Carbon::createFromDate(2023, 1, 3)->toDateTimeString(), + 'last_posted_at' => Carbon::createFromDate(2023, 1, 3)->toDateTimeString(), + 'user_id' => 1, + 'first_post_id' => 3, + 'comment_count' => 1, + 'is_private' => 0, + ], ], Post::class => [ ['id' => 1, 'discussion_id' => 1, 'created_at' => Carbon::createFromDate(2023, 1, 1)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

Test content

'], + ['id' => 2, 'discussion_id' => 2, 'created_at' => Carbon::createFromDate(2023, 1, 2)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

Test content

'], + ['id' => 3, 'discussion_id' => 3, 'created_at' => Carbon::createFromDate(2023, 1, 3)->toDateTimeString(), 'user_id' => 1, 'type' => 'comment', 'content' => '

Test content

'], ], User::class => [ ['id' => 2, 'username' => 'testuser', 'email' => 'test@example.com', 'joined_at' => Carbon::createFromDate( @@ -280,4 +307,156 @@ public function unified_extender_can_add_static_url() $this->assertTrue($foundStaticUrl, 'Unified extender should include static URL from registered route'); $this->assertTrue($foundDiscussionUrl, 'Unified extender should still include existing resources when adding static URLs'); } + + #[Test] + public function modify_resource_query_closure_filters_discussions() + { + $this->extend( + (new Sitemap()) + ->modifyResourceQuery(\FoF\Sitemap\Resources\Discussion::class, fn (Builder $query) => $query->where('discussions.id', '!=', 2)) + ); + + $this->assertEquals([1, 3], $this->getIdsFromUrls($this->getAllSitemapUrls(), '/d/')); + } + + #[Test] + public function modify_resource_query_modifiers_from_separate_extenders_stack() + { + $this->extend( + (new Sitemap()) + ->modifyResourceQuery(\FoF\Sitemap\Resources\Discussion::class, function (Builder $query) { + $query->where('discussions.id', '!=', 2); + }), + (new Sitemap()) + ->modifyResourceQuery(\FoF\Sitemap\Resources\Discussion::class, fn (Builder $query) => $query->where('discussions.id', '!=', 3)) + ); + + $this->assertEquals([1], $this->getIdsFromUrls($this->getAllSitemapUrls(), '/d/')); + } + + public static function modifierRegisteredBeforeReplace(): array + { + return [ + 'modifier registered before replace' => [true], + 'modifier registered after replace' => [false], + ]; + } + + #[Test] + #[DataProvider('modifierRegisteredBeforeReplace')] + public function modify_resource_query_applies_to_replacement_subclass(bool $modifierFirst) + { + $modifier = (new Sitemap()) + ->modifyResourceQuery(\FoF\Sitemap\Resources\Discussion::class, fn (Builder $query) => $query->where('discussions.id', '!=', 2)); + $replacement = (new Sitemap()) + ->replaceResource(\FoF\Sitemap\Resources\Discussion::class, TestDiscussionResource::class); + + $this->extend(...($modifierFirst ? [$modifier, $replacement] : [$replacement, $modifier])); + + $urls = $this->getAllSitemapUrls(); + + $this->assertEquals([1, 3], $this->getIdsFromUrls($urls, '/custom-discussion/')); + $this->assertEquals([], $this->getIdsFromUrls($urls, '/d/'), 'The replaced Discussion resource should not be in the sitemap'); + } + + #[Test] + public function modify_resource_query_resolves_invokable_class_from_container() + { + $this->setting('fof-sitemap-test.excluded_discussion_id', '3'); + + $this->extend( + (new Sitemap()) + ->modifyResourceQuery(\FoF\Sitemap\Resources\Discussion::class, ExcludeDiscussionQueryModifier::class) + ); + + $this->assertEquals([1, 2], $this->getIdsFromUrls($this->getAllSitemapUrls(), '/d/')); + } + + #[Test] + public function modify_resource_query_can_replace_static_urls_collection() + { + $this->extend( + (new \Flarum\Extend\Routes('forum')) + ->get('/test-static-page', 'test.static.route', function () { + return new \Laminas\Diactoros\Response\HtmlResponse('

Test Static Page

'); + }), + (new Sitemap()) + ->addStaticUrl('test.static.route') + ->modifyResourceQuery(StaticUrls::class, fn (Collection $routes) => $routes->reject(fn (string $route) => $route === 'test.static.route')) + ); + + $urls = $this->getAllSitemapUrls(); + + $this->assertEmpty(array_filter($urls, fn (string $url) => str_contains($url, '/test-static-page')), 'Filtered static route should not be in the sitemap'); + $this->assertEquals([1, 2, 3], $this->getIdsFromUrls($urls, '/d/')); + } + + #[Test] + public function modify_resource_query_returning_empty_collection_skips_resource() + { + $this->extend( + (new Sitemap()) + ->modifyResourceQuery(StaticUrls::class, fn (Collection $routes) => collect()) + ); + + $urls = $this->getAllSitemapUrls(); + + $this->assertNotEmpty($urls); + $this->assertEquals([1, 2, 3], $this->getIdsFromUrls($urls, '/d/')); + } + + #[Test] + public function modify_resource_query_leaves_other_resources_untouched() + { + $this->extend( + (new Sitemap()) + ->modifyResourceQuery(\FoF\Sitemap\Resources\User::class, fn (Builder $query) => $query->whereRaw('1 = 0')) + ); + + $urls = $this->getAllSitemapUrls(); + + $this->assertEmpty(array_filter($urls, fn (string $url) => preg_match('/\/u\/\w+/', $url)), 'User URLs should be filtered out'); + $this->assertEquals([1, 2, 3], $this->getIdsFromUrls($urls, '/d/')); + } + + #[Test] + public function modify_resource_query_validates_resource_inheritance() + { + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('has to extend'); + + (new Sitemap())->modifyResourceQuery(\stdClass::class, fn ($query) => $query); + } + + #[Test] + public function modify_resource_query_rejects_invalid_return_value() + { + $this->extend( + (new Sitemap()) + ->modifyResourceQuery(\FoF\Sitemap\Resources\Discussion::class, fn (Builder $query) => true) + ); + + $response = $this->send($this->request('GET', '/sitemap.xml')); + + $this->assertEquals(500, $response->getStatusCode()); + } + + /** + * Return the sorted, unique numeric IDs that follow $pathPrefix in the given URLs. + */ + private function getIdsFromUrls(array $urls, string $pathPrefix): array + { + $ids = []; + + foreach ($urls as $url) { + if (preg_match('#'.preg_quote($pathPrefix, '#').'(\d+)#', $url, $matches)) { + $ids[] = (int) $matches[1]; + } + } + + $ids = array_values(array_unique($ids)); + sort($ids); + + return $ids; + } } diff --git a/tests/integration/console/CachedModeTest.php b/tests/integration/console/CachedModeTest.php index 0ccced4..7fa506d 100644 --- a/tests/integration/console/CachedModeTest.php +++ b/tests/integration/console/CachedModeTest.php @@ -19,6 +19,7 @@ use Flarum\User\User; use FoF\Sitemap\Extend\Sitemap; use FoF\Sitemap\Tests\integration\XmlSitemapTestTrait; +use Illuminate\Database\Eloquent\Builder; use PHPUnit\Framework\Attributes\Test; class CachedModeTest extends ConsoleTestCase @@ -312,4 +313,26 @@ public function unified_extender_forced_cached_mode_creates_physical_files() $this->assertTrue($container->has('fof-sitemaps.forceCached')); $this->assertTrue($container->get('fof-sitemaps.forceCached')); } + + #[Test] + public function cached_mode_applies_query_modifiers() + { + $this->setting('fof-sitemap.mode', 'multi-file'); + + $this->extend( + (new Sitemap()) + ->modifyResourceQuery(\FoF\Sitemap\Resources\Discussion::class, fn (Builder $query) => $query->where('discussions.id', '!=', 1)) + ); + + $output = $this->runCommand([ + 'command' => 'fof:sitemap:build', + ]); + + $this->assertStringContainsString('Completed', $output); + + $urls = $this->getAllSitemapUrls(); + + $this->assertEmpty(array_filter($urls, fn (string $url) => preg_match('/\/d\/\d+/', $url)), 'Cached sitemap should not include the filtered discussion'); + $this->assertNotEmpty(array_filter($urls, fn (string $url) => preg_match('/\/u\/\w+/', $url)), 'Cached sitemap should still include user URLs'); + } } From 593fe849f0f0f151c919a2c5562bf3e5fa3f453f Mon Sep 17 00:00:00 2001 From: StyleCI Bot Date: Fri, 6 Nov 2026 08:43:25 +0000 Subject: [PATCH 2/2] Apply fixes from StyleCI --- src/Extend/Sitemap.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Extend/Sitemap.php b/src/Extend/Sitemap.php index 5abb7a2..05a92c0 100644 --- a/src/Extend/Sitemap.php +++ b/src/Extend/Sitemap.php @@ -85,8 +85,8 @@ public function replaceResource(string $oldResource, string $newResource): self * resource instance. A Builder can be modified in place; any non-null return value replaces the query. * The modifier can be a callable or the ::class of an invokable class, which is resolved from the container. * - * @param class-string $resourceClass - * @param callable|class-string $modifier + * @param class-string $resourceClass + * @param callable|class-string $modifier * * @return self */