Oo

[MrIbrahem]

Summary

Reworks the statistics/status endpoints: adds a dedicated graph_data handler with filtering, introduces a real get_lang_years endpoint (replacing the user_lang_status alias), adds a new user_data_status endpoint, and removes the legacy status endpoint. Campaign/category filtering is centralized in EndpointContext::applyCampaignCategory, and empty/All filter values are now treated as inactive across endpoints.

Changes

• graph_data — replaced the static SQL with a new GraphDataHandler supporting year, month, user, lang, user_group, category/cat, and campaign filters, grouping monthly page counts by date (src/app/Endpoints/Handlers/GraphDataHandler.php, endpoint_params.json).
• New get_lang_years — replaces the user_lang_status redirect alias with an actual query returning distinct years, with an optional lang filter (EndpointRegistry.php, request.php, openapi.json, README.md).
• New user_data_status — backed by UserDataStatusHandler + UserDataStatusFormatter, returning per-year / per-language / per-campaign counts.
• Removed legacy status — deletes StatusHandler.php, status.php, its request.php case, the bootstrap.php include, the README row, and the OpenAPI entry.
• Centralized campaign/category filtering — new EndpointContext::applyCampaignCategory() (shared PATTERN + InputSanitizer), now used by both PagesHandler and GraphDataHandler.
• Consistent filter handling — new FilterBuilder::isValid() treats empty and All values as inactive in one place; EndpointContext::isValid() exposes it.
• Error reporting — APIController now attaches the exception message to 500 responses; Database::fetchQuery logs the failing query at debug level.
• Docs/cleanup — README, OpenAPI, docs/new-structure-oc.md updates and small cleanups in subs/missing_exists.php.

Testing

No tests included in this PR.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

• Configuration used: Repository: Mdwiki-TD/TD_API/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7d43e56d-7221-4c61-ad66-89d9ced1540b

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[MrIbrahem]

Review summary

This PR cleans up the statistics endpoints nicely — centralizing campaign/category filtering into EndpointContext and replacing ad-hoc handlers with dedicated ones is a real improvement, and the legacy status endpoint is removed completely (no dangling references). That said, it introduces a few regressions that should be fixed before merge: a TypeError that makes user_data_status fail whenever user is omitted, a shared filter helper that now silently drops 0-valued filters (e.g. pages?Deleted=0), and raw exception messages echoed to API clients.

Verdict: Changes requested 🔴 — submitted as COMMENT 💬 because the reviewer is the PR author, and GitHub does not allow requesting changes on your own PR. The blocking issues are listed below and in the inline comments.

Title

The current title Oo doesn't describe the change. The repo has no strict convention, but recent descriptive titles use imperative mood (e.g. #48 "Fix PHPStan and PHPUnit tests", #43 "Migrate exists_by_qids_query from all_articles to category_members"). Suggested alternatives:

• Add graph_data/get_lang_years endpoints and user_data_status flow
• Replace legacy status endpoint and centralize campaign/category filtering
• Rework statistics endpoints (graph_data, get_lang_years, user_data_status)

Highlights

• EndpointContext::applyCampaignCategory() removes genuine duplication between PagesHandler and the new GraphDataHandler, and keeps the InputSanitizer pattern allowlist.
• FilterBuilder::isValid() is a good single place to define the empty / All semantics.
• The status removal is thorough — handler, status.php, bootstrap.php include, request.php case, README row, and OpenAPI entry are all gone.
• The new endpoints are consistent with the existing handler/formatter architecture.

Issues found

• 🔴 Blocking: 3 — see inline comments (UserDataStatusHandler, APIController, FilterBuilder)
• 🟡 Suggestions: 6
• Minor nits: redundant Logger::debug in Database::fetchQuery (line 122 duplicates the Logger::error above it, which already logs message and query).

Cross-cutting notes

• user_data_status is undocumented: it's registered in EndpointRegistry and formatted by UserDataStatusFormatter, but has no entry in endpoint_params.json, openapi.json, or README.md. That also means supported_params/supported_values come back empty. Add the config + docs, or mark it internal.
• Removing status is a breaking change: api.php?get=status now returns invalid get request. The old user_lang_status used a redirect entry — consider the same for status → graph_data (or user_status) if any external client still calls it.
• graph_data response shape changed: it returned rows keyed m/c and now returns date/count. Confirm the consuming dashboard expects the new keys (the GraphDataFormatter that would produce labels/counts is currently disabled — see inline comment).
• Diff scope: most of docs/new-structure-oc.md is cosmetic markdown reformatting (list indentation / table alignment) unrelated to the endpoint changes. Fine, but splitting such churn out keeps the functional diff easier to review.

[MrIbrahem]

🔴 Bug — user_data_status throws whenever user is not supplied.

Request::get() returns ?string (null when the parameter is absent), but EndpointContext::isValid() is declared as isValid(string $value) — and both files declare strict_types=1. So api.php?get=user_data_status (no user) raises TypeError: Argument #1 ($value) must be of type string, null given, which the controller converts into a 500.

The if clearly intends user to be optional, so widen the signature:

// EndpointContext.php
public function isValid(mixed $value): bool
{
    return FilterBuilder::isValid($value);
}

(or pass $user ?? '' at the call site). This is reachable today — user_data_status is registered in EndpointRegistry, so it's a live endpoint, and it currently 500s on its most basic call.

[MrIbrahem]

🔴 Security — raw exception messages are now echoed to API clients.

$e->getMessage() for a PDOException / DatabaseException typically contains the failing SQL and schema details (e.g. Unknown column 'p.campaign' in 'where clause'), and other throwables leak file/class/method internals. For a public API this is information disclosure, and it also contradicts the deliberate errorOnly('internal error') user-facing message.

The codebase already has a gate for exactly this — ResponseBuilder::build() only exposes query when Environment::isLocalhost(). Reuse it:

$result = $this->builder->errorOnly('internal error');
if (Environment::isLocalhost()) {
    $result['e'] = $e->getMessage();
}
$this->emit($result);

(Side note: this PR also removed the Logger::error() doc-comment stating internal details never reach the user — worth preserving that intent.)

[MrIbrahem]

🔴 Regression — filter values of "0" are now silently dropped.

empty("0") is true in PHP, so isValid("0") returns false and the filter is skipped. The old check only excluded all, so a 0 value was applied. This affects every endpoint: pages has a Deleted → p.deleted param, so api.php?get=pages&Deleted=0 now silently returns both deleted and non-deleted pages. The same risk applies to other status/numeric params (result, translate_type, …).

Use an explicit emptiness check rather than empty():

public static function isValid(mixed $str): bool
{
    return $str !== null && $str !== '' && strtolower((string)$str) !== 'all';
}

Two related cleanups:

• This makes the no_empty_value branch below dead code (!isValid($value) already continues on empty values), so the new no_empty_value: true flags on graph_data's month/year have no effect. Either drop the flags or keep isValid strict only about all.
• != "all" works but should be !== 'all' on the lowercased value for a strict comparison.

[MrIbrahem]

🟡 COUNT(*) can be inflated by the joins; DISTINCT is redundant here.

Since you GROUP BY LEFT(p.pupdate, 7) with an aggregate, SELECT DISTINCT does nothing — each group yields exactly one row. The real risk is fan-in: if categories.category isn't unique per cat, or a username appears more than once in users, these LEFT JOINs multiply rows and COUNT(*) overcounts. The old graph_data had no joins at all.

For exact counts, count the pages themselves:

SELECT LEFT(p.pupdate, 7) AS date, COUNT(DISTINCT p.id) AS count

(adjust if pages has no id), or push the campaign filter into a subquery.

Two semantic notes on campaign:

• endpoint_params.json declares campaign → p.campaign, but the applied filter is ca.campaign (via applyCampaignCategory). If pages.campaign and its category's campaign can disagree, ?campaign=X won't match the documented column.
• The removed StatusHandler filtered campaigns as p.cat IN (SELECT category FROM categories WHERE campaign = ?); the new shared helper uses ca.campaign = ?. They're equivalent only if each category maps to exactly one campaign — worth confirming, since this helper now backs pages too.

[MrIbrahem]

🟡 Consider aggregating in SQL instead of counting in PHP.

Without a user filter this pulls one row per page joined to categories (potentially the whole pages table) into PHP and loops over it, just to produce per-year/lang/campaign counts. A GROUP BY YEAR(p.pupdate), p.lang, ca.campaign with COUNT(*) in UserDataStatusHandler returns the same buckets in one round-trip and scales far better.

Also: the header comment on line 2 says // src/app/Formatting/LeaderboardFormatter.php — a copy-paste leftover; it should be UserDataStatusFormatter.php.

[MrIbrahem]

🟡 Unnecessary join, and the SQL is duplicated.

The LEFT JOIN categories ca isn't needed here — the only filter is p.lang, and DISTINCT YEAR(...) already collapses any fan-in, so the join just adds cost. Drop it:

'get_lang_years' => new FilteredSqlHandler(
    'SELECT DISTINCT YEAR(p.pupdate) AS year FROM pages p'
),

The same query is also duplicated in the legacy request.php (case 'get_lang_years'). If that path is still served for this endpoint the two definitions can drift; if it isn't, the legacy case can go.

[MrIbrahem]

🟡 GraphDataFormatter is dead code — what shape should graph_data return?

This mapping is commented out, so graph_data falls through to default and returns raw rows ([{ "date": …, "count": … }, …]), while GraphDataFormatter (and its labels/counts output) is never called. Either enable the line or delete the class and its import.

Worth confirming against the consumer: this PR also renamed the response keys from m/c (the old static SQL) to date/count, so any dashboard still reading m/c breaks silently.

(Nit: the four new use App\Formatting\* imports are no-ops — this file already lives in namespace App\Formatting, so those classes resolve without imports.)

[MrIbrahem]

🟡 This description is inaccurate for get_lang_years.

It's copied from the old user_lang_status alias, but get_lang_years no longer redirects — it's a real endpoint returning the distinct years present in pages, optionally filtered by lang. Something like:

| `/api.php?get=lang_years` | GET | Distinct years present in pages (optional lang filter) |

Also consider the endpoint name itself: every other endpoint is a bare noun (langs, pages, views, …), so get_lang_years reads like a function call. lang_years would match the house style — and this row is also out of alphabetical order, since it was left where user_lang_status used to sit.

[MrIbrahem]

🟡 Empty summary, and user_data_status is missing entirely.

Swagger UI renders a blank title for this path. Fill it in, e.g. "summary": "List distinct years".

Also, the new user_data_status endpoint is registered in EndpointRegistry and formatted by UserDataStatusFormatter, but has no OpenAPI entry (and no README row or endpoint_params.json entry). If it's meant to be public, add all three; if not, it should be excluded from the registry or marked internal.

[MrIbrahem]

Nit: this Logger::debug(...) line duplicates the Logger::error(...) two lines above it, which already logs the message and the full query unconditionally. This one logs a strict subset (query only) and only in development — it adds nothing, so it can go.