Skip to content

refactor: split eight PHP god-objects (>1000 LOC each) into focused responsibilities (#141) - #154

Merged
rpgmem merged 10 commits into
mainfrom
claude/refactor-split-god-objects-php
May 10, 2026
Merged

refactor: split eight PHP god-objects (>1000 LOC each) into focused responsibilities (#141)#154
rpgmem merged 10 commits into
mainfrom
claude/refactor-split-god-objects-php

Conversation

@rpgmem

@rpgmem rpgmem commented May 10, 2026

Copy link
Copy Markdown
Owner

Closes #141.

Summary

Eight commits — one per sprint — splitting eight PHP files (>1000 LOC each) into focused responsibilities. Pure code movement; no behavior changes at any commit. Each commit keeps tests + PHPStan green.

LOC overview (before → after, primary file)

Sprint File Before After
S1 recruitment/class-ffc-recruitment-notice-edit-page.php 1727 291
S2 recruitment/class-ffc-recruitment-rest-controller.php (deleted) 1456 n/a (split into 4)
S3 admin/class-ffc-form-editor-metabox-renderer.php (facade) 1245 102
S4 security/class-ffc-rate-limiter.php (facade) 1178 213
S5 frontend/class-ffc-public-csv-download.php (facade) 1144 717
S6 reregistration/class-ffc-reregistration-admin.php (facade) 1132 1002 ⚠️
S7 recruitment/class-ffc-recruitment-public-shortcode.php (facade) 1136 476
S8 settings/views/ffc-tab-documentation.php (shell) 1474 83

Per-sprint changes

  • S1 — Extracted RecruitmentNoticeEditPageRenderer (1330 LOC) and RecruitmentClassificationFilterManager (169 LOC). Orchestrator keeps register / handle_save / handle_transition / etc.
  • S2 — Deleted the monolithic RecruitmentRestController. Replaced by 4 domain controllers (Notices 179, Classifications 509, Adjutancies 420 [incl. Reasons], Candidates 344) + shared RecruitmentRestSupport trait (164 LOC) for cap-checks and wp_error_* helpers. Loader registers each.
  • S3 — Facade keeps 10 public forwarders; 9 new per-metabox classes (FormEditor*Metabox) each with a single render(WP_Post $post). The builder class also exposes render_field_row for the facade's helper forwarder.
  • S4 — Facade keeps the entire public surface (13 external callers). Internals split into RateLimitChecker (1073, all check_* per spec), RateLimitLogger (99), RateLimitStats (44). 9 self::log_attempt → RateLimitLogger::log_attempt. get_user_ip promoted to public so the logger can use it. CACHE_GROUP stays on the facade (used externally).
  • S5 — CsvDownloadValidator (291) + CsvDownloadFormInfoBuilder (277). Facade constructor wires them as private properties; validation methods become one-line forwarders; ajax_info() calls the builder inline. Two reflection-based tests repointed to the new home.
  • S6 — ReregistrationAjaxHandler (142) + ReregistrationSubmissionDetailsRenderer (123). init() registers the 3 wp_ajax callbacks against the handler instance; three thin public delegators on the facade preserve the test surface that calls AJAX methods directly on ReregistrationAdmin.
  • S7 — RecruitmentPublicShortcodeRenderer (692) owns all badge / label / format / section / row / pagination helpers. Facade keeps register / render / render_uncached / cache + rate-limit infra.
  • S8 — ffc-tab-documentation.php becomes an 83-LOC shell that emits the wrap div + TOC card inline, then requires 19 numbered partials from includes/settings/views/documentation/. md5 of the rendered output is byte-identical before/after.

Acceptance criteria

  • PHPStan + PHPUnit green at every commit (verified per-sprint).
  • No behavior changes — public APIs preserved; tests adjusted only where private-method reflection or class-name imports referenced the old home.
  • No new files outside the existing includes/<feature>/ structure.
  • [~] Every touched file <800 LOC after split — 6 of 8 sprints met this; 2 deviations documented in their commit messages:
    • S4 RateLimitChecker = 1073 LOC. The issue's explicit 3-class spec (Checker / Logger / Stats) puts every check_* method in Checker by design. Hitting <800 would require sub-splitting (Device-fingerprint, Blocklist) the spec did not authorize.
    • S6 ReregistrationAdmin = 1002 LOC. The issue's spec listed render_list, render_form, render_submissions, render_submission_row, render_audience_*, handle_save, handle_delete to keep on the facade. Hitting <800 would require extracting render helpers the spec did not authorize.

If those LOC bars are binding, the path forward is named in each deviation note.

Test plan

  • vendor/bin/phpunit — 3870 tests / 9810 assertions / 0 failures (verified after every commit).
  • vendor/bin/phpstan analyse <touched dir> — no errors (verified per-sprint).
  • S8 byte-stream parity — md5(17ec376b4ccef6aeba4b9fe94c977454) unchanged.

https://claude.ai/code/session_01HiExaniSqvNBLpVTszCLNx

Generated by Claude Code

claude added 10 commits May 10, 2026 20:38
@claude
refactor(recruitment): split notice-edit-page god-object (S1 of #141)
4f9804a 
Pure code movement — no behavior changes.

class-ffc-recruitment-notice-edit-page.php drops from 1727 to 291 LOC.
The orchestrator class keeps register(), handle_download_csv_example(),
render(), handle_save(), handle_transition(), back_url(), and
redirect_with_notice(). The 5 section-render calls in render() and the
columns_label_map() call in handle_save() now delegate to
RecruitmentNoticeEditPageRenderer.

Two new files:

  - class-ffc-recruitment-notice-edit-page-renderer.php (1330 LOC)
    All render_*, columns_label_map, preview_status_label_map, cls_button,
    lookup_map, transitions_from, and the two inline-script emitters.
    The 6 methods called from the EditPage facade are public; the rest
    stay private (only sibling-renderer callers).

  - class-ffc-recruitment-classification-filter-manager.php (169 LOC)
    read_classification_filters() → read_filters() and
    apply_classification_filters() → apply_filters(). Bodies byte-
    identical; only the names changed per the issue spec.

PSR-4 autoloader resolves both new class names from kebab-case
filenames automatically — no loader/composer changes needed.

Verification:
  - php -l: clean on all 3 files.
  - vendor/bin/phpunit --filter Recruitment: 227/227 OK.
  - vendor/bin/phpstan analyse <3 files> --level=8: no errors.
  - Re-ran phpstan over includes/recruitment/ (34 files): no errors.

https://claude.ai/code/session_01HiExaniSqvNBLpVTszCLNx
@claude
refactor(recruitment): split rest-controller god-object into 4 domain…
075a329 
… controllers (S2 of #141)

Pure code movement — no runtime behavior changes. The single
RecruitmentRestController (1456 LOC) is replaced by four focused
domain controllers + a shared support trait.

New controllers (each with its own register_routes()):

  - RecruitmentNoticesRestController        — 179 LOC,  3 endpoints
    (notices CRUD: list, create, update)
  - RecruitmentClassificationsRestController — 509 LOC, 9 endpoints
    (list, import, promote-preview, call, bulk-call, change status,
     change preview status, delete, cancel-call)
  - RecruitmentAdjutanciesRestController     — 420 LOC, 10 endpoints
    (adjutancy CRUD, notice attach/detach, reasons CRUD — Reasons
     folded here since both are catalog domains and the issue spec
     names only 4 controllers)
  - RecruitmentCandidatesRestController      — 344 LOC, 5 endpoints
    (candidates CRUD + me/recruitment)

Shared trait `RecruitmentRestSupport` (164 LOC) holds the 6 cap-check
methods and the 2 wp_error envelope helpers used by all controllers.

The loader (`RecruitmentLoader`) now instantiates and registers all
four controllers on `rest_api_init`. The original
`class-ffc-recruitment-rest-controller.php` is deleted (no
class_alias — user explicitly approved no aliases for this refactor).

Tests: `tests/Unit/RecruitmentRestControllerTest.php` updated to
instantiate the new controllers — cap-check tests use
RecruitmentNoticesRestController (any controller works since they
all `use` the trait); the route-registration tests aggregate
register_routes() from all four into a single accumulator and run
the original spot-checks unchanged (the union of registered routes
is identical to before).

Verification:
  - php -l: clean on all 5 new files + loader + test.
  - vendor/bin/phpunit --filter Recruitment: 227/227 OK.
  - vendor/bin/phpstan analyse includes/recruitment/ --level=8: no errors.

https://claude.ai/code/session_01HiExaniSqvNBLpVTszCLNx
@claude
refactor(admin): split form-editor metabox renderer into per-metabox …
dccb4df 
…classes (S3 of #141)

Pure code movement — no behavior changes. The single
FormEditorMetaboxRenderer (1245 LOC) becomes a thin facade (99 LOC)
that delegates each metabox to its own class.

Nine new per-metabox classes (one render(WP_Post $post) method each):

  - FormEditorShortcodeMetabox             52 LOC
  - FormEditorLayoutMetabox                98 LOC
  - FormEditorBuilderMetabox              201 LOC (incl. render_field_row helper)
  - FormEditorRestrictionMetabox          152 LOC
  - FormEditorEmailMetabox                 89 LOC
  - FormEditorGeofenceMetabox             329 LOC (incl. inline toggleGeoSource JS)
  - FormEditorQuizMetabox                  87 LOC
  - FormEditorPublicCsvDownloadMetabox    300 LOC
  - FormEditorDeviceLimitMetabox          151 LOC

The facade `FormEditorMetaboxRenderer` keeps its public API intact
(render_box_*, render_shortcode_metabox, render_field_row) so existing
call sites in `FormEditor::add_custom_metaboxes()` and the unit test
work unchanged. Each public method is now a one-line forwarder.

PSR-4 autoloader resolves all 9 new class names from kebab-case
filenames automatically — no loader/composer changes needed.

Verification:
  - php -l: clean on all 10 files.
  - vendor/bin/phpunit --filter FormEditorMetaboxRenderer|FormEditorSaveHandler: 29/29 OK.
  - vendor/bin/phpstan analyse includes/admin/ --level=8: no errors.

https://claude.ai/code/session_01HiExaniSqvNBLpVTszCLNx
@claude
refactor(security): split rate-limiter god-object behind facade (S4 of …
851c45a 
…#141)

Pure code movement — no behavior changes. The single `RateLimiter`
(1178 LOC) becomes a thin facade (213 LOC) that forwards every
public static call to one of three focused classes.

  - RateLimitChecker (1073 LOC) — get_settings, check_all, check_*
    (ip/email/cpf/global/device/verification/user_limit), record_*,
    plus all internal helpers (counters, blacklist, whitelist,
    temporary blocks, format/window utilities, get_user_ip).
  - RateLimitLogger (99 LOC) — log_attempt, cleanup_old_logs (private),
    cleanup_expired (cron entry point).
  - RateLimitStats (44 LOC) — get_stats aggregation.

Cross-class adjustments (no behavior change):
  - 9 `self::log_attempt(...)` call sites in `check_all` and
    `check_verification` rewritten to `RateLimitLogger::log_attempt(...)`.
  - `get_user_ip()` promoted from private to public so
    `RateLimitLogger::log_attempt` can call it as
    `RateLimitChecker::get_user_ip()` (alternative would have been to
    duplicate the helper, violating the no-logic-changes rule).
  - `CACHE_GROUP` constant kept on the facade — many external callers
    reference `RateLimiter::CACHE_GROUP` directly. Methods inside the
    new classes use `RateLimiter::CACHE_GROUP` (a deliberate one-way
    dependency that preserves the public API).

Test harness updated (4 files): the reflection-based `$settings_cache`
reset in setUp() now targets `RateLimitChecker` (the property's new
home) instead of `RateLimiter`. Two of the test files used
`hasProperty` guards that would have silently no-op'd after the move
and leaked settings between tests — repointing the guard restores
their original protective behavior.

Public API of `RateLimiter` is byte-identical: every external caller
(13 production files + tests) keeps working without any change.

Deviation: `RateLimitChecker` is 1073 LOC, above the issue's <800-LOC
target. The three-class split named in the issue spec (Checker /
Logger / Stats) puts every `check_*` method in Checker by design;
hitting <800 would require further sub-splitting (e.g. extracting
Device-fingerprint or Blocklist sub-classes), which deviates from the
explicit issue plan more than the LOC overrun does. Flagging here so
this can be revisited if the LOC bar is the binding constraint.

Verification:
  - php -l: clean on all 4 files.
  - vendor/bin/phpunit: 3870/3870 OK.
  - vendor/bin/phpstan analyse includes/security/: no errors.

https://claude.ai/code/session_01HiExaniSqvNBLpVTszCLNx
@claude
refactor(frontend): split public-csv-download god-object (S5 of #141)
1d60d6b 
Pure code movement — no behavior changes. PublicCsvDownload drops
from 1143 to 717 LOC; two new focused collaborators take the
extracted slices.

  - CsvDownloadValidator (291 LOC) — validate_form_access,
    validate_hash_only, validate_cpf_requirement, plus the private
    record_download_log_entry helper they share.
  - CsvDownloadFormInfoBuilder (277 LOC) — build_form_info plus its
    private build_restrictions_info / build_datetime_info /
    build_geolocation_info / format_locations_for_info /
    build_quiz_info helpers.

The facade keeps its public API intact so external callers
(PublicCsvExporter, FormEditorPublicCsvDownloadMetabox, Frontend) and
the test (`$this->handler->validate_cpf_requirement(...)`) keep
working unchanged. Constructor wires two private properties
(`$validator`, `$form_info_builder`); the three validation methods
become one-line forwarders; `ajax_info()` calls
`$this->form_info_builder->build_form_info(...)` inline.

Constants stay on PublicCsvDownload (SHORTCODE, ACTION,
NONCE_ACTION, META_*, DOWNLOAD_LOG_MAX). Methods inside the new
classes reference them as fully-qualified `PublicCsvDownload::*`.
The static helpers `get_audit_log_summary`, `maybe_wipe_legacy_logs`,
and the private `decrypt_log_entry_cpf` stay on the facade — they
are not validation/info-builder logic.

Test update: two `\ReflectionMethod( PublicCsvDownload::class, ... )`
tests targeting the moved `build_datetime_info` private method now
reflect on `CsvDownloadFormInfoBuilder::class` — same coverage, new
home.

Verification:
  - php -l: clean on all 3 files.
  - vendor/bin/phpunit --filter PublicCsvDownload: 42/42 OK (119 assertions).
  - vendor/bin/phpunit: 3870/3870 OK.
  - vendor/bin/phpstan analyse includes/frontend/: no errors.

https://claude.ai/code/session_01HiExaniSqvNBLpVTszCLNx
@claude
refactor(reregistration): extract AJAX handler + submission details r…
559f412 
…enderer (S6 of #141)

Pure code movement — no behavior changes. Two focused
collaborators extracted from ReregistrationAdmin (1132 LOC).

  - ReregistrationSubmissionDetailsRenderer (123 LOC) — owns
    build_submission_details_html, the modal HTML builder for the
    "View submission" AJAX endpoint. Promoted from private to
    public so the new AJAX handler can call it.
  - ReregistrationAjaxHandler (142 LOC) — owns the three wp_ajax_*
    callbacks (ajax_generate_ficha, ajax_view_submission_details,
    ajax_count_members). Composes the details renderer via
    constructor and stores it as a private property. The handler
    carries its own CAPABILITY constant; no facade helpers are
    needed because the bodies only touch WP globals + static
    repository/generator classes.

The facade `ReregistrationAdmin::init()` now instantiates the
AJAX handler and registers the three wp_ajax_* hooks against
`array( $this->ajax_handler, '...' )` instead of `array( $this, ... )`.

Public surface preserved: existing tests in
`tests/Unit/ReregistrationAdminTest.php` (and any external
caller) invoke `$admin->ajax_generate_ficha()` /
`->ajax_count_members()` / `->ajax_view_submission_details()`
directly on the facade. To honor that surface without keeping the
bodies on the facade, added three one-line public delegators plus
a `private get_ajax_handler()` that lazily instantiates the
handler when the test calls a delegator before `init()` has run.
Logic still lives entirely in `ReregistrationAjaxHandler`.

Cleanup: removed a stale duplicate copy of `ajax_count_members()`
that earlier work had left in the facade alongside the moved
version.

Deviation: facade is 1002 LOC (above the issue's <800 target).
The issue spec explicitly named multiple large render_* methods
(render_list, render_form, render_submissions,
render_submission_row, render_audience_options,
render_audience_transfer_list) plus handle_save/handle_delete to
keep on the facade. Hitting <800 would require extracting render
helpers the spec did not authorize. Net drop is ~130 LOC for the
two extractions the spec did call out.

Verification:
  - php -l: clean on all 3 files.
  - grep '$this->' on new files: 2 refs, both resolve internally.
  - vendor/bin/phpunit --filter Reregistration: 299/299 OK.
  - vendor/bin/phpunit: 3870/3870 OK.
  - vendor/bin/phpstan analyse includes/reregistration/: no errors.

https://claude.ai/code/session_01HiExaniSqvNBLpVTszCLNx
@claude
refactor(recruitment): extract public-shortcode renderer (S7 of #141)
0b7c641 
Pure code movement — no behavior changes. RecruitmentPublicShortcode
drops from 1136 to 476 LOC after extracting all the badge/label/
format/layout helpers into a focused renderer.

  - RecruitmentPublicShortcodeRenderer (692 LOC) — render_section,
    render_row, render_filters_bar, render_subscription_filter_inputs,
    render_name_search_input, render_adjutancy_filter_inputs,
    render_pagination, parse_columns_config, wrap_with_banner,
    decrypt_field, status_label, render_*_badge methods,
    preview_status_label, format_date_br, format_time_hm, msg.

Visibility: 5 methods called from the facade (render_uncached, render)
are public — render_section, render_filters_bar, parse_columns_config,
wrap_with_banner, msg. The remaining 14 helpers stay private since
they're only called from sibling renderer methods.

Facade keeps the public API + cache/rate-limit infrastructure:
register, render, render_uncached, wrap_output, enqueue_public_css,
cache_key, invalidate_public_cache, check_rate_limit, client_ip, plus
all SHORTCODE_TAG / CACHE_PREFIX / RATE_PREFIX / CACHE_VERSION_OPTION
/ CACHE_DIRTY_ACTION constants. Inside render_uncached() and render(),
the 5 calls to moved helpers became
RecruitmentPublicShortcodeRenderer::method(...). Unused use imports
for BadgeHtml, DocumentFormatter, and Encryption migrated to the
renderer where they're now used.

Verification:
  - php -l: clean on both files.
  - grep self:: in facade: only references own methods/constants.
  - vendor/bin/phpunit --filter RecruitmentPublicShortcode: 10/10 OK.
  - vendor/bin/phpunit: 3870/3870 OK.
  - vendor/bin/phpstan analyse includes/recruitment/: no errors.

https://claude.ai/code/session_01HiExaniSqvNBLpVTszCLNx
@claude
refactor(settings): split tab-documentation view into per-section par…
84af08a 
…tials (S8 of #141)

Pure file-system reorganization — the rendered HTML is byte-identical.
md5 of the rendered output stream is unchanged
(17ec376b4ccef6aeba4b9fe94c977454).

The 1474-LOC procedural view `ffc-tab-documentation.php` becomes an
83-LOC shell that emits the wrap div + the intro/TOC card inline,
then requires 19 numbered partials in order from a new
`includes/settings/views/documentation/` directory:

  01-shortcodes.php           (72)   11-ficha-pdf.php            (140)
  02-variables.php           (101)   12-geofence-locations.php    (50)
  03-quiz-variables.php       (51)   13-features.php              (97)
  04-appointment-variables.php(44)   14-security.php              (57)
  05-qr-code.php              (66)   15-examples.php             (102)
  06-validation-url.php       (49)   16-url-shortener.php        (114)
  07-html-styling.php        (135)   17-hooks.php                (176)
  08-custom-fields.php        (39)   18-troubleshooting.php       (82)
  09-audience-custom-fields.php(65)  19-rest-api-auth.php        (127)
  10-reregistration.php      (109)

Each partial has a standard preamble (`<?php` + docblock + ABSPATH
guard + `?>`) followed by the section's `<div class="card">…</div>`
block lifted byte-identical from the original.

No class changes, no autoloader changes (procedural view).
TabDocumentation::render() still does
`include FFC_PLUGIN_DIR . 'includes/settings/views/ffc-tab-documentation.php'`
exactly as before; the cascade of `require __DIR__ . '/documentation/NN-x.php'`
inside the shell handles the rest.

Verification:
  - php -l: clean on shell + all 19 partials.
  - md5 of rendered output: identical before/after.
  - vendor/bin/phpunit: 3870/3870 OK.

https://claude.ai/code/session_01HiExaniSqvNBLpVTszCLNx
@claude
fix(wpcs): add missing PHPDoc to S3 facade properties and forwarders
0d6d0af 
WPCS gating failed on the S3 facade
(class-ffc-form-editor-metabox-renderer.php) because the 9 private
properties wiring sub-renderers and the 10 thin forwarder methods
lacked docblocks. Pure documentation fix — no code or behavior
change.

https://claude.ai/code/session_01HiExaniSqvNBLpVTszCLNx
@claude
fix(wpcs): add short descriptions to S3 facade docblocks
9312091 
WPCS gating still failed after the previous PHPDoc pass because
@var/@PARAM tags alone don't satisfy "Missing short description in
doc comment" — each property and forwarder needed a one-line
summary above the tags. Pure documentation fix; no code change.

https://claude.ai/code/session_01HiExaniSqvNBLpVTszCLNx
@rpgmem
rpgmem marked this pull request as ready for review May 10, 2026 22:13
@rpgmem
rpgmem merged commit f8fffd3 into main May 10, 2026
11 checks passed
@rpgmem
rpgmem deleted the claude/refactor-split-god-objects-php branch May 10, 2026 22:13
@rpgmem rpgmem mentioned this pull request May 12, 2026
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Refactor] Split PHP god-objects (>1000 LOC) into focused responsibilities

2 participants

@rpgmem @claude