refactor: finish the repeater rendering complexity - #241
Merged
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
erseco
force-pushed
the
refactor/split-sections-metabox
branch
from
July 29, 2026 17:51
a048bda to
b9c851b
Compare
erseco
force-pushed
the
refactor/finish-repeater-complexity
branch
from
July 29, 2026 17:51
f0363e8 to
aede9a6
Compare
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
force-pushed
the
refactor/finish-repeater-complexity
branch
from
July 29, 2026 18:06
aede9a6 to
1d603ef
Compare
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
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.
Stacked on #240, which is stacked on #239. Review those first; GitHub retargets the base as each merges.
Fifth and last round on
class-documentate-documents.php. The file now has no method-level PHPMD alerts.What is left
mainIn this file, only the four class-level alerts remain. More on those below.
The last two alerts
render_array_field_item()render_array_field()Both came from redoing work the caller had already done.
render_array_field()re-resolved the repeater's title and hover text from the raw definition. Butrender_repeater_field_row()resolves exactly the same values before calling it — it has to, to draw the surrounding table row. The$labelparameter 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 secondsanitize_text_field()over the same string changes nothing.render_array_field_item()spelled out, per column, the same eight linesprepare_schema_row()already spells out per schema row:Both now call
prepare_field_control().prepare_array_item_field()adds what only a repeater column needs — submitted name, DOM id, stored value, and the item schema entry the single control reads itsdata_typefrom. The three-way type dispatch moved torender_array_item_control().Why the class-level alerts are not touched
#239 said the next step was moving repeater rendering into its own class. Measuring before writing anything showed why that does not work yet, and why no single extraction closes these:
The field-help subsystem — the most coherent movable unit, 13 methods — is ~252 lines. Extracting it closes zero alerts. These four need
Documentate_Documentsbroken into several classes (metabox rendering, the save/compose pipeline, the admin list table), which is a multi-PR architectural project with a different risk profile, not a step that belongs here.Worth stating plainly: extracting methods within one class, which is what #234, #239, #240 and this PR all did, resolves method alerts and cannot resolve class ones. The remaining four are the honest residue of that approach.
Verification
Same technique as #240, since this is again a pure rendering refactor. A schema with two repeater rows plus the clone template, four column types, titles, descriptions and validation messages, and the field-and-repeater-sharing-a-slug case was rendered against the pre-refactor code and against this one.
Byte-for-byte identical — no deliberate exception this time.
DocumentateSchemaRowTestgains 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 #239 introduced, so it now has a test that does not depend on rendering a full row to catch it.