Refactor: Extract business logic into Action classes (v1.4.0) #144

New issue

Closed

opened 2026-08-13 21:37:13 +02:00 by myrmidex · 1 comment

myrmidex commented

2026-08-13 21:37:13 +02:00

Owner

Refactor the areas below to improve testability and separation of concerns.

Scope

Originally 17 items; narrowed to 7 on 2026-08-13 after inspecting each candidate. Three of those seven were then also found not to hold up, and were replaced — see below.

1. Extract SaveArticleAction from ArticleFetcher
2. Extract ValidateArticleAction from ValidationService — delivered as a rename. After item 3, ValidationService was one method with one caller; extracting would have left a pass-through class. Moved and renamed instead: ValidationService::validate() → ValidateArticleAction::execute().
3. Extract CreateRouteArticlesAction from ValidationService
4. Extract FetchRssArticlesAction and FetchWebsiteArticlesAction from ArticleFetcher
~~5. Extract RefreshArticlesAction from ArticlesController~~
~~6. Extract UpdateFeedAction from FeedsController~~
~~7. Extract UpdateRouteAction from RoutingController~~

Items 5–7 replaced. On inspection each controller method was a one-liner — ArticleDiscoveryJob::dispatch(), $feed->update($validated), Route::where(...)->update($validated) — with validation already in a FormRequest and the try/catch being HTTP concern. Extracting would have produced actions wrapping a single model call: the same pass-through objection that removed ten items from the original list.

They were replaced with deleting ArticleFetcher entirely, which items 1 and 4 had left as a 58-line class whose two methods shared no callers and no state:

8. Split ArticleFetcher into FetchFeedArticlesAction + FetchArticleDataAction, and delete it

Dropped from the original list

Inspected and found to be already single-purpose — extracting an Action would produce a class whose only method delegates to another class with one method:

GetActivitySummaryAction from ActivitySummary — 34 lines, one public method
SaveLogAction from LogSaver — 65 lines, five one-line wrappers around a single log()
UploadThumbnailAction from ThumbnailUploader — one public method plus private helpers
GetCommunitiesAction from CommunityDirectory
GetLemmyTokenAction from LemmyAuthService

Also dropped as not worth the churn without a specific complaint driving them: GetSystemStatusAction, CheckOnboardingStatusAction, SendNotificationAction, SyncLemmyChannelPostsAction, CheckPlatformCredentialsAction.

Any of these can be re-added later if a concrete testability problem shows up.

Acceptance criteria

The extractions above are complete
No behaviour change — every moved method body verified byte-identical against the pre-move commit
Each new Action has unit tests
ArticleFetcher and ValidationService are gone; app/Services/Article/ no longer exists

Refactor the areas below to improve testability and separation of concerns. ## Scope Originally 17 items; narrowed to 7 on 2026-08-13 after inspecting each candidate. Three of those seven were then also found not to hold up, and were replaced — see below. - [x] 1. Extract `SaveArticleAction` from `ArticleFetcher` - [x] 2. Extract `ValidateArticleAction` from `ValidationService` — **delivered as a rename.** After item 3, `ValidationService` was one method with one caller; extracting would have left a pass-through class. Moved and renamed instead: `ValidationService::validate()` → `ValidateArticleAction::execute()`. - [x] 3. Extract `CreateRouteArticlesAction` from `ValidationService` - [x] 4. Extract `FetchRssArticlesAction` and `FetchWebsiteArticlesAction` from `ArticleFetcher` - [ ] ~~5. Extract `RefreshArticlesAction` from `ArticlesController`~~ - [ ] ~~6. Extract `UpdateFeedAction` from `FeedsController`~~ - [ ] ~~7. Extract `UpdateRouteAction` from `RoutingController`~~ **Items 5–7 replaced.** On inspection each controller method was a one-liner — `ArticleDiscoveryJob::dispatch()`, `$feed->update($validated)`, `Route::where(...)->update($validated)` — with validation already in a FormRequest and the try/catch being HTTP concern. Extracting would have produced actions wrapping a single model call: the same pass-through objection that removed ten items from the original list. They were replaced with **deleting `ArticleFetcher` entirely**, which items 1 and 4 had left as a 58-line class whose two methods shared no callers and no state: - [x] 8. Split `ArticleFetcher` into `FetchFeedArticlesAction` + `FetchArticleDataAction`, and delete it ## Dropped from the original list Inspected and found to be **already single-purpose** — extracting an Action would produce a class whose only method delegates to another class with one method: - `GetActivitySummaryAction` from `ActivitySummary` — 34 lines, one public method - `SaveLogAction` from `LogSaver` — 65 lines, five one-line wrappers around a single `log()` - `UploadThumbnailAction` from `ThumbnailUploader` — one public method plus private helpers - `GetCommunitiesAction` from `CommunityDirectory` - `GetLemmyTokenAction` from `LemmyAuthService` Also dropped as not worth the churn without a specific complaint driving them: `GetSystemStatusAction`, `CheckOnboardingStatusAction`, `SendNotificationAction`, `SyncLemmyChannelPostsAction`, `CheckPlatformCredentialsAction`. Any of these can be re-added later if a concrete testability problem shows up. ## Acceptance criteria - [x] The extractions above are complete - [x] No behaviour change — every moved method body verified byte-identical against the pre-move commit - [x] Each new Action has unit tests - [x] `ArticleFetcher` and `ValidationService` are gone; `app/Services/Article/` no longer exists

myrmidex added this to the v1.4.0 milestone 2026-08-13 21:37:13 +02:00

myrmidex added the

enhancement

label 2026-08-13 21:37:13 +02:00

myrmidex commented

2026-08-14 00:20:22 +02:00

Author

Owner

Delivered in five commits, d8b85f8..dc64dd8.

d8b85f8  Extract SaveArticleAction from ArticleFetcher
dd8f799  Extract CreateRouteArticlesAction from ValidationService
d58fd8a  Rename ValidationService to ValidateArticleAction
eefadff  Extract the RSS and website fetch actions from ArticleFetcher
dc64dd8  Replace ArticleFetcher with focused fetch actions

Result

app/Services/Article/ no longer exists. Eight Actions in app/Actions/, each with one public execute() and constructor-injected collaborators, forming a shallow DAG with no cycles:

ValidateArticleAction ──> FetchArticleDataAction
                      └─> CreateRouteArticlesAction

PublishRouteArticleAction ──> FetchArticleDataAction

FetchFeedArticlesAction ──> FetchRssArticlesAction ────> SaveArticleAction
                        └─> FetchWebsiteArticlesAction ─┘

The testability gain is measurable, not nominal

ArticleFetcherTest used ReflectionClass + setAccessible(true) three times to reach the private saveArticle(). SaveArticleActionTest calls execute() directly with zero reflection, and picked up two new tests for the fallback-title logic that were awkward to write under reflection.
evaluateKeywords() and shouldAutoApprove() were private inside ValidationService, reachable only by mocking ArticleFetcher and driving the whole validate() flow. CreateRouteArticlesActionTest now covers keyword matching, auto-approve precedence and decision stamping directly, with no mocks at all.

Behaviour preservation

Every moved method body was diffed against its pre-move commit with git show HEAD:… | diff and confirmed byte-identical — the only differences are method signatures and two comments dropped as restating the code below them. The finish-phase pr-reviewer independently re-diffed and found no logic drift.

The strongest evidence is the test suite itself: 1219 tests / 2924 assertions, unchanged in count across the whole ticket, with the pre-existing ValidationServiceTest (19 tests) and ArticleFetcherTest (11) passing throughout with only their construction lines edited.

Scope changes, all reported before acting

Three of the seven listed items were not done as written. Each was inspected, the finding reported with evidence, and the alternative chosen deliberately — details in the ticket body above. The short version: extraction is warranted when a class does several things, not to satisfy a naming convention. Ten items were dropped on that basis before work started, and three more during it.

Verification

1219 tests / 2924 assertions green, Pint clean (350 files), PHPStan clean. Per-commit code-reviewer on all five commits plus a finish-phase pr-reviewer over the combined diff — no critical or must-fix issues at any point.

Noted, not fixed

Both are behaviour changes with no place in a pure refactor:

FetchFeedArticlesAction's elseif/fallthrough for an unsupported feed type is unreachable — feeds.type is a DB enum('website','rss') with matching validation. A match that throws would be more honest than a warning-and-empty-collection.
Error handling is asymmetric: SaveArticleAction logs and rethrows, while every fetch action logs and swallows. Inherited from the original ArticleFetcher, so preserved deliberately — but worth deciding what a failed fetch should do to a discovery run.

Delivered in five commits, `d8b85f8`..`dc64dd8`. ``` d8b85f8 Extract SaveArticleAction from ArticleFetcher dd8f799 Extract CreateRouteArticlesAction from ValidationService d58fd8a Rename ValidationService to ValidateArticleAction eefadff Extract the RSS and website fetch actions from ArticleFetcher dc64dd8 Replace ArticleFetcher with focused fetch actions ``` ## Result `app/Services/Article/` no longer exists. Eight Actions in `app/Actions/`, each with one public `execute()` and constructor-injected collaborators, forming a shallow DAG with no cycles: ``` ValidateArticleAction ──> FetchArticleDataAction └─> CreateRouteArticlesAction PublishRouteArticleAction ──> FetchArticleDataAction FetchFeedArticlesAction ──> FetchRssArticlesAction ────> SaveArticleAction └─> FetchWebsiteArticlesAction ─┘ ``` ## The testability gain is measurable, not nominal - `ArticleFetcherTest` used `ReflectionClass` + `setAccessible(true)` **three times** to reach the private `saveArticle()`. `SaveArticleActionTest` calls `execute()` directly with zero reflection, and picked up two new tests for the fallback-title logic that were awkward to write under reflection. - `evaluateKeywords()` and `shouldAutoApprove()` were private inside `ValidationService`, reachable only by mocking `ArticleFetcher` and driving the whole `validate()` flow. `CreateRouteArticlesActionTest` now covers keyword matching, auto-approve precedence and decision stamping directly, with **no mocks at all**. ## Behaviour preservation Every moved method body was diffed against its pre-move commit with `git show HEAD:… | diff` and confirmed byte-identical — the only differences are method signatures and two comments dropped as restating the code below them. The finish-phase `pr-reviewer` independently re-diffed and found no logic drift. The strongest evidence is the test suite itself: **1219 tests / 2924 assertions, unchanged in count** across the whole ticket, with the pre-existing `ValidationServiceTest` (19 tests) and `ArticleFetcherTest` (11) passing throughout with only their construction lines edited. ## Scope changes, all reported before acting Three of the seven listed items were not done as written. Each was inspected, the finding reported with evidence, and the alternative chosen deliberately — details in the ticket body above. The short version: **extraction is warranted when a class does several things, not to satisfy a naming convention.** Ten items were dropped on that basis before work started, and three more during it. ## Verification 1219 tests / 2924 assertions green, Pint clean (350 files), PHPStan clean. Per-commit `code-reviewer` on all five commits plus a finish-phase `pr-reviewer` over the combined diff — no critical or must-fix issues at any point. ## Noted, not fixed Both are behaviour changes with no place in a pure refactor: - `FetchFeedArticlesAction`'s `elseif`/fallthrough for an unsupported feed type is unreachable — `feeds.type` is a DB `enum('website','rss')` with matching validation. A `match` that throws would be more honest than a warning-and-empty-collection. - Error handling is asymmetric: `SaveArticleAction` logs and rethrows, while every fetch action logs and swallows. Inherited from the original `ArticleFetcher`, so preserved deliberately — but worth deciding what a failed fetch should do to a discovery run.

myrmidex closed this issue

2026-08-14 00:20:34 +02:00

myrmidex referenced this issue

2026-08-15 00:02:58 +02:00

Release v1.4.0 #146