yv01p/Umbraco-CMS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Delete docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-4.md

Browse Source
This commit is contained in:
yv01p
2026-01-02 22:02:37 +00:00
parent b23e192ab4
commit b254df3c62
1 changed files with 0 additions and 400 deletions
Download Patch File Download Diff File Expand all files Collapse all files

400
docs/plans/2025-12-24-contentservice-refactor-phase8-implementation-critical-review-4.md
View File

@@ -1,400 +0,0 @@
# 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<IContent>(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<IContent>[] moveInfo = moves
.Select(x => new MoveToRecycleBinEventInfo<IContent>(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<IContentQueryOperationService>? _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<IContentCrudService, ContentCrudService>()`), 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**