chore: generate codebase map in .paul/codebase/
INDEX, STACK, ARCHITECTURE, CONVENTIONS, TESTING, INTEGRATIONS, CONCERNS
This commit is contained in:
@@ -1,264 +1,93 @@
|
||||
# Codebase Concerns
|
||||
# Technical Concerns & Debt
|
||||
|
||||
**Analysis Date:** 2026-03-12
|
||||
## 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 |
|
||||
|
||||
## Tech Debt
|
||||
**Planned fix:** Phase 68 (Code Deduplication Refactor) — deferred, never started.
|
||||
|
||||
## SonarQube Issues (new code since 2026-03-28)
|
||||
|
||||
### [MEDIUM] SonarQube — 327 Open Issues (2026-03-12)
|
||||
**Total: 174 issues** (BLOCKER=1, CRITICAL=47, MAJOR=110, MINOR=16)
|
||||
|
||||
Listed in `DOCS/todo.md`:
|
||||
- 95x `php:S112` — generic `new \Exception` instead of typed exceptions
|
||||
- 57x `php:S1142` — excessive `return` statements per method
|
||||
- 40x `php:S1192` — repeated string literals not extracted to constants
|
||||
- 31x `php:S3776` — high cognitive complexity
|
||||
- 11x `php:S1448` — classes with too many methods (god classes)
|
||||
- 4x `php:S138` — methods over the allowed line limit
|
||||
| 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">` |
|
||||
|
||||
The most critical from a maintainability standpoint are the complexity and god-class violations.
|
||||
**Note:** SonarQube scan not run for phases 105–107 — baseline may be stale.
|
||||
|
||||
---
|
||||
## Known Bugs & Issues
|
||||
|
||||
### [LOW] Duplicate Migration Number `000014`
|
||||
| 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 |
|
||||
|
||||
- Issue: Two migration files share sequence number `000014`:
|
||||
- `20260227_000014_create_product_integration_translations.sql`
|
||||
- `20260301_000014_add_products_sku_format_setting.sql`
|
||||
- File: `database/migrations/`
|
||||
- Impact: Depending on how the migrator detects "already run" (by filename vs. sequence number), one of these may silently never execute or may fail.
|
||||
- Fix approach: Rename one to `000014b` or renumber all subsequent ones. Verify migrator uses full filename as key.
|
||||
## Deferred Indexes (Phase 106)
|
||||
|
||||
---
|
||||
After Phase 106 (customer return alert), two indexes should be added before dataset exceeds ~50k orders:
|
||||
|
||||
## Security Concerns
|
||||
```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);
|
||||
```
|
||||
|
||||
### [HIGH] No SSL Verification on cURL Calls
|
||||
These support the correlated subquery in `OrdersRepository` used for return-risk detection.
|
||||
|
||||
- Issue: None of the `AllegroApiClient`, `ShopproApiClient`, `ApaczkaApiClient`, or `AllegroOAuthClient` set `CURLOPT_SSL_VERIFYPEER` or `CURLOPT_SSL_VERIFYHOST`. PHP's default is to verify SSL, but explicitly not setting it means behavior depends on the server's `php.ini` `curl.cainfo` configuration.
|
||||
- Files:
|
||||
- `src/Modules/Settings/AllegroApiClient.php`
|
||||
- `src/Modules/Settings/AllegroOAuthClient.php`
|
||||
- `src/Modules/Settings/ShopproApiClient.php`
|
||||
- `src/Modules/Settings/ApaczkaApiClient.php`
|
||||
- Impact: On environments without a properly configured CA bundle (common on shared hosting / Windows XAMPP), all API calls may proceed without SSL verification, enabling MITM attacks on OAuth tokens and API keys.
|
||||
- Fix approach: Explicitly set `CURLOPT_SSL_VERIFYPEER => true` and configure `CURLOPT_CAINFO` pointing to a known CA bundle. Document this in `.env.example`.
|
||||
## 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 |
|
||||
|
||||
### [MEDIUM] CSRF Token Is Never Rotated After Login
|
||||
## Architecture Concerns
|
||||
|
||||
- Issue: `Csrf::token()` in `src/Core/Security/Csrf.php` generates the token once per session and never regenerates it. The token is never rotated after a successful login, and it is never invalidated after a POST form is submitted.
|
||||
- File: `src/Core/Security/Csrf.php`
|
||||
- Impact: If a CSRF token leaks (e.g., via Referer header, browser history), it remains valid for the entire session lifetime.
|
||||
- Fix approach: Rotate the CSRF token on login. Optionally regenerate after each use (per-form tokens). At minimum, invalidate the session token on `AuthService::logout()`.
|
||||
| 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)
|
||||
|
||||
### [MEDIUM] Label File Served with `file_get_contents($fullPath)` — Partial Path Traversal Risk
|
||||
- `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
|
||||
|
||||
- Issue: In `ShipmentController::label()`, the file path is built from `$package['label_path']` retrieved from the database. While `basename()` is used for the filename in the response header, `$fullPath` itself is derived by concatenating `$storagePath . '/labels/' . $filename` from the shipment service. There is no `realpath()` check confirming the file is actually within the storage directory before serving it.
|
||||
- File: `src/Modules/Shipments/ShipmentController.php` (lines 292–316)
|
||||
- Impact: Low risk currently (path comes from DB, not user input directly), but if `label_path` column is ever injectable or incorrectly set, arbitrary file reads become possible.
|
||||
- Fix approach: Validate `realpath($fullPath)` starts with `realpath($storagePath)` before calling `file_get_contents()`.
|
||||
## 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 |
|
||||
|
||||
### [LOW] Allegro OAuth Callback Endpoint Has No Auth Middleware
|
||||
## Performance Risks
|
||||
|
||||
- Issue: `GET /settings/integrations/allegro/oauth/callback` is registered without `$authMiddleware`. The OAuth state parameter is validated via session, which does provide CSRF protection for the OAuth flow, but any authenticated check is absent.
|
||||
- File: `routes/web.php` (line 239)
|
||||
- Impact: An unauthenticated actor with knowledge of the state parameter could complete an OAuth token exchange. In practice, the state must match the session, limiting exploitability to session-fixation scenarios.
|
||||
- Fix approach: Add `$authMiddleware` to the callback route. Handle the "not authenticated yet" edge case by storing the session state before redirect.
|
||||
|
||||
---
|
||||
|
||||
## Performance Concerns
|
||||
|
||||
### [HIGH] N+1 Correlated Subqueries on Every Order List Row
|
||||
|
||||
- Issue: The `OrdersRepository::paginate()` list SQL includes three correlated subqueries per row: `items_count`, `items_qty`, `shipments_count`, and `documents_count`. These fire once per order in the result set.
|
||||
- File: `src/Modules/Orders/OrdersRepository.php` (lines 124–127)
|
||||
- Impact: For a page of 50 orders: 4 × 50 = 200 additional subquery executions per page load. Degrades significantly as the `orders` table grows.
|
||||
- Fix approach: Replace correlated subqueries with LEFT JOIN + GROUP BY aggregates, or preload counts in a single separate query (`SELECT order_id, COUNT(*) ... GROUP BY order_id WHERE order_id IN (...)`).
|
||||
|
||||
---
|
||||
|
||||
### [HIGH] `canResolveMappedMedia()` Queries `information_schema.COLUMNS` on Every Request
|
||||
|
||||
- Issue: `OrdersRepository::canResolveMappedMedia()` runs a query against `information_schema.COLUMNS` to check whether optional product mapping tables exist. It caches the result in a private property, but this property is instance-scoped — the check fires once per HTTP request that instantiates `OrdersRepository`.
|
||||
- File: `src/Modules/Orders/OrdersRepository.php` (lines 604–648)
|
||||
- Impact: `information_schema` queries are notoriously slow on MySQL. This fires on every order-related page load (list, detail, shipment).
|
||||
- Fix approach: Cache the result in a static class property or in the app-level cache/session so it survives across requests. Alternatively, remove the check entirely — if the schema is managed by migrations, the tables either exist or don't.
|
||||
|
||||
---
|
||||
|
||||
### [MEDIUM] AllegroStatusSyncService Fetches Up to 50 Orders Then Re-Imports Each One Fully
|
||||
|
||||
- Issue: `AllegroStatusSyncService::findOrdersNeedingStatusSync()` returns up to 50 Allegro orders. For each, `importSingleOrder()` is called, which makes a full Allegro API call per order (checkout-form fetch + optional shipments fetch + optional offer image fetch per line item). This is 50+ API calls per cron run.
|
||||
- File: `src/Modules/Settings/AllegroStatusSyncService.php`
|
||||
- Impact: Rate-limit exhaustion on Allegro API. Slow cron runs.
|
||||
- Fix approach: Use a lighter-weight API call to fetch only status fields for multiple orders (e.g., `listCheckoutForms` with a filter) rather than a full re-import per order.
|
||||
- Note (2026-03-13): Plan 02-02 dodał kursor `last_status_checked_at` — eliminuje re-import zamówień bez zmiany statusu. Full API call per order pozostaje osobnym concernem.
|
||||
|
||||
---
|
||||
|
||||
### [MEDIUM] No Database Indexes Verified for Key Queries
|
||||
|
||||
- Issue: Queries frequently filter and sort on `orders.source`, `orders.external_status_id`, `orders.ordered_at`, `order_items.order_id`, and `allegro_order_status_mappings.allegro_status_code`. Whether indexes exist for these columns is not documented.
|
||||
- Files: `src/Modules/Orders/OrdersRepository.php`, `database/migrations/20260302_000018_create_orders_tables_and_schedule.sql`
|
||||
- Impact: Full table scans as data grows.
|
||||
- Fix approach: Audit the migration that creates `orders` and related tables for index definitions. Add missing indexes for `(source, external_status_id)`, `(ordered_at)`, and `allegro_status_code`.
|
||||
|
||||
---
|
||||
|
||||
## Architectural Concerns
|
||||
|
||||
### [HIGH] `Settings` Module Is a God Module — Contains Unrelated Concerns
|
||||
|
||||
- Issue: The `src/Modules/Settings/` directory contains 30+ classes covering: Allegro integration, Apaczka integration, InPost integration, shopPRO integration, company settings, cron settings, order statuses, carrier mappings, delivery method mappings, and OAuth. These are logically distinct domains crammed into a single module namespace.
|
||||
- Files: All files in `src/Modules/Settings/`
|
||||
- Impact: The module boundary is meaningless. New integrations will continue to pile into Settings. `AllegroIntegrationController` (923 lines) and `ShopproIntegrationsController` (901 lines) are god classes by SonarQube rules (S1448).
|
||||
- Fix approach: Gradually decompose into purpose-built modules: `src/Modules/Integrations/Allegro/`, `src/Modules/Integrations/Shoppro/`, `src/Modules/Integrations/Apaczka/`, `src/Modules/Integrations/Inpost/`.
|
||||
|
||||
---
|
||||
|
||||
### [HIGH] `AllegroIntegrationController` and `ShopproIntegrationsController` Are God Classes (900+ lines)
|
||||
|
||||
- Issue: Each controller handles: displaying settings, saving credentials, OAuth flow, status mapping CRUD, delivery method mapping CRUD, and import triggering. Each is 900+ lines.
|
||||
- Files:
|
||||
- `src/Modules/Settings/AllegroIntegrationController.php` (923 lines)
|
||||
- `src/Modules/Settings/ShopproIntegrationsController.php` (901 lines)
|
||||
- Impact: Violates SRP. Methods average far more than 50 lines. Cognitive complexity is high (SonarQube S3776, 31 violations).
|
||||
- Fix approach: Split each into focused controllers per tab/concern: e.g., `AllegroCredentialsController`, `AllegroStatusMappingController`, `AllegroDeliveryMappingController`.
|
||||
|
||||
---
|
||||
|
||||
### [HIGH] `ShopproOrdersSyncService` Is 1192 Lines — Largest Single File
|
||||
|
||||
- Issue: `ShopproOrdersSyncService` handles the full import lifecycle for shopPRO orders including: pagination, cursor management, order mapping, address building, item building, image resolution, payment mapping, shipment mapping, status history, and activity logging — all in one class.
|
||||
- File: `src/Modules/Settings/ShopproOrdersSyncService.php`
|
||||
- Impact: Highest complexity class in the codebase. Hard to test, hard to modify, prone to regression.
|
||||
- Fix approach: Extract a `ShopproOrderMapper` (mapping logic), a `ShopproImageResolver` (image fetch), and keep `ShopproOrdersSyncService` as the orchestrator. Mirror the pattern of `AllegroOrderImportService` which is better separated.
|
||||
|
||||
---
|
||||
|
||||
### [MEDIUM] No Dependency Injection Container — Full Object Graph in `routes/web.php`
|
||||
|
||||
- Issue: `routes/web.php` manually constructs all services and controllers, including multiple separate instantiations of identical objects (e.g., `new AllegroApiClient()` appears 3 times, `new OrdersRepository($app->db())` appears 4 times).
|
||||
- File: `routes/web.php`
|
||||
- Impact: Multiple object instances where singletons are implied. Adding new dependencies requires editing this file, which is already 257 lines. Risk of silently passing the wrong instance.
|
||||
- Fix approach: Introduce a lightweight DI container or service locator. At minimum, hoist shared instances (`$allegroApiClient`, `$ordersRepository`) to named variables and reuse them.
|
||||
|
||||
---
|
||||
|
||||
|
||||
## Incomplete Features
|
||||
|
||||
### [HIGH] 5 Stub Buttons with No Actions in Order Detail View
|
||||
|
||||
- Issue: The order detail page (`resources/views/orders/show.php`, lines 48–53) contains 5 buttons that render in the UI but have no `href`, no form action, and no JavaScript handler: "Strefa klienta", "Platnosc", "Drukuj", "Pakuj", "Edytuj".
|
||||
- File: `resources/views/orders/show.php`
|
||||
- Impact: Users see clickable but non-functional UI elements. The "Pakuj" (pack) button is styled as the primary CTA (`btn--primary`).
|
||||
- Fix approach: Either implement the features or hide the buttons until they are ready.
|
||||
|
||||
---
|
||||
|
||||
### [HIGH] `orderpro_to_allegro` Status Sync Direction Not Implemented
|
||||
|
||||
- Issue: The setting UI offers two sync directions: Allegro → orderPRO and orderPRO → Allegro. The second direction explicitly returns early with "Kierunek orderPRO -> Allegro nie jest jeszcze wdrozony" ("not yet implemented").
|
||||
- File: `src/Modules/Settings/AllegroStatusSyncService.php` (lines 38–45)
|
||||
- Impact: Users who configure the "push status to Allegro" direction get silent no-op behavior with no clear user-facing indication in the cron log.
|
||||
- Fix approach: Either implement the feature or disable the `orderpro_to_allegro` direction option in the UI until it is ready.
|
||||
|
||||
---
|
||||
|
||||
### [HIGH] TODO in `DOCS/todo.md` — UI Issues Open
|
||||
|
||||
The following items from `DOCS/todo.md` are marked incomplete:
|
||||
- Item 12: Status change sync with Allegro after manual user change
|
||||
- Item 14: Input/select/textarea border color needs to be darker
|
||||
- Item 15: Source and ID display order should be reversed (source first, then ID)
|
||||
- Item 16: Order list statuses should be colored according to settings
|
||||
- Item 17: Source display should show specific integration name instead of "shopPRO"; ID label missing
|
||||
- Files: `resources/views/orders/list.php`, `resources/views/orders/show.php`
|
||||
|
||||
---
|
||||
|
||||
### [MEDIUM] InPost Integration — Settings Page Exists But No Shipment Provider
|
||||
|
||||
- Issue: `InpostIntegrationController` and `InpostIntegrationRepository` exist and the settings page at `/settings/integrations/inpost` is functional. However, there is no `InpostShipmentService` implementing `ShipmentProviderInterface`. InPost labels are currently routed through `allegro_wza` as a workaround (`provider_code === 'inpost'` is remapped to `'allegro_wza'` in `ShipmentController::create()`).
|
||||
- Files:
|
||||
- `src/Modules/Settings/InpostIntegrationController.php`
|
||||
- `src/Modules/Settings/InpostIntegrationRepository.php`
|
||||
- `src/Modules/Shipments/ShipmentController.php` (line 165–166)
|
||||
- Impact: InPost credentials are stored but not used by their own provider. The remap to `allegro_wza` means InPost shipments are actually created via Allegro WZA. This breaks if the user has only InPost credentials and no Allegro integration.
|
||||
- Fix approach: Implement `InpostShipmentService implements ShipmentProviderInterface` using the stored InPost API token.
|
||||
|
||||
---
|
||||
|
||||
### [MEDIUM] `database/drafts/` Contains Uncommitted Schema SQL
|
||||
|
||||
- Issue: `database/drafts/20260302_orders_schema_v1.sql` exists in the `drafts` subdirectory. This is not a migration — it will not be executed by the migrator.
|
||||
- File: `database/drafts/20260302_orders_schema_v1.sql`
|
||||
- Impact: Unclear whether this represents schema that has not yet been migrated or schema that was migrated by other means. Creates confusion about source of truth.
|
||||
- Fix approach: Either move to `database/migrations/` with a proper sequence number, or delete it if already superseded.
|
||||
|
||||
---
|
||||
|
||||
## Known Bugs
|
||||
|
||||
|
||||
## Test Coverage Gaps
|
||||
|
||||
### [HIGH] No Integration or Functional Tests Exist
|
||||
|
||||
- Issue: The `tests/` directory contains only `bootstrap.php`. No test files exist. The `archive/` directory contains unit tests from a previous reset, but they are not in active use.
|
||||
- Files: `tests/bootstrap.php`, `archive/2026-03-02_users-only-reset/tests/`
|
||||
- Risk: Zero automated coverage for any critical path: order import, shipment creation, OAuth flow, cron sync, status updates.
|
||||
- Priority: High
|
||||
|
||||
---
|
||||
|
||||
### [HIGH] Allegro OAuth Token Refresh Logic Has No Tests
|
||||
|
||||
- Issue: The token refresh logic is centralized in `AllegroTokenManager` but untested. It includes complex edge cases: token expiry within 5-minute window, empty refresh token fallback, write-then-re-read pattern.
|
||||
- Files: `src/Modules/Settings/AllegroTokenManager.php`
|
||||
- Risk: Token refresh failures cause complete import failure. Silent breakage possible.
|
||||
- Priority: High
|
||||
|
||||
---
|
||||
|
||||
### [MEDIUM] No Error Path Tests for Order Import
|
||||
|
||||
- Issue: `AllegroOrderImportService::importSingleOrder()` and `ShopproOrdersSyncService::sync()` have complex error handling (401 retry, silent Throwable catch blocks). None of this is tested.
|
||||
- Files:
|
||||
- `src/Modules/Settings/AllegroOrderImportService.php`
|
||||
- `src/Modules/Settings/ShopproOrdersSyncService.php`
|
||||
- Risk: Error paths that swallow exceptions silently (multiple `catch (Throwable) {}` blocks in `OrdersRepository`) may hide data integrity issues.
|
||||
- Priority: Medium
|
||||
|
||||
---
|
||||
|
||||
## Fragile Areas
|
||||
|
||||
### [HIGH] `AllegroStatusSyncService` — `orderpro_to_allegro` Silent No-Op
|
||||
|
||||
- Files: `src/Modules/Settings/AllegroStatusSyncService.php`
|
||||
- Why fragile: Returns `ok: true` when direction is `orderpro_to_allegro` with zero processing, no log, no user feedback. Cron logs will show "success" even though nothing happened.
|
||||
- Safe modification: Do not add code after the early return without understanding why it was left as a no-op. Consider returning `ok: false` or a warning until the feature is implemented.
|
||||
- Test coverage: None.
|
||||
|
||||
---
|
||||
|
||||
### [MEDIUM] Web Cron Throttling Uses Session Timestamp (`$_SESSION['cron_web_last_run_at']`)
|
||||
|
||||
- Issue: `Application::isWebCronThrottled()` reads and writes a cron timestamp from `$_SESSION`. In stateless or load-balanced deployments, different users hit different sessions, meaning the throttle is per-session (per-user), not global.
|
||||
- File: `src/Core/Application.php` (lines 363–375)
|
||||
- Why fragile: With two active sessions, the cron may run twice within the throttle window. The DB-level `GET_LOCK` is the real protection, but this creates confusing interactions.
|
||||
- Fix approach: Move last-run timestamp to the database (e.g., an `app_settings` key) instead of the session.
|
||||
|
||||
---
|
||||
|
||||
*Concerns audit: 2026-03-12*
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user