refactor: split the documents CPT into single-responsibility classes - #243
Merged
Conversation
…builds
A previous refactor extracted Documents_CPT_Registration and
Documents_Revision_Handler, and the constructor does build both. But only
one method ever delegated - set_default_comment_status_open. The other
fourteen kept their own copy of the logic, so the extracted classes were
almost entirely unreachable code while the CPT ran its own version.
The copies had drifted, and one drift mattered. The extracted
register_taxonomies() had lost the capability map:
'capabilities' => array(
'manage_terms' => 'manage_options',
...
)
Without it WordPress falls back to manage_categories for term management,
which editors hold. A document type defines the template and fields its
documents render from, so editing one rewrites existing documents - that is
meant to be an administrator action. Nothing was exposed, because the
unreachable copy is the one that had drifted, but wiring the extraction up
naively would have quietly widened who can create and delete document types.
The capability map is ported back before any delegation is switched over,
and DocumentsCPTRegistrationTest now asserts it. No test covered it, which
is how the two copies drifted apart unnoticed. Verified by removing the map
again and watching the new test fail.
The remaining fourteen methods now delegate. Hooks stay registered against
$this, as the comment in define_hooks() intends, so no callback identity
changes.
Class complexity drops 537 to 495, and 187 lines of duplicate leave the
file. This is groundwork: the class-level PHPMD alerts need the metabox
rendering, the save pipeline and the admin list table moved out too.
Seven helpers sat in the CPT while being shared between rendering the editor and saving it: reading a post's structured values, decoding a repeater's stored rows, normalising an item schema, and humanising a slug into a label. They belong with parse_structured_content() and get_meta_fields_for_post(), which already live in Documents_Meta_Handler and do the same kind of work. Moving them is what makes the class split possible: it is the only set of methods both the metabox renderer and the save pipeline reach into, so leaving them behind would have forced one of those extractions to call back into the CPT. fix_unescaped_unicode_sequences() turns out to be a third duplicate - the CPT's copy is byte-identical to the handler's - so the CPT's is dropped rather than moved. Two things stay put deliberately. ARRAY_FIELD_MAX_ITEMS and decode_array_field_value() are asserted by name against Documentate_Documents in the test suite, so both keep that home: the constant now takes its value from the handler and the method delegates, which leaves the published surface untouched while the implementation moves. Eleven tests reached these through ReflectionClass on a private method. They now call the public static methods directly, which is what they wanted to say in the first place. Markup verified identical, by the same dump-and-diff used for the previous two commits. Class complexity drops 495 to 446.
Moves 57 methods and ~1080 lines out of Documentate_Documents into Documentate_Document_Meta_Boxes, following the shape wp-decker uses for its event CPT: the post type keeps the hooks, a collaborator does the work. The boundary is exact. After the previous commit moved the shared field-value helpers to Documents_Meta_Handler, the rendering block reaches nothing that stays behind - no methods, no properties, no constants. That was the point of doing the helpers first; before it there were five calls crossing the line. register_meta_boxes(), render_type_metabox() and render_sections_metabox() stay on the CPT as delegators. The first is a hook callback, and the other two are called directly by 59 tests, so all three keep their published home while their bodies move. Three imports in the CPT are now unused and dropped. One of them, SchemaConverter, was already unused before this commit. About 60 tests reflected on private methods that moved. The two files whose every target moved now instantiate the renderer directly; the two mixed files resolve the owner, which for DocumentateDocumentsHelpersTest means its invoke() helper picks whichever object still declares the method. Markup verified identical, sections and type metabox both, by rendering a schema covering every control type against the pre-extraction code. The CPT is now 1717 lines from 3357, its complexity 245 from 446, and ExcessiveClassLength is resolved. Plugin-wide the alert count does not move: the new class carries an ExcessiveClassComplexity of 204 of its own. Splitting the class was never going to fix that by itself - what it does is make the remaining two extractions possible, after which the rendering class can be divided along the scalar/repeater line it already has.
Completes the split. Documentate_Documents keeps the post type, the hooks and the document-type lock; the four jobs it used to do itself now live in collaborators, the way wp-decker arranges its event CPT. Documentate_Document_Meta_Boxes 57 methods draws the editor Documentate_Document_Meta_Saver 27 methods writes meta, composes content Documentate_Document_Admin_List 14 methods filters, columns, views Documentate_Documents 560 lines registration and hooks Both boundaries were clean before the move: neither block reached anything that stayed behind, and the two share nothing with each other. The two classes are hooked differently, on purpose. The admin list registers its own hooks, which is what wp-decker does and what takes eight public methods off the CPT - enough to resolve TooManyPublicMethods, which no amount of moving private methods could. The saver keeps its hooks on the CPT and delegates, matching the metabox class, because 27 tests call filter_post_data_compose_content() and save_meta_boxes() by name and the public-method budget did not need them. Two mistakes worth recording. The extraction rewrote $this-> calls but not self:: ones, so three references in the saver silently retargeted to the new class and 30 tests failed with "undefined method Documentate_Document_Meta_Saver::parse_structured_content". Four more failed downstream of those, which is why "Documentate_Conversion_Manager not found" appeared and then vanished once the real cause was fixed - it was never a loading problem. Separately, injecting the property docblocks put one between an existing docblock and its declaration, orphaning it; PHPCS caught it. Two tests asserted the hook callbacks by object identity, which is exactly what moving the admin list changes. They now instantiate the class and assert it registers its own hooks, which is the behaviour worth pinning. Verified by rendering the editor, composing a saved post_content and building the list columns and views, then diffing all three against the pre-split code: identical. Documentate_Documents now has no PHPMD alerts at all, down from four. Plugin-wide 52 to 50: Meta_Boxes and Meta_Saver each carry an ExcessiveClassComplexity of their own, 204 and 138 against a threshold of 100.
…rity
The four-class split closed every alert on Documentate_Documents but left two
new ones behind: Meta_Boxes at complexity 204 and Meta_Saver at 138, against a
threshold of 100. Running PHPMD over wp-decker's own custom-post-types with
this project's ruleset settles what "like wp-decker" means - it reports zero
alerts, and its largest class there is Decker_Tasks at 48. The reference keeps
classes an order of magnitude smaller than what the first split produced.
Four more extractions, each a group that reached nothing outside itself and
touched no properties, so all four are static helpers:
Documentate_Document_Field_Help 36 leading text, description,
validation message and the ids
tying them to a control
Documentate_Document_Scalar_Field 35 text, number, date, select,
checkbox, textarea, TinyMCE
Documentate_Document_Repeater_Field 45 rows, the clone template, and one
control per column
Documentate_Document_Content_Writer 95 composing post_content, and the
sanitizing the save path shares
Order mattered. The repeater closure was 33 methods while the help subsystem
was still inline and 12 once it was out, so each boundary was recomputed after
the previous move rather than planned up front.
includes/custom-post-types/ now reports no PHPMD alerts at all, from four when
this branch started. Plugin-wide 52 to 48.
Two things worth saying plainly. Content_Writer at 95 clears the threshold but
is still twice wp-decker's largest; the sanitizers travel with it because both
composing and saving call them, and separating those needs a decision about
where sanitizing belongs rather than another mechanical move.
Documentate_Document_Repeater_Field also exposes prepare_field_control() and
the title helpers, because both row builders share that resolution - it reads
oddly on a class named after repeaters, and the docblock says so.
filter_post_data_compose_content() became private when its group moved, which
would have broken the CPT delegator; the entry point is public and the
delegator now points at the writer.
Verified as before: the editor markup, a composed post_content and the admin
list columns and views are identical to the pre-split code.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
codecov reported the split as a large coverage regression - project 85.71% to 77.57%, and five of the new classes at 0% or near it. The code was running: the same tests passed, the rendered markup was identical, and a single test that calls render_sections_metabox() showed the CPT's delegator line executing exactly once while the class it delegates to recorded nothing. The cause is @Covers. Where a test class carries the annotation, PHPUnit attributes coverage only to the units it names, and discards the rest - forceCoversAnnotation is not needed for that. Five suites said @Covers Documentate_Documents, which was accurate when the CPT did the work itself and became wrong the moment it delegated. Everything those tests drove through the collaborators was thrown away. It matched the symptom exactly: the classes reached only through those suites read 0%, while Content_Writer and Meta_Saver read 89% and 83% because integration tests with no annotation also reach them. The annotations now name the classes each suite actually drives: field-help 0.00% -> 98.88% admin-list 5.33% -> 92.00% content-writer 89.01% -> 96.70% repeater-field 0.00% -> 84.32% meta-boxes 0.00% -> 80.41% scalar-field 0.00% -> 78.00% Project coverage 84.78%, against 85.71% before the split. The small remaining gap is the duplicate code this branch deleted, which was covered. Ruled out first, since a wrong diagnosis here would have meant chasing the production code: the classes load from the paths they should and the property holds the right object; a brand-new file in the same directory instruments fine, so it is not the whitelist or the container; and restarting wp-env with xdebug changed nothing.
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
1 participant
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.
Splits
Documentate_Documentsfollowing the arrangement wp-decker uses for its CPTs: the post type keeps registration and hooks, and each job it used to do itself lives in its own class.includes/custom-post-types/now reports no PHPMD alerts at all, down from four. Plugin-wide 52 to 48.Result
Documentate_DocumentsDocumentate_Document_Meta_BoxesDocumentate_Document_Content_Writerpost_contentDocumentate_Document_Admin_ListDocumentate_Document_Repeater_FieldDocumentate_Document_Meta_SaverDocumentate_Document_Field_HelpDocumentate_Document_Scalar_Field3755 lines in one class becomes 527 in the CPT plus seven collaborators. The field-value reading moved into the existing
Documents_Meta_Handlerrather than a new class, since that is what it already does.Where the bar came from
The four-class split closed every alert on the CPT but left
Meta_Boxesat 204 andMeta_Saverat 138, against a threshold of 100 — complexity relocated, not removed. Rather than argue about whether chasing the threshold over-fragments a rendering class, running PHPMD over wp-decker's owncustom-post-typeswith this project's ruleset settled it: zero alerts, largest classDecker_Tasksat 48. The reference meets the bar, so four more extractions followed.An unfinished extraction, and one drift that mattered
Documents_CPT_RegistrationandDocuments_Revision_Handleralready existed and the constructor already built both — but only one of their fifteen methods ever delegated. The other fourteen kept their own copy, so the extracted classes were nearly unreachable while the CPT ran its own version.The copies had drifted, and the extracted
register_taxonomies()had lost this:Without it WordPress falls back to
manage_categories, which editors hold. A document type defines the template and fields its documents render from, so editing one rewrites existing documents.Nothing was exposed — the copy that drifted is the unreachable one. But wiring the extraction up without checking would have quietly widened who can create and delete document types. The map is ported back before any delegation switched over, and
DocumentsCPTRegistrationTestnow asserts it; no test covered it, which is how the two copies drifted apart unnoticed. Verified by removing it again and watching the new test fail.Two more duplicates turned up:
fix_unescaped_unicode_sequences()was byte-identical to the handler's copy, andSchemaConverterwas an already-dead import.Sequencing
Each boundary was recomputed after the previous move rather than planned up front, which mattered more than expected:
What is not clean
Content_Writerat 95 clears the threshold but is twice wp-decker's largest. The sanitizers travel with it because both composing and saving call them; separating those needs a decision about where sanitizing belongs, not another mechanical move.Repeater_Fieldexposesprepare_field_control()and the title helpers, because both row builders share that resolution. That reads oddly on a class named after repeaters, and its docblock says so.Admin_Listregisters its own hooks, which is what wp-decker does and what takes eight public methods off the CPT — the only way to resolveTooManyPublicMethods, since moving private methods cannot.Meta_Saverkeeps its hooks on the CPT and delegates, because 27 tests call its two entry points by name and the public-method budget did not need them.Verification
Behaviour is verified by output, not just by the suite passing. The editor markup, a composed
post_contentand the admin list's columns and views were captured against the pre-split code and after every extraction: identical each time.Three mistakes the checks caught, recorded because each is a repeatable trap:
$this->calls but notself::ones, so three references silently retargeted to the new class. 30 tests failed on it, and 4 more failed downstream — which is whyDocumentate_Conversion_Manager not foundappeared and then vanished once the real cause was fixed. It was never a loading problem.filter_post_data_compose_content()became private when its group moved, which would have broken the CPT delegator.Around 200 test reflection sites moved with their methods. Two tests asserted hook callbacks by object identity — exactly what moving
Admin_Listchanges — and now instantiate the class and assert it registers its own hooks, which is the behaviour worth pinning.