Files
xamxam/REFACTORING_RECOMMENDATIONS.md

616 lines
22 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Posterg: Refactoring Recommendations
Concrete improvements to the separation between templating, routing, and backend logic — staying in PHP, no framework required.
---
## 1. Extract a Micro-Router + Middleware Pipeline
### Problem
Every file repeats the same preamble. The 7 action handlers and 17 page controllers all independently:
```php
require_once __DIR__ . '/../../config/bootstrap.php';
require_once __DIR__ . '/../../src/AdminAuth.php';
AdminAuth::requireLogin();
if (empty($_SESSION['csrf_token'])) {
$_SESSION['csrf_token'] = bin2hex(random_bytes(32));
}
require_once __DIR__ . '/../../src/Database.php';
```
This is ~6-8 identical lines per file × 24 files = ~170 lines of pure duplication. When the CSRF check pattern changes, every action handler must be updated in lockstep.
### Solution
Create `src/App.php` — a thin request dispatcher with middleware hooks:
```php
// src/App.php
class App {
private static bool $booted = false;
/** Boot once per request: load Database, ensure CSRF token exists. */
public static function boot(): Database {
if (!self::$booted) {
require_once APP_ROOT . '/src/Database.php';
self::$booted = true;
}
if (empty($_SESSION['csrf_token'])) {
$_SESSION['csrf_token'] = bin2hex(random_bytes(32));
}
return Database::getInstance();
}
/** Gate for admin pages: auth + CSRF token. */
public static function adminGuard(): Database {
require_once APP_ROOT . '/src/AdminAuth.php';
AdminAuth::requireLogin();
return self::boot();
}
/** Validate CSRF on POST. Call at the top of every action handler. */
public static function verifyCsrf(): void {
if ($_SERVER['REQUEST_METHOD'] !== 'POST'
|| !isset($_POST['csrf_token'], $_SESSION['csrf_token'])
|| !hash_equals($_SESSION['csrf_token'], $_POST['csrf_token'])) {
http_response_code(403);
exit('CSRF token invalide.');
}
}
/** Regenerate CSRF after a successful mutation. */
public static function rotateCsrf(): void {
$_SESSION['csrf_token'] = bin2hex(random_bytes(32));
}
/** Flash a message into the session and redirect. */
public static function redirect(string $url, ?string $success = null, ?string $error = null): never {
if ($success) $_SESSION['success'] = $success;
if ($error) $_SESSION['error'] = $error;
header('Location: ' . $url);
exit;
}
}
```
Every admin page becomes:
```php
<?php
require_once __DIR__ . '/../../config/bootstrap.php';
$db = App::adminGuard();
// ... page-specific logic
```
Every action handler becomes:
```php
<?php
require_once __DIR__ . '/../../../config/bootstrap.php';
$db = App::adminGuard();
App::verifyCsrf();
// ... mutation logic
App::rotateCsrf();
App::redirect('/admin/', success: 'Done.');
```
**Impact**: Eliminates ~170 lines of duplication. Centralises the CSRF lifecycle (generate, verify, rotate) in one place. Any change to the auth or CSRF pattern is a single-file edit.
---
## 2. Separate Controllers from Templates
### Problem
Every page file (e.g. `public/index.php`, `public/search.php`, `public/admin/edit.php`) is a single file that mixes three concerns:
1. **Data fetching** (DB queries, input validation, pagination math)
2. **View variable preparation** (`$pageTitle`, `$ogTags`, `$extraCss`, `$bodyClass`)
3. **HTML rendering** (the entire template, inline)
`system.php` is the extreme case: 400+ lines of PHP logic (systemd checks, curl pings, disk stats, log parsing, nginx syntax highlighting) followed by 200+ lines of inline `<style>`, then 200+ lines of HTML, then 30+ lines of inline `<script>`.
`search.php` is another: the répertoire index view and the search results view are two entirely different pages sharing a single file because they share a URL.
### Solution
Introduce a `controllers/` directory. Each controller is a function that does the data work and returns an associative array. The page file becomes a thin bridge.
**Directory structure:**
```
src/
App.php ← new: middleware/helpers
controllers/
HomeController.php
SearchController.php
TfeController.php
admin/
ThesisListController.php
ThesisEditController.php
ThesisAddController.php
TagController.php
PageController.php
SystemController.php
AccountController.php
...existing files...
templates/
public/
home.php
search-results.php
search-index.php
tfe.php
apropos.php
licence.php
admin/
thesis-list.php
thesis-edit.php
thesis-add.php
tags.php
pages-list.php
pages-edit.php
system.php
account.php
...existing shared templates (head.php, header.php, footer.php)...
```
**Example — `search.php` split:**
```php
// src/controllers/SearchController.php
class SearchController {
public static function index(Database $db): array {
// Collect params, run queries, compute pagination
// ...all current logic from the top of search.php...
return [
'hasSearch' => $hasSearch,
'results' => $results,
'totalItems' => $totalItems,
'totalPages' => $totalPages,
'years' => $years,
'orientations'=> $orientations,
'apPrograms' => $apPrograms,
'keywords' => $keywords,
'students' => $students,
'authorMap' => $authorMap,
'page' => $page,
'error' => $validationError,
// Template config
'pageTitle' => 'Répertoire Posterg',
'bodyClass' => 'search-body',
'extraCss' => ['/assets/css/search.css'],
'currentNav' => 'repertoire',
];
}
}
```
```php
// public/search.php (entire file)
<?php
require_once __DIR__ . '/../config/bootstrap.php';
require_once APP_ROOT . '/src/RateLimit.php';
// Rate limiting (stays here — it's a routing concern)
$rateLimit = new RateLimit(30, 60);
if (!$rateLimit->check()) { /* ...429 response... */ }
$rateLimit->sendHeaders();
$db = App::boot();
require_once APP_ROOT . '/src/controllers/SearchController.php';
$data = SearchController::index($db);
extract($data); // populates $hasSearch, $results, $pageTitle, etc.
include APP_ROOT . '/templates/head.php';
include APP_ROOT . '/templates/header.php';
if ($hasSearch) {
include APP_ROOT . '/templates/public/search-results.php';
} else {
include APP_ROOT . '/templates/public/search-index.php';
}
include APP_ROOT . '/templates/footer.php';
```
```
// templates/public/search-results.php
// Pure HTML + minimal <?= ?> for output. No DB queries. No input processing.
```
**Impact**: Templates become auditable for XSS — they only do output. Controllers are testable — they return arrays, no output buffering needed. The `extract()` bridge keeps the familiar variable-name convention without changing every template.
---
## 3. Introduce a `render()` Helper to Replace the 5-Line Include Chain
### Problem
Every page ends with the same sequence:
```php
include APP_ROOT . '/templates/head.php';
include APP_ROOT . '/templates/header.php';
// ... main content ...
include APP_ROOT . '/templates/footer.php'; // or admin/footer.php
```
The admin variant also requires `$isAdmin = true; $bodyClass = 'admin-body';` to be set before the head include. Forgetting any variable or include breaks the page silently.
### Solution
Add a `render()` function to `App`:
```php
// In src/App.php
public static function render(string $template, array $vars = []): void {
extract($vars);
include APP_ROOT . '/templates/head.php';
include APP_ROOT . '/templates/header.php';
include APP_ROOT . '/templates/' . $template;
// Choose footer based on admin flag
if (!empty($isAdmin)) {
include APP_ROOT . '/templates/admin/footer.php';
} else {
include APP_ROOT . '/templates/footer.php';
}
}
```
Every page becomes:
```php
// public/licence.php
<?php
require_once __DIR__ . '/../config/bootstrap.php';
$db = App::boot();
require_once APP_ROOT . '/src/controllers/LicenceController.php';
App::render('public/licence.php', LicenceController::index($db));
```
Every admin page becomes:
```php
// public/admin/tags.php
<?php
require_once __DIR__ . '/../../config/bootstrap.php';
$db = App::adminGuard();
require_once APP_ROOT . '/src/controllers/admin/TagController.php';
App::render('admin/tags.php', TagController::index($db));
```
**Impact**: No more forgotten includes. The head→header→content→footer pipeline is enforced. Admin footer selection is automatic. Template variables are explicit (passed as array keys, not ambient scope).
---
## 4. Consolidate Action Handlers into Controller Methods
### Problem
The `public/admin/actions/` directory contains 7 POST-only files that each:
1. Require bootstrap + auth
2. Verify CSRF
3. Extract + validate `$_POST` data
4. Call `Database` methods
5. Rotate CSRF
6. Flash a message
7. Redirect
Steps 1-2 and 5-7 are identical in every file. The actual business logic (step 4) is usually 5-15 lines.
`actions/publish.php` (95 lines) does exactly one thing: flip `is_published` on 1-N theses. The other 80 lines are auth, CSRF, validation boilerplate.
### Solution
Merge each action into its controller as a `handlePost()` or action-specific static method:
```php
// src/controllers/admin/ThesisListController.php
class ThesisListController {
public static function index(Database $db): array {
// ... current admin/index.php data logic ...
}
public static function publish(Database $db): never {
App::verifyCsrf();
$action = $_POST['action'] ?? '';
$isBulk = !empty($_POST['bulk']);
// ... 15 lines of actual logic ...
App::rotateCsrf();
App::redirect('/admin/', success: "$count TFE(s) publié(s).");
}
public static function toggleMaintenance(Database $db): never {
App::verifyCsrf();
// ... 6 lines ...
App::rotateCsrf();
App::redirect('/admin/', success: 'Maintenance toggled.');
}
}
```
The page file handles dispatch:
```php
// public/admin/index.php
<?php
require_once __DIR__ . '/../../config/bootstrap.php';
$db = App::adminGuard();
require_once APP_ROOT . '/src/controllers/admin/ThesisListController.php';
if ($_SERVER['REQUEST_METHOD'] === 'POST') {
$action = $_POST['_action'] ?? '';
match($action) {
'publish' => ThesisListController::publish($db),
'maintenance' => ThesisListController::toggleMaintenance($db),
default => App::redirect('/admin/', error: 'Action inconnue.'),
};
}
App::render('admin/thesis-list.php', ThesisListController::index($db));
```
**Or** keep the existing `actions/*.php` files but reduce each to 3 lines:
```php
// public/admin/actions/publish.php (entire file)
<?php
require_once __DIR__ . '/../../../config/bootstrap.php';
$db = App::adminGuard();
App::verifyCsrf();
require_once APP_ROOT . '/src/controllers/admin/ThesisListController.php';
ThesisListController::publish($db);
```
**Impact**: The `actions/` directory goes from 7 files × 50-100 lines each ≈ 500 lines, to 7 files × 5 lines each ≈ 35 lines. The business logic moves into testable controller methods.
---
## 5. Extract Inline CSS and JS from `system.php`
### Problem
`system.php` contains:
- 180 lines of `<style>` embedded in the page
- 40 lines of `<script>` embedded in the page
- 12 PHP helper functions defined inline (`safeExec`, `systemdStatus`, `localHttpCheck`, `humanBytes`, `statusLabel`, `statusClass`, `readLogTail`, `logLineClass`, `nginxLineClass`)
This makes it the largest file in the project (500+ lines) and impossible to cache the CSS/JS independently.
### Solution
1. Move the `<style>` block → `public/assets/css/system.css`, reference it via `$extraCss`
2. Move the `<script>` block → `public/assets/js/system.js`, reference it via `$extraJs`
3. Move the 12 helper functions → `src/controllers/admin/SystemController.php`
4. Move the data-gathering logic (checks array, PHP info, disk stats, log reading) into `SystemController::index()`
The template becomes pure HTML with `<?= ?>` interpolation.
**Impact**: `system.php` goes from ~500 lines to ~5 lines (boot + controller + render). The CSS/JS becomes cacheable by the browser (nginx `expires 30d` rule already exists for `.css`/`.js`). The helpers become unit-testable.
---
## 6. Introduce Template Partials for Repeated UI Patterns
### Problem
Several HTML patterns are copy-pasted across templates:
**Flash messages** — Identical block in `admin/index.php`, `admin/edit.php`, `admin/tags.php`, `admin/pages.php`, `admin/account.php`:
```php
<?php if (isset($_SESSION['error'])): ?>
<div class="admin-alert admin-alert--error">⚠ <?= htmlspecialchars($_SESSION['error']); unset($_SESSION['error']); ?></div>
<?php endif; ?>
<?php if (isset($_SESSION['success'])): ?>
<div class="admin-alert admin-alert--success">✓ <?= htmlspecialchars($_SESSION['success']); unset($_SESSION['success']); ?></div>
<?php endif; ?>
```
**Pagination** — Near-identical block in `index.php` and `search.php` (30 lines each, slightly different URL building).
**Select dropdowns with "selected" logic** — Repeated in `admin/add.php` and `admin/edit.php` for orientation, AP, finality, license, access type.
**Jury fieldset + JS** — Duplicated between `admin/add.php` and `admin/edit.php` (50+ lines of identical HTML + 20 lines of identical JS).
### Solution
Create small partial templates:
```
templates/
partials/
flash-messages.php ← reads $_SESSION['error'] / $_SESSION['success']
pagination.php ← receives $page, $totalPages, $baseUrl
admin/
select-field.php ← receives $name, $label, $options, $selected
checkbox-list.php ← receives $name, $label, $options, $checked
jury-fieldset.php ← receives $jury (array), outputs fieldset + JS
```
**Example — `flash-messages.php`:**
```php
<?php
// templates/partials/flash-messages.php
$_flashError = $_SESSION['error'] ?? $_SESSION['admin_error'] ?? $_SESSION['edit_error'] ?? null;
$_flashSuccess = $_SESSION['success'] ?? $_SESSION['admin_success'] ?? $_SESSION['edit_success'] ?? null;
unset($_SESSION['error'], $_SESSION['success'], $_SESSION['admin_error'],
$_SESSION['admin_success'], $_SESSION['edit_error'], $_SESSION['edit_success']);
?>
<?php if ($_flashError): ?>
<div class="admin-alert admin-alert--error">⚠ <?= htmlspecialchars($_flashError) ?></div>
<?php endif; ?>
<?php if ($_flashSuccess): ?>
<div class="admin-alert admin-alert--success">✓ <?= htmlspecialchars($_flashSuccess) ?></div>
<?php endif; ?>
```
This also fixes a latent bug: the project uses 6 different session keys for flash messages (`error`, `success`, `admin_error`, `admin_success`, `edit_error`, `edit_success`, `form_error`). Centralising flash handling would unify these into two keys.
**Impact**: Eliminates ~200 lines of duplicated HTML. The jury fieldset (duplicated between add and edit) becomes a single 50-line partial. Form field partials make admin pages shorter and more consistent.
---
## 7. Unify Flash Message Keys
### Problem
The project uses 7 different session keys for flash messages across different pages:
| Key | Used by |
|-----|---------|
| `$_SESSION['error']` | `admin/index.php`, `visibility.php`, `account.php` |
| `$_SESSION['success']` | `admin/index.php`, `visibility.php`, `pages.php`, `account.php` |
| `$_SESSION['admin_error']` | `tags.php` |
| `$_SESSION['admin_success']` | `tags.php` |
| `$_SESSION['edit_error']` | `admin/edit.php` |
| `$_SESSION['edit_success']` | `admin/edit.php` |
| `$_SESSION['form_error']` | `admin/add.php` |
| `$_SESSION['form_data']` | `admin/add.php` (re-population) |
This means flash messages can silently persist across pages if a redirect sends the user somewhere that doesn't read the matching key.
### Solution
Standardise on two keys: `$_SESSION['_flash_error']` and `$_SESSION['_flash_success']`. Consume them in the shared `flash-messages.php` partial. Add `App::flash(string $type, string $message)` helper.
---
## 8. Move OG Tag Construction into Controller Logic
### Problem
Every public page constructs `$ogTags` inline before the template, with ~15 lines of boilerplate. `tfe.php` has 25 lines of OG logic including image resolution (banner → cover → none) and description truncation.
### Solution
Move OG tag logic into each controller's return array. Create a helper:
```php
// In src/App.php or a dedicated helpers file
public static function ogTags(array $overrides = []): array {
return array_merge([
'type' => 'website',
'site_name' => 'Posterg ERG',
'title' => 'Posterg',
'description' => '',
'url' => '',
'image' => '',
], $overrides);
}
```
The TFE controller builds the OG image resolution as part of its data preparation. The template never touches it.
---
## Summary: Proposed File Layout
```
src/
App.php ← NEW: boot, adminGuard, verifyCsrf, render, redirect, flash
Database.php ← unchanged
AdminAuth.php ← unchanged
RateLimit.php ← unchanged
Parsedown.php ← unchanged
config.php ← unchanged
controllers/
HomeController.php ← extracted from public/index.php
SearchController.php ← extracted from public/search.php
TfeController.php ← extracted from public/tfe.php
AproposController.php ← extracted from public/apropos.php
LicenceController.php ← extracted from public/licence.php
admin/
ThesisListController.php ← extracted from admin/index.php + actions/publish.php + actions/maintenance.php
ThesisEditController.php ← extracted from admin/edit.php + actions/edit.php
ThesisAddController.php ← extracted from admin/add.php + actions/formulaire.php
TagController.php ← extracted from admin/tags.php + actions/tag.php
PageController.php ← extracted from admin/pages.php + pages-edit.php + actions/page.php
SystemController.php ← extracted from admin/system.php (400 lines of logic)
AccountController.php ← extracted from admin/account.php + actions/account.php
ImportController.php ← extracted from admin/import.php
templates/
head.php ← unchanged
header.php ← unchanged
footer.php ← unchanged
search-bar.php ← unchanged
admin/
footer.php ← unchanged
partials/
flash-messages.php ← NEW
pagination.php ← NEW
admin/
jury-fieldset.php ← NEW (deduplicated from add.php + edit.php)
select-field.php ← NEW
checkbox-list.php ← NEW
public/
home.php ← extracted HTML from index.php
search-results.php ← extracted HTML from search.php (results view)
search-index.php ← extracted HTML from search.php (répertoire view)
tfe.php ← extracted HTML from tfe.php
apropos.php ← extracted HTML from apropos.php
licence.php ← extracted HTML from licence.php
admin/
thesis-list.php ← extracted HTML from admin/index.php
thesis-edit.php ← extracted HTML from admin/edit.php
thesis-add.php ← extracted HTML from admin/add.php
tags.php ← extracted HTML from admin/tags.php
pages-list.php ← extracted HTML from admin/pages.php
pages-edit.php ← extracted HTML from admin/pages-edit.php
system.php ← extracted HTML from admin/system.php
account.php ← extracted HTML from admin/account.php
public/
index.php ← 5-8 lines: boot, controller, render
search.php ← 8-10 lines: boot, rate limit, controller, render
tfe.php ← 5-8 lines
apropos.php ← 5-8 lines
licence.php ← 5-8 lines
media.php ← unchanged (already clean)
maintenance.php ← unchanged
live-reload.php ← unchanged
admin/
index.php ← 8-10 lines: boot, dispatch POST or render
edit.php ← 8-10 lines
add.php ← 8-10 lines
tags.php ← 6-8 lines
pages.php ← 5-8 lines
pages-edit.php ← 5-8 lines
system.php ← 5-8 lines
account.php ← 8-10 lines
import.php ← 5-8 lines
login.php ← unchanged
logout.php ← unchanged
actions/ ← can be REMOVED (merged into controllers)
OR reduced to 3-5 line stubs
```
## Estimated Impact
| Metric | Before | After |
|--------|--------|-------|
| Lines in `public/` page files | ~4,200 | ~200 |
| Lines in `actions/` handlers | ~500 | 0 (merged) |
| Duplicated boilerplate | ~300 lines | ~0 |
| Duplicated HTML (jury, flash, pagination) | ~200 lines | ~0 |
| `system.php` | 530 lines | ~8 lines (page) + ~180 (controller) + ~100 (CSS) + ~30 (JS) |
| Testable controller methods | 0 | ~15 |
| Flash session key variants | 7 | 2 |
Total net reduction: ~8001,000 lines eliminated through deduplication, with cleaner separation making the remaining code auditable and testable.
## Execution Order
This can be done incrementally, one page at a time, with zero disruption:
1. **Create `src/App.php`** with `boot()`, `adminGuard()`, `verifyCsrf()`, `rotateCsrf()`, `redirect()`, `render()`, `flash()`
2. **Create `templates/partials/flash-messages.php`** and adopt it in one admin page
3. **Extract `SystemController`** — the biggest single-file win (500 → 8 lines)
4. **Extract `SearchController`** — the most complex public page
5. **Extract `ThesisEditController`** — merges `edit.php` + `actions/edit.php`, deduplicates jury fieldset
6. **Do remaining controllers one by one**, smallest first
7. **Unify flash keys** project-wide as the last step