[2.x] feat: fail integration tests that run N+1 queries (and fix the first one it found)

[imorland]

Changes proposed in this pull request

Every request sent through Flarum\Testing\integration\TestCase::send() is now inspected for N+1 query patterns, and the test fails when one is found.

The motivation is concrete: while profiling discussion-view performance this week, one extension turned out to be issuing a query per post on every page of every discussion, and another declared default includes that were never eager-loaded. Both were found by hand-profiling a live forum — no test noticed, because no test could. Extension authors should get that feedback while writing the feature.

And it immediately proved the point. On this PR's first CI run the detector failed two tests in flarum/flags — a bundled extension nobody had profiled, whose suite was green. GET /api/flags declares post.discussion and post.user as default includes but never eager-loaded what those nested resources need: core's DiscussionResource eager-loads the actor's discussion state on its own endpoints, and that doesn't carry over when a discussion is included by another resource. So every flag on the moderation page read discussion_user on its own, and the flag authors' groups were re-fetched per flag. That fix is included here (FlagResource), because a detector that lands with a known finding allow-listed on day one teaches the wrong lesson.

How it detects. An N+1 is one query shape executed once per record. RepeatedQueryDetector groups a request's query log by normalised SQL (IN lists collapsed so batched loads of differing sizes count as one shape, numeric and quoted literals replaced, transaction/savepoint noise skipped) and fails when a shape repeats past a threshold (default 5).

Two tiers, decided by whether the work grows with the data. Bindings are counted rather than folded into the shape, and that count is what separates a defect from mere waste:

Signature	Meaning	Result
distinct bindings ≈ executions (10x, 10 distinct)	one query per record — add rows, add queries	fails
a few values repeated (8x, 1 distinct)	wasteful but bounded; five queries for two users stays five at two million	warns

The warning is a E_USER_WARNING, which PHPUnit attributes to the triggering test — the author sees it without the build going red.

Making the warnings actually visible. As first written the warning tier was invisible three times over: the trigger_error call was @-silenced so PHPUnit never saw it, only one of eighteen phpunit configs in this repo enabled displayDetailsOnTestsThatTriggerWarnings, and a developer who relies on CI would have had to read the middle of a job log regardless. Fixed here: the @ is gone, every integration config displays warning details, and the detector appends findings to FLARUM_REPEATED_QUERY_LOG when set. The reusable backend workflow points that at a temp file and turns it into GitHub annotations (errors for N+1s, warnings for non-scaling repetition) plus a table in the run summary — so findings land on the pull request itself, where the CI-only audience is looking.

This calibration came from running the detector across every bundled extension: beyond the flags N+1 it produced 22 further findings, and all of them were the second kind (5–10 executions, 1–4 distinct values), mostly on post-save paths where a formatter resolves each mention. Failing on those would have meant rewriting formatter and write-path code for no scaling benefit — the threshold was what was wrong, not the extensions. mentions (19 findings) and subscriptions (1) now pass with warnings; the flags N+1 still fails.

On by default, with graduated escapes:

Escape	Scope	When
allowedRepeatedQueries()	one query shape	Preferred — rest of the request stays covered
detectsRepeatedQueries()	one test case	A test that legitimately can't satisfy it
FLARUM_DETECT_REPEATED_QUERIES=0	whole run	Bisecting an unrelated failure

Verification

• 11 unit tests on the detector: the real N+1 shape, batched in (…) loads of differing sizes (must not flag), sub-threshold loops, same-bindings repetition, transaction noise, inlined vs bound literals, ordering, and the message format.
• Against a real N+1: reintroducing the per-post loading in fof/moderator-warnings fails with 10x (10 distinct bindings): select * from warningswherewarnings.post_id = ?. With the fix restored, that extension's whole suite passes with detection on.
• Against core: tests/integration/api gives identical results with detection on and off — 353 tests, the same pre-existing failures, zero findings.
• flags: 16 tests green with detection on, after the FlagResource fix (was 2 failures).
• Bundled extensions: flags, mentions, subscriptions and approval all pass with detection on. tags (5) and likes (1) have pre-existing failures unrelated to queries — identical with FLARUM_DETECT_REPEATED_QUERIES=0.

Reviewers should focus on

• The threshold (5) and whether the normalisation is too aggressive or not aggressive enough for query shapes I haven't seen.
• The distinctBindings >= count - 1 rule that decides fail vs warn (one duplicate is tolerated, since batch loaders often re-read a single value while still doing per-record work), and whether that boundary holds for shapes I haven't seen.
• Implementation note worth knowing: processIsolation="true" makes PHPUnit parse a subprocess's entire output as its result protocol, so printing findings — STDOUT, STDERR, /dev/tty, shutdown functions — all surface as test errors. Failing the assertion is the only channel PHPUnit sanctions here.

Developer documentation: flarum/docs#568.

Confirmed

• Backend changes: tests are green (run composer test).

[imorland]

Changes proposed in this pull request

Every request sent through Flarum\Testing\integration\TestCase::send() is now inspected for N+1 query patterns, and the test fails when one is found.

The motivation is concrete: while profiling discussion-view performance this week, one extension turned out to be issuing a query per post on every page of every discussion, and another declared default includes that were never eager-loaded. Both were found by hand-profiling a live forum — no test noticed, because no test could. Extension authors should get that feedback while writing the feature.

And it immediately proved the point. On this PR's first CI run the detector failed two tests in flarum/flags — a bundled extension nobody had profiled, whose suite was green. GET /api/flags declares post.discussion and post.user as default includes but never eager-loaded what those nested resources need: core's DiscussionResource eager-loads the actor's discussion state on its own endpoints, and that doesn't carry over when a discussion is included by another resource. So every flag on the moderation page read discussion_user on its own, and the flag authors' groups were re-fetched per flag. That fix is included here (FlagResource), because a detector that lands with a known finding allow-listed on day one teaches the wrong lesson.

How it detects. An N+1 is one query shape executed once per record. RepeatedQueryDetector groups a request's query log by normalised SQL (IN lists collapsed so batched loads of differing sizes count as one shape, numeric and quoted literals replaced, transaction/savepoint noise skipped) and fails when a shape repeats past a threshold (default 5).

Two tiers, decided by whether the work grows with the data. Bindings are counted rather than folded into the shape, and that count is what separates a defect from mere waste:

Signature	Meaning	Result
distinct bindings ≈ executions (10x, 10 distinct)	one query per record — add rows, add queries	fails
a few values repeated (8x, 1 distinct)	wasteful but bounded; five queries for two users stays five at two million	warns

The warning is a E_USER_WARNING, which PHPUnit attributes to the triggering test — the author sees it without the build going red.

Making the warnings actually visible. As first written the warning tier was invisible three times over: the trigger_error call was @-silenced so PHPUnit never saw it, only one of eighteen phpunit configs in this repo enabled displayDetailsOnTestsThatTriggerWarnings, and a developer who relies on CI would have had to read the middle of a job log regardless. Fixed here: the @ is gone, every integration config displays warning details, and the detector appends findings to FLARUM_REPEATED_QUERY_LOG when set. The reusable backend workflow points that at a temp file and turns it into GitHub annotations (errors for N+1s, warnings for non-scaling repetition) plus a table in the run summary — so findings land on the pull request itself, where the CI-only audience is looking.

This calibration came from running the detector across every bundled extension: beyond the flags N+1 it produced 22 further findings, and all of them were the second kind (5–10 executions, 1–4 distinct values), mostly on post-save paths where a formatter resolves each mention. Failing on those would have meant rewriting formatter and write-path code for no scaling benefit — the threshold was what was wrong, not the extensions. mentions (19 findings) and subscriptions (1) now pass with warnings; the flags N+1 still fails.

On by default, with graduated escapes:

Escape	Scope	When
allowedRepeatedQueries()	one query shape	Preferred — rest of the request stays covered
detectsRepeatedQueries()	one test case	A test that legitimately can't satisfy it
FLARUM_DETECT_REPEATED_QUERIES=0	whole run	Bisecting an unrelated failure

Verification

• 11 unit tests on the detector: the real N+1 shape, batched in (…) loads of differing sizes (must not flag), sub-threshold loops, same-bindings repetition, transaction noise, inlined vs bound literals, ordering, and the message format.
• Against a real N+1: reintroducing the per-post loading in fof/moderator-warnings fails with 10x (10 distinct bindings): select * from warningswherewarnings.post_id = ?. With the fix restored, that extension's whole suite passes with detection on.
• Against core: tests/integration/api gives identical results with detection on and off — 353 tests, the same pre-existing failures, zero findings.
• flags: 16 tests green with detection on, after the FlagResource fix (was 2 failures).
• Bundled extensions: flags, mentions, subscriptions and approval all pass with detection on. tags (5) and likes (1) have pre-existing failures unrelated to queries — identical with FLARUM_DETECT_REPEATED_QUERIES=0.

Reviewers should focus on

• The threshold (5) and whether the normalisation is too aggressive or not aggressive enough for query shapes I haven't seen.
• The distinctBindings >= count - 1 rule that decides fail vs warn (one duplicate is tolerated, since batch loaders often re-read a single value while still doing per-record work), and whether that boundary holds for shapes I haven't seen.
• Implementation note worth knowing: processIsolation="true" makes PHPUnit parse a subprocess's entire output as its result protocol, so printing findings — STDOUT, STDERR, /dev/tty, shutdown functions — all surface as test errors. Failing the assertion is the only channel PHPUnit sanctions here.

Developer documentation: flarum/docs#568.

Confirmed

• Backend changes: tests are green (run composer test).

[imorland]

Changes proposed in this pull request

Every request sent through Flarum\Testing\integration\TestCase::send() is now inspected for N+1 query patterns, and the test fails when one is found.

The motivation is concrete: while profiling discussion-view performance this week, one extension turned out to be issuing a query per post on every page of every discussion, and another declared default includes that were never eager-loaded. Both were found by hand-profiling a live forum — no test noticed, because no test could. Extension authors should get that feedback while writing the feature.

And it immediately proved the point. On this PR's first CI run the detector failed two tests in flarum/flags — a bundled extension nobody had profiled, whose suite was green. GET /api/flags declares post.discussion and post.user as default includes but never eager-loaded what those nested resources need: core's DiscussionResource eager-loads the actor's discussion state on its own endpoints, and that doesn't carry over when a discussion is included by another resource. So every flag on the moderation page read discussion_user on its own, and the flag authors' groups were re-fetched per flag. That fix is included here (FlagResource), because a detector that lands with a known finding allow-listed on day one teaches the wrong lesson.

How it detects. An N+1 is one query shape executed once per record. RepeatedQueryDetector groups a request's query log by normalised SQL (IN lists collapsed so batched loads of differing sizes count as one shape, numeric and quoted literals replaced, transaction/savepoint noise skipped) and fails when a shape repeats past a threshold (default 5).

Two tiers, decided by whether the work grows with the data. Bindings are counted rather than folded into the shape, and that count is what separates a defect from mere waste:

Signature	Meaning	Result
distinct bindings ≈ executions (10x, 10 distinct)	one query per record — add rows, add queries	fails
a few values repeated (8x, 1 distinct)	wasteful but bounded; five queries for two users stays five at two million	warns

The warning is a E_USER_WARNING, which PHPUnit attributes to the triggering test — the author sees it without the build going red.

Making the warnings actually visible. As first written the warning tier was invisible three times over: the trigger_error call was @-silenced so PHPUnit never saw it, only one of eighteen phpunit configs in this repo enabled displayDetailsOnTestsThatTriggerWarnings, and a developer who relies on CI would have had to read the middle of a job log regardless. Fixed here: the @ is gone, every integration config displays warning details, and the detector appends findings to FLARUM_REPEATED_QUERY_LOG when set. The reusable backend workflow points that at a temp file and turns it into GitHub annotations (errors for N+1s, warnings for non-scaling repetition) plus a table in the run summary — so findings land on the pull request itself, where the CI-only audience is looking.

This calibration came from running the detector across every bundled extension: beyond the flags N+1 it produced 22 further findings, and all of them were the second kind (5–10 executions, 1–4 distinct values), mostly on post-save paths where a formatter resolves each mention. Failing on those would have meant rewriting formatter and write-path code for no scaling benefit — the threshold was what was wrong, not the extensions. mentions (19 findings) and subscriptions (1) now pass with warnings; the flags N+1 still fails.

On by default, with graduated escapes:

Escape	Scope	When
allowedRepeatedQueries()	one query shape	Preferred — rest of the request stays covered
detectsRepeatedQueries()	one test case	A test that legitimately can't satisfy it
FLARUM_DETECT_REPEATED_QUERIES=0	whole run	Bisecting an unrelated failure

Verification

• 11 unit tests on the detector: the real N+1 shape, batched in (…) loads of differing sizes (must not flag), sub-threshold loops, same-bindings repetition, transaction noise, inlined vs bound literals, ordering, and the message format.
• Against a real N+1: reintroducing the per-post loading in fof/moderator-warnings fails with 10x (10 distinct bindings): select * from warningswherewarnings.post_id = ?. With the fix restored, that extension's whole suite passes with detection on.
• Against core: tests/integration/api gives identical results with detection on and off — 353 tests, the same pre-existing failures, zero findings.
• flags: 16 tests green with detection on, after the FlagResource fix (was 2 failures).
• Bundled extensions: flags, mentions, subscriptions and approval all pass with detection on. tags (5) and likes (1) have pre-existing failures unrelated to queries — identical with FLARUM_DETECT_REPEATED_QUERIES=0.

Reviewers should focus on

• The threshold (5) and whether the normalisation is too aggressive or not aggressive enough for query shapes I haven't seen.
• The distinctBindings >= count - 1 rule that decides fail vs warn (one duplicate is tolerated, since batch loaders often re-read a single value while still doing per-record work), and whether that boundary holds for shapes I haven't seen.
• Implementation note worth knowing: processIsolation="true" makes PHPUnit parse a subprocess's entire output as its result protocol, so printing findings — STDOUT, STDERR, /dev/tty, shutdown functions — all surface as test errors. Failing the assertion is the only channel PHPUnit sanctions here.

Developer documentation: flarum/docs#568.

Confirmed

• Backend changes: tests are green (run composer test).

[imorland]

Changes proposed in this pull request

Every request sent through Flarum\Testing\integration\TestCase::send() is now inspected for N+1 query patterns, and the test fails when one is found.

The motivation is concrete: while profiling discussion-view performance this week, one extension turned out to be issuing a query per post on every page of every discussion, and another declared default includes that were never eager-loaded. Both were found by hand-profiling a live forum — no test noticed, because no test could. Extension authors should get that feedback while writing the feature.

And it immediately proved the point. On this PR's first CI run the detector failed two tests in flarum/flags — a bundled extension nobody had profiled, whose suite was green. GET /api/flags declares post.discussion and post.user as default includes but never eager-loaded what those nested resources need: core's DiscussionResource eager-loads the actor's discussion state on its own endpoints, and that doesn't carry over when a discussion is included by another resource. So every flag on the moderation page read discussion_user on its own, and the flag authors' groups were re-fetched per flag. That fix is included here (FlagResource), because a detector that lands with a known finding allow-listed on day one teaches the wrong lesson.

How it detects. An N+1 is one query shape executed once per record. RepeatedQueryDetector groups a request's query log by normalised SQL (IN lists collapsed so batched loads of differing sizes count as one shape, numeric and quoted literals replaced, transaction/savepoint noise skipped) and fails when a shape repeats past a threshold (default 5).

Two tiers, decided by whether the work grows with the data. Bindings are counted rather than folded into the shape, and that count is what separates a defect from mere waste:

Signature	Meaning	Result
distinct bindings ≈ executions (10x, 10 distinct)	one query per record — add rows, add queries	fails
a few values repeated (8x, 1 distinct)	wasteful but bounded; five queries for two users stays five at two million	warns

The warning is a E_USER_WARNING, which PHPUnit attributes to the triggering test — the author sees it without the build going red.

Making the warnings actually visible. As first written the warning tier was invisible three times over: the trigger_error call was @-silenced so PHPUnit never saw it, only one of eighteen phpunit configs in this repo enabled displayDetailsOnTestsThatTriggerWarnings, and a developer who relies on CI would have had to read the middle of a job log regardless. Fixed here: the @ is gone, every integration config displays warning details, and the detector appends findings to FLARUM_REPEATED_QUERY_LOG when set. The reusable backend workflow points that at a temp file and turns it into GitHub annotations (errors for N+1s, warnings for non-scaling repetition) plus a table in the run summary — so findings land on the pull request itself, where the CI-only audience is looking.

This calibration came from running the detector across every bundled extension: beyond the flags N+1 it produced 22 further findings, and all of them were the second kind (5–10 executions, 1–4 distinct values), mostly on post-save paths where a formatter resolves each mention. Failing on those would have meant rewriting formatter and write-path code for no scaling benefit — the threshold was what was wrong, not the extensions. mentions (19 findings) and subscriptions (1) now pass with warnings; the flags N+1 still fails.

On by default, with graduated escapes:

Escape	Scope	When
allowedRepeatedQueries()	one query shape	Preferred — rest of the request stays covered
detectsRepeatedQueries()	one test case	A test that legitimately can't satisfy it
FLARUM_DETECT_REPEATED_QUERIES=0	whole run	Bisecting an unrelated failure

Verification

• 11 unit tests on the detector: the real N+1 shape, batched in (…) loads of differing sizes (must not flag), sub-threshold loops, same-bindings repetition, transaction noise, inlined vs bound literals, ordering, and the message format.
• Against a real N+1: reintroducing the per-post loading in fof/moderator-warnings fails with 10x (10 distinct bindings): select * from warningswherewarnings.post_id = ?. With the fix restored, that extension's whole suite passes with detection on.
• Against core: tests/integration/api gives identical results with detection on and off — 353 tests, the same pre-existing failures, zero findings.
• flags: 16 tests green with detection on, after the FlagResource fix (was 2 failures).
• Bundled extensions: flags, mentions, subscriptions and approval all pass with detection on. tags (5) and likes (1) have pre-existing failures unrelated to queries — identical with FLARUM_DETECT_REPEATED_QUERIES=0.

Reviewers should focus on

• The threshold (5) and whether the normalisation is too aggressive or not aggressive enough for query shapes I haven't seen.
• The distinctBindings >= count - 1 rule that decides fail vs warn (one duplicate is tolerated, since batch loaders often re-read a single value while still doing per-record work), and whether that boundary holds for shapes I haven't seen.
• Implementation note worth knowing: processIsolation="true" makes PHPUnit parse a subprocess's entire output as its result protocol, so printing findings — STDOUT, STDERR, /dev/tty, shutdown functions — all surface as test errors. Failing the assertion is the only channel PHPUnit sanctions here.

Developer documentation: flarum/docs#568.

Confirmed

• Backend changes: tests are green (run composer test).

[imorland]

Changes proposed in this pull request

Every request sent through Flarum\Testing\integration\TestCase::send() is now inspected for N+1 query patterns, and the test fails when one is found.

The motivation is concrete: while profiling discussion-view performance this week, one extension turned out to be issuing a query per post on every page of every discussion, and another declared default includes that were never eager-loaded. Both were found by hand-profiling a live forum — no test noticed, because no test could. Extension authors should get that feedback while writing the feature.

And it immediately proved the point. On this PR's first CI run the detector failed two tests in flarum/flags — a bundled extension nobody had profiled, whose suite was green. GET /api/flags declares post.discussion and post.user as default includes but never eager-loaded what those nested resources need: core's DiscussionResource eager-loads the actor's discussion state on its own endpoints, and that doesn't carry over when a discussion is included by another resource. So every flag on the moderation page read discussion_user on its own, and the flag authors' groups were re-fetched per flag. That fix is included here (FlagResource), because a detector that lands with a known finding allow-listed on day one teaches the wrong lesson.

How it detects. An N+1 is one query shape executed once per record. RepeatedQueryDetector groups a request's query log by normalised SQL (IN lists collapsed so batched loads of differing sizes count as one shape, numeric and quoted literals replaced, transaction/savepoint noise skipped) and fails when a shape repeats past a threshold (default 5).

Two tiers, decided by whether the work grows with the data. Bindings are counted rather than folded into the shape, and that count is what separates a defect from mere waste:

Signature	Meaning	Result
distinct bindings ≈ executions (10x, 10 distinct)	one query per record — add rows, add queries	fails
a few values repeated (8x, 1 distinct)	wasteful but bounded; five queries for two users stays five at two million	warns

The warning is a E_USER_WARNING, which PHPUnit attributes to the triggering test — the author sees it without the build going red.

Making the warnings actually visible. As first written the warning tier was invisible three times over: the trigger_error call was @-silenced so PHPUnit never saw it, only one of eighteen phpunit configs in this repo enabled displayDetailsOnTestsThatTriggerWarnings, and a developer who relies on CI would have had to read the middle of a job log regardless. Fixed here: the @ is gone, every integration config displays warning details, and the detector appends findings to FLARUM_REPEATED_QUERY_LOG when set. The reusable backend workflow points that at a temp file and turns it into GitHub annotations (errors for N+1s, warnings for non-scaling repetition) plus a table in the run summary — so findings land on the pull request itself, where the CI-only audience is looking.

This calibration came from running the detector across every bundled extension: beyond the flags N+1 it produced 22 further findings, and all of them were the second kind (5–10 executions, 1–4 distinct values), mostly on post-save paths where a formatter resolves each mention. Failing on those would have meant rewriting formatter and write-path code for no scaling benefit — the threshold was what was wrong, not the extensions. mentions (19 findings) and subscriptions (1) now pass with warnings; the flags N+1 still fails.

On by default, with graduated escapes:

Escape	Scope	When
allowedRepeatedQueries()	one query shape	Preferred — rest of the request stays covered
detectsRepeatedQueries()	one test case	A test that legitimately can't satisfy it
FLARUM_DETECT_REPEATED_QUERIES=0	whole run	Bisecting an unrelated failure

Verification

• 11 unit tests on the detector: the real N+1 shape, batched in (…) loads of differing sizes (must not flag), sub-threshold loops, same-bindings repetition, transaction noise, inlined vs bound literals, ordering, and the message format.
• Against a real N+1: reintroducing the per-post loading in fof/moderator-warnings fails with 10x (10 distinct bindings): select * from warningswherewarnings.post_id = ?. With the fix restored, that extension's whole suite passes with detection on.
• Against core: tests/integration/api gives identical results with detection on and off — 353 tests, the same pre-existing failures, zero findings.
• flags: 16 tests green with detection on, after the FlagResource fix (was 2 failures).
• Bundled extensions: flags, mentions, subscriptions and approval all pass with detection on. tags (5) and likes (1) have pre-existing failures unrelated to queries — identical with FLARUM_DETECT_REPEATED_QUERIES=0.

Reviewers should focus on

• The threshold (5) and whether the normalisation is too aggressive or not aggressive enough for query shapes I haven't seen.
• The distinctBindings >= count - 1 rule that decides fail vs warn (one duplicate is tolerated, since batch loaders often re-read a single value while still doing per-record work), and whether that boundary holds for shapes I haven't seen.
• Implementation note worth knowing: processIsolation="true" makes PHPUnit parse a subprocess's entire output as its result protocol, so printing findings — STDOUT, STDERR, /dev/tty, shutdown functions — all surface as test errors. Failing the assertion is the only channel PHPUnit sanctions here.

Developer documentation: flarum/docs#568.

Confirmed

• Backend changes: tests are green (run composer test).

[imorland]

Changes proposed in this pull request

Every request sent through Flarum\Testing\integration\TestCase::send() is now inspected for N+1 query patterns, and the test fails when one is found.

The motivation is concrete: while profiling discussion-view performance this week, one extension turned out to be issuing a query per post on every page of every discussion, and another declared default includes that were never eager-loaded. Both were found by hand-profiling a live forum — no test noticed, because no test could. Extension authors should get that feedback while writing the feature.

And it immediately proved the point. On this PR's first CI run the detector failed two tests in flarum/flags — a bundled extension nobody had profiled, whose suite was green. GET /api/flags declares post.discussion and post.user as default includes but never eager-loaded what those nested resources need: core's DiscussionResource eager-loads the actor's discussion state on its own endpoints, and that doesn't carry over when a discussion is included by another resource. So every flag on the moderation page read discussion_user on its own, and the flag authors' groups were re-fetched per flag. That fix is included here (FlagResource), because a detector that lands with a known finding allow-listed on day one teaches the wrong lesson.

How it detects. An N+1 is one query shape executed once per record. RepeatedQueryDetector groups a request's query log by normalised SQL (IN lists collapsed so batched loads of differing sizes count as one shape, numeric and quoted literals replaced, transaction/savepoint noise skipped) and fails when a shape repeats past a threshold (default 5).

Two tiers, decided by whether the work grows with the data. Bindings are counted rather than folded into the shape, and that count is what separates a defect from mere waste:

Signature	Meaning	Result
distinct bindings ≈ executions (10x, 10 distinct)	one query per record — add rows, add queries	fails
a few values repeated (8x, 1 distinct)	wasteful but bounded; five queries for two users stays five at two million	warns

The warning is a E_USER_WARNING, which PHPUnit attributes to the triggering test — the author sees it without the build going red.

Making the warnings actually visible. As first written the warning tier was invisible three times over: the trigger_error call was @-silenced so PHPUnit never saw it, only one of eighteen phpunit configs in this repo enabled displayDetailsOnTestsThatTriggerWarnings, and a developer who relies on CI would have had to read the middle of a job log regardless. Fixed here: the @ is gone, every integration config displays warning details, and the detector appends findings to FLARUM_REPEATED_QUERY_LOG when set. The reusable backend workflow points that at a temp file and turns it into GitHub annotations (errors for N+1s, warnings for non-scaling repetition) plus a table in the run summary — so findings land on the pull request itself, where the CI-only audience is looking.

This calibration came from running the detector across every bundled extension: beyond the flags N+1 it produced 22 further findings, and all of them were the second kind (5–10 executions, 1–4 distinct values), mostly on post-save paths where a formatter resolves each mention. Failing on those would have meant rewriting formatter and write-path code for no scaling benefit — the threshold was what was wrong, not the extensions. mentions (19 findings) and subscriptions (1) now pass with warnings; the flags N+1 still fails.

On by default, with graduated escapes:

Escape	Scope	When
allowedRepeatedQueries()	one query shape	Preferred — rest of the request stays covered
detectsRepeatedQueries()	one test case	A test that legitimately can't satisfy it
FLARUM_DETECT_REPEATED_QUERIES=0	whole run	Bisecting an unrelated failure

Verification

• 11 unit tests on the detector: the real N+1 shape, batched in (…) loads of differing sizes (must not flag), sub-threshold loops, same-bindings repetition, transaction noise, inlined vs bound literals, ordering, and the message format.
• Against a real N+1: reintroducing the per-post loading in fof/moderator-warnings fails with 10x (10 distinct bindings): select * from warningswherewarnings.post_id = ?. With the fix restored, that extension's whole suite passes with detection on.
• Against core: tests/integration/api gives identical results with detection on and off — 353 tests, the same pre-existing failures, zero findings.
• flags: 16 tests green with detection on, after the FlagResource fix (was 2 failures).
• Bundled extensions: flags, mentions, subscriptions and approval all pass with detection on. tags (5) and likes (1) have pre-existing failures unrelated to queries — identical with FLARUM_DETECT_REPEATED_QUERIES=0.

Reviewers should focus on

• The threshold (5) and whether the normalisation is too aggressive or not aggressive enough for query shapes I haven't seen.
• The distinctBindings >= count - 1 rule that decides fail vs warn (one duplicate is tolerated, since batch loaders often re-read a single value while still doing per-record work), and whether that boundary holds for shapes I haven't seen.
• Implementation note worth knowing: processIsolation="true" makes PHPUnit parse a subprocess's entire output as its result protocol, so printing findings — STDOUT, STDERR, /dev/tty, shutdown functions — all surface as test errors. Failing the assertion is the only channel PHPUnit sanctions here.

Developer documentation: flarum/docs#568.

Confirmed

• Backend changes: tests are green (run composer test).

[imorland]

Changes proposed in this pull request

Every request sent through Flarum\Testing\integration\TestCase::send() is now inspected for N+1 query patterns, and the test fails when one is found.

The motivation is concrete: while profiling discussion-view performance this week, one extension turned out to be issuing a query per post on every page of every discussion, and another declared default includes that were never eager-loaded. Both were found by hand-profiling a live forum — no test noticed, because no test could. Extension authors should get that feedback while writing the feature.

And it immediately proved the point. On this PR's first CI run the detector failed two tests in flarum/flags — a bundled extension nobody had profiled, whose suite was green. GET /api/flags declares post.discussion and post.user as default includes but never eager-loaded what those nested resources need: core's DiscussionResource eager-loads the actor's discussion state on its own endpoints, and that doesn't carry over when a discussion is included by another resource. So every flag on the moderation page read discussion_user on its own, and the flag authors' groups were re-fetched per flag. That fix is included here (FlagResource), because a detector that lands with a known finding allow-listed on day one teaches the wrong lesson.

How it detects. An N+1 is one query shape executed once per record. RepeatedQueryDetector groups a request's query log by normalised SQL (IN lists collapsed so batched loads of differing sizes count as one shape, numeric and quoted literals replaced, transaction/savepoint noise skipped) and fails when a shape repeats past a threshold (default 5).

Two tiers, decided by whether the work grows with the data. Bindings are counted rather than folded into the shape, and that count is what separates a defect from mere waste:

Signature	Meaning	Result
distinct bindings ≈ executions (10x, 10 distinct)	one query per record — add rows, add queries	fails
a few values repeated (8x, 1 distinct)	wasteful but bounded; five queries for two users stays five at two million	warns

The warning is a E_USER_WARNING, which PHPUnit attributes to the triggering test — the author sees it without the build going red.

Making the warnings actually visible. As first written the warning tier was invisible three times over: the trigger_error call was @-silenced so PHPUnit never saw it, only one of eighteen phpunit configs in this repo enabled displayDetailsOnTestsThatTriggerWarnings, and a developer who relies on CI would have had to read the middle of a job log regardless. Fixed here: the @ is gone, every integration config displays warning details, and the detector appends findings to FLARUM_REPEATED_QUERY_LOG when set. The reusable backend workflow points that at a temp file and turns it into GitHub annotations (errors for N+1s, warnings for non-scaling repetition) plus a table in the run summary — so findings land on the pull request itself, where the CI-only audience is looking.

This calibration came from running the detector across every bundled extension: beyond the flags N+1 it produced 22 further findings, and all of them were the second kind (5–10 executions, 1–4 distinct values), mostly on post-save paths where a formatter resolves each mention. Failing on those would have meant rewriting formatter and write-path code for no scaling benefit — the threshold was what was wrong, not the extensions. mentions (19 findings) and subscriptions (1) now pass with warnings; the flags N+1 still fails.

On by default, with graduated escapes:

Escape	Scope	When
allowedRepeatedQueries()	one query shape	Preferred — rest of the request stays covered
detectsRepeatedQueries()	one test case	A test that legitimately can't satisfy it
FLARUM_DETECT_REPEATED_QUERIES=0	whole run	Bisecting an unrelated failure

Verification

• 11 unit tests on the detector: the real N+1 shape, batched in (…) loads of differing sizes (must not flag), sub-threshold loops, same-bindings repetition, transaction noise, inlined vs bound literals, ordering, and the message format.
• Against a real N+1: reintroducing the per-post loading in fof/moderator-warnings fails with 10x (10 distinct bindings): select * from warningswherewarnings.post_id = ?. With the fix restored, that extension's whole suite passes with detection on.
• Against core: tests/integration/api gives identical results with detection on and off — 353 tests, the same pre-existing failures, zero findings.
• flags: 16 tests green with detection on, after the FlagResource fix (was 2 failures).
• Bundled extensions: flags, mentions, subscriptions and approval all pass with detection on. tags (5) and likes (1) have pre-existing failures unrelated to queries — identical with FLARUM_DETECT_REPEATED_QUERIES=0.

Reviewers should focus on

• The threshold (5) and whether the normalisation is too aggressive or not aggressive enough for query shapes I haven't seen.
• The distinctBindings >= count - 1 rule that decides fail vs warn (one duplicate is tolerated, since batch loaders often re-read a single value while still doing per-record work), and whether that boundary holds for shapes I haven't seen.
• Implementation note worth knowing: processIsolation="true" makes PHPUnit parse a subprocess's entire output as its result protocol, so printing findings — STDOUT, STDERR, /dev/tty, shutdown functions — all surface as test errors. Failing the assertion is the only channel PHPUnit sanctions here.

Developer documentation: flarum/docs#568.

Confirmed

• Backend changes: tests are green (run composer test).

[imorland]

Changes proposed in this pull request

Every request sent through Flarum\Testing\integration\TestCase::send() is now inspected for N+1 query patterns, and the test fails when one is found.

The motivation is concrete: while profiling discussion-view performance this week, one extension turned out to be issuing a query per post on every page of every discussion, and another declared default includes that were never eager-loaded. Both were found by hand-profiling a live forum — no test noticed, because no test could. Extension authors should get that feedback while writing the feature.

And it immediately proved the point. On this PR's first CI run the detector failed two tests in flarum/flags — a bundled extension nobody had profiled, whose suite was green. GET /api/flags declares post.discussion and post.user as default includes but never eager-loaded what those nested resources need: core's DiscussionResource eager-loads the actor's discussion state on its own endpoints, and that doesn't carry over when a discussion is included by another resource. So every flag on the moderation page read discussion_user on its own, and the flag authors' groups were re-fetched per flag. That fix is included here (FlagResource), because a detector that lands with a known finding allow-listed on day one teaches the wrong lesson.

How it detects. An N+1 is one query shape executed once per record. RepeatedQueryDetector groups a request's query log by normalised SQL (IN lists collapsed so batched loads of differing sizes count as one shape, numeric and quoted literals replaced, transaction/savepoint noise skipped) and fails when a shape repeats past a threshold (default 5).

Two tiers, decided by whether the work grows with the data. Bindings are counted rather than folded into the shape, and that count is what separates a defect from mere waste:

Signature	Meaning	Result
distinct bindings ≈ executions (10x, 10 distinct)	one query per record — add rows, add queries	fails
a few values repeated (8x, 1 distinct)	wasteful but bounded; five queries for two users stays five at two million	warns

The warning is a E_USER_WARNING, which PHPUnit attributes to the triggering test — the author sees it without the build going red.

Making the warnings actually visible. As first written the warning tier was invisible three times over: the trigger_error call was @-silenced so PHPUnit never saw it, only one of eighteen phpunit configs in this repo enabled displayDetailsOnTestsThatTriggerWarnings, and a developer who relies on CI would have had to read the middle of a job log regardless. Fixed here: the @ is gone, every integration config displays warning details, and the detector appends findings to FLARUM_REPEATED_QUERY_LOG when set. The reusable backend workflow points that at a temp file and turns it into GitHub annotations (errors for N+1s, warnings for non-scaling repetition) plus a table in the run summary — so findings land on the pull request itself, where the CI-only audience is looking.

This calibration came from running the detector across every bundled extension: beyond the flags N+1 it produced 22 further findings, and all of them were the second kind (5–10 executions, 1–4 distinct values), mostly on post-save paths where a formatter resolves each mention. Failing on those would have meant rewriting formatter and write-path code for no scaling benefit — the threshold was what was wrong, not the extensions. mentions (19 findings) and subscriptions (1) now pass with warnings; the flags N+1 still fails.

On by default, with graduated escapes:

Escape	Scope	When
allowedRepeatedQueries()	one query shape	Preferred — rest of the request stays covered
detectsRepeatedQueries()	one test case	A test that legitimately can't satisfy it
FLARUM_DETECT_REPEATED_QUERIES=0	whole run	Bisecting an unrelated failure

Verification

• 11 unit tests on the detector: the real N+1 shape, batched in (…) loads of differing sizes (must not flag), sub-threshold loops, same-bindings repetition, transaction noise, inlined vs bound literals, ordering, and the message format.
• Against a real N+1: reintroducing the per-post loading in fof/moderator-warnings fails with 10x (10 distinct bindings): select * from warningswherewarnings.post_id = ?. With the fix restored, that extension's whole suite passes with detection on.
• Against core: tests/integration/api gives identical results with detection on and off — 353 tests, the same pre-existing failures, zero findings.
• flags: 16 tests green with detection on, after the FlagResource fix (was 2 failures).
• Bundled extensions: flags, mentions, subscriptions and approval all pass with detection on. tags (5) and likes (1) have pre-existing failures unrelated to queries — identical with FLARUM_DETECT_REPEATED_QUERIES=0.

Reviewers should focus on

• The threshold (5) and whether the normalisation is too aggressive or not aggressive enough for query shapes I haven't seen.
• The distinctBindings >= count - 1 rule that decides fail vs warn (one duplicate is tolerated, since batch loaders often re-read a single value while still doing per-record work), and whether that boundary holds for shapes I haven't seen.
• Implementation note worth knowing: processIsolation="true" makes PHPUnit parse a subprocess's entire output as its result protocol, so printing findings — STDOUT, STDERR, /dev/tty, shutdown functions — all surface as test errors. Failing the assertion is the only channel PHPUnit sanctions here.

Developer documentation: flarum/docs#568.

Confirmed

• Backend changes: tests are green (run composer test).