refactor: reduce PHPMD complexity in the documents CPT - #239
Merged
Conversation
#236 stopped committing admin/vendor/libreoffice-converter, generating it from node_modules instead. That silently broke a test added in #234: test_browser_wasm_engine_enables_popup_conversion needs assets_available() to be true, so on a checkout that has not run `npm install` the metabox renders the disabled buttons and the assertion fails. `make test` stopped being self-contained. CI never showed it, because lint_and_test runs npm ci before PHPUnit and the postinstall hook regenerates the assets. State the precondition instead of assuming it: the test skips with an actionable message when the glue is absent, and still runs in CI and on any machine that has installed the npm dependencies. Verified both ways: 1 skipped without the assets, 16 passing with them.
Four methods in Documentate_Documents carried straightforward sequences of conditionals that multiply into large path counts. enforce_locked_doc_type() (NPath 576) inlined both branches of the lock: recording the first assignment and detecting drift from it. Each is now a named method, and doc_type_drifted_from_lock() collapses the is_wp_error / empty / count triple into one predicate. sanitize_array_field_items() (complexity 15, NPath 1300) filtered a repeater row and then decided whether it held anything, in one body. Split into sanitize_array_item() and array_item_has_content(). The trailing empty-check disappears: array_values(array_slice(array(), ...)) is already an empty array. save_meta_boxes() (complexity 15, NPath 540) chained four preconditions before handling the type selection. The preconditions move into can_save_meta_boxes() and the selection into save_doc_type_selection(), which keeps its own nonce check so the sniff can still see it. add_admin_filters() (complexity 15, NPath 1080) built three near-identical dropdowns by hand. They now share render_admin_filter_select(), fed by render_author_filter() and a parameterised render_term_filter(); the category cap moves into that call's arguments. Clears 8 method-level PHPMD alerts, 24 down to 16 in this file.
apply_admin_filters() (complexity 16, NPath 2700) mixed three concerns: deciding whether the query is ours, hiding archived documents, and registering a posts_clauses closure per sortable taxonomy. The two closures were near-identical, differing only in taxonomy and SQL alias. They collapse into one parameterised sort_by_term_name(), driven by a small map, and the guards become is_documents_list_query() and hide_archived_by_default(). The taxonomy now reaches the join as a placeholder through $wpdb->prepare() rather than being interpolated. The alias is still interpolated - it comes from the hardcoded map, never from a request - so the sniff is disabled just around that statement with the reason stated. get_structured_field_values() (complexity 18, NPath 2952) nested the whole meta fallback, including the repeater special case with its two legacy meta keys, inside the parse-content path. The fallback becomes build_field_values_from_meta() and the repeater lookup read_array_field_from_meta(), which returns the JSON payload or an empty string instead of assigning through a flag. Clears 4 more method-level alerts: 16 down to 13 in this file.
get_raw_schema_for_post() (complexity 27, NPath 43920) repeated the same "slug, or name as a fallback" derivation three times and nested the repeater field loop inside the repeater loop. That derivation becomes schema_entry_slug(), and the two indexes become index_schema_entries() and index_schema_repeaters(), the latter reusing the former for its own fields. The term lookup moves to get_assigned_doc_type_id(). save_dynamic_fields_meta() (complexity 22, NPath 113337) handled schema fields, repeaters and unknown posted keys in one body, and spelled out "empty value deletes, otherwise update" three separate times. That becomes write_or_delete_meta(); the per-field work becomes save_schema_field(), the JSON encoding encode_array_field_items() (which now returns an empty string for no rows, so the delete branch falls out of the same helper), and the trailing loop save_unknown_field_meta(). The double wp_unslash() on unknown values is preserved: the request body is already unslashed, but removing the second call would change what is stored for values containing backslashes, which is not this change's business. Clears 4 more method-level alerts: 13 down to 9 in this file. Verified with the full suite and the generation suite, which exercises the saved values end to end.
filter_post_data_compose_content() (complexity 41, NPath 172540368) built the stored post_content from three different sources in one body: the fields the schema declares, values already stored that the schema no longer declares, and anything else posted under the field prefix. Each source becomes its own method - compose_schema_fields(), compose_carried_over_fields(), compose_posted_fields() - with the repeater case in compose_array_field() and the serialisation in build_structured_content(). The early "nothing to store" return disappears: imploding no fragments already yields an empty string. compose_array_field() now encodes through encode_array_field_items(), the helper the meta save path uses, so the comment claiming both use the same JSON flags is enforced by construction rather than by two copies staying in sync. Also drops $dynamic_schema, assigned twice and never read. Clears 2 more method-level alerts: 9 down to 7 in this file. Verified with the full suite and the generation suite, which renders documents from the post_content this method writes.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Codecov flagged the previous commits: 78.20% of the diff hit against an
84.05% target. The refactors created many small methods whose guard
branches nothing exercised, so the extraction was unverified exactly
where it was easiest to get wrong.
Two focused suites, 32 tests:
DocumentateDocumentsHelpersTest pins the pure helpers - how a schema
entry derives its slug and what happens when it has neither slug nor
name, which entries the indexes drop, how repeater rows are judged empty
(markup stripped for rich values), the JSON flags used for storage, and
that the list filters escape their output and render nothing when there
is nothing to choose from.
DocumentateDocumentsQueryTest covers the query side by applying the
posts_clauses filter directly rather than driving a real admin request:
the join, the ASC/DESC handling, and that an unrelated ordering is left
untouched. It also covers the archived-status default, the three meta
shapes read_array_field_from_meta() understands including the historical
documentate_annexes key, and the precedence rules in the carried-over and
posted composition passes.
One test asserted sanitize_key('Solo Nombre') === 'solo_nombre'. It does
not - characters outside a-z0-9_- are dropped, so spaces collapse. The
expectation was wrong, not the code; corrected and commented.
Diff coverage for this file goes from 79.6% to 91.6%.
render_array_field_item() was 259 lines with a cyclomatic complexity of 44 and an NPath of 3407880: a per-field loop whose body branched into single, rich and textarea rendering, 80, 73 and 39 lines respectively. Each branch becomes its own method. The bodies are copied verbatim - only dedented - and take the loop variables under their existing names, so nothing inside them was retyped. Rendering has no unit coverage, only E2E, so a mechanical move was worth more here than a tidier signature. render_array_field_item() drops to complexity 15 and loses the length alert. The extracted methods carry NPath of their own (single 1024, rich 512), addressed next. Verified with the CPT unit tests and the document-fields E2E spec, which exercises adding, removing and persisting repeater rows.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Splitting render_array_field_item() by control type left two variables behind that used to come from the enclosing scope. Both failed silently. render_array_item_single() reads $definition['data_type'], where $definition was the `foreach ( $item_schema as $key => $definition )` loop variable. It was never a parameter, and the read is wrapped in isset(), so PHP raised nothing and $raw_data_type became permanently ''. That disables the data_type fallback in map_single_input_type(): a repeater column declared date, number or boolean rendered as a plain text input. render_array_item_rich() reads $is_template, a parameter of the caller marking the hidden row the front-end clones. Not being a parameter either, PHP 8 warned and the documentate-array-rich-template class was never applied, so the JS could no longer tell the template row from a live one. Both are now passed explicitly. The extraction was checked before landing, but the check only looked for variables assigned with `=` in the loop preamble. $definition enters scope through `foreach ... as`, and $is_template is a method parameter, so neither matched the pattern and both were reported as safe. The check was wrong, not the conclusion it produced. Adds DocumentateArrayFieldRenderTest, which renders a repeater row and asserts what these variables control: the input type produced by each data_type, and that the template marker class appears on the template row and only there. Verified against the broken code - 4 of its 8 tests fail there.
Code scanning flagged the two methods this PR introduced: render_array_item_single() at NPath 1024 and render_array_item_rich() at 512, both over the 500 threshold. A PR whose purpose is lowering these should not add new ones. The cause was duplication rather than any one method being convoluted. All three controls carried the same block: collect the leading text, the description and the validation message, derive their ids, point the control at them through aria-describedby, hand the message to the JS validator, and finally echo the two paragraphs. Six binary decisions, multiplied into whatever else the method did - a factor of 64 each, three times over. Extracted into build_field_help_context(), apply_help_attributes() and render_help_descriptions(). Single drops to 16, rich to 8, and textarea loses the same weight. Overall class complexity drops 546 to 537, so this removes complexity rather than relocating it - though the class grows by three methods and 20 lines, which nudges the class-level alerts the wrong way. That tension only resolves by moving work out of this class. The accessibility wiring is what makes this worth sharing: the id in aria-describedby and the paragraph it points at are produced in two separate places, and a control that renders one without the other is inaccessible without looking broken. One definition now, not three. Covered by the tests added in the previous commit, which assert exactly these branches across all three control types.
erseco
marked this pull request as ready for review
July 29, 2026 14:05
This was referenced Jul 29, 2026
erseco
added a commit
that referenced
this pull request
Jul 29, 2026
Clears the last two method-level alerts in the documents CPT: render_array_field_item() at NPath 6,152 with cyclomatic complexity 15, and render_array_field() at NPath 576. The file now has none. Both came from work the caller had already done. render_array_field() re-resolved the repeater's title and hover text from the raw definition, but render_repeater_field_row() resolves exactly the same values before calling it - it has to, to draw the surrounding table row. The label parameter was being handed in and then overwritten with an identical value. It now takes the hover text alongside the label and trusts both. (get_field_title() sanitizes before returning, so dropping the second sanitize_text_field() over the same string changes nothing.) render_array_field_item() spelled out, per column, the same eight lines prepare_schema_row() already spells out per schema row: read the label or fall back, read the type, look up the raw field, let a declared title win, prefer the pattern message for hover text. Both now call prepare_field_control(), with prepare_array_item_field() adding what only a repeater column needs - submitted name, DOM id, stored value, and the item schema entry the single control reads its data_type from. The three-way type dispatch moved to render_array_item_control(). Markup verified identical to the previous commit, byte for byte, by rendering a schema with two repeater rows plus the clone template, four column types, titles, descriptions and validation messages, and diffing against the pre-refactor code. DocumentateSchemaRowTest gains six cases for the column preparation, including that the item schema entry survives the trip - that variable going missing was one of the two bugs in #239.
erseco
added a commit
that referenced
this pull request
Jul 29, 2026
Clears the last two method-level alerts in the documents CPT: render_array_field_item() at NPath 6,152 with cyclomatic complexity 15, and render_array_field() at NPath 576. The file now has none. Both came from work the caller had already done. render_array_field() re-resolved the repeater's title and hover text from the raw definition, but render_repeater_field_row() resolves exactly the same values before calling it - it has to, to draw the surrounding table row. The label parameter was being handed in and then overwritten with an identical value. It now takes the hover text alongside the label and trusts both. (get_field_title() sanitizes before returning, so dropping the second sanitize_text_field() over the same string changes nothing.) render_array_field_item() spelled out, per column, the same eight lines prepare_schema_row() already spells out per schema row: read the label or fall back, read the type, look up the raw field, let a declared title win, prefer the pattern message for hover text. Both now call prepare_field_control(), with prepare_array_item_field() adding what only a repeater column needs - submitted name, DOM id, stored value, and the item schema entry the single control reads its data_type from. The three-way type dispatch moved to render_array_item_control(). Markup verified identical to the previous commit, byte for byte, by rendering a schema with two repeater rows plus the clone template, four column types, titles, descriptions and validation messages, and diffing against the pre-refactor code. DocumentateSchemaRowTest gains six cases for the column preparation, including that the item schema entry survives the trip - that variable going missing was one of the two bugs in #239.
erseco
added a commit
that referenced
this pull request
Jul 29, 2026
Clears the last two method-level alerts in the documents CPT: render_array_field_item() at NPath 6,152 with cyclomatic complexity 15, and render_array_field() at NPath 576. The file now has none. Both came from work the caller had already done. render_array_field() re-resolved the repeater's title and hover text from the raw definition, but render_repeater_field_row() resolves exactly the same values before calling it - it has to, to draw the surrounding table row. The label parameter was being handed in and then overwritten with an identical value. It now takes the hover text alongside the label and trusts both. (get_field_title() sanitizes before returning, so dropping the second sanitize_text_field() over the same string changes nothing.) render_array_field_item() spelled out, per column, the same eight lines prepare_schema_row() already spells out per schema row: read the label or fall back, read the type, look up the raw field, let a declared title win, prefer the pattern message for hover text. Both now call prepare_field_control(), with prepare_array_item_field() adding what only a repeater column needs - submitted name, DOM id, stored value, and the item schema entry the single control reads its data_type from. The three-way type dispatch moved to render_array_item_control(). Markup verified identical to the previous commit, byte for byte, by rendering a schema with two repeater rows plus the clone template, four column types, titles, descriptions and validation messages, and diffing against the pre-refactor code. DocumentateSchemaRowTest gains six cases for the column preparation, including that the item schema entry survives the trip - that variable going missing was one of the two bugs in #239.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third round on the PHPMD Code Size alerts, this time in
class-documentate-documents.php— the largest remaining source after #234.Result
mainNPathComplexityCyclomaticComplexityExcessiveMethodLengthIn
class-documentate-documents.phpspecifically: 28 alerts down to 10.mainfilter_post_data_compose_content()save_dynamic_fields_meta()get_raw_schema_for_post()render_array_field_item()get_structured_field_values()apply_admin_filters()What the refactors found
The recurring shape in this file is the same block copied per type, format or taxonomy. Parameterising those removes the complexity and the risk that the copies drift apart:
add_admin_filters()built three near-identical<select>dropdowns by hand — now onerender_admin_filter_select().apply_admin_filters()registered twoposts_clausesclosures differing only in taxonomy and SQL alias — now one parameterisedsort_by_term_name(). The taxonomy reaches the join through$wpdb->prepare()instead of being interpolated.get_raw_schema_for_post()derived "slug, or name as a fallback" three times — nowschema_entry_slug().save_dynamic_fields_meta()spelled out "empty value deletes, otherwise update" three times — nowwrite_or_delete_meta().compose_array_field()and the meta save path both encoded repeater rows with a specific set of JSON flags, with a comment asking the two to stay in sync. They now shareencode_array_field_items(), so that is enforced by construction.build_field_help_context(),apply_help_attributes()andrender_help_descriptions().That last one is worth stating beyond the metric: the id in
aria-describedbyand the paragraph it points at were produced in two separate places, in triplicate. A control that renders one without the other is inaccessible without looking broken.Two dead assignments were removed along the way:
$dynamic_schemain the compose filter, assigned twice and never read.Two bugs this PR introduced, and how they were caught
Splitting
render_array_field_item()by control type dropped two variables that came from the enclosing scope, and both failed silently:render_array_item_single()read$definition['data_type'], where$definitionwas theforeachloop variable. The read is wrapped inisset(), so PHP raised nothing — repeater columns declareddate,numberorbooleanquietly rendered as plain text inputs.render_array_item_rich()read$is_template, a parameter of the caller. Thedocumentate-array-rich-templateclass was never applied, so the front-end could no longer tell the clone template from a live row.Neither PHPCS, nor the 1840 tests, nor the E2E suite detected them.
DocumentateArrayFieldRenderTestnow asserts what these variables control, and was verified against the broken code — 4 of its 8 tests fail there.Patch coverage then showed
render_array_item_textarea()was uncovered in full: the third branch of the split that no test reached. Eleven more tests cover it and the help-text branches across all three control types, including that the description is actually wired to its control viaaria-describedby.A pre-existing failure fixed first
make testfailed onmainbefore any of this. #236 stopped committingadmin/vendor/libreoffice-converter, generating it fromnode_modulesinstead, which silently broke a test added in #234: it needsassets_available()to be true, so on a checkout that has not runnpm installthe metabox renders disabled buttons and the assertion fails. CI never showed it, becauselint_and_testrunsnpm cibefore PHPUnit.The test now states that precondition and skips with an actionable message, so
make testis self-contained again.The class-level trade-off, stated plainly
Extracting methods within a class resolves method alerts and worsens the class ones:
mainThat is the cost of this approach. Class complexity is roughly flat — the deduplication above removed real complexity rather than relocating it — but the class is larger and has more methods, so its four class-level alerts stay open.
Closing those means moving repeater rendering out of
Documentate_Documentsinto its own class, the pattern used forDocuments_Input_Attributesin #234. That is an architectural change with a different risk profile, so it belongs in its own PR rather than being bolted onto this one.Remaining alerts in this file
render_sections_metabox()render_array_field_item()render_array_field()Verification
Coverage: project 84.05% → 85.54%, patch 88.11%.
The generation suite matters here:
filter_post_data_compose_content()writes thepost_contentthat document rendering reads back, so it exercises the change end to end rather than just at the unit level.