Skip to content

Tighten DDD layering: pure Domain, injected renderers, trimmed EntryOperations#874

Open
GaryJones wants to merge 4 commits into2.xfrom
GaryJones/2x-arch-review
Open

Tighten DDD layering: pure Domain, injected renderers, trimmed EntryOperations#874
GaryJones wants to merge 4 commits into2.xfrom
GaryJones/2x-arch-review

Conversation

@GaryJones
Copy link
Copy Markdown
Contributor

Summary

Architectural follow-up against 2.x after reviewing the DDD refactor. Three concerns surfaced from the review and are addressed here in three commits, each independently green.

LiveblogPost lived under Domain/Entity/ but called update_post_meta, delete_post_meta, post_type_supports, current_user_can, do_action and apply_filters directly. That made it a WordPress-aware aggregate, not a persistence-agnostic domain entity. Moving it to Application/Aggregate/ matches what the class actually is, and leaves the Domain layer holding only Entry, the value objects and EntryRepositoryInterface — all of which are genuinely free of WordPress dependencies. No behaviour change; only the namespace and 28 imports move with it.

Three Application-layer classes (EntryPresenter, KeyEventShortcodeHandler, LazyloadConfiguration) reached back into Container::instance() at runtime to fetch a renderer. That inverts the dependency direction and hides the dependency from the constructor. Each one now declares the renderer it needs as a constructor parameter, with the wiring done once in Container or PluginBootstrapper. A new Application\Renderer\TemplateRendererInterface mirrors the existing ContentRendererInterface, so Application code never imports the concrete TemplateRenderer from Infrastructure. EntryPresenter loses an unreachable render() method in passing.

The same commit also untangles a circular dependency: EntryService accepted an optional KeyEventService constructor argument with a lazy fallback that constructed a fresh KeyEventService and EntryQueryService on demand. The lazy create silently bypassed any test double a caller had wired up. The only path that needed it — delete_key — is called exclusively from EntryOperations::execute_delete_key, which already has both services injected. Moving the two-step orchestration up to that caller lets EntryService collapse to a single repository-only constructor, and the cycle is gone.

Finally, EntryOperations had seven public pass-through methods (insert, update, delete, is_key_event, remove_key_action plus entry_service() and key_event_service() accessors) that call-site mapping showed have no external callers. They were leftover scaffolding from an earlier refactor, documented as a facade for legacy container access that no longer exists. Removing them, together with rewriting the class docblock to describe what the class actually does — dispatch CRUD action strings, apply the liveblog_*_entry filters and actions around each mutation, and prepare the JSON payload for REST and admin-ajax callers — leaves a clear public surface of do_crud, format_preview and is_valid_action.

What this PR does not do: replace the hand-rolled service-locator container, collapse the single-implementation ContentRendererInterface and EmbedHandlerInterface, or extract a LiveblogPostRepository. Those were considered and judged either out of scope or actively unhelpful at the current size of the codebase.

Test plan

  • composer install && vendor/bin/phpunit --testsuite unit — 190 tests, all green at HEAD and at each intermediate commit.
  • vendor/bin/phpcs src/php --standard=.phpcs.xml.dist — clean.
  • Smoke test in wp-env: create a liveblog, post an entry, edit an entry, mark an entry as key event then unmark it, archive the post. The CRUD path exercises both REST and admin-ajax routes that depend on the new constructor wiring.

LiveblogPost calls update_post_meta, delete_post_meta,
post_type_supports, current_user_can, do_action and apply_filters
directly. That makes it a WordPress-aware aggregate, not the
persistence-agnostic entity that the Domain layer is supposed to hold.

Move it to Application/Aggregate/, where calling WordPress functions is
appropriate. The Domain layer now contains only Entry, the value objects
and EntryRepositoryInterface, all of which are genuinely free of
WordPress dependencies. The class itself is unchanged; only its
namespace and the imports that reference it are updated.

This is the lighter of two options. A heavier alternative would extract
a LiveblogPostRepository so the aggregate held only state, but that is
a larger refactor with no current query need driving it.
Two related changes that together stop the Application layer from
reaching back into the Infrastructure container at runtime, and untangle
the cycle between EntryService and KeyEventService.

EntryPresenter, KeyEventShortcodeHandler and LazyloadConfiguration each
called Container::instance() inside Application code to fetch the
template or content renderer. That inverted the dependency direction
(Application reaching into Infrastructure) and made dependencies
implicit at construction time. Each now declares the renderer it needs
as a constructor parameter, with the wiring done once in Container or
PluginBootstrapper. A new Application\Renderer\TemplateRendererInterface
mirrors the existing ContentRendererInterface so Application code never
imports the concrete TemplateRenderer from Infrastructure. EntryPresenter
loses an unreachable render() method in passing.

EntryService had an optional KeyEventService constructor parameter with
a lazy fallback that constructed a fresh KeyEventService (and a fresh
EntryQueryService) on demand. That was the classic shape of a circular
dependency: KeyEventService also wanted EntryService in some flows, and
the lazy create silently bypassed any test double a caller had wired up.
The only path that needed it was EntryService::delete_key, which is
called exclusively from EntryOperations::execute_delete_key. The
orchestration moves up to that caller, which already has both services
injected, so EntryService now has a single repository-only constructor
and the cycle is gone.
Call-site mapping showed seven public methods on EntryOperations
(insert, update, delete, is_key_event, remove_key_action, plus
entry_service() and key_event_service() accessors) had no callers
outside the class itself. They were leftover pass-throughs from an
earlier refactor that had been documented as a facade for legacy
container access. Anyone now reading the class is forced to pattern
match across four similar-sounding services to find which one a method
actually lives on.

Remove the dead methods and rewrite the class docblock to describe what
the class actually does: it dispatches HTTP CRUD action strings to the
appropriate flow, applies the liveblog_before_*_entry filters and
liveblog_*_entry actions around each mutation, and prepares the JSON
payload that REST and admin-ajax callers return to the client. The
public surface is now do_crud, format_preview and is_valid_action plus
the private execute helpers, which matches what RestApiController and
RequestRouter actually use.
@GaryJones GaryJones requested a review from a team as a code owner April 26, 2026 22:35
Integration tests caught a regression: the previous commit gave
LazyloadConfiguration a required TemplateRendererInterface constructor
parameter, but six other callers (RequestRouter, AssetManager,
AmpIntegration, the liveblog-loop template) instantiate it directly
with `new LazyloadConfiguration()` for read-only config access. Forcing
the renderer on those callers would have spread an admin-only,
edge-case dependency through the read path.

The renderer was only needed for one method, render_deprecated_plugin_notice,
which surfaces an admin notice when the obsolete standalone Lazyload
Liveblog Entries plugin is active. That logic doesn't belong inside a
config reader anyway, so it moves up to PluginBootstrapper alongside
the other one-time hook wiring. PluginBootstrapper already has the
template renderer via the container.

LazyloadConfiguration is now a plain stateless config reader again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant