refactor: split the sections metabox into per-row renderers - #242
Merged
Conversation
render_sections_metabox() was the worst method in the plugin: cyclomatic
complexity 37, NPath 906,854,404, 184 lines. It opened the table, then ran a
loop whose body held four skip conditions, the label and title resolution,
a ~45-line repeater branch and a ~55-line scalar branch, all inline.
Split along the seams that were already there:
prepare_schema_row() the skip rules and the label/type resolution,
returning null for a row that cannot be drawn
render_schema_row() dispatch, returning the meta key the row claims
render_repeater_field_row()
render_scalar_field_row()
render_scalar_control() single / rich / textarea
get_repeater_rows() stored rows, or one blank row to type into
All three of its alerts are gone; the method is now the table and the loop.
The scalar and repeater branches each carried their own copy of the help
text block - collect description, validation and leading text, derive the
ids, echo the trailing paragraphs. That is the same block deduplicated in
the previous commit for the three repeater controls, so there were five
copies, not three. Both now call build_field_help_context() and
render_help_descriptions().
prepare_schema_row() is pure, so the skip rules can finally be tested
directly instead of only through a rendered metabox.
Markup is byte-for-byte identical, verified by rendering a schema covering
every control type, both branches, help text, stored values and skipped
rows, then diffing the output against the pre-refactor code. One deliberate
exception: the repeater row's help paragraphs now carry the same ids the
scalar row's always had. Nothing references them, and it only shows up when
a schema declares a field and a repeater under one slug - the omission was
an inconsistency between two copies of the same block, not a decision.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
Follows #239, now merged; this branch is rebased onto
mainand carries a single commit.(Reopened as a new PR: #240 was auto-closed when #239's branch was deleted on merge, and GitHub would not let it reopen.)
Fourth round on the PHPMD Code Size alerts. This one takes the worst method in the plugin.
render_sections_metabox()It opened the table, then ran a loop whose body held four skip conditions, the label and title resolution, a ~45-line repeater branch and a ~55-line scalar branch — all inline. Split along the seams that were already there:
prepare_schema_row()nullfor a row that cannot be drawnrender_schema_row()render_repeater_field_row()render_scalar_field_row()render_scalar_control()get_repeater_rows()render_sections_metabox()is now the table and the loop.Five copies, not three
#239 deduplicated the help-text block across the three repeater controls. Tracing what the repeater rendering actually depends on — 30 methods in the transitive closure, 18 of them shared with
render_sections_metabox— turned up two more copies of the same block inside the metabox itself, one per branch:Both branches now call
build_field_help_context()andrender_help_descriptions().That closure analysis also ruled out the plan stated in #239 — moving repeater rendering into its own class. With 18 helpers shared with the metabox, that would have duplicated exactly what #239 removed. Sharing the help subsystem first is the prerequisite; the class extraction becomes viable afterwards.
Result
mainRemaining in
class-documentate-documents.php:render_array_field()(NPath 576),render_array_field_item()(CC 15 · NPath 6,152), and the four class-level alerts.Verification
This is a pure rendering refactor, so the suite passing is necessary but not sufficient. The markup was diffed directly: a schema covering every control type, both branches, help text,
title/patternmsgresolution, stored values and skipped rows was rendered against the pre-refactor code and against this one.Byte-for-byte identical, with one deliberate exception: the repeater row's help paragraphs now carry the same ids the scalar row's always had. Nothing references them, and it only surfaces when a schema declares a field and a repeater under one slug. The omission was an inconsistency between two copies of the same block, not a decision.
Two things that nearly produced a false result, and how they were caught:
SchemaStoragesetup the existing tests use, and asserts on the dump's size and content before the comparison is trusted.repeaterswhile that branch reads fromfields. Forcing that case is what surfaced the id change above.prepare_schema_row()is pure, so the skip rules can finally be tested on their own rather than only through a rendered metabox.DocumentateSchemaRowTestcovers them, plus the title/pattern precedence and the repeater row fallback.