94 lines
5.4 KiB
Markdown
94 lines
5.4 KiB
Markdown
# Technical Concerns & Debt
|
||
|
||
## God Classes (Priority Refactor Targets)
|
||
|
||
| Class | LOC | Methods | Issue |
|
||
|-------|-----|---------|-------|
|
||
| `src/Modules/Orders/OrdersRepository.php` | 1,221 | 29 | Query building spread across 29 methods; SonarQube S1448 |
|
||
| `src/Modules/Orders/OrdersController.php` | 1,187 | 22 | UI + AJAX + list + detail + search combined; S1448 |
|
||
| `src/Modules/Automation/AutomationService.php` | 834 | 24 | All action handlers in one class; S1448 |
|
||
| `src/Modules/Settings/ShopproOrderMapper.php` | 867 | 25 | 25+ transformation methods; S1448 |
|
||
| `src/Modules/Settings/ApaczkaShipmentService.php` | 1,044 | — | API payload deeply nested |
|
||
| `src/Modules/Settings/ShopproIntegrationsController.php` | 1,044 | — | OAuth + mapping + sync combined |
|
||
|
||
**Planned fix:** Phase 68 (Code Deduplication Refactor) — deferred, never started.
|
||
|
||
## SonarQube Issues (new code since 2026-03-28)
|
||
|
||
**Total: 174 issues** (BLOCKER=1, CRITICAL=47, MAJOR=110, MINOR=16)
|
||
|
||
| Rule | Count | Severity | Examples |
|
||
|------|-------|----------|---------|
|
||
| `php:S112` — Generic exceptions | 95+ | MAJOR | `throw new \Exception(...)` everywhere; use domain exceptions |
|
||
| `php:S1142` — Excess return statements | 57+ | MAJOR | Many service methods have 4-10 returns |
|
||
| `php:S1192` — Duplicated string literals | 39 | CRITICAL | Route paths, status strings, HTTP headers |
|
||
| `php:S3776` — Cognitive complexity > 15 | 9+ | CRITICAL | `ShopproOrderMapper::initOrderFromArray()` (28), `ShipmentTrackingHandler` (27) |
|
||
| `php:S1448` — Class too large | 6+ | MAJOR | See god classes above |
|
||
| `php:S1172` — Unused parameters | 11+ | MAJOR | `$request` params in handlers, unused payload params |
|
||
| `php:S4423` — Weak TLS protocol | 1 | **CRITICAL** | `EmailMailboxController::testConnection()` line ~223: `fsockopen('ssl://...')` — deprecated |
|
||
| `php:S5911` — Missing import | 1 | **BLOCKER** | `AllegroOrderImportService` — `RuntimeException` not imported |
|
||
| `php:S4833` — Use namespace import | 2 | MAJOR | `resources/views/accounting/index.php:31`, `orders/show.php:780` |
|
||
| `Web:S6827` — Anchors without accessible text | 9+ | MINOR | Icon-only buttons need `aria-label` |
|
||
| `Web:S6819` — Accessibility | 7+ | MAJOR | Use `<output>` instead of `<span role="status">` |
|
||
|
||
**Note:** SonarQube scan not run for phases 105–107 — baseline may be stale.
|
||
|
||
## Known Bugs & Issues
|
||
|
||
| Issue | Location | Status |
|
||
|-------|----------|--------|
|
||
| `STAT-NET`: hardcoded 23% VAT fallback for net calculations | `src/Modules/Statistics/OrdersStatisticsRepository.php:471` | Deferred (`.paul/TODO.md`) |
|
||
| Missing net amounts for shopPRO orders | `.paul/TODO.md` (STAT-NET) | Deferred |
|
||
| `order.status_aged` condition fallback | `AutomationService` | Fixed 2026-04-25 |
|
||
|
||
## Deferred Indexes (Phase 106)
|
||
|
||
After Phase 106 (customer return alert), two indexes should be added before dataset exceeds ~50k orders:
|
||
|
||
```sql
|
||
-- INDEX-106-01 (deferred in SUMMARY.md)
|
||
CREATE INDEX idx_order_addresses_order_type ON order_addresses(order_id, address_type);
|
||
CREATE INDEX idx_shipment_packages_order_delivery ON shipment_packages(order_id, delivery_status);
|
||
```
|
||
|
||
These support the correlated subquery in `OrdersRepository` used for return-risk detection.
|
||
|
||
## Security Items to Verify
|
||
|
||
| Item | Risk | Action |
|
||
|------|------|--------|
|
||
| `print_api_keys.api_key` encryption | MEDIUM | Verify column is encrypted (same as `integrations.api_key_encrypted`) |
|
||
| `fsockopen('ssl://...')` in `EmailMailboxController::testConnection()` | MEDIUM | Replace with `stream_socket_client()` + `stream_context_create(['ssl' => ['verify_peer' => true]])` |
|
||
| Email variable injection via `{{var}}` templates | LOW | Only predefined variables allowed — verify automation rule creation doesn't accept arbitrary variable names |
|
||
|
||
## Architecture Concerns
|
||
|
||
| Concern | Impact | Notes |
|
||
|---------|--------|-------|
|
||
| No repository interfaces | LOW | Cannot mock repositories cleanly in tests without `bypass-finals` workaround |
|
||
| String-typed event/action names | LOW | `event_type = 'order.status_changed'` — typos not caught at compile time |
|
||
| No validation layer | LOW | Validation scattered across controllers and repositories |
|
||
| No HTTP caching headers | LOW | Responses don't set ETag, Cache-Control; acceptable for low-concurrency use |
|
||
| No query caching | LOW | Every request re-queries; no Redis/Memcached layer |
|
||
|
||
## Duplication Areas (Phase 68 scope)
|
||
|
||
- `SslCertificateResolver` — pattern duplicated across multiple API client files
|
||
- `ToggleableRepositoryTrait` — mixes query building + toggling
|
||
- `RedirectPathResolver` — similar redirect logic in 4+ controllers
|
||
- `ReceiptService` vs accounting logic — overlapping responsibilities
|
||
|
||
## Legacy / Deprecated Patterns
|
||
|
||
| Pattern | Location | Status |
|
||
|---------|----------|--------|
|
||
| `fsockopen('ssl://')` | `EmailMailboxController::testConnection()` | Deprecated PHP TLS approach — fix when touching that method |
|
||
| `require` in views | `resources/views/accounting/index.php:31` | Should use namespace `use` — minor |
|
||
| Raw `$_SESSION` access | Some older controllers | Should use `Session::get()` / `set()` helpers |
|
||
|
||
## Performance Risks
|
||
|
||
1. **Correlated subquery for return-risk** (per-row COUNT) — slow at >50k orders without `INDEX-106-01`
|
||
2. **N+1 potential in order details** — `findDetails()` queries orders + addresses + items separately (not verified as actual problem)
|
||
3. **Cron queue growth** — no exponential backoff if queue grows; may pile up on slow syncs
|