diff --git a/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-1.md b/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-1.md new file mode 100644 index 0000000000..3d08535ea8 --- /dev/null +++ b/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-1.md @@ -0,0 +1,203 @@ +# Critical Implementation Review: Phase 8 - Facade Finalization + +**Plan File:** `2025-12-24-contentservice-refactor-phase8-implementation.md` +**Review Date:** 2025-12-24 +**Reviewer:** Critical Implementation Review System +**Review Version:** 1 + +--- + +## 1. Overall Assessment + +**Summary:** The plan has clear goals and good structure, but contains **several critical inaccuracies** about the current state of the codebase that would lead to implementation failures or duplication. The plan was apparently written based on assumptions about the code state rather than verification of the actual implementation. + +**Strengths:** +- Well-structured task breakdown with clear steps +- Risk mitigation section is comprehensive +- Rollback plan is sensible +- Commit messages follow project conventions + +**Major Concerns:** +1. **Task 1 proposes extracting methods that already exist** in `ContentMoveOperationService` +2. **Task 2 proposes extracting `DeleteLocked` that already exists** in both `ContentCrudService` and `ContentMoveOperationService` +3. **Task 4's field removal list is inaccurate** - some fields listed don't exist or are already used by remaining methods +4. **Constructor parameter count claims are inconsistent** with actual code +5. **Line count estimates are outdated** (claims 1330 lines, actual is ~1330 but layout differs) + +--- + +## 2. Critical Issues + +### 2.1 Task 1: Duplicate Method Extraction (Already Exists) + +**Description:** Task 1 proposes adding `PerformMoveLocked` to `IContentMoveOperationService` and extracting its implementation. However, **this method already exists** in `ContentMoveOperationService.cs` (lines 140-184). + +**Why it matters:** Attempting to add this method to the interface will cause a compilation error (duplicate method signature). Following the plan as-is will waste time and introduce confusion. + +**Current State:** +- `ContentMoveOperationService` already has: + - `PerformMoveLocked` (private, lines 140-184) + - `PerformMoveContentLocked` (private, lines 186-195) + - `GetPagedDescendantQuery` (private, lines 591-600) + - `GetPagedLocked` (private, lines 602-617) + +**Actionable Fix:** +- **Skip Task 1 entirely** or rewrite it to: + 1. Make the existing private `PerformMoveLocked` method public + 2. Add the method signature to `IContentMoveOperationService` + 3. Update `ContentService.MoveToRecycleBin` to call `MoveOperationService.PerformMoveLocked` + 4. Remove the duplicate methods from `ContentService` + +### 2.2 Task 2: Duplicate DeleteLocked Extraction (Already Exists) + +**Description:** Task 2 proposes adding `DeleteLocked` to `IContentCrudService`. However, **both services already have this method**: +- `ContentCrudService.DeleteLocked` (lines 637-692) +- `ContentMoveOperationService.DeleteLocked` (lines 295-348) + +**Why it matters:** The plan's code snippet differs from the existing implementation (missing iteration bounds, logging). Following the plan would downgrade the existing robust implementation. + +**Current State in ContentCrudService:** +```csharp +private void DeleteLocked(ICoreScope scope, IContent content, EventMessages evtMsgs) +{ + // Already has iteration bounds (maxIterations = 10000) + // Already has proper logging for edge cases + // Already has empty batch detection +} +``` + +**Actionable Fix:** +- **Skip Task 2 entirely** - the work is already done +- Alternatively, if the goal is to expose `DeleteLocked` on the interface: + 1. Add the signature to `IContentCrudService` + 2. Change visibility from `private` to `public` in `ContentCrudService` + 3. Remove `DeleteLocked` from `ContentService` (if it still exists there) + +### 2.3 Task 3: CheckDataIntegrity - Missing IShortStringHelper + +**Description:** Task 3 correctly identifies that `CheckDataIntegrity` needs extraction, but **the constructor modification is more complex** than stated. + +**Why it matters:** `ContentCrudService` constructor (lines 25-41) does not currently have `IShortStringHelper`. The plan says to "add it" but doesn't account for the DI registration update in `UmbracoBuilder.cs`. + +**Actionable Fix:** +- Step 4 must also include updating `UmbracoBuilder.cs` to provide `IShortStringHelper` to `ContentCrudService` +- Add explicit verification step: run build after constructor change, before moving implementation + +### 2.4 Task 4: Field Analysis Inaccuracies + +**Description:** The "Fields Analysis" table contains multiple errors: + +| Plan Claim | Reality | +|------------|---------| +| `_documentBlueprintRepository` - "No (delegated)" | **Correct** - can remove | +| `_propertyValidationService` - "No" | **Correct** - can remove | +| `_cultureImpactFactory` - "No" | **Correct** - can remove | +| `_propertyEditorCollection` - "No" | **Correct** - can remove | +| `_contentSettings` - "No" | **Incorrect** - still referenced in line 168-172 for `optionsMonitor.OnChange` | +| `_relationService` - "No" | **Correct** - can remove | +| `_queryNotTrashed` - "Yes (GetAllPublished)" | **Correct** - used in `GetAllPublished` | +| `_documentRepository` - "Yes" | **Correct** - used in `DeleteOfTypes`, `MoveToRecycleBin`, helper methods | +| `_entityRepository` - "No" | **Correct** - can remove | +| `_contentTypeRepository` - "Yes" | **Correct** - used in `GetContentType`, `DeleteOfTypes` | +| `_languageRepository` - "No" | **Correct** - can remove | +| `_shortStringHelper` - "Yes (CheckDataIntegrity)" | Only IF CheckDataIntegrity is NOT extracted | + +**Why it matters:** Removing `_contentSettings` without also removing the `optionsMonitor.OnChange` callback will cause a null reference or leave dangling code. + +**Actionable Fix:** +- Update Task 4 to note that removing `_contentSettings` also requires removing the `optionsMonitor.OnChange` callback (lines 168-172) +- Keep `_shortStringHelper` ONLY if CheckDataIntegrity is not extracted; otherwise remove it + +### 2.5 Task 4: Proposed Constructor Has Wrong Parameter Count + +**Description:** The plan shows the simplified constructor with 15 parameters, but the current constructor has 24 parameters. The plan's proposed constructor is missing `optionsMonitor` for the `_contentSettings` field that the plan claims to remove but is actually still used. + +**Why it matters:** The proposed constructor won't compile or will leave the class in an inconsistent state. + +**Actionable Fix:** +- If `_contentSettings` is truly unused after refactoring, also remove the `optionsMonitor` parameter and the `OnChange` callback +- If `_contentSettings` IS used (e.g., by methods that remain in the facade), keep both + +### 2.6 Task 5: Obsolete Constructor Removal - Breaking Change Risk + +**Description:** The plan correctly identifies that removing obsolete constructors is a breaking change. However, it doesn't verify whether any external code (packages, user code) might be using these constructors. + +**Why it matters:** The `[Obsolete]` attribute includes "Scheduled removal in v19" which suggests this is a v18 codebase. Removing in v18 would be premature. + +**Actionable Fix:** +- Add a verification step to check if the current major version is v19 or if this is approved for early removal +- Consider keeping obsolete constructors until the documented removal version +- Or, if removal is approved, update the commit message to clearly indicate the breaking change version + +--- + +## 3. Minor Issues & Improvements + +### 3.1 Task 1 Code Snippet: Missing Null Check + +The plan's proposed `PerformMoveLocked` code uses `query?.Where(...)` but doesn't handle the case where `Query()` returns null. The existing implementation in `ContentMoveOperationService` handles this correctly. + +### 3.2 Task 6: GetAllPublished Analysis Incomplete + +The plan says to run `grep` to check if `GetAllPublished` is used externally. This method is `internal` (line 729 in ContentService), so external usage is unlikely but possible via `InternalsVisibleTo`. The plan should note that internal methods can still be used by test projects. + +### 3.3 Task 7: Line Count Target Unclear + +The plan says target is "~200 lines" but then accepts "~200-300 lines" as the expected outcome. These should be consistent. Given that orchestration methods `MoveToRecycleBin` (~44 lines) and `DeleteOfTypes` (~78 lines) will remain, plus constructor, fields, and delegation methods, 250-300 lines is more realistic. + +### 3.4 Documentation: Version Mismatch + +The plan references moving `IShortStringHelper` to `ContentCrudService` in Task 3, but the interface `IContentCrudService` doesn't have `CheckDataIntegrity` method. Either: +- Add the method to the interface (as stated in Task 3 Step 1), OR +- Keep `CheckDataIntegrity` as a facade-only method + +### 3.5 Commit Granularity + +Task 3 bundles constructor changes with method extraction. If the constructor change fails, the entire commit must be reverted. Consider splitting into two commits: +1. Add `IShortStringHelper` to `ContentCrudService` constructor +2. Extract `CheckDataIntegrity` implementation + +--- + +## 4. Questions for Clarification + +1. **Task 1-2 Duplication:** Were Tasks 1 and 2 written before Phases 4-7 were completed? The methods they propose to extract already exist in the target services. Should these tasks be: + - Skipped entirely? + - Rewritten to expose existing private methods on interfaces? + - Rewritten to remove duplicate code from ContentService? + +2. **Breaking Change Timeline:** The obsolete constructors are marked "Scheduled removal in v19." Is Phase 8 intended to be part of v19, or should removal be deferred? + +3. **`_contentSettings` Usage:** Is the `optionsMonitor.OnChange` callback (lines 168-172) still needed? If no remaining facade methods use `_contentSettings`, the callback can be removed. If any do, it must stay. + +4. **Interface vs. Internal Methods:** Should `PerformMoveLocked` and `DeleteLocked` be exposed on the public interfaces, or should `ContentService` call them via a different pattern (e.g., `internal` visibility)? + +--- + +## 5. Final Recommendation + +### **Major Revisions Needed** + +The plan cannot be executed as written due to the critical inaccuracies about the current state of the codebase. Before proceeding: + +1. **Verify current state of each target file** before writing extraction steps +2. **Update or skip Tasks 1-2** which propose extracting already-existing methods +3. **Correct the field analysis in Task 4** for `_contentSettings` +4. **Decide on obsolete constructor removal timing** relative to versioning +5. **Add DI registration updates** where constructor signatures change + +### Key Changes Required + +| Task | Required Action | +|------|-----------------| +| 1 | **Rewrite**: Make existing `PerformMoveLocked` public + add to interface, or skip entirely | +| 2 | **Skip or Rewrite**: `DeleteLocked` already exists in both services | +| 3 | **Add step**: Update `UmbracoBuilder.cs` for `IShortStringHelper` injection | +| 4 | **Correct**: `_contentSettings` is still used; handle `OnChange` callback | +| 5 | **Verify**: Confirm breaking change is acceptable for current version | +| 6 | **Correct**: `GetAllPublished` is internal, update grep command | +| 7 | **Correct**: Realistic line count is 250-300, not 200 | + +--- + +**End of Critical Review** diff --git a/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-2.md b/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-2.md new file mode 100644 index 0000000000..10e485b851 --- /dev/null +++ b/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-2.md @@ -0,0 +1,275 @@ +# Critical Implementation Review: Phase 8 - Facade Finalization (v2.0) + +**Plan File:** `2025-12-24-contentservice-refactor-phase8-implementation.md` +**Plan Version:** 2.0 +**Review Date:** 2025-12-24 +**Reviewer:** Critical Implementation Review System +**Review Version:** 2 + +--- + +## 1. Overall Assessment + +**Summary:** Version 2.0 of the plan successfully addresses the critical issues identified in Review 1. The fundamental approach is now correct: exposing existing private methods rather than re-extracting them. However, several **execution-level issues** remain that could cause implementation failures or leave the codebase in an inconsistent state. + +**Strengths:** +- Tasks 1-2 now correctly identify the pattern: make private methods public, add to interface, remove duplicates +- Field analysis table correctly identifies `_contentSettings` usage via `OnChange` callback +- Added DI registration verification steps +- Realistic line count target (250-300 instead of 200) +- Good version history tracking and change summary + +**Remaining Concerns:** +1. **Duplicate `DeleteLocked` in two services** - ambiguity about which to use +2. **Task execution order dependency** - Task 5 removes code that Task 4 also references +3. **Missing concrete DI registration update** for `IShortStringHelper` in ContentCrudService +4. **Incomplete interface exposure** - `PerformMoveLocked` parameters don't match Plan's Step 1 +5. **Potential null reference** in ContentService.MoveToRecycleBin after migration + +--- + +## 2. Critical Issues + +### 2.1 Duplicate `DeleteLocked` in Two Services (Architecture Ambiguity) + +**Description:** Both `ContentCrudService.DeleteLocked` (line 637) and `ContentMoveOperationService.DeleteLocked` (line 295) implement the same delete logic. The plan proposes exposing `ContentCrudService.DeleteLocked` for use by `ContentService.DeleteOfTypes`, but doesn't address the duplicate in `ContentMoveOperationService`. + +**Why it matters:** +- Two implementations of the same logic creates maintenance burden +- `ContentMoveOperationService.EmptyRecycleBin` (line 245) calls its own `DeleteLocked` +- If both are kept, future bug fixes must be applied twice + +**Current State:** +``` +ContentService.DeleteOfTypes → calls local DeleteLocked (simpler, no iteration bounds) +ContentService.DeleteLocked → simple version (lines 825-848) +ContentCrudService.DeleteLocked → robust version with iteration bounds (lines 637-692) +ContentMoveOperationService.DeleteLocked → robust version with iteration bounds (lines 295-348) +``` + +**Actionable Fix:** +Option A (Recommended): Have `ContentMoveOperationService.EmptyRecycleBin` call `IContentCrudService.DeleteLocked` instead of its own method. Remove the duplicate from `ContentMoveOperationService`. + +Option B: Document in the plan that two implementations are intentionally kept (rationale: different scoping requirements). Add a comment in code explaining this. + +### 2.2 Task Execution Order Creates Redundant Work + +**Description:** Task 4 removes the `optionsMonitor.OnChange` callback from "lines 168-172, 245-247, 328-330". However, lines 245-247 and 328-330 are in the **obsolete constructors** that Task 5 will remove entirely. + +**Why it matters:** +- Following Task 4 as written will edit code that Task 5 will delete +- Inefficient and potentially confusing during implementation +- Could cause merge conflicts if tasks are done by different people + +**Actionable Fix:** +Reorder or clarify: +- **Option A**: Swap Task 4 and Task 5 execution order - remove obsolete constructors first, then only one `OnChange` callback (line 168-172) needs removal +- **Option B**: Update Task 4 to only reference line 168-172, noting "The callbacks in obsolete constructors (lines 245-247, 328-330) will be removed with Task 5" + +### 2.3 Task 3: Missing Concrete DI Registration Change + +**Description:** Task 3 Step 4 says to "verify" the DI registration auto-resolves `IShortStringHelper`. However, `IShortStringHelper` registration might not be automatic if it's registered differently (e.g., factory method, named instance). + +**Why it matters:** +- Build may succeed but runtime DI resolution could fail +- The current ContentCrudService constructor doesn't take `IShortStringHelper` + +**Current State (ContentCrudService constructor):** +```csharp +public ContentCrudService( + ICoreScopeProvider provider, + ILoggerFactory loggerFactory, + IEventMessagesFactory eventMessagesFactory, + IDocumentRepository documentRepository, + IAuditService auditService, + IUserIdKeyResolver userIdKeyResolver, + IEntityRepository entityRepository, + IContentTypeRepository contentTypeRepository, + // ... other parameters ... +) +``` +`IShortStringHelper` is NOT currently a parameter. + +**Actionable Fix:** +Update Task 3 to include explicit steps: +1. Add `IShortStringHelper shortStringHelper` parameter to ContentCrudService constructor +2. Add private field `private readonly IShortStringHelper _shortStringHelper;` +3. Verify `IShortStringHelper` is registered in DI (search for `AddShortString` or similar in UmbracoBuilder) +4. Run integration test to verify runtime resolution + +### 2.4 Task 1 Interface Signature Mismatch + +**Description:** The plan's Step 1 proposes adding this signature to `IContentMoveOperationService`: +```csharp +void PerformMoveLocked(IContent content, int parentId, IContent? parent, int userId, ICollection<(IContent, string)> moves, bool? trash); +``` + +But the existing private method signature in `ContentMoveOperationService` (line 140) is: +```csharp +private void PerformMoveLocked(IContent content, int parentId, IContent? parent, int userId, ICollection<(IContent, string)> moves, bool? trash) +``` + +The signatures match, which is good. **However**, the `ICollection<(IContent, string)>` parameter type is a mutable collection passed by reference - this is an **internal implementation detail** being exposed on a public interface. + +**Why it matters:** +- Exposing `ICollection<(IContent, string)>` as a parameter on a public interface creates a leaky abstraction +- Callers must create and manage this collection, which is an implementation detail +- Future refactoring of the move tracking mechanism will be a breaking change + +**Actionable Fix:** +Consider whether `PerformMoveLocked` should be on the interface at all, or if it should remain internal. If it must be exposed: +- Add XML doc comment explaining the `moves` collection is mutated by the method +- Consider alternative signature that returns moves rather than mutating a passed collection: + ```csharp + IReadOnlyList<(IContent, string)> PerformMoveLocked(IContent content, int parentId, IContent? parent, int userId, bool? trash); + ``` +- Or use `internal` visibility instead of adding to public interface (via `InternalsVisibleTo`) + +### 2.5 Task 4: Potential Null Reference After Field Removal + +**Description:** After removing `_relationService` field, verify no remaining code in ContentService references it. The plan says it's "delegated to ContentMoveOperationService" but doesn't verify the delegation path. + +**Why it matters:** +- If any ContentService method still references `_relationService`, the build will fail +- More subtly, if the delegation doesn't cover all scenarios, runtime behavior changes + +**Current State Analysis:** +`_relationService` is used in `ContentMoveOperationService.EmptyRecycleBin` (line 239-242): +```csharp +if (_contentSettings.DisableDeleteWhenReferenced && + _relationService.IsRelated(content.Id, RelationDirectionFilter.Child)) +{ + continue; +} +``` + +This is correct - `ContentMoveOperationService` has its own `_relationService` field (line 34). + +**Verification Needed:** +Run: `grep -n "_relationService" src/Umbraco.Core/Services/ContentService.cs` +Expected: Only field declaration (to be removed) - no method body references. + +--- + +## 3. Minor Issues & Improvements + +### 3.1 Task 1 Step 4: Verify `MoveOperationService` Property Exists + +The plan assumes `ContentService` has a property or field called `MoveOperationService`. Looking at the code: +```csharp +private IContentMoveOperationService MoveOperationService => + _moveOperationService ?? _moveOperationServiceLazy?.Value + ?? throw new InvalidOperationException("MoveOperationService not initialized..."); +``` +This exists (line 98-100), so the plan is correct. Just noting for verification. + +### 3.2 Task 5 Step 3: Service Accessor Properties Simplification + +The plan proposes simplified accessors like: +```csharp +private IContentQueryOperationService QueryOperationService => + _queryOperationService ?? throw new InvalidOperationException("QueryOperationService not initialized."); +``` + +This requires removing the `_queryOperationServiceLazy` field as well, which means updating **all** the accessor properties consistently. The plan mentions removing "Lazy field initializers" but should list all: +- `_queryOperationServiceLazy` +- `_versionOperationServiceLazy` +- `_moveOperationServiceLazy` +- `_publishOperationServiceLazy` +- `_permissionManagerLazy` +- `_blueprintManagerLazy` +- `_crudServiceLazy` (keep this one - used by main constructor) + +### 3.3 Task 6: Internal Method Check Should Include Web Projects + +The plan checks for `GetAllPublished` usage in `src/` and `tests/`. However, `InternalsVisibleTo` might also expose it to other Umbraco projects. Consider also checking: +- `Umbraco.Web.Common` +- `Umbraco.Infrastructure` + +Run: `grep -rn "GetAllPublished" src/Umbraco.Infrastructure/ src/Umbraco.Web.Common/ --include="*.cs"` + +### 3.4 Task 7: Line Count Verification Method + +The plan uses `wc -l` which counts lines including blank lines and comments. For a more accurate "code lines" count: +```bash +grep -c "." src/Umbraco.Core/Services/ContentService.cs +``` +Or accept that 250-300 includes blanks/comments (which is fine for tracking purposes). + +### 3.5 Task 8 Step 3: Full Integration Test Duration + +Running `dotnet test tests/Umbraco.Tests.Integration` can take 10+ minutes. Consider adding a note about expected duration or using `--filter` to run critical paths first: +```bash +# Quick verification (2-3 min) +dotnet test tests/Umbraco.Tests.Integration --filter "Category=Quick" +# Full suite (10+ min) +dotnet test tests/Umbraco.Tests.Integration +``` + +### 3.6 Commit Atomicity for Task 3 + +Task 3 bundles: +1. Add method signature to interface +2. Add `IShortStringHelper` to constructor +3. Move implementation +4. Update ContentService delegation + +If the constructor change fails or tests break, the entire commit is reverted. Consider splitting: +- Commit 3a: Add `IShortStringHelper` to ContentCrudService (infrastructure change) +- Commit 3b: Extract `CheckDataIntegrity` to ContentCrudService (functional change) + +--- + +## 4. Questions for Clarification + +1. **DeleteLocked Unification**: Should `ContentMoveOperationService` call `IContentCrudService.DeleteLocked` instead of having its own implementation? This would reduce duplication but create a dependency from MoveOperationService to CrudService. + +2. **Interface Stability**: Is adding `PerformMoveLocked` to `IContentMoveOperationService` intended to be a permanent public API, or should it use `internal` visibility with `InternalsVisibleTo` for the facade? + +3. **Breaking Change Approval**: Has the removal of obsolete constructors (scheduled for v19) been approved for this phase? The plan adds a verification step but doesn't specify what to do if not approved. + +4. **`_crudServiceLazy` Retention**: The main constructor wraps `crudService` in a `Lazy<>`. Should this be simplified to direct assignment like the other services, or is the Lazy pattern intentional? + +5. **Test Coverage for Exposed Methods**: After exposing `PerformMoveLocked` and `DeleteLocked` on interfaces, should new unit tests be added for these methods specifically, or rely on existing integration tests? + +--- + +## 5. Final Recommendation + +### **Approve with Changes** + +Version 2.0 of the plan is fundamentally sound and addresses the critical review 1 findings. The remaining issues are **execution-level refinements** rather than architectural problems. + +### Required Changes Before Implementation + +| Priority | Issue | Resolution | +|----------|-------|------------| +| **High** | 2.1 - Duplicate DeleteLocked | Add decision to plan: unify or document intentional duplication | +| **High** | 2.3 - IShortStringHelper DI | Add explicit constructor modification steps | +| **Medium** | 2.2 - Task order redundancy | Swap Task 4 and 5, or update Task 4 to reference only line 168-172 | +| **Medium** | 2.4 - Interface signature | Add note about mutating collection parameter, or keep method internal | +| **Low** | 3.2 - Lazy field removal | List all Lazy fields to remove explicitly | + +### Implementation Checklist + +Before executing each task, verify: +- [ ] Target method/field exists at expected line numbers (re-check after each task as lines shift) +- [ ] All references to removed code have been updated +- [ ] Build succeeds after each step +- [ ] No new compilation warnings introduced + +### Suggested Execution Order (Updated) + +1. **Task 5** - Remove obsolete constructors first (cleans up code before other changes) +2. **Task 4** - Remove unused fields and simplify constructor (now only one OnChange to remove) +3. **Task 1** - Expose PerformMoveLocked +4. **Task 2** - Expose DeleteLocked (decide on unification first) +5. **Task 3** - Extract CheckDataIntegrity +6. **Task 6** - Clean up internal methods +7. **Task 7** - Verify line count +8. **Task 8** - Full test suite +9. **Task 9** - Update design document + +--- + +**End of Critical Review 2** diff --git a/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-3.md b/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-3.md new file mode 100644 index 0000000000..b442db3cd5 --- /dev/null +++ b/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-3.md @@ -0,0 +1,274 @@ +# Critical Implementation Review: ContentService Phase 8 Facade Finalization v3.0 + +**Review Date:** 2025-12-24 +**Reviewer:** Critical Implementation Review Process +**Plan Version Reviewed:** 3.0 +**Plan File:** `docs/plans/2025-12-24-contentservice-refactor-phase8-implementation.md` + +--- + +## 1. Overall Assessment + +**Summary:** The Phase 8 implementation plan is well-structured, detailed, and demonstrates significant improvement from prior reviews. Version 3.0 correctly addresses task ordering for efficiency and unifies duplicate DeleteLocked implementations. The plan shows strong understanding of the codebase dependencies and provides clear, actionable steps. + +**Strengths:** +- Task reordering (obsolete constructors first) is a smart optimization that eliminates redundant OnChange callback handling +- DeleteLocked unification removes duplicate code across ContentCrudService and ContentMoveOperationService +- Explicit listing of all Lazy fields to remove prevents oversights +- Interface documentation for mutable collection parameter addresses encapsulation concern transparently +- Verification steps after each major change provide safety gates +- Commit messages are well-formatted with clear change descriptions + +**Major Concerns:** +- Task 4 Step 6 contains an incorrect assertion about adding a dependency that already exists +- Some verification steps could benefit from additional boundary checks + +--- + +## 2. Critical Issues + +### 2.1 Task 4 Step 6: Incorrect Dependency Addition Assertion + +**Description:** Task 4 Step 6 states: "Add `IContentCrudService` as a constructor parameter to `ContentMoveOperationService`". However, `ContentMoveOperationService` **already has** `IContentCrudService` as a dependency. + +**Evidence from codebase:** +```csharp +// ContentMoveOperationService.cs +private readonly IContentCrudService _crudService; // Line 32 +// Constructor: +IContentCrudService crudService, // Line 46 +``` + +**Why it matters:** Following this step as written could lead to: +- Duplicate constructor parameters +- Confusion about what needs to be done +- Build errors if taken literally + +**Specific Fix:** Revise Task 4 Step 6 to: +```markdown +### Step 6: Unify ContentMoveOperationService.EmptyRecycleBin (v3.0 addition) + +ContentMoveOperationService **already** has IContentCrudService as a constructor parameter +(assigned to `_crudService` field). Update `EmptyRecycleBin` to call `IContentCrudService.DeleteLocked` +instead of its own local `DeleteLocked` method: + +1. In `EmptyRecycleBin`, replace: + ```csharp + // FROM: + DeleteLocked(scope, content, eventMessages); + // TO: + _crudService.DeleteLocked(scope, content, eventMessages); + ``` +2. Remove the local `DeleteLocked` method from `ContentMoveOperationService` (lines ~295-348) + +This eliminates duplicate implementations and ensures bug fixes only need to be applied once. +``` + +--- + +### 2.2 Task 3: PerformMoveLocked Interface Design Exposes Implementation Detail + +**Description:** The `PerformMoveLocked` method signature includes `ICollection<(IContent, string)> moves` - a mutable collection that callers must provide. While the documentation warns about this, exposing mutable collection parameters in a public interface violates encapsulation principles and makes the API harder to use correctly. + +**Why it matters:** +- Callers must understand the internal tracking mechanism +- The mutable parameter pattern is prone to misuse (passing wrong collection type, expecting immutability) +- Interface pollution - internal orchestration details leak into public contract + +**Recommended Fix Options:** + +**Option A (Preferred - Clean Interface):** Return the moves collection instead of mutating a parameter: +```csharp +/// +/// Performs the locked move operation for a content item and its descendants. +/// +/// Collection of moved items with their original paths. +IReadOnlyCollection<(IContent Content, string OriginalPath)> PerformMoveLocked( + IContent content, int parentId, IContent? parent, int userId, bool? trash); +``` + +**Option B (Minimal Change):** Keep internal method private and create a facade method in ContentService that manages the collection internally: +```csharp +// In ContentService.MoveToRecycleBin - don't expose internal collection management +private void PerformMoveToRecycleBinLocked(IContent content, int userId, ICollection<(IContent, string)> moves) +{ + MoveOperationService.PerformMoveLockedInternal(content, Constants.System.RecycleBinContent, null, userId, moves, true); +} +``` + +**If keeping current design:** Add an extension method or factory for creating the expected collection: +```csharp +// Add to IContentMoveOperationService or a helper class +ICollection<(IContent, string)> CreateMoveTrackingCollection() => new List<(IContent, string)>(); +``` + +--- + +### 2.3 Task 2 Step 1: Missing Verification of OnChange Callback Removal Impact + +**Description:** The plan removes the `optionsMonitor.OnChange` callback and `_contentSettings` field together. However, there's no verification step to ensure that removing the callback won't affect service behavior if `_contentSettings` is accessed via closure in any extracted services. + +**Why it matters:** If any of the extracted services were passed `_contentSettings` by reference or use it through a closure, removing the OnChange callback would prevent them from seeing configuration updates during runtime. + +**Specific Fix:** Add verification step before removal: +```markdown +### Step 1a: Verify _contentSettings is not shared with extracted services + +Check that no extracted services receive _contentSettings or depend on its live updates: + +Run: `grep -rn "_contentSettings" src/Umbraco.Core/Services/Content*.cs | grep -v ContentService.cs` + +Expected: No matches in ContentCrudService, ContentQueryOperationService, +ContentVersionOperationService, ContentMoveOperationService, ContentPublishOperationService, +ContentPermissionManager, or ContentBlueprintManager. + +If any matches found, those services must either: +- Inject IOptionsMonitor directly +- Or the callback must be preserved +``` + +--- + +## 3. Minor Issues & Improvements + +### 3.1 Task 5: CheckDataIntegrity Creates Artificial Content Object + +**Location:** Task 5 Step 5 + +**Issue:** The implementation creates a dummy Content object to publish a notification: +```csharp +var root = new Content("root", -1, new ContentType(_shortStringHelper, -1)) { Id = -1, Key = Guid.Empty }; +scope.Notifications.Publish(new ContentTreeChangeNotification(root, TreeChangeTypes.RefreshAll, EventMessagesFactory.Get())); +``` + +**Concern:** +- Using Id=-1 and Key=Guid.Empty could confuse logging or debugging +- Creating a ContentType just for notification feels heavyweight + +**Suggestion:** Consider using a dedicated marker constant or null content pattern if the notification system supports it, or document why this pattern is acceptable. This is minor since it's contained behavior. + +--- + +### 3.2 Task 6: Missing Check for Umbraco.Cms.Api.* Projects + +**Location:** Task 6 Step 1 and Step 3 + +**Issue:** The plan checks Infrastructure and Web.Common for internal method usage, but not the API projects which may also have InternalsVisibleTo access. + +**Fix:** Add to Step 1: +```bash +# Also check API projects +grep -rn "GetAllPublished" src/Umbraco.Cms.Api.Management/ src/Umbraco.Cms.Api.Delivery/ --include="*.cs" +``` + +--- + +### 3.3 Task 8: No Unit Test Updates for New Interface Methods + +**Issue:** When exposing `PerformMoveLocked` and `DeleteLocked` as public interface methods, no unit tests are mentioned for the new public signatures. + +**Recommendation:** Add a step to Task 8: +```markdown +### Step 2a: Add unit tests for newly exposed interface methods + +Create or update unit tests to cover: +- IContentMoveOperationService.PerformMoveLocked (ensure delegation works correctly) +- IContentCrudService.DeleteLocked (ensure it handles edge cases: empty tree, large tree, null content) +``` + +--- + +### 3.4 Task 1 Step 2: Line Number References May Be Stale + +**Location:** Task 1 Step 2 + +**Issue:** References to specific line numbers (210-289, 291-369) can become stale if any prior changes shift line positions. + +**Fix:** Use method signatures or searchable patterns instead: +```markdown +### Step 2: Remove obsolete constructors + +Delete both constructors marked with `[Obsolete("Use the non-obsolete constructor instead. Scheduled removal in v19.")]`: +- First: The one with `IAuditRepository auditRepository` parameter +- Second: The one without the Phase 2-7 service parameters + +Search pattern: `[Obsolete("Use the non-obsolete constructor instead. Scheduled removal in v19.")]` +``` + +--- + +### 3.5 Task 7 Step 1: Line Count Verification Could Be More Specific + +**Location:** Task 7 Step 1 + +**Issue:** The expected range "~250-300 lines" is quite broad. A more specific target based on actual expected removals would be helpful. + +**Calculation:** +- Current: 1330 lines +- Obsolete constructors: ~160 lines +- Lazy fields and duplicate properties: ~40 lines +- Duplicate methods (PerformMoveLocked, DeleteLocked, etc.): ~100 lines +- Field declarations removal: ~10 lines +- Internal method cleanup: ~30 lines +- **Total removal:** ~340 lines +- **Expected result:** ~990 lines, not 250-300 + +**Concern:** The 250-300 target seems to assume more aggressive removal than the plan details. Either: +1. The plan is missing significant removal steps, or +2. The target is incorrect + +**Recommendation:** Recalculate expected line count based on actual removal steps, or clarify if additional cleanup beyond what's documented is expected. + +--- + +## 4. Questions for Clarification + +### Q1: Breaking Change Version Confirmation +Task 1 Step 1 asks to verify if v19 removal is acceptable. What is the current version, and is there a policy document or issue tracker reference for breaking change approvals? + +### Q2: _queryNotTrashed Field Disposition +The Current State Analysis mentions `_queryNotTrashed` is "Used in `GetAllPublished`" and action is "Keep or move". Task 6 mentions possibly removing `GetAllPublished`. If GetAllPublished is removed, should `_queryNotTrashed` also be removed? This needs explicit resolution. + +### Q3: DeleteLocked Iteration Bound Difference +ContentCrudService.DeleteLocked uses: +```csharp +const int maxIterations = 10000; +``` + +ContentMoveOperationService.DeleteLocked uses: +```csharp +MaxDeleteIterations // Class-level constant +``` + +When unifying, which value should be canonical? Are they the same? If different, which behavior is preferred? + +--- + +## 5. Final Recommendation + +**APPROVE WITH CHANGES** + +The plan is well-conceived and v3.0 represents significant improvement. However, the following changes are required before implementation: + +### Required Changes (Must Fix): +1. **Fix Task 4 Step 6:** Remove the incorrect instruction to add IContentCrudService dependency - it already exists. Update to simply redirect EmptyRecycleBin to use `_crudService.DeleteLocked()`. + +2. **Recalculate Task 7 line count target:** The 250-300 line target doesn't match the ~340 lines of removal documented. Either add missing removal steps or correct the target to ~990 lines. + +3. **Add Task 2 Step 1a verification:** Verify that `_contentSettings` isn't shared with extracted services before removing the OnChange callback. + +### Recommended Changes (Should Fix): +4. Consider returning moves collection from PerformMoveLocked instead of mutating a parameter (Option A in issue 2.2). + +5. Add unit test step to Task 8 for newly exposed interface methods. + +6. Add API project checks to Task 6 internal method verification. + +### Optional Improvements: +7. Use method signatures instead of line numbers for obsolete constructor removal. + +8. Resolve _queryNotTrashed disposition explicitly. + +--- + +**End of Critical Implementation Review 3** diff --git a/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-4.md b/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-4.md new file mode 100644 index 0000000000..ae025282bd --- /dev/null +++ b/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-4.md @@ -0,0 +1,400 @@ +# Critical Implementation Review: ContentService Phase 8 Facade Finalization v4.0 + +**Review Date:** 2025-12-24 +**Reviewer:** Critical Implementation Review Process +**Plan Version Reviewed:** 4.0 +**Plan File:** `docs/plans/2025-12-24-contentservice-refactor-phase8-implementation.md` + +--- + +## 1. Overall Assessment + +**Summary:** The Phase 8 implementation plan v4.0 is mature, well-documented, and addresses all prior review feedback comprehensively. The plan demonstrates strong understanding of the codebase, provides clear step-by-step instructions, and includes appropriate verification gates. Version 4.0 correctly addresses the PerformMoveLocked return type improvement, fixes the Task 4 Step 6 dependency issue, and recalculates the line count target accurately. + +**Strengths:** +- Comprehensive version history with change tracking across all 4 versions +- Task reordering optimization (obsolete constructors first) reduces redundant work +- DeleteLocked unification eliminates duplicate implementations across two services +- PerformMoveLocked now returns `IReadOnlyCollection` instead of mutating a parameter (Option A) +- Explicit verification steps including `_contentSettings` shared dependency check +- Well-formed commit messages with BREAKING CHANGE notation +- Accurate line count calculation (~990 lines target from 1330 - 340 removal) +- API project checks added for internal method verification (v4.0) +- Unit test step added for newly exposed interface methods (v4.0) + +**Major Concerns:** +- **Critical:** `DeleteOfTypes` method also uses both `PerformMoveLocked` and `DeleteLocked` but is not updated in the plan - will cause compilation failure +- One implementation gap: the wrapper pattern for PerformMoveLocked needs internal caller updates +- One missing safety check for DeleteLocked unification (constant value verification) + +--- + +## 2. Critical Issues + +### 2.1 Task 3 Step 2: PerformMoveLocked Internal Method Rename Creates Signature Mismatch Risk + +**Description:** Task 3 Step 2 proposes renaming the existing private method to `PerformMoveLockedInternal`: + +```csharp +// Rename existing private method to: +private void PerformMoveLockedInternal(IContent content, int parentId, IContent? parent, int userId, ICollection<(IContent, string)> moves, bool? trash) +``` + +However, this method is called from multiple places within `ContentMoveOperationService`: +- `Move()` method (line ~120) +- `PerformMoveLockedInternal` must also update any internal callers + +**Evidence from codebase:** +```csharp +// ContentMoveOperationService.cs line 140 (current): +private void PerformMoveLocked(IContent content, int parentId, IContent? parent, int userId, ICollection<(IContent, string)> moves, bool? trash) +``` + +This method is called from `Move()`: +```csharp +// Line ~120 in Move() +PerformMoveLocked(content, parentId, parent, userId, moves, trash); +``` + +**Why it matters:** +- If the rename is done without updating internal callers, the build will fail +- The plan doesn't explicitly mention updating these internal call sites + +**Specific Fix:** Add explicit step after rename: +```markdown +### Step 2a: Update internal callers to use renamed method + +After renaming to `PerformMoveLockedInternal`, update all internal call sites: + +1. In `Move()` method, update: + ```csharp + // FROM: + PerformMoveLocked(content, parentId, parent, userId, moves, trash); + // TO: + PerformMoveLockedInternal(content, parentId, parent, userId, moves, trash); + ``` + +Run grep to find all internal callers: +```bash +grep -n "PerformMoveLocked" src/Umbraco.Core/Services/ContentMoveOperationService.cs +``` +``` + +--- + +### 2.2 Task 4 Step 6: Missing Verification That DeleteLocked Implementations Are Semantically Identical + +**Description:** The plan unifies `ContentMoveOperationService.DeleteLocked` with `ContentCrudService.DeleteLocked`. Both implementations appear similar but have subtle differences that could cause behavioral changes. + +**Evidence from codebase comparison:** + +**ContentCrudService.DeleteLocked (line 637):** +```csharp +const int pageSize = 500; +const int maxIterations = 10000; +// Uses GetPagedDescendantsLocked (internal method) +``` + +**ContentMoveOperationService.DeleteLocked (line 295):** +```csharp +// Uses MaxDeleteIterations (class constant) and DefaultPageSize (class constant) +// Uses GetPagedDescendantsLocked (internal method) +``` + +**Why it matters:** +- If `MaxDeleteIterations` or `DefaultPageSize` differ from `10000` and `500`, behavior changes +- Need to verify constant values match before unification + +**Specific Fix:** Add verification step to Task 4: +```markdown +### Step 5a: Verify DeleteLocked constant values match + +Before unification, verify both implementations use equivalent values: + +```bash +# Check ContentCrudService constants +grep -n "pageSize = \|maxIterations = " src/Umbraco.Core/Services/ContentCrudService.cs + +# Check ContentMoveOperationService constants +grep -n "MaxDeleteIterations\|DefaultPageSize" src/Umbraco.Core/Services/ContentMoveOperationService.cs +``` + +Expected: pageSize = 500 and maxIterations = 10000 in both. + +If values differ, document the change in the commit message and update tests if behavior changes. +``` + +--- + +### 2.3 Task 3: Missing Update for DeleteOfTypes Method (Uses Both PerformMoveLocked AND DeleteLocked) + +**Description:** The `DeleteOfTypes` method in ContentService (lines 1154-1231) uses both methods being refactored: + +```csharp +// Line 1207 +PerformMoveLocked(child, Constants.System.RecycleBinContent, null, userId, moves, true); + +// Line 1213 +DeleteLocked(scope, content, eventMessages); +``` + +**Why it matters:** +1. When `PerformMoveLocked` signature changes from `ICollection<> moves` parameter to returning `IReadOnlyCollection<>`, `DeleteOfTypes` **will fail to compile** because it passes a `moves` list that it manages locally. +2. The plan only mentions updating `MoveToRecycleBin` in Task 3 Step 4, not `DeleteOfTypes`. +3. This is a compilation-breaking omission. + +**Evidence from plan:** Task 3 Step 4 says: +> "Replace the `PerformMoveLocked` call in ContentService with delegation." + +But only shows the `MoveToRecycleBin` update, not `DeleteOfTypes`. + +**Specific Fix:** Add step to Task 3: +```markdown +### Step 4a: Update ContentService.DeleteOfTypes to use new signature + +The `DeleteOfTypes` method also calls `PerformMoveLocked`. Update the loop to use the new return signature: + +```csharp +// In DeleteOfTypes, replace the loop (lines ~1200-1209): +foreach (IContent child in children) +{ + // OLD: + // PerformMoveLocked(child, Constants.System.RecycleBinContent, null, userId, moves, true); + + // NEW: + var childMoves = MoveOperationService.PerformMoveLocked(child, Constants.System.RecycleBinContent, null, userId, true); + foreach (var move in childMoves) + { + moves.Add(move); // Aggregate into the overall moves list + } + changes.Add(new TreeChange(content, TreeChangeTypes.RefreshBranch)); +} +``` + +Also update the `DeleteLocked` call: +```csharp +// OLD: +DeleteLocked(scope, content, eventMessages); +// NEW: +CrudService.DeleteLocked(scope, content, eventMessages); +``` +``` + +**Alternative approach:** If `DeleteOfTypes` orchestration is complex, consider keeping the internal `PerformMoveLockedInternal` method callable from ContentService (would require making it internal, not private). + +--- + +## 3. Minor Issues & Improvements + +### 3.1 Task 3 Step 4: MoveToRecycleBin Update Incomplete + +**Location:** Task 3 Step 4 + +**Issue:** The plan shows updating the variable assignment but the existing `MoveToRecycleBin` code has additional logic using the `moves` collection that must be preserved: + +**Current code (ContentService line 907-918):** +```csharp +PerformMoveLocked(content, Constants.System.RecycleBinContent, null, userId, moves, true); +scope.Notifications.Publish(new ContentTreeChangeNotification(...)); + +MoveToRecycleBinEventInfo[] moveInfo = moves + .Select(x => new MoveToRecycleBinEventInfo(x.Item1, x.Item2)) + .ToArray(); + +scope.Notifications.Publish(new ContentMovedToRecycleBinNotification(moveInfo, eventMessages)...); +``` + +**Concern:** The plan's new signature returns `IReadOnlyCollection<(IContent Content, string OriginalPath)>` which uses named tuple elements. The existing code uses `x.Item1` and `x.Item2`. While compatible, explicit naming would be cleaner. + +**Suggestion:** Enhance Step 4 to show complete code update: +```csharp +// Replace: +// var moves = new List<(IContent, string)>(); +// PerformMoveLocked(content, Constants.System.RecycleBinContent, null, userId, moves, true); + +// With: +var moves = MoveOperationService.PerformMoveLocked(content, Constants.System.RecycleBinContent, null, userId, true); + +// The rest of the code using 'moves' works as-is since tuple destructuring is compatible +``` + +--- + +### 3.2 Task 1 Step 3: Lazy Field List May Be Incomplete + +**Location:** Task 1 Step 3 + +**Issue:** The plan lists 6 Lazy fields to remove but the code shows there are corresponding nullable non-lazy fields too: + +```csharp +private readonly IContentQueryOperationService? _queryOperationService; +private readonly Lazy? _queryOperationServiceLazy; +``` + +**Concern:** Removing only the Lazy fields but keeping the nullable service fields could leave dead code if those fields are only populated via the obsolete constructors. + +**Suggestion:** Add clarification: +```markdown +### Step 3: Remove Lazy field declarations (v3.0 explicit list) + +Remove these Lazy fields that are no longer needed: +- `_queryOperationServiceLazy` +- `_versionOperationServiceLazy` +- `_moveOperationServiceLazy` +- `_publishOperationServiceLazy` +- `_permissionManagerLazy` +- `_blueprintManagerLazy` + +**Note:** Keep the non-lazy versions (`_queryOperationService`, `_versionOperationService`, etc.) +as they are populated by the main constructor. Only the Lazy variants are removed. +Also keep `_crudServiceLazy` - it is used by the main constructor. +``` + +--- + +### 3.3 Task 6: GetAllPublished Method Not Found in Current Codebase + +**Location:** Task 6 Step 1 + +**Issue:** The plan references checking usage of `GetAllPublished`, but the grep search shows this method only exists in `ContentService.cs`. Let me verify if it actually exists: + +**Evidence:** My grep for `GetAllPublished` found only 1 file: `ContentService.cs`. However, when I read the file, I didn't see this method. The `_queryNotTrashed` field exists but `GetAllPublished` may have already been removed or never existed. + +**Suggestion:** Add fallback handling: +```markdown +### Step 1: Check GetAllPublished usage (v4.0 expanded) + +First, verify if GetAllPublished exists: +```bash +grep -n "GetAllPublished" src/Umbraco.Core/Services/ContentService.cs +``` + +If no matches found, the method has already been removed. Proceed to verify `_queryNotTrashed` usage only. + +If matches found, continue with the usage check... +``` + +--- + +### 3.4 Task 5 Step 3: Constructor Parameter Order Matters + +**Location:** Task 5 Step 3 + +**Issue:** When adding `IShortStringHelper shortStringHelper` to `ContentCrudService` constructor, parameter order affects DI resolution for positional construction. The plan shows it at the end, which is correct, but doesn't mention verifying existing factory registrations. + +**Suggestion:** Add verification step: +```markdown +### Step 3a: Verify ContentCrudService DI registration pattern + +Check if ContentCrudService is registered with explicit factory or auto-resolution: + +```bash +grep -n "ContentCrudService" src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +``` + +If using auto-resolution (`AddUnique()`), parameter order +doesn't matter - DI will resolve by type. If using explicit factory, update the factory registration. +``` + +--- + +### 3.5 Task 8 Step 2a: Test File Location May Need Creation + +**Location:** Task 8 Step 2a + +**Issue:** The plan says to add tests to `ContentServiceRefactoringTests.cs` but doesn't check if this file exists or if tests should go elsewhere. + +**Suggestion:** Enhance step: +```markdown +### Step 2a: Add unit tests for newly exposed interface methods (v4.0 addition) + +First, verify test file exists: +```bash +ls tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentServiceRefactoringTests.cs +``` + +If file doesn't exist, create it or add tests to the most appropriate existing test file +(e.g., `ContentServiceTests.cs`). + +Create or update unit tests to cover... +``` + +--- + +## 4. Questions for Clarification + +### Q1: ContentMoveOperationService.Move() Method Call Pattern + +The `Move()` method in `ContentMoveOperationService` currently creates its own `moves` list and calls `PerformMoveLocked`. After the refactoring, should it: +- A) Continue using the internal `PerformMoveLockedInternal` method (keeps current behavior) +- B) Call the new public `PerformMoveLocked` method (uses new interface) + +Option A seems implied by the plan but should be explicit. This affects whether internal moves are tracked via the new return type or the old mutation pattern. + +### Q2: DeleteOfTypes Update Pattern Confirmation + +The critical issue 2.3 identifies that `DeleteOfTypes` must be updated. The suggested pattern aggregates child moves into the overall moves list: + +```csharp +var childMoves = MoveOperationService.PerformMoveLocked(...); +foreach (var move in childMoves) +{ + moves.Add(move); +} +``` + +Is this aggregation pattern acceptable, or should `DeleteOfTypes` be refactored to use a different approach (e.g., collecting all moves first, then processing)? + +--- + +## 5. Final Recommendation + +**APPROVE WITH CHANGES** + +The plan is comprehensive and well-structured. The v4.0 updates address most concerns from prior reviews. However, one critical issue remains that will cause compilation failure if not addressed. + +### Required Changes (Must Fix): + +1. **Task 3 Step 4a (NEW - CRITICAL):** Add explicit step to update `DeleteOfTypes` method which also calls `PerformMoveLocked` and `DeleteLocked`. Without this update, the build will fail after removing the local methods. + +2. **Task 3 Step 2a (NEW):** Add explicit step to update internal callers of `PerformMoveLocked` when renaming to `PerformMoveLockedInternal`. + +3. **Task 4 Step 5a (NEW):** Add verification that `DeleteLocked` constant values (`maxIterations`, `pageSize`) match between ContentCrudService and ContentMoveOperationService before unification. + +### Recommended Changes (Should Fix): + +4. Enhance Task 3 Step 4 to show complete `MoveToRecycleBin` update pattern. + +5. Clarify in Task 1 Step 3 that non-lazy service fields (`_queryOperationService`, etc.) are kept. + +6. Add fallback handling in Task 6 for case where `GetAllPublished` doesn't exist. + +### Implementation Notes: + +- The line count target of ~990 lines is correctly calculated and realistic +- The task ordering (obsolete constructors first) is optimal +- The breaking change versioning (v19) is clearly documented +- Commit messages are well-structured with appropriate footers + +--- + +## Appendix: Codebase Verification Summary + +Verified during review: + +| Item | Expected | Actual | Status | +|------|----------|--------|--------| +| ContentService line count | 1330 | 1330 | | +| ContentMoveOperationService has `_crudService` | Yes | Line 32 | | +| `PerformMoveLocked` in ContentMoveOperationService | Private, line 140 | | | +| `PerformMoveLocked` in ContentService | Private, line 950 | | | +| `DeleteLocked` in ContentCrudService | Private, line 637 | | | +| `DeleteLocked` in ContentMoveOperationService | Private, line 295 | | | +| `DeleteLocked` in ContentService | Private, line 825 | | | +| `GetPagedDescendantQuery` duplicated | Yes | ContentService:671, MoveOp:591 | | +| `GetPagedLocked` duplicated | Yes | ContentService:682, MoveOp:602 | | +| 2 obsolete constructors exist | Yes | Lines 210-289, 291-369 | | + +--- + +**End of Critical Implementation Review 4** diff --git a/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation.md b/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation.md new file mode 100644 index 0000000000..a05bdee69b --- /dev/null +++ b/docs/plans/2025-12-24-contentservice-refactor-phase8-implementation.md @@ -0,0 +1,1101 @@ +# ContentService Phase 8: Facade Finalization Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Version:** 4.0 (Updated based on Critical Review 3) +**Last Updated:** 2025-12-24 +**Change Summary:** Fixed Task 4 Step 6 (IContentCrudService already exists); corrected line count target (~990 lines); added _contentSettings verification step; changed PerformMoveLocked to return collection (Option A); added unit tests for new interface methods; added API project checks; use method signatures instead of line numbers; resolved _queryNotTrashed disposition. + +--- + +**Goal:** Finalize the ContentService refactoring by cleaning up the facade to ~990 lines (from 1330), removing dead code, simplifying constructor dependencies, and running full validation. + +**Architecture:** ContentService becomes a thin facade delegating to extracted services (ContentCrudService, ContentQueryOperationService, ContentVersionOperationService, ContentMoveOperationService, ContentPublishOperationService) and managers (ContentPermissionManager, ContentBlueprintManager). Remaining orchestration methods (MoveToRecycleBin, DeleteOfTypes) stay in the facade. + +**Tech Stack:** C# 12, .NET 10.0, Umbraco CMS patterns, xUnit integration tests + +--- + +## Version History + +| Version | Date | Changes | +|---------|------|---------| +| 1.0 | 2025-12-24 | Initial plan | +| 2.0 | 2025-12-24 | Updated based on Critical Review 1 - see change summary | +| 3.0 | 2025-12-24 | Updated based on Critical Review 2 - reordered tasks, unified DeleteLocked, explicit constructor steps | +| 4.0 | 2025-12-24 | Updated based on Critical Review 3 - fixed Task 4 Step 6, corrected line count, PerformMoveLocked returns collection | + +--- + +## Current State Analysis + +### ContentService Metrics +- **Current lines**: 1330 +- **Target lines**: ~990 (based on calculated removal of ~340 lines) +- **Lines to remove**: ~340 (obsolete constructors ~160, Lazy fields ~40, duplicate methods ~100, field declarations ~10, internal methods ~30) + +### Key Discovery from Critical Review + +**Methods already exist in target services (as private):** +- `ContentMoveOperationService.PerformMoveLocked` (line 140) - private +- `ContentMoveOperationService.PerformMoveContentLocked` (line 186) - private +- `ContentMoveOperationService.GetPagedDescendantQuery` (line 591) - private +- `ContentMoveOperationService.GetPagedLocked` (line 602) - private +- `ContentCrudService.DeleteLocked` (line 637) - private + +**Duplicate methods still in ContentService:** +- `ContentService.PerformMoveLocked` (line 950) - duplicate +- `ContentService.PerformMoveContentLocked` (line 1002) - duplicate +- `ContentService.DeleteLocked` (line 825) - duplicate +- `ContentService.GetPagedDescendantQuery` (line 671) - duplicate +- `ContentService.GetPagedLocked` (line 682) - duplicate + +**Action:** Expose existing service methods on interfaces, then remove ContentService duplicates. + +### Methods Already Delegating (Keep as-is) +All CRUD, Query, Version, Move, Publish, Permission, and Blueprint methods are already one-liners delegating to extracted services. + +### Methods With Implementation Logic (Analyze for cleanup/extraction) +| Method | Lines | Status | Action | +|--------|-------|--------|--------| +| `MoveToRecycleBin` | 44 | Orchestration | Keep (unpublish+move coordination) | +| `PerformMoveLocked` | 50 | Duplicate | **Remove** - use MoveOperationService version | +| `PerformMoveContentLocked` | 10 | Duplicate | **Remove** - use MoveOperationService version | +| `DeleteLocked` | 24 | Duplicate | **Remove** - use CrudService version | +| `DeleteOfTypes/DeleteOfType` | 80 | Orchestration | Keep (descendant handling) | +| `GetPagedLocked` | 18 | Duplicate | **Remove** - use MoveOperationService version | +| `GetPagedDescendantQuery` | 10 | Duplicate | **Remove** - use MoveOperationService version | +| `CheckDataIntegrity` | 20 | Feature | Delegate to new method in ContentCrudService | +| `GetContentType` | 40 | Helper | Already used internally, keep minimal | +| `GetAllPublished` | 7 | Internal | Remove or inline | +| `Audit/AuditAsync` | 15 | Helper | Already used by services, can remove | +| Obsolete constructors | 160 | Legacy | Remove (per design) | + +### Fields Analysis (CORRECTED from v1.0) +| Field | Still Used | Reason | Action | +|-------|------------|--------|--------| +| `_documentBlueprintRepository` | No | Delegated to BlueprintManager | Remove | +| `_propertyValidationService` | No | Delegated to extracted services | Remove | +| `_cultureImpactFactory` | No | Delegated to extracted services | Remove | +| `_propertyEditorCollection` | No | Delegated to extracted services | Remove | +| `_contentSettings` | **Yes** | Used via `optionsMonitor.OnChange` (lines 168-172) | **Keep OR remove with callback** | +| `_relationService` | No | Delegated to ContentMoveOperationService | Remove | +| `_queryNotTrashed` | Yes | Used in `GetAllPublished` | **Remove with GetAllPublished** (if GetAllPublished is removed in Task 6) | +| `_documentRepository` | Yes | Used in DeleteOfTypes, GetContentType | Keep | +| `_entityRepository` | No | Unused | Remove | +| `_contentTypeRepository` | Yes | Used in GetContentType, DeleteOfTypes | Keep | +| `_languageRepository` | No | Delegated to extracted services | Remove | +| `_shortStringHelper` | Yes | Used in CheckDataIntegrity | Keep until CheckDataIntegrity extracted | + +**v2.0 Correction:** `_contentSettings` is used by the `optionsMonitor.OnChange` callback. If no remaining facade methods use `_contentSettings`, the callback AND the field must be removed together. + +--- + +## Execution Order (v3.0 Update) + +**Previous order:** 1 → 2 → 3 → 4 → 5 → 6 → 7 → 8 → 9 + +**New order:** 5 → 4 → 1 → 2 → 3 → 6 → 7 → 8 → 9 + +**Rationale:** Removing obsolete constructors first (old Task 5) eliminates the OnChange callbacks in those constructors, so Task 4 only needs to handle one callback location (line 168-172) instead of three. This reduces redundant work and potential merge conflicts. + +**Renumbered Tasks:** +| New # | Old # | Description | +|-------|-------|-------------| +| 1 | 5 | Remove Obsolete Constructors | +| 2 | 4 | Remove Unused Fields and Simplify Constructor | +| 3 | 1 | Expose PerformMoveLocked on IContentMoveOperationService | +| 4 | 2 | Expose DeleteLocked on IContentCrudService + Unify Implementations | +| 5 | 3 | Extract CheckDataIntegrity to ContentCrudService | +| 6 | 6 | Clean Up Remaining Internal Methods | +| 7 | 7 | Verify Line Count and Final Cleanup | +| 8 | 8 | Run Full Test Suite (Phase Gate) | +| 9 | 9 | Update Design Document | + +--- + +## Task 1: Remove Obsolete Constructors (was Task 5) + +**Goal:** Remove backward-compatibility constructors and simplify service accessor properties. This must be done first to eliminate duplicate OnChange callbacks. + +**Files:** +- Modify: `src/Umbraco.Core/Services/ContentService.cs` + +### Step 1: Verify breaking change is acceptable + +The obsolete constructors are marked "Scheduled removal in v19". Verify this is acceptable: + +1. Check if current version is v19 or if early removal is approved +2. Review the obsolete message text + +If removal is NOT approved for current version, **skip this task** and keep obsolete constructors. + +### Step 2: Remove obsolete constructors + +Delete both constructors marked with `[Obsolete("Use the non-obsolete constructor instead. Scheduled removal in v19.")]`: + +**Finding the constructors:** +```bash +grep -n 'Obsolete.*Scheduled removal in v19' src/Umbraco.Core/Services/ContentService.cs +``` + +**Identification:** +- First obsolete constructor: The one with `IAuditRepository auditRepository` parameter (legacy signature) +- Second obsolete constructor: The one without the Phase 2-7 service parameters (intermediate signature) + +Delete both constructors entirely, including their `[Obsolete]` attributes and method bodies. + +### Step 3: Remove Lazy field declarations (v3.0 explicit list) + +Remove these Lazy fields that are no longer needed: +- `_queryOperationServiceLazy` +- `_versionOperationServiceLazy` +- `_moveOperationServiceLazy` +- `_publishOperationServiceLazy` +- `_permissionManagerLazy` +- `_blueprintManagerLazy` + +**Note:** Keep `_crudServiceLazy` - it is used by the main constructor. + +### Step 4: Simplify service accessor properties + +Update the service accessor properties to remove null checks for lazy fields: + +```csharp +private IContentQueryOperationService QueryOperationService => + _queryOperationService ?? throw new InvalidOperationException("QueryOperationService not initialized."); + +private IContentVersionOperationService VersionOperationService => + _versionOperationService ?? throw new InvalidOperationException("VersionOperationService not initialized."); + +private IContentMoveOperationService MoveOperationService => + _moveOperationService ?? throw new InvalidOperationException("MoveOperationService not initialized."); + +private IContentPublishOperationService PublishOperationService => + _publishOperationService ?? throw new InvalidOperationException("PublishOperationService not initialized."); + +private ContentPermissionManager PermissionManager => + _permissionManager ?? throw new InvalidOperationException("PermissionManager not initialized."); + +private ContentBlueprintManager BlueprintManager => + _blueprintManager ?? throw new InvalidOperationException("BlueprintManager not initialized."); +``` + +### Step 5: Run build to verify compilation + +Run: `dotnet build src/Umbraco.Core/Umbraco.Core.csproj` +Expected: Build succeeds + +### Step 6: Run tests to verify functionality + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentService"` +Expected: All tests pass + +### Step 7: Commit + +```bash +git add src/Umbraco.Core/Services/ContentService.cs +git commit -m "$(cat <<'EOF' +refactor(core): remove obsolete constructors from ContentService + +Remove backward-compatibility constructors that used StaticServiceProvider +for lazy resolution. All dependencies are now injected directly through +the main constructor. + +Remove Lazy fields no longer needed: +- _queryOperationServiceLazy +- _versionOperationServiceLazy +- _moveOperationServiceLazy +- _publishOperationServiceLazy +- _permissionManagerLazy +- _blueprintManagerLazy + +BREAKING CHANGE: External code using obsolete constructors must update +to use dependency injection. + +🤖 Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 2: Remove Unused Fields and Simplify Constructor (was Task 4) + +**Goal:** Remove fields that are no longer used after service extraction and simplify the constructor. Only one OnChange callback location (line 168-172) needs removal now that obsolete constructors are gone. + +**Files:** +- Modify: `src/Umbraco.Core/Services/ContentService.cs` +- Modify: `src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs` + +### Step 1: Verify _contentSettings usage (v3.0 simplified) + +After Task 1, only one OnChange callback remains (line 168-172). Check if any remaining ContentService methods use `_contentSettings`: + +Run: `grep -n "_contentSettings" src/Umbraco.Core/Services/ContentService.cs` + +Expected findings: +- Field declaration +- optionsMonitor.OnChange callback (line 168-172 only) +- No actual usage in remaining methods + +**Decision:** Remove BOTH the field AND the optionsMonitor.OnChange callback together. + +### Step 1a: Verify _contentSettings is not shared with extracted services (v4.0 addition) + +Check that no extracted services receive _contentSettings or depend on its live updates: + +```bash +grep -rn "_contentSettings" src/Umbraco.Core/Services/Content*.cs | grep -v ContentService.cs +``` + +Expected: No matches in ContentCrudService, ContentQueryOperationService, ContentVersionOperationService, ContentMoveOperationService, ContentPublishOperationService, ContentPermissionManager, or ContentBlueprintManager. + +If any matches found, those services must either: +- Inject `IOptionsMonitor` directly +- Or the callback must be preserved in ContentService + +### Step 2: Verify _relationService is not referenced (v3.0 addition) + +Run: `grep -n "_relationService" src/Umbraco.Core/Services/ContentService.cs` + +Expected: Only field declaration (to be removed) - no method body references. If any method body references exist, investigate before removal. + +### Step 3: Identify fields to remove + +Remove these fields that are no longer used directly by ContentService: +- `_documentBlueprintRepository` (delegated to BlueprintManager) +- `_propertyValidationService` (delegated to extracted services) +- `_cultureImpactFactory` (delegated to extracted services) +- `_propertyEditorCollection` (delegated to extracted services) +- `_contentSettings` AND `optionsMonitor.OnChange` callback +- `_relationService` (delegated to ContentMoveOperationService) +- `_entityRepository` (not used) +- `_languageRepository` (delegated to extracted services) +- `_shortStringHelper` (will be moved to ContentCrudService in Task 5) + +Keep: +- `_documentRepository` (still used in DeleteOfTypes) +- `_contentTypeRepository` (still used in GetContentType, DeleteOfTypes) +- `_auditService` (still used in DeleteOfTypes, MoveToRecycleBin) +- `_idKeyMap` (still used in TryGetParentKey) +- `_userIdKeyResolver` (still used in AuditAsync) +- `_logger` (always useful) + +### Step 4: Remove unused fields from ContentService + +Edit ContentService.cs to remove the field declarations: + +```csharp +// Remove these lines: +private readonly IDocumentBlueprintRepository _documentBlueprintRepository; +private readonly Lazy _propertyValidationService; +private readonly ICultureImpactFactory _cultureImpactFactory; +private readonly PropertyEditorCollection _propertyEditorCollection; +private ContentSettings _contentSettings; // Remove WITH OnChange callback +private readonly IRelationService _relationService; +private readonly IEntityRepository _entityRepository; +private readonly ILanguageRepository _languageRepository; +private readonly IShortStringHelper _shortStringHelper; +``` + +Also remove the `optionsMonitor.OnChange` callback block (line 168-172 only - others already removed in Task 1). + +### Step 5: Update main constructor to remove unused parameters + +Update the `[ActivatorUtilitiesConstructor]` constructor to remove unused parameters: + +```csharp +[Microsoft.Extensions.DependencyInjection.ActivatorUtilitiesConstructor] +public ContentService( + ICoreScopeProvider provider, + ILoggerFactory loggerFactory, + IEventMessagesFactory eventMessagesFactory, + IDocumentRepository documentRepository, + IAuditService auditService, + IContentTypeRepository contentTypeRepository, + IUserIdKeyResolver userIdKeyResolver, + IIdKeyMap idKeyMap, + IContentCrudService crudService, + IContentQueryOperationService queryOperationService, + IContentVersionOperationService versionOperationService, + IContentMoveOperationService moveOperationService, + IContentPublishOperationService publishOperationService, + ContentPermissionManager permissionManager, + ContentBlueprintManager blueprintManager) + : base(provider, loggerFactory, eventMessagesFactory) +{ + _documentRepository = documentRepository; + _auditService = auditService; + _contentTypeRepository = contentTypeRepository; + _userIdKeyResolver = userIdKeyResolver; + _idKeyMap = idKeyMap; + _logger = loggerFactory.CreateLogger(); + + ArgumentNullException.ThrowIfNull(crudService); + _crudServiceLazy = new Lazy(() => crudService); + + ArgumentNullException.ThrowIfNull(queryOperationService); + _queryOperationService = queryOperationService; + + ArgumentNullException.ThrowIfNull(versionOperationService); + _versionOperationService = versionOperationService; + + ArgumentNullException.ThrowIfNull(moveOperationService); + _moveOperationService = moveOperationService; + + ArgumentNullException.ThrowIfNull(publishOperationService); + _publishOperationService = publishOperationService; + + ArgumentNullException.ThrowIfNull(permissionManager); + _permissionManager = permissionManager; + + ArgumentNullException.ThrowIfNull(blueprintManager); + _blueprintManager = blueprintManager; +} +``` + +### Step 6: Update UmbracoBuilder.cs DI registration + +Update the ContentService factory in UmbracoBuilder.cs to match new constructor: + +```csharp +Services.AddUnique(sp => + new ContentService( + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService(), + sp.GetRequiredService())); +``` + +### Step 7: Run build to verify compilation + +Run: `dotnet build src/Umbraco.Core/Umbraco.Core.csproj` +Expected: Build succeeds + +### Step 8: Run tests to verify functionality + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentService"` +Expected: All tests pass + +### Step 9: Commit + +```bash +git add src/Umbraco.Core/Services/ContentService.cs \ + src/Umbraco.Core/DependencyInjection/UmbracoBuilder.cs +git commit -m "$(cat <<'EOF' +refactor(core): remove unused fields from ContentService + +Remove fields that are now handled by extracted services: +- IDocumentBlueprintRepository (BlueprintManager) +- IPropertyValidationService (extracted services) +- ICultureImpactFactory (extracted services) +- PropertyEditorCollection (extracted services) +- ContentSettings + optionsMonitor.OnChange callback +- IRelationService (ContentMoveOperationService) +- IEntityRepository (unused) +- ILanguageRepository (extracted services) +- IShortStringHelper (ContentCrudService) + +Simplify constructor to only inject dependencies still used directly. + +🤖 Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 3: Expose PerformMoveLocked on IContentMoveOperationService (was Task 1) + +**Goal:** Make existing private `PerformMoveLocked` in `ContentMoveOperationService` public and accessible via interface, then remove duplicate from `ContentService`. + +**Files:** +- Modify: `src/Umbraco.Core/Services/IContentMoveOperationService.cs` +- Modify: `src/Umbraco.Core/Services/ContentMoveOperationService.cs` +- Modify: `src/Umbraco.Core/Services/ContentService.cs` + +### Step 1: Add method signature to IContentMoveOperationService (v4.0 - returns collection) + +Add interface method for the internal move logic. **v4.0 change:** Returns the moves collection instead of mutating a parameter (cleaner API): + +```csharp +// At end of IContentMoveOperationService.cs, add: + +/// +/// Performs the locked move operation for a content item and its descendants. +/// Used internally by MoveToRecycleBin orchestration. +/// +/// The content to move. +/// The target parent id. +/// The target parent content (can be null for root/recycle bin). +/// The user performing the operation. +/// Whether to mark as trashed (true), un-trashed (false), or unchanged (null). +/// Collection of moved items with their original paths. +IReadOnlyCollection<(IContent Content, string OriginalPath)> PerformMoveLocked( + IContent content, int parentId, IContent? parent, int userId, bool? trash); +``` + +### Step 2: Update existing method in ContentMoveOperationService (v4.0 - new signature) + +The existing private method has signature: +```csharp +private void PerformMoveLocked(IContent content, int parentId, IContent? parent, int userId, ICollection<(IContent, string)> moves, bool? trash) +``` + +Create a new public method with the cleaner signature that wraps the existing implementation: + +```csharp +/// +public IReadOnlyCollection<(IContent Content, string OriginalPath)> PerformMoveLocked( + IContent content, int parentId, IContent? parent, int userId, bool? trash) +{ + var moves = new List<(IContent, string)>(); + PerformMoveLockedInternal(content, parentId, parent, userId, moves, trash); + return moves.AsReadOnly(); +} + +// Rename existing private method to: +private void PerformMoveLockedInternal(IContent content, int parentId, IContent? parent, int userId, ICollection<(IContent, string)> moves, bool? trash) +``` + +This keeps the internal recursive implementation unchanged while providing a clean public interface. + +### Step 3: Verify build compiles + +Run: `dotnet build src/Umbraco.Core/Umbraco.Core.csproj` +Expected: Build succeeds + +### Step 4: Update ContentService.MoveToRecycleBin to use service (v4.0 - new return value) + +Replace the `PerformMoveLocked` call in ContentService with delegation. The new signature returns the collection: + +```csharp +// In MoveToRecycleBin, replace: +// var moves = new List<(IContent, string)>(); +// PerformMoveLocked(content, Constants.System.RecycleBinContent, null, userId, moves, true); +// With: +var moves = MoveOperationService.PerformMoveLocked(content, Constants.System.RecycleBinContent, null, userId, true); +``` + +The caller no longer needs to create the collection - it's returned by the service. + +### Step 5: Remove duplicate methods from ContentService + +Delete these methods from ContentService.cs: +- `PerformMoveLocked` (lines ~950-1000) +- `PerformMoveContentLocked` (lines ~1002-1011) +- `GetPagedDescendantQuery` (lines ~671-680) +- `GetPagedLocked` (lines ~682-700) + +### Step 6: Run tests to verify move operations work + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentService"` +Expected: All tests pass + +### Step 7: Commit + +```bash +git add src/Umbraco.Core/Services/IContentMoveOperationService.cs \ + src/Umbraco.Core/Services/ContentMoveOperationService.cs \ + src/Umbraco.Core/Services/ContentService.cs +git commit -m "$(cat <<'EOF' +refactor(core): expose PerformMoveLocked on IContentMoveOperationService + +Make existing private PerformMoveLocked method public and add to interface. +Update ContentService.MoveToRecycleBin to delegate to the service. +Remove duplicate helper methods from ContentService: +- PerformMoveLocked +- PerformMoveContentLocked +- GetPagedDescendantQuery +- GetPagedLocked + +This reduces ContentService complexity while maintaining MoveToRecycleBin +orchestration in the facade. + +🤖 Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 4: Expose DeleteLocked on IContentCrudService + Unify Implementations (was Task 2) + +**Goal:** Make existing private `DeleteLocked` in `ContentCrudService` public and accessible via interface. Also unify with `ContentMoveOperationService.DeleteLocked` by having EmptyRecycleBin call the CrudService version. This eliminates duplicate implementations. + +**Files:** +- Modify: `src/Umbraco.Core/Services/IContentCrudService.cs` +- Modify: `src/Umbraco.Core/Services/ContentCrudService.cs` +- Modify: `src/Umbraco.Core/Services/ContentService.cs` +- Modify: `src/Umbraco.Core/Services/ContentMoveOperationService.cs` (v3.0 addition for unification) + +### Step 1: Add DeleteLocked to IContentCrudService + +```csharp +// Add to IContentCrudService.cs: + +/// +/// Performs the locked delete operation including descendants. +/// Used internally by DeleteOfTypes orchestration and EmptyRecycleBin. +/// +void DeleteLocked(ICoreScope scope, IContent content, EventMessages evtMsgs); +``` + +### Step 2: Change existing method visibility in ContentCrudService + +Change the existing private method to public: + +```csharp +// In ContentCrudService.cs, change line 637: +// FROM: +private void DeleteLocked(ICoreScope scope, IContent content, EventMessages evtMsgs) +// TO: +public void DeleteLocked(ICoreScope scope, IContent content, EventMessages evtMsgs) +``` + +**Important:** The existing implementation in ContentCrudService already has: +- Iteration bounds (maxIterations = 10000) +- Proper logging for edge cases +- Empty batch detection + +Do NOT replace this with the simplified version from v1.0 of the plan. + +### Step 3: Run build to verify interface compiles + +Run: `dotnet build src/Umbraco.Core/Umbraco.Core.csproj` +Expected: Build succeeds + +### Step 4: Update ContentService.DeleteOfTypes to use service + +Replace the `DeleteLocked` call: + +```csharp +// Replace: +// DeleteLocked(scope, content, eventMessages); +// With: +CrudService.DeleteLocked(scope, content, eventMessages); +``` + +### Step 5: Remove DeleteLocked from ContentService + +Delete the `DeleteLocked` method (lines ~825-848). + +### Step 6: Unify ContentMoveOperationService.EmptyRecycleBin (v4.0 corrected) + +ContentMoveOperationService **already has** `IContentCrudService` as a constructor parameter (assigned to `_crudService` field). Update `EmptyRecycleBin` to call `IContentCrudService.DeleteLocked` instead of its own local `DeleteLocked` method: + +1. In `EmptyRecycleBin`, replace: + ```csharp + // FROM: + DeleteLocked(scope, content, eventMessages); + // TO: + _crudService.DeleteLocked(scope, content, eventMessages); + ``` +2. Remove the local `DeleteLocked` method from `ContentMoveOperationService` (search for `private void DeleteLocked`) + +This eliminates duplicate implementations and ensures bug fixes only need to be applied once. + +### Step 7: Run tests to verify delete operations work + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentService"` +Expected: All tests pass + +### Step 8: Commit + +```bash +git add src/Umbraco.Core/Services/IContentCrudService.cs \ + src/Umbraco.Core/Services/ContentCrudService.cs \ + src/Umbraco.Core/Services/ContentService.cs \ + src/Umbraco.Core/Services/ContentMoveOperationService.cs +git commit -m "$(cat <<'EOF' +refactor(core): expose DeleteLocked on IContentCrudService and unify implementations + +Make existing private DeleteLocked method public and add to interface. +Update ContentService.DeleteOfTypes to delegate to the service. +Remove duplicate DeleteLocked from ContentService. + +Unify implementations by having ContentMoveOperationService.EmptyRecycleBin +call IContentCrudService.DeleteLocked instead of its own local method. +This eliminates the duplicate implementation and reduces maintenance burden. + +The ContentCrudService implementation includes iteration bounds +and proper logging for edge cases. + +🤖 Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 5: Extract CheckDataIntegrity to ContentCrudService (was Task 3) + +**Goal:** Move `CheckDataIntegrity` from ContentService to ContentCrudService. + +**Files:** +- Modify: `src/Umbraco.Core/Services/IContentCrudService.cs` +- Modify: `src/Umbraco.Core/Services/ContentCrudService.cs` +- Modify: `src/Umbraco.Core/Services/ContentService.cs` + +### Step 1: Add CheckDataIntegrity to IContentCrudService + +```csharp +// Add to IContentCrudService.cs: + +/// +/// Checks content data integrity and optionally fixes issues. +/// +ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options); +``` + +### Step 2: Run build to verify interface compiles + +Run: `dotnet build src/Umbraco.Core/Umbraco.Core.csproj` +Expected: Build succeeds + +### Step 3: Add IShortStringHelper to ContentCrudService constructor (v3.0 explicit steps) + +1. Add `IShortStringHelper shortStringHelper` parameter to ContentCrudService constructor +2. Add private field `private readonly IShortStringHelper _shortStringHelper;` +3. Assign in constructor: `_shortStringHelper = shortStringHelper;` + +```csharp +// In ContentCrudService constructor, add parameter: +public ContentCrudService( + // ... existing parameters ... + IShortStringHelper shortStringHelper) +{ + // ... existing assignments ... + _shortStringHelper = shortStringHelper; +} +``` + +### Step 4: Verify IShortStringHelper DI registration + +Verify that ContentCrudService DI registration in UmbracoBuilder.cs will resolve IShortStringHelper. Since it uses `AddUnique()`, DI should auto-resolve the new dependency. + +Run build after this step to confirm: `dotnet build src/Umbraco.Core/Umbraco.Core.csproj` + +If build fails with DI resolution error, check that `IShortStringHelper` is registered (search for `AddShortString` or `IShortStringHelper` in UmbracoBuilder.cs). + +### Step 5: Move CheckDataIntegrity implementation to ContentCrudService + +Add to ContentCrudService.cs: + +```csharp +/// +public ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options) +{ + using (ICoreScope scope = ScopeProvider.CreateCoreScope()) + { + scope.WriteLock(Constants.Locks.ContentTree); + + ContentDataIntegrityReport report = _documentRepository.CheckDataIntegrity(options); + + if (report.FixedIssues.Count > 0) + { + var root = new Content("root", -1, new ContentType(_shortStringHelper, -1)) { Id = -1, Key = Guid.Empty }; + scope.Notifications.Publish(new ContentTreeChangeNotification(root, TreeChangeTypes.RefreshAll, EventMessagesFactory.Get())); + } + + scope.Complete(); + + return report; + } +} +``` + +### Step 6: Update ContentService.CheckDataIntegrity to delegate + +```csharp +public ContentDataIntegrityReport CheckDataIntegrity(ContentDataIntegrityReportOptions options) + => CrudService.CheckDataIntegrity(options); +``` + +### Step 7: Run tests to verify data integrity operations work + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentService"` +Expected: All tests pass + +### Step 8: Commit + +```bash +git add src/Umbraco.Core/Services/IContentCrudService.cs \ + src/Umbraco.Core/Services/ContentCrudService.cs \ + src/Umbraco.Core/Services/ContentService.cs +git commit -m "$(cat <<'EOF' +refactor(core): extract CheckDataIntegrity to ContentCrudService + +Move CheckDataIntegrity from ContentService to ContentCrudService. +Add IShortStringHelper dependency to ContentCrudService. + +🤖 Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 6: Clean Up Remaining Internal Methods + +**Files:** +- Modify: `src/Umbraco.Core/Services/ContentService.cs` + +### Step 1: Check GetAllPublished usage (v4.0 expanded) + +This method is `internal` (not public). Check if it's called externally: + +Run: `grep -rn "GetAllPublished" src/ --include="*.cs" | grep -v ContentService.cs` + +**Note (v2.0):** Internal methods can be used by test projects via `InternalsVisibleTo`. Check test projects too: + +Run: `grep -rn "GetAllPublished" tests/ --include="*.cs"` + +**Note (v3.0):** Also check web projects that may have access via `InternalsVisibleTo`: + +Run: `grep -rn "GetAllPublished" src/Umbraco.Infrastructure/ src/Umbraco.Web.Common/ --include="*.cs"` + +**Note (v4.0):** Also check API projects which may have InternalsVisibleTo access: + +Run: `grep -rn "GetAllPublished" src/Umbraco.Cms.Api.Management/ src/Umbraco.Cms.Api.Delivery/ --include="*.cs"` + +If not used externally, in tests, or in API projects, remove it along with `_queryNotTrashed` field. If used, keep both. + +### Step 2: Remove HasUnsavedChanges if unused + +Check usage: +Run: `grep -rn "HasUnsavedChanges" src/ tests/ --include="*.cs" | grep -v ContentService.cs` + +If not used, remove the static method. + +### Step 3: Remove TryGetParentKey if unused + +Check usage: +Run: `grep -rn "TryGetParentKey" src/ tests/ --include="*.cs" | grep -v ContentService.cs` + +If not used externally and not used in remaining ContentService methods, remove it. + +### Step 4: Simplify Audit methods + +If AuditAsync is only called by the sync Audit method, and Audit is only used in MoveToRecycleBin and DeleteOfTypes, keep them but simplify. + +### Step 5: Run build to verify compilation + +Run: `dotnet build src/Umbraco.Core/Umbraco.Core.csproj` +Expected: Build succeeds + +### Step 6: Run tests to verify functionality + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentService"` +Expected: All tests pass + +### Step 7: Commit + +```bash +git add src/Umbraco.Core/Services/ContentService.cs +git commit -m "$(cat <<'EOF' +refactor(core): clean up remaining internal methods in ContentService + +Remove or simplify internal helper methods that are no longer needed +after service extraction. + +🤖 Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 7: Verify Line Count and Final Cleanup + +**Files:** +- Review: `src/Umbraco.Core/Services/ContentService.cs` + +### Step 1: Count lines (v4.0 corrected) + +Run: `wc -l src/Umbraco.Core/Services/ContentService.cs` + +**Expected: ~990 lines** (calculated from 1330 - ~340 lines of removal) + +Line removal breakdown: +- Obsolete constructors: ~160 lines +- Lazy fields and duplicate properties: ~40 lines +- Duplicate methods (PerformMoveLocked, DeleteLocked, etc.): ~100 lines +- Field declarations removal: ~10 lines +- Internal method cleanup: ~30 lines +- **Total removal:** ~340 lines + +### Step 2: Review remaining structure + +The file should contain: +1. Field declarations (~10-15 lines) +2. Constructor (~30-40 lines) +3. Service accessor properties (~30 lines) +4. One-liner delegation methods (~100-150 lines) +5. Orchestration methods: MoveToRecycleBin, DeleteOfTypes/DeleteOfType (~80 lines) +6. Helper methods: GetContentType, Audit (~30 lines) + +### Step 3: Format code + +Run: `dotnet format src/Umbraco.Core/Umbraco.Core.csproj` +Expected: No formatting changes or only minor whitespace + +### Step 4: Commit if formatting changed + +```bash +git add src/Umbraco.Core/Services/ContentService.cs +git commit -m "$(cat <<'EOF' +style(core): format ContentService.cs + +🤖 Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Task 8: Run Full Test Suite (Phase Gate) + +**Files:** +- None (verification only) + +### Step 1: Run refactoring-specific tests + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentServiceRefactoringTests"` +Expected: All 15+ tests pass + +### Step 2: Run all ContentService tests + +Run: `dotnet test tests/Umbraco.Tests.Integration --filter "FullyQualifiedName~ContentService"` +Expected: All tests pass + +### Step 2a: Add unit tests for newly exposed interface methods (v4.0 addition) + +Create or update unit tests to cover the newly public interface methods: + +**For IContentMoveOperationService.PerformMoveLocked:** +- Test that it returns a non-null collection +- Test that moved items are included in the returned collection +- Test edge cases: single item, nested hierarchy + +**For IContentCrudService.DeleteLocked:** +- Test that it handles empty tree (no descendants) +- Test that it handles large tree (iteration bounds) +- Test that null content throws appropriate exception + +Add tests to: `tests/Umbraco.Tests.Integration/Umbraco.Core/Services/ContentServiceRefactoringTests.cs` + +### Step 3: Run full integration test suite (v3.0 duration note) + +**Note:** The full integration test suite can take 10+ minutes. Consider quick verification first: + +```bash +# Quick verification (2-3 min) - run critical paths first +dotnet test tests/Umbraco.Tests.Integration --filter "Category=Quick" + +# Full suite (10+ min) +dotnet test tests/Umbraco.Tests.Integration +``` + +Run: `dotnet test tests/Umbraco.Tests.Integration` +Expected: All tests pass (or only pre-existing failures) + +### Step 4: Document any test failures + +If tests fail, document them and investigate whether they're: +1. Pre-existing failures (acceptable) +2. Regressions from this phase (must fix) + +### Step 5: Create Phase 8 git tag + +```bash +git tag -a phase-8-facade-finalization -m "Phase 8: ContentService facade finalization complete" +``` + +--- + +## Task 9: Update Design Document + +**Files:** +- Modify: `docs/plans/2025-12-19-contentservice-refactor-design.md` + +### Step 1: Mark Phase 8 complete + +Update the implementation order table: + +```markdown +| 8 | Facade | **Full test suite** | All pass | ✅ Complete | +``` + +### Step 2: Update success criteria + +Check off completed items: +- [x] All existing tests pass +- [x] No public API breaking changes +- [x] ContentService reduced to ~990 lines (from 1330) +- [x] Each new service independently testable +- [x] Notification ordering matches current behavior +- [x] All 80+ IContentService methods mapped to new services + +### Step 3: Add Phase 8 details + +Add to the phase details section: + +```markdown +10. **Phase 8: Facade Finalization** ✅ - Complete! Changes: + - Exposed PerformMoveLocked on IContentMoveOperationService (returns collection - clean API) + - Exposed DeleteLocked on IContentCrudService + - Unified DeleteLocked implementations (ContentMoveOperationService now calls CrudService) + - Extracted CheckDataIntegrity to ContentCrudService + - Removed 9 unused fields from ContentService + - Removed optionsMonitor.OnChange callback (no longer needed) + - Removed 2 obsolete constructors (~160 lines) + - Simplified constructor to 15 parameters (services only) + - ContentService reduced from 1330 lines to ~990 lines + - Git tag: `phase-8-facade-finalization` +``` + +### Step 4: Commit + +```bash +git add docs/plans/2025-12-19-contentservice-refactor-design.md +git commit -m "$(cat <<'EOF' +docs: mark Phase 8 complete in design document + +Update ContentService refactoring design document to reflect +Phase 8 facade finalization completion. + +🤖 Generated with [Claude Code](https://claude.com/claude-code) + +Co-Authored-By: Claude +EOF +)" +``` + +--- + +## Summary + +### Tasks Overview (v4.0 - Updated) + +| Task | Description | Est. Steps | v4.0 Changes | +|------|-------------|------------|--------------| +| 1 | Remove Obsolete Constructors | 7 | Use method signatures instead of line numbers | +| 2 | Remove Unused Fields and Simplify Constructor | 10 | **Added** Step 1a: _contentSettings verification | +| 3 | Expose PerformMoveLocked on interface | 7 | **Changed** to return collection (Option A - clean API) | +| 4 | Expose DeleteLocked + Unify Implementations | 8 | **Fixed** Step 6: IContentCrudService already exists | +| 5 | Extract CheckDataIntegrity to ContentCrudService | 8 | No change | +| 6 | Clean Up Remaining Internal Methods | 7 | **Added** API project checks | +| 7 | Verify Line Count and Final Cleanup | 4 | **Fixed** target: ~990 lines | +| 8 | Run Full Test Suite (Phase Gate) | 6 | **Added** Step 2a: unit tests for new interface methods | +| 9 | Update Design Document | 4 | Updated line count references | +| **Total** | | **61** | | + +### Expected Outcomes + +1. **ContentService reduced to ~990 lines** (from 1330) - v4.0 corrected calculation +2. **Constructor simplified** to only essential dependencies +3. **No obsolete constructors** remaining (if version approved) +4. **All duplicate methods** removed (not re-extracted) +5. **DeleteLocked unified** - Single implementation in ContentCrudService (v3.0) +6. **PerformMoveLocked returns collection** - Clean interface without mutable parameter (v4.0) +7. **All tests passing** (full integration suite) +8. **Design document updated** with Phase 8 completion + +### Risk Mitigation + +| Risk | Mitigation | +|------|------------| +| Breaking existing callers | Only remove internal/private methods, keep public API | +| Test failures | Run tests after each extraction, fix before proceeding | +| Missing dependencies | Verify each exposed method has access to required repositories | +| DI registration issues | Update UmbracoBuilder.cs in sync with constructor changes | +| Breaking change version | Verify obsolete constructor removal is approved for current version | + +### Rollback Plan + +If issues are discovered: +1. Revert to the commit before the problematic change +2. Investigate the root cause +3. Fix and retry the specific task +4. Do not batch multiple risky changes + +--- + +## Review Feedback Incorporated (v2.0) + +This version addresses all findings from Critical Review 1: + +| Review Finding | Resolution | +|----------------|------------| +| Task 1-2: Methods already exist | Rewritten to expose existing methods, not re-extract | +| Task 3: Missing DI registration step | Added Step 4 for UmbracoBuilder.cs verification | +| Task 4: _contentSettings still used | Added removal of OnChange callback with field | +| Task 5: Breaking change timing | Added version verification step | +| Task 6: Internal method grep incorrect | Fixed grep to include tests folder | +| Task 7: Unrealistic line target | Changed from ~200 to ~250-300 | +| Constructor parameter count | Updated to reflect actual 15 parameters | + +--- + +## Review Feedback Incorporated (v3.0) + +This version addresses all findings from Critical Review 2: + +| Review Finding | Section | Resolution | +|----------------|---------|------------| +| Duplicate DeleteLocked in two services | 2.1 | **Unify implementations** - ContentMoveOperationService calls IContentCrudService.DeleteLocked | +| Task execution order creates redundant work | 2.2 | **Reordered tasks** - Task 5 (obsolete constructors) now first, then Task 4 (fields) | +| Missing IShortStringHelper constructor steps | 2.3 | Added explicit steps: add parameter, add field, verify DI | +| Interface signature exposes mutable collection | 2.4 | Added XML documentation warning about collection mutation | +| Potential null reference after field removal | 2.5 | Added verification step to check _relationService references | +| Lazy field removal not explicit | 3.2 | Listed all 6 Lazy fields to remove explicitly | +| Internal method check should include web projects | 3.3 | Added grep for Umbraco.Infrastructure and Umbraco.Web.Common | +| Full integration test duration | 3.5 | Added note about 10+ minute duration and quick verification option | + +--- + +## Review Feedback Incorporated (v4.0) + +This version addresses all findings from Critical Review 3: + +| Review Finding | Section | Resolution | +|----------------|---------|------------| +| Task 4 Step 6: IContentCrudService already exists | 2.1 | **Fixed** - Removed incorrect instruction to add dependency; clarified it already exists as `_crudService` | +| Task 7 line count target incorrect (~250-300 vs ~990) | 2.5 | **Fixed** - Recalculated to ~990 lines based on ~340 lines of removal | +| Missing _contentSettings verification before removal | 2.3 | **Added** Step 1a to Task 2 - verify no extracted services depend on _contentSettings | +| PerformMoveLocked exposes mutable collection parameter | 2.2 | **Fixed** - Changed to Option A: returns `IReadOnlyCollection<(IContent, string)>` instead of mutating parameter | +| No unit tests for newly exposed interface methods | 3.3 | **Added** Step 2a to Task 8 - add tests for PerformMoveLocked and DeleteLocked | +| Missing API project checks in Task 6 | 3.2 | **Added** grep for Umbraco.Cms.Api.Management and Umbraco.Cms.Api.Delivery | +| Line numbers for obsolete constructors may be stale | 3.4 | **Fixed** - Use method signatures and grep patterns instead of line numbers | +| _queryNotTrashed disposition unclear | Q2 | **Resolved** - Remove with GetAllPublished if GetAllPublished is removed | + +--- + +**End of Phase 8 Implementation Plan v4.0**