-
Notifications
You must be signed in to change notification settings - Fork 0
Open
Description
Summary
The ImportService::importFile() method in Classes/Service/ImportService.php is currently 270+ lines long (lines 88-358), handling all 5 phases of the DBAL bulk import process in a single method. This should be refactored to extract each phase into its own private method to improve readability and align code structure with the documented 5-phase architecture.
Current State
✅ The method works correctly and is well-tested:
- 70 tests passing (52 unit + 18 functional)
- All CI checks passing (PHPStan level 10, Rector, PHP-CS-Fixer)
- Performance verified: 18-33x faster than original implementation
- Comprehensive batching tests validate array_chunk operations with 1000+ records
Problem
The importFile() method combines all 5 import phases in a single large method:
- Phase 1 (lines 100-163): Extract unique component/type names and validate entries
- Phase 2 (lines 165-216): Find/create reference records (environment, components, types)
- Phase 3 (lines 218-239): Bulk lookup existing translations
- Phase 4 (lines 241-296): Prepare bulk INSERT and UPDATE arrays
- Phase 5 (lines 298-357): Execute bulk operations using DBAL with transaction safety
Proposed Solution
Extract each phase into focused private methods:
public function importFile(string $file, bool $forceUpdate, int &$imported, int &$updated, array &$errors): void
{
$languageKey = $this->getLanguageKeyFromFile($file);
$languageUid = $this->getLanguageId($languageKey);
$fileContent = $this->xliffParser->getParsedData($file, $languageKey);
$entries = $fileContent[$languageKey];
$validatedEntries = $this->validateAndExtractEntries($entries, $errors);
if ($validatedEntries === []) {
return;
}
$referenceData = $this->createReferenceData($validatedEntries, $errors);
if ($referenceData === null) {
return;
}
$translationMap = $this->lookupExistingTranslations(
$referenceData['environmentUid'],
$languageUid
);
[$inserts, $updates] = $this->prepareInsertAndUpdateArrays(
$validatedEntries,
$translationMap,
$referenceData,
$languageUid,
$forceUpdate,
$imported,
$updated,
$errors
);
$this->executeBulkOperations($inserts, $updates, $errors);
}
private function validateAndExtractEntries(array $entries, array &$errors): array { /* Phase 1 */ }
private function createReferenceData(array $validatedEntries, array &$errors): ?array { /* Phase 2 */ }
private function lookupExistingTranslations(int $environmentUid, int $languageUid): array { /* Phase 3 */ }
private function prepareInsertAndUpdateArrays(/* ... */): array { /* Phase 4 */ }
private function executeBulkOperations(array $inserts, array $updates, array &$errors): void { /* Phase 5 */ }Benefits
- ✅ Improved readability and maintainability
- ✅ Clearer separation of concerns
- ✅ Code structure aligns with ADR-001 documentation
- ✅ Easier to test individual phases in isolation
- ✅ Reduced cognitive complexity per method
Rationale for Deferral
This refactoring was intentionally deferred during the async import queue PR to:
- ✅ Avoid regression risks in critical performance optimization code
- ✅ Complete and merge the high-priority async import functionality first
- ✅ Maintain focus on delivering working, tested bulk operations
- ✅ Allow time for comprehensive testing before structural changes
References
- ADR:
Documentation/TechnicalAnalysis/ADR-001-DBAL-Bulk-Import.rst(lines 71-87) - Code:
Classes/Service/ImportService.php:88-358 - Tests:
Tests/Functional/Service/ImportServiceTest.php(batching validation) - Original PR Code Review: Finding Update dependency typo3/cms-backend to v13 #4 - LOW priority improvement
Acceptance Criteria
- Extract Phase 1:
validateAndExtractEntries() - Extract Phase 2:
createReferenceData() - Extract Phase 3:
lookupExistingTranslations() - Extract Phase 4:
prepareInsertAndUpdateArrays() - Extract Phase 5:
executeBulkOperations() - Update method documentation to reference phase extraction
- Verify all 70 tests still pass after refactoring
- Run full CI suite (PHPStan, Rector, PHP-CS-Fixer)
- No functional changes - purely structural improvement
Priority
LOW - Code quality improvement, no functional impact
Metadata
Metadata
Assignees
Labels
No labels