Refactor attribute type system to use enums and manage dynamic list attributes table names #369
Conversation
📝 WalkthroughWalkthroughAdds enum-based attribute types, refactors CSV importer/mappers, implements dynamic-list DTOs, repository and managers with message-driven dynamic table creation, updates DI/config and many tests, and removes multiple PHPMD suppression annotations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant DynamicTablesManager
participant MessageBus
participant Handler
participant SchemaManager
Caller->>DynamicTablesManager: createOptionsTableIfNotExists(table)
DynamicTablesManager->>MessageBus: dispatch(DynamicTableMessage)
MessageBus->>Handler: __invoke(DynamicTableMessage)
rect rgb(235, 245, 255)
Note over Handler: Idempotency check
Handler->>SchemaManager: tablesExist([table])
end
alt table exists
Handler-->>Caller: return (no-op)
else table missing
rect rgb(225, 255, 230)
Note over Handler: validate name & create
Handler->>Handler: validate /^[A-Za-z0-9_]+$/
Handler->>SchemaManager: createTable(Table)
end
alt create succeeded
SchemaManager-->>Handler: success
else TableExistsException (race)
SchemaManager-->>Handler: TableExistsException
Handler->>Handler: swallow exception
end
Handler-->>Caller: return
end
sequenceDiagram
autonumber
participant CSV
participant SubscriberCsvImporter
participant CsvToDtoImporter
participant ArrayToDtoMapper
participant SubscriberManager
CSV->>SubscriberCsvImporter: import(file)
SubscriberCsvImporter->>CsvToDtoImporter: parseAndValidate(file)
rect rgb(245, 235, 255)
Note over CsvToDtoImporter: parse CSV rows
CsvToDtoImporter->>ArrayToDtoMapper: mapRow(array)
ArrayToDtoMapper-->>CsvToDtoImporter: DTO
end
CsvToDtoImporter-->>SubscriberCsvImporter: {valid, errors}
rect rgb(230, 255, 230)
Note over SubscriberCsvImporter: process results
alt valid items
SubscriberCsvImporter->>SubscriberManager: createOrUpdate(dto)
end
alt invalid email rows
SubscriberCsvImporter->>SubscriberCsvImporter: increment invalid_email stat
end
SubscriberCsvImporter-->>CSV: aggregated stats
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
config/services/mappers.yml(1 hunks)src/Domain/Subscription/Service/ArrayToDtoMapper.php(1 hunks)src/Domain/Subscription/Service/CsvToDtoImporter.php(2 hunks)src/Domain/Subscription/Service/SubscriberCsvImporter.php(7 hunks)tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce strict domain purity:
- ❌ Do not allow infrastructure persistence side effects here.
- Flag ANY usage of Doctrine persistence APIs, especially:
- $entityManager->flush(...), $this->entityManager->flush(...)
- $em->persist(...), $em->remove(...)
- direct transaction control ($em->beginTransaction(), commit(), rollback())
- If found, request moving these calls to application-layer Command handlers or background Jobs.
- Also flag repositories in Domain that invoke flush/transactional logic; Domain repositories should be abstractions without side effects.
- Encourage domain events/outbox or return-values to signal write-intent, leaving orchestration to Commands/Jobs.
Files:
src/Domain/Subscription/Service/ArrayToDtoMapper.phpsrc/Domain/Subscription/Service/CsvToDtoImporter.phpsrc/Domain/Subscription/Service/SubscriberCsvImporter.php
🧬 Code graph analysis (3)
src/Domain/Subscription/Service/CsvToDtoImporter.php (2)
src/Domain/Subscription/Service/SubscriberCsvImporter.php (1)
__construct(47-67)src/Domain/Subscription/Service/ArrayToDtoMapper.php (1)
ArrayToDtoMapper(9-52)
src/Domain/Subscription/Service/SubscriberCsvImporter.php (1)
src/Domain/Subscription/Service/CsvToDtoImporter.php (2)
CsvToDtoImporter(14-62)parseAndValidate(28-61)
tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php (1)
src/Domain/Subscription/Service/CsvToDtoImporter.php (1)
CsvToDtoImporter(14-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (11)
src/Domain/Subscription/Service/ArrayToDtoMapper.php (1)
9-9: LGTM! Clean semantic rename.The rename from
CsvRowToDtoMappertoArrayToDtoMapperbetter reflects that this mapper works with generic arrays rather than being tightly coupled to CSV-specific concepts. The implementation remains unchanged and continues to handle the mapping logic correctly.config/services/mappers.yml (1)
7-13: LGTM! Service configuration properly updated.The service definitions have been correctly updated to reference the renamed classes while preserving all autowiring and autoconfiguration settings.
src/Domain/Subscription/Service/CsvToDtoImporter.php (3)
14-14: LGTM! Better semantic clarity.The rename to
CsvToDtoImportermore explicitly conveys that this class transforms CSV data into DTOs, making the codebase more self-documenting.
17-17: LGTM! Dependency updated consistently.The constructor parameter type correctly reflects the
ArrayToDtoMapperrename, maintaining consistency across the refactoring.
28-28: LGTM! Method name is more descriptive.Renaming
import()toparseAndValidate()better describes what this method actually does—it parses CSV data and validates DTOs without performing any persistence. This improves code readability and sets clearer expectations for callers.tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php (2)
15-15: LGTM! Tests properly updated for API changes.The test imports, mock declarations, and constructor invocations have been correctly updated to use
CsvToDtoImporterinstead ofCsvImporter, maintaining test coverage for the renamed classes.Also applies to: 32-32, 42-42, 51-51
111-111: LGTM! Method expectations consistently updated.All mock method expectations have been updated from
importtoparseAndValidate, ensuring the tests align with the new public API surface.Also applies to: 176-176, 247-247, 312-312, 372-372, 432-432
src/Domain/Subscription/Service/SubscriberCsvImporter.php (4)
41-41: LGTM! Dependency properly updated.The property, constructor parameter, and assignment have been correctly updated from
CsvImportertoCsvToDtoImporter, maintaining consistency with the class rename.Also applies to: 52-52, 62-62
84-84: LGTM! Stats array extended for invalid email tracking.The addition of
invalid_emailto the stats array initialization properly supports the new tracking requirement.
96-96: LGTM! Method call updated consistently.The call correctly uses the renamed
parseAndValidate()method, maintaining alignment with theCsvToDtoImporterAPI.
251-252: LGTM! Invalid email tracking implemented.When
skipInvalidEmailis enabled, the code now properly increments theinvalid_emailcounter and records a translated error message, providing better visibility into data quality issues during import.
There was a problem hiding this comment.
Actionable comments posted: 11
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
composer.json(1 hunks)config/parameters.yml.dist(1 hunks)config/services/managers.yml(1 hunks)config/services/repositories.yml(1 hunks)src/Domain/Subscription/Exception/AttributeTypeChangeNotAllowed.php(1 hunks)src/Domain/Subscription/Model/AttributeTypeEnum.php(1 hunks)src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php(1 hunks)src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php(1 hunks)src/Domain/Subscription/Model/SubscriberAttributeDefinition.php(2 hunks)src/Domain/Subscription/Repository/DynamicListAttrRepository.php(4 hunks)src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php(1 hunks)src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php(3 hunks)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php(1 hunks)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php(1 hunks)src/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.php(2 hunks)src/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.php(2 hunks)tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php(1 hunks)tests/Unit/Domain/Subscription/Repository/DynamicListAttrRepositoryTest.php(6 hunks)tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php(8 hunks)tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php(1 hunks)tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrTablesManagerTest.php(1 hunks)tests/Unit/Domain/Subscription/Service/Provider/CheckboxGroupValueProviderTest.php(3 hunks)tests/Unit/Domain/Subscription/Service/Provider/ScalarValueProviderTest.php(2 hunks)tests/Unit/Domain/Subscription/Service/Provider/SelectOrRadioValueProviderTest.php(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce strict domain purity:
- ❌ Do not allow infrastructure persistence side effects here.
- Flag ANY usage of Doctrine persistence APIs, especially:
- $entityManager->flush(...), $this->entityManager->flush(...)
- $em->persist(...), $em->remove(...)
- direct transaction control ($em->beginTransaction(), commit(), rollback())
- If found, request moving these calls to application-layer Command handlers or background Jobs.
- Also flag repositories in Domain that invoke flush/transactional logic; Domain repositories should be abstractions without side effects.
- Encourage domain events/outbox or return-values to signal write-intent, leaving orchestration to Commands/Jobs.
Files:
src/Domain/Subscription/Model/AttributeTypeEnum.phpsrc/Domain/Subscription/Model/SubscriberAttributeDefinition.phpsrc/Domain/Subscription/Exception/AttributeTypeChangeNotAllowed.phpsrc/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.phpsrc/Domain/Subscription/Service/Manager/AttributeDefinitionManager.phpsrc/Domain/Subscription/Model/Dto/DynamicListAttrDto.phpsrc/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.phpsrc/Domain/Subscription/Service/Manager/DynamicListAttrManager.phpsrc/Domain/Subscription/Repository/DynamicListAttrRepository.phpsrc/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.phpsrc/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.phpsrc/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php
🧬 Code graph analysis (17)
tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrTablesManagerTest.php (2)
src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php (1)
SubscriberAttributeDefinitionRepository(12-31)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (3)
DynamicListAttrTablesManager(14-82)resolveTableName(27-55)createOptionsTableIfNotExists(60-81)
tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php (5)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (1)
DynamicListAttrDto(10-33)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (4)
DynamicListAttrRepository(16-120)getAll(104-119)fetchOptionNames(46-65)fetchSingleOptionName(67-84)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (2)
DynamicListAttrManager(15-267)insertOptions(35-82)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (2)
DynamicListAttrTablesManager(14-82)createOptionsTableIfNotExists(60-81)src/TestingSupport/Traits/DatabaseTestTrait.php (1)
loadSchema(81-92)
src/Domain/Subscription/Model/AttributeTypeEnum.php (1)
src/Domain/Subscription/Exception/AttributeTypeChangeNotAllowed.php (1)
AttributeTypeChangeNotAllowed(9-19)
src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (1)
src/Domain/Subscription/Model/AttributeTypeEnum.php (1)
assertTransitionAllowed(50-55)
tests/Unit/Domain/Subscription/Service/Provider/SelectOrRadioValueProviderTest.php (2)
src/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.php (1)
supports(18-21)src/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.php (1)
supports(19-22)
src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (3)
src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php (2)
SubscriberAttributeDefinitionRepository(12-31)existsByTableName(21-30)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (1)
__construct(20-27)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (1)
__construct(19-26)
tests/Unit/Domain/Subscription/Repository/DynamicListAttrRepositoryTest.php (1)
src/Domain/Subscription/Repository/DynamicListAttrRepository.php (1)
DynamicListAttrRepository(16-120)
src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (6)
src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (4)
DynamicListAttrManager(15-267)__construct(19-26)insertOptions(35-82)syncOptions(84-131)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (4)
DynamicListAttrTablesManager(14-82)__construct(18-25)resolveTableName(27-55)createOptionsTableIfNotExists(60-81)src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php (1)
__construct(17-28)src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (1)
__construct(19-32)src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php (1)
SubscriberAttributeDefinitionRepository(12-31)src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (4)
setType(85-98)setDefaultValue(106-110)setTableName(118-122)getTableName(72-75)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (2)
src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php (1)
__construct(17-28)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (1)
__construct(20-27)
tests/Unit/Domain/Subscription/Service/Provider/CheckboxGroupValueProviderTest.php (1)
src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (1)
setType(85-98)
src/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.php (1)
src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (1)
getType(52-55)
src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (3)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (2)
DynamicListAttrDto(10-33)__construct(19-32)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (4)
DynamicListAttrRepository(16-120)__construct(20-27)getAll(104-119)existsByName(86-102)src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (2)
__construct(22-34)update(68-94)
src/Domain/Subscription/Repository/DynamicListAttrRepository.php (3)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (2)
DynamicListAttrDto(10-33)__construct(19-32)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (1)
__construct(19-26)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (1)
__construct(18-25)
src/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.php (1)
src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (1)
getType(52-55)
tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php (6)
src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php (1)
AttributeDefinitionDto(10-29)src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php (1)
SubscriberAttributeDefinitionRepository(12-31)src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (1)
AttributeDefinitionManager(14-110)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (1)
DynamicListAttrManager(15-267)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (1)
DynamicListAttrTablesManager(14-82)src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (6)
getType(52-55)getListOrder(57-60)getDefaultValue(62-65)isRequired(67-70)getTableName(72-75)setName(78-82)
tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php (3)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (1)
DynamicListAttrDto(10-33)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (1)
DynamicListAttrRepository(16-120)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (3)
DynamicListAttrManager(15-267)insertOptions(35-82)syncOptions(84-131)
src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php (1)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (1)
__construct(19-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (16)
composer.json (2)
80-81: Minor observation: PR objective mismatch.The PR title is "Rename classes/functions," but these composer.json changes are purely dependency additions (not renaming-related). Confirm this is intentional and part of supporting the refactoring described in the broader PR summary, or consider clarifying the PR scope.
80-81: Verify the tatevikgr/rss-feed dependency—package not found publicly.The original review warns about using
dev-main, but we couldn't locatetatevikgr/rss-feedon Packagist or GitHub. Before addressing branch stability, confirm:
- Does this package exist? If it's a private or internal dependency (GitHub/GitLab VCS), it needs proper repository configuration in composer.json.
- Is it intentional? The PR title mentions "Rename classes/functions"—verify this dependency addition aligns with the changes.
- If public: Use a stable release instead of
dev-main. If no stable release exists, request one from the maintainer.- If private: Ensure the VCS repository URL is configured and credentials are properly managed (not in composer.json).
The
ext-pdoaddition is fine—it's a platform extension already matching the existing pattern.config/services/repositories.yml (1)
73-74: LGTM! Clear parameter naming.The rename from
$prefixto$dbPrefiximproves clarity, and the new$dynamicListTablePrefixparameter properly wires the dynamic list table support.config/parameters.yml.dist (1)
26-27: LGTM! Consistent configuration pattern.The new
list_table_prefixparameter follows the established pattern and provides a sensible default for dynamic list attribute tables.config/services/managers.yml (1)
51-65: LGTM! Clean service definitions.Both new manager services follow the established DI patterns and properly wire the prefix configuration parameters. The explicit
public: trueonDynamicListAttrTablesManageris appropriate for direct container access scenarios.tests/Unit/Domain/Subscription/Service/Provider/ScalarValueProviderTest.php (2)
7-7: LGTM! Enum import for type-safe testing.
30-30: LGTM! Adapted to enum-based types.The test correctly uses the enum constant instead of a string literal, aligning with the type system migration.
src/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.php (2)
7-7: LGTM! Enum import for type checking.
20-20: LGTM! Type-safe enum comparison.The migration from string literals to enum constants provides better type safety and IDE support.
src/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.php (3)
7-7: LGTM! Enum import for type checking.
11-11: LGTM! Explicit function import.While not strictly necessary in PHP, explicitly importing
in_arrayimproves clarity.
21-21: LGTM! Type-safe multi-value enum check.The migration to enum constants with strict comparison provides better type safety for the select/radio type check.
tests/Unit/Domain/Subscription/Service/Provider/SelectOrRadioValueProviderTest.php (4)
7-7: LGTM! Enum import for type-safe testing.
22-22: LGTM! Adapted to enum-based types.
26-26: LGTM! Adapted to enum-based types.
36-36: LGTM! Adapted to enum-based types.The test correctly uses enum constants to verify the provider behavior with different attribute types.
src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php
Show resolved
Hide resolved
src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php
Outdated
Show resolved
Hide resolved
src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php
Outdated
Show resolved
Hide resolved
src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php
Show resolved
Hide resolved
src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php
Outdated
Show resolved
Hide resolved
tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php
Show resolved
Hide resolved
tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php
Show resolved
Hide resolved
tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php
Outdated
Show resolved
Hide resolved
TatevikGr
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Domain/Subscription/Repository/DynamicListAttrRepository.php (1)
21-188: Repository still couples the domain to Doctrine.This class lives in
src/Domain/**yet it opens transactions, runs inserts/updates, etc. directly onDoctrine\DBAL\Connection. Our domain guideline forbids persistence side effects here—repositories in the domain should be pure abstractions. Please move this concrete DBAL implementation into the infrastructure layer (or wrap it behind an interface) and keep only the contract in the domain. As per coding guidelines
♻️ Duplicate comments (1)
src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (1)
53-78: Move schema creation out of the domain layer.This manager sits under
src/Domain/**yet it still drives Doctrine schema APIs (AbstractSchemaManager::createTable, transactions, validation). Per our domain-purity rule we can’t have persistence side effects here—table creation has to live in application/infrastructure. Please strip the SchemaManager dependency and push this logic into an infra service or handler, leaving the domain to express intent only. As per coding guidelines
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
config/services/managers.yml(1 hunks)resources/translations/messages.en.xlf(1 hunks)src/Domain/Subscription/Model/SubscriberAttributeDefinition.php(2 hunks)src/Domain/Subscription/Repository/DynamicListAttrRepository.php(4 hunks)src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php(1 hunks)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php(1 hunks)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php(1 hunks)tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php(1 hunks)tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php(8 hunks)tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php(1 hunks)tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrTablesManagerTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php
- src/Domain/Subscription/Model/SubscriberAttributeDefinition.php
🧰 Additional context used
📓 Path-based instructions (1)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce strict domain purity:
- ❌ Do not allow infrastructure persistence side effects here.
- Flag ANY usage of Doctrine persistence APIs, especially:
- $entityManager->flush(...), $this->entityManager->flush(...)
- $em->persist(...), $em->remove(...)
- direct transaction control ($em->beginTransaction(), commit(), rollback())
- If found, request moving these calls to application-layer Command handlers or background Jobs.
- Also flag repositories in Domain that invoke flush/transactional logic; Domain repositories should be abstractions without side effects.
- Encourage domain events/outbox or return-values to signal write-intent, leaving orchestration to Commands/Jobs.
Files:
src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.phpsrc/Domain/Subscription/Service/Manager/DynamicListAttrManager.phpsrc/Domain/Subscription/Repository/DynamicListAttrRepository.phpsrc/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php
🧬 Code graph analysis (6)
tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrTablesManagerTest.php (2)
src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php (1)
SubscriberAttributeDefinitionRepository(12-31)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (3)
DynamicListAttrTablesManager(15-80)resolveTableName(28-48)createOptionsTableIfNotExists(53-79)
tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php (6)
src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php (1)
AttributeDefinitionDto(10-29)src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php (1)
SubscriberAttributeDefinitionRepository(12-31)src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (2)
AttributeDefinitionManager(14-110)update(68-94)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (1)
DynamicListAttrManager(11-244)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (1)
DynamicListAttrTablesManager(15-80)src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (7)
getType(52-55)getListOrder(57-60)getDefaultValue(62-65)isRequired(67-70)getTableName(72-75)setName(78-82)getName(46-49)
src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (2)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (2)
DynamicListAttrDto(10-33)__construct(19-32)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (10)
DynamicListAttrRepository(16-188)__construct(20-27)transactional(29-40)insertMany(73-80)updateById(82-89)deleteById(91-98)existsById(100-108)getAll(172-187)insertOne(55-67)existsByName(154-170)
tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php (2)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (1)
DynamicListAttrDto(10-33)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (1)
DynamicListAttrRepository(16-188)
src/Domain/Subscription/Repository/DynamicListAttrRepository.php (2)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (2)
DynamicListAttrDto(10-33)__construct(19-32)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (1)
__construct(19-26)
src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (4)
src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php (2)
SubscriberAttributeDefinitionRepository(12-31)existsByTableName(21-30)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (1)
__construct(20-27)src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (1)
__construct(22-34)src/Domain/Subscription/Model/AttributeTypeEnum.php (1)
isMultiValued(25-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (1)
resources/translations/messages.en.xlf (1)
737-740: No issues found—the distinction is intentional.The new translation "Invalid email: %email%" serves a different purpose than the existing "Invalid email address: %email%". The former is used in CSV subscriber import (SubscriberCsvImporter.php), while the latter is used in the test email command (SendTestEmailCommand.php). These are separate features with appropriately different messaging, so no consolidation is needed.
tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Domain/Subscription/Repository/DynamicListAttrRepository.php(4 hunks)src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.php(1 hunks)src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php(3 hunks)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php(1 hunks)tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php(1 hunks)tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce strict domain purity:
- ❌ Do not allow infrastructure persistence side effects here.
- Flag ANY usage of Doctrine persistence APIs, especially:
- $entityManager->flush(...), $this->entityManager->flush(...)
- $em->persist(...), $em->remove(...)
- direct transaction control ($em->beginTransaction(), commit(), rollback())
- If found, request moving these calls to application-layer Command handlers or background Jobs.
- Also flag repositories in Domain that invoke flush/transactional logic; Domain repositories should be abstractions without side effects.
- Encourage domain events/outbox or return-values to signal write-intent, leaving orchestration to Commands/Jobs.
Files:
src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.phpsrc/Domain/Subscription/Service/Manager/DynamicListAttrManager.phpsrc/Domain/Subscription/Service/Manager/AttributeDefinitionManager.phpsrc/Domain/Subscription/Repository/DynamicListAttrRepository.php
🧬 Code graph analysis (5)
tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php (4)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (1)
DynamicListAttrDto(10-33)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (1)
DynamicListAttrRepository(17-203)src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.php (1)
SubscriberAttributeValueRepository(16-93)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (3)
DynamicListAttrManager(12-263)insertOptions(23-64)syncOptions(66-143)
tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php (6)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (1)
DynamicListAttrDto(10-33)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (4)
DynamicListAttrRepository(17-203)getAll(187-202)fetchOptionNames(115-134)fetchSingleOptionName(136-153)src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.php (1)
SubscriberAttributeValueRepository(16-93)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (2)
DynamicListAttrManager(12-263)insertOptions(23-64)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (2)
DynamicListAttrTablesManager(15-80)createOptionsTableIfNotExists(53-79)src/TestingSupport/Traits/DatabaseTestTrait.php (1)
loadSchema(81-92)
src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (4)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (2)
DynamicListAttrDto(10-33)__construct(19-32)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (9)
DynamicListAttrRepository(17-203)__construct(21-28)transactional(30-41)insertMany(74-81)updateById(83-90)deleteById(92-99)getAll(187-202)insertOne(56-68)existsByName(155-181)src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.php (2)
SubscriberAttributeValueRepository(16-93)existsByAttributeAndValue(80-92)src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (1)
__construct(22-34)
src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (5)
src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (4)
DynamicListAttrManager(12-263)__construct(14-18)insertOptions(23-64)syncOptions(66-143)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (4)
DynamicListAttrTablesManager(15-80)__construct(19-26)resolveTableName(28-48)createOptionsTableIfNotExists(53-79)src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php (1)
__construct(17-28)src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (1)
__construct(19-32)src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (3)
setDefaultValue(107-111)setTableName(119-123)getTableName(72-75)
src/Domain/Subscription/Repository/DynamicListAttrRepository.php (2)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (2)
DynamicListAttrDto(10-33)__construct(19-32)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (1)
__construct(19-26)
🪛 GitHub Actions: phpList Core Build
tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php
[error] 30-30: Unknown named parameter $subscriberAttributeValueRepository. Command: vendor/bin/phpunit tests/Unit/ --testdox
[error] 47-47: Unknown named parameter $subscriberAttributeValueRepository. Command: vendor/bin/phpunit tests/Unit/ --testdox
[error] 58-58: Unknown named parameter $subscriberAttributeValueRepository. Command: vendor/bin/phpunit tests/Unit/ --testdox
src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php
Outdated
Show resolved
Hide resolved
src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php
Outdated
Show resolved
Hide resolved
tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php
Show resolved
Hide resolved
tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php (1)
101-137: Add test coverage for upgrading to multi-valued types.The current test uses
AttributeTypeEnum::Text(non-multi-valued), which skips the entire multi-valued branch inupdate()(lines 88-94 of the production code). You'll want a test that verifies updating an existing attribute from a non-multi-valued type to a multi-valued one (e.g., Text → Checkbox) correctly provisions the table, assigns the tableName, and syncs options.Would you like me to generate a test case that covers this scenario?
♻️ Duplicate comments (2)
src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (2)
88-88: Null check required before calling isMultiValued().The DTO's type field is nullable, so calling
->isMultiValued()directly will crash when type is null. Add a null guard before method invocation.Apply this diff:
- if ($attributeDefinitionDto->type->isMultiValued()) { + if ($attributeDefinitionDto->type !== null && $attributeDefinitionDto->type->isMultiValued()) {
88-94: Assign resolved table name to the entity.When the attribute has no existing table name and a new one is resolved (line 89-90), that
$tableNameis used to create the physical table and sync options, but it's never assigned back to$attributeDefinitionviasetTableName(). This leaves the entity's tableName property null even though the table exists and has data, creating an inconsistency with your create() flow (which does set it at line 56).Apply this diff:
if ($attributeDefinitionDto->type !== null && $attributeDefinitionDto->type->isMultiValued()) { $tableName = $attributeDefinition->getTableName() ?? $this->dynamicTablesManager ->resolveTableName(name: $attributeDefinitionDto->name, type: $attributeDefinitionDto->type); + if ($attributeDefinition->getTableName() === null) { + $attributeDefinition->setTableName($tableName); + } $this->dynamicTablesManager->createOptionsTableIfNotExists($tableName); $this->dynamicListAttrManager->syncOptions($tableName, $attributeDefinitionDto->options); }
🧹 Nitpick comments (4)
tests/Support/DBAL/FakeDriverException.php (1)
18-18: Minor: Consider nullable type consistency.Line 18 uses
string|nullwhile lines 22 and 30 use?string. Both forms are equivalent, but using?stringconsistently would align better with the rest of the codebase style.Apply this diff if you prefer consistent nullable syntax:
- private string|null $sqlState; + private ?string $sqlState;tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php (3)
47-48: Remove redundant initialization.Line 47 initializes the variable to
nullonly to immediately overwrite it on line 48. You can simplify this:- $subscriberAttributeValueRepo = null; - $subscriberAttributeValueRepo = self::getContainer()->get(SubscriberAttributeValueRepository::class); + $subscriberAttributeValueRepo = self::getContainer()->get(SubscriberAttributeValueRepository::class);
62-63: Fix comment to match actual table name.The comment says "colors" but the code uses "colours". Keep them consistent.
- // Create the dynamic options table for logical name "colors" + // Create the dynamic options table for logical name "colours" $this->tablesManager->createOptionsTableIfNotExists('colours');
60-95: Consider adding test coverage for syncOptions.This test validates
insertOptionsand fetch operations nicely, butDynamicListAttrManager::syncOptionshandles more complex scenarios (updates, case-insensitive matching, pruning unused options). Adding a test for that method would strengthen confidence in the refactored attribute system.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.coderabbit.yaml(1 hunks)config/packages/messenger.yaml(1 hunks)config/services/messenger.yml(1 hunks)resources/translations/messages.en.xlf(1 hunks)src/Domain/Subscription/Message/DynamicTableMessage.php(1 hunks)src/Domain/Subscription/MessageHandler/DynamicTableMessageHandler.php(1 hunks)src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php(2 hunks)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php(1 hunks)tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php(1 hunks)tests/Support/DBAL/FakeDriverException.php(1 hunks)tests/Unit/Domain/Subscription/MessageHandler/DynamicTableMessageHandlerTest.php(1 hunks)tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php(8 hunks)tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php(1 hunks)tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrTablesManagerTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php
- tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrTablesManagerTest.php
- tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php
🧰 Additional context used
📓 Path-based instructions (2)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce strict domain purity:
- ❌ Do not allow infrastructure persistence side effects here.
- Flag ANY usage of Doctrine persistence APIs, especially:
- $entityManager->flush(...), $this->entityManager->flush(...)
- $em->persist(...), $em->remove(...)
- direct transaction control ($em->beginTransaction(), commit(), rollback())
- If found, request moving these calls to application-layer Command handlers or background Jobs.
- Also flag repositories in Domain that invoke flush/transactional logic; Domain repositories should be abstractions without side effects.
- Encourage domain events/outbox or return-values to signal write-intent, leaving orchestration to Commands/Jobs.
- Only exception to this rule is when the service is orchestrating multiple domain services with complex use cases
Files:
src/Domain/Subscription/MessageHandler/DynamicTableMessageHandler.phpsrc/Domain/Subscription/Message/DynamicTableMessage.phpsrc/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php
src/**/MessageHandler/**
⚙️ CodeRabbit configuration file
src/**/MessageHandler/**: Background jobs/workers may perform persistence.
- ✅ Allow $entityManager->flush() here when the job is the orchestration boundary.
- Verify idempotency and that flush frequency is appropriate (batching where practical).
- Ensure no domain-layer code invoked by the job performs flush/transaction control.
Files:
src/Domain/Subscription/MessageHandler/DynamicTableMessageHandler.php
🧬 Code graph analysis (6)
src/Domain/Subscription/MessageHandler/DynamicTableMessageHandler.php (2)
src/Domain/Subscription/Message/DynamicTableMessage.php (3)
DynamicTableMessage(7-20)__construct(11-14)getTableName(16-19)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (1)
__construct(17-24)
src/Domain/Subscription/Message/DynamicTableMessage.php (2)
src/Domain/Subscription/MessageHandler/DynamicTableMessageHandler.php (1)
__construct(18-20)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (1)
__construct(17-24)
tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php (6)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (1)
DynamicListAttrDto(10-33)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (4)
DynamicListAttrRepository(17-203)getAll(187-202)fetchOptionNames(115-134)fetchSingleOptionName(136-153)src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.php (1)
SubscriberAttributeValueRepository(16-93)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (2)
DynamicListAttrManager(12-263)insertOptions(23-64)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (2)
DynamicListAttrTablesManager(13-54)createOptionsTableIfNotExists(48-53)src/TestingSupport/Traits/DatabaseTestTrait.php (1)
loadSchema(81-92)
tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php (6)
src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php (1)
AttributeDefinitionDto(10-29)src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php (1)
SubscriberAttributeDefinitionRepository(12-31)src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (2)
AttributeDefinitionManager(14-113)update(68-97)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (1)
DynamicListAttrTablesManager(13-54)src/Domain/Subscription/Validator/AttributeTypeValidator.php (1)
AttributeTypeValidator(13-51)src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (3)
getType(52-55)getTableName(72-75)setName(78-82)
src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (6)
src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (4)
DynamicListAttrManager(12-263)__construct(14-18)insertOptions(23-64)syncOptions(66-143)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (4)
DynamicListAttrTablesManager(13-54)__construct(17-24)resolveTableName(26-46)createOptionsTableIfNotExists(48-53)src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php (2)
__construct(17-28)AttributeDefinitionDto(10-29)src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php (2)
SubscriberAttributeDefinitionRepository(12-31)findOneByName(16-19)src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (3)
setDefaultValue(107-111)setTableName(119-123)getTableName(72-75)src/Domain/Subscription/Model/AttributeTypeEnum.php (1)
isMultiValued(25-35)
tests/Unit/Domain/Subscription/MessageHandler/DynamicTableMessageHandlerTest.php (2)
src/Domain/Subscription/Message/DynamicTableMessage.php (1)
DynamicTableMessage(7-20)tests/Support/DBAL/FakeDriverException.php (1)
FakeDriverException(16-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (8)
.coderabbit.yaml (1)
23-23: Exception aligns with PR orchestration patterns.The exception appropriately documents the orchestration scenario introduced in this PR (e.g., DynamicListAttrManager coordinating multiple repositories and domain services). However, consider being more prescriptive to prevent future misuse—perhaps clarifying what constitutes "complex use cases" or linking to concrete examples. For now, the phrasing is permissive enough to support legitimate orchestration without being a major concern.
tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php (2)
1-23: LGTM!Clean setup with proper imports and test infrastructure. The docblock clearly describes the test's purpose.
97-116: LGTM!Solid cleanup logic. Using raw SQL for teardown is pragmatic, and the try-catch-finally ensures test failures aren't masked by cleanup issues.
resources/translations/messages.en.xlf (1)
733-736: No issues found—both translation keys serve different purposes.The verification confirms these aren't duplicates. "Invalid email address: %email%" is used in the test email command, while "Invalid email: %email%" is used in the CSV importer. Both are legitimately needed for their respective contexts.
src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (2)
19-34: Dependencies wired correctly for dynamic list attribute orchestration.The new managers are properly injected to support the dynamic table name resolution and option synchronization features. The orchestration of multiple domain services here is appropriate.
47-63: Table provisioning in create() looks solid.The flow correctly resolves the table name, assigns it to the entity, persists, then creates the physical table and seeds options. The null guard at line 60 properly handles non-multi-valued types.
tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php (2)
27-41: Test setup looks good.The MessageBusInterface mock returns an Envelope as expected, and using a real
DynamicListAttrTablesManagerinstance provides better integration coverage while keeping the test fast. The wiring correctly reflects the production constructor signature.
69-194: Remaining test methods updated appropriately.The exception tests and delete test correctly use
AttributeTypeEnumand wire in the new dependencies. No concerns here.
tests/Unit/Domain/Subscription/MessageHandler/DynamicTableMessageHandlerTest.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.coderabbit.yaml (1)
12-46: Solid architectural guidelines added to support the refactoring.The three new
path_instructionssections establish clear boundaries for where persistence operations belong in your codebase—Domain (no side effects), Command (orchestration), and MessageHandler (schema management allowed). This aligns nicely with the PR's objective of managing dynamic attribute table names and will help reviewers flag placement issues early.The guidance is concrete (specific Doctrine API calls flagged, idempotency checks, transactional patterns emphasized) and well-formatted. A few minor observations:
- Terminology: The Domain section mentions "Infrastructure or Application layer (e.g. MessageHandler)" but MessageHandler is already its own path rule. You might refine "Infrastructure" to just "Application layer" for clarity if there's no separate Infrastructure rule.
- Emoji rendering: The ❌✅
⚠️ symbols are helpful for scannability but may not render identically on all systems (e.g., some terminals). Consider adding fallback text like[No],[Yes],[Warning]if broad platform support matters.- Maintainability: These are quite detailed instructions; as the codebase evolves, keep them in sync with actual patterns or they may become sources of false positives.
Overall, this is a strong foundation for enforcing architectural intent during code review.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.coderabbit.yaml(1 hunks)tests/Unit/Domain/Subscription/MessageHandler/DynamicTableMessageHandlerTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Unit/Domain/Subscription/MessageHandler/DynamicTableMessageHandlerTest.php
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php (1)
129-137: Guardupdate()against null attribute types
AttributeDefinitionDto::typeis declared nullable, butAttributeDefinitionManager::update()now calls$attributeDefinitionDto->type->isMultiValued()without a null-check. When a legacy attribute (or any edit where the UI sends no type) hits this path, PHP will fatally error before we ever reach the validator. While touching that branch, we also need to persist the newly resolved table name back onto the entity—right now the code computes$tableNamebut never callssetTableName(), so multi-valued upgrades never retain the generated table.Please apply a fix along these lines:
diff --git a/src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php b/src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php @@ - if ($attributeDefinitionDto->type->isMultiValued()) { - $tableName = $attributeDefinition->getTableName() ?? $this->dynamicTablesManager - ->resolveTableName(name: $attributeDefinitionDto->name, type: $attributeDefinitionDto->type); - $this->dynamicTablesManager->createOptionsTableIfNotExists($tableName); - - $this->dynamicListAttrManager->syncOptions($tableName, $attributeDefinitionDto->options); - } + $type = $attributeDefinitionDto->type; + if ($type !== null && $type->isMultiValued()) { + $tableName = $attributeDefinition->getTableName() + ?? $this->dynamicTablesManager->resolveTableName( + name: $attributeDefinitionDto->name, + type: $type, + ); + $attributeDefinition->setTableName($tableName); + $this->dynamicTablesManager->createOptionsTableIfNotExists($tableName); + $this->dynamicListAttrManager->syncOptions($tableName, $attributeDefinitionDto->options); + }That keeps the update flow null-safe and ensures the generated list table sticks. Let me know if you’d like a follow-up test to cover the legacy-null case.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
config/PHPMD/rules.xml(0 hunks)resources/translations/messages.en.xlf(1 hunks)src/Core/Bootstrap.php(0 hunks)src/Domain/Common/Model/AttributeTypeEnum.php(1 hunks)src/Domain/Common/SystemInfoCollector.php(0 hunks)src/Domain/Configuration/Service/Provider/ConfigProvider.php(0 hunks)src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php(0 hunks)src/Domain/Identity/Command/ImportDefaultsCommand.php(0 hunks)src/Domain/Identity/Model/AdminAttributeDefinition.php(0 hunks)src/Domain/Identity/Model/Administrator.php(0 hunks)src/Domain/Identity/Model/Dto/AdminAttributeDefinitionDto.php(0 hunks)src/Domain/Identity/Model/Privileges.php(0 hunks)src/Domain/Identity/Service/AdminAttributeDefinitionManager.php(3 hunks)src/Domain/Identity/Validator/AttributeTypeValidator.php(1 hunks)src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php(0 hunks)src/Domain/Messaging/Model/Message/MessageMetadata.php(0 hunks)src/Domain/Messaging/Model/UserMessage.php(0 hunks)src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php(1 hunks)src/Domain/Subscription/Model/SubscriberAttributeDefinition.php(3 hunks)src/Domain/Subscription/Repository/DynamicListAttrRepository.php(4 hunks)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php(1 hunks)src/Domain/Subscription/Service/Manager/SubscriberManager.php(0 hunks)src/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.php(2 hunks)src/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.php(2 hunks)src/Domain/Subscription/Validator/AttributeTypeValidator.php(2 hunks)tests/Unit/Domain/Identity/Model/AdminAttributeDefinitionTest.php(0 hunks)tests/Unit/Domain/Identity/Service/AdminAttributeDefinitionManagerTest.php(2 hunks)tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php(8 hunks)tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrTablesManagerTest.php(1 hunks)tests/Unit/Domain/Subscription/Service/Provider/CheckboxGroupValueProviderTest.php(3 hunks)tests/Unit/Domain/Subscription/Service/Provider/ScalarValueProviderTest.php(2 hunks)tests/Unit/Domain/Subscription/Service/Provider/SelectOrRadioValueProviderTest.php(3 hunks)tests/Unit/Domain/Subscription/Validator/AttributeTypeValidatorTest.php(1 hunks)
💤 Files with no reviewable changes (15)
- src/Domain/Identity/Model/AdminAttributeDefinition.php
- src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php
- config/PHPMD/rules.xml
- src/Core/Bootstrap.php
- src/Domain/Configuration/Service/Provider/ConfigProvider.php
- src/Domain/Identity/Model/Dto/AdminAttributeDefinitionDto.php
- src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php
- src/Domain/Subscription/Service/Manager/SubscriberManager.php
- src/Domain/Common/SystemInfoCollector.php
- src/Domain/Messaging/Model/Message/MessageMetadata.php
- src/Domain/Identity/Model/Privileges.php
- src/Domain/Messaging/Model/UserMessage.php
- src/Domain/Identity/Command/ImportDefaultsCommand.php
- tests/Unit/Domain/Identity/Model/AdminAttributeDefinitionTest.php
- src/Domain/Identity/Model/Administrator.php
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/Unit/Domain/Subscription/Service/Provider/ScalarValueProviderTest.php
- tests/Unit/Domain/Subscription/Service/Provider/SelectOrRadioValueProviderTest.php
- tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrTablesManagerTest.php
- src/Domain/Subscription/Model/SubscriberAttributeDefinition.php
- resources/translations/messages.en.xlf
- tests/Unit/Domain/Subscription/Service/Provider/CheckboxGroupValueProviderTest.php
🧰 Additional context used
📓 Path-based instructions (1)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce strict domain purity:
- ❌ Do not allow persistence or transaction side effects here.
- Flag ANY usage of Doctrine persistence APIs, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()- ✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.⚠️ If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Also flag repositories in Domain that invoke flush/transactional logic;
Domain repositories should be abstractions without side effects.- Encourage using domain events or return-values to signal intent to write,
leaving persistence orchestration to Commands/Jobs.
Files:
src/Domain/Common/Model/AttributeTypeEnum.phpsrc/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.phpsrc/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.phpsrc/Domain/Subscription/Validator/AttributeTypeValidator.phpsrc/Domain/Subscription/Model/Dto/AttributeDefinitionDto.phpsrc/Domain/Identity/Validator/AttributeTypeValidator.phpsrc/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.phpsrc/Domain/Identity/Service/AdminAttributeDefinitionManager.phpsrc/Domain/Subscription/Repository/DynamicListAttrRepository.php
🧬 Code graph analysis (11)
src/Domain/Common/Model/AttributeTypeEnum.php (1)
src/Domain/Subscription/Exception/AttributeTypeChangeNotAllowed.php (1)
AttributeTypeChangeNotAllowed(9-19)
src/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.php (2)
src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (1)
getType(52-55)src/Domain/Identity/Model/AdminAttributeDefinition.php (1)
getType(66-69)
src/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.php (1)
src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (1)
getType(52-55)
tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php (6)
src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php (1)
AttributeDefinitionDto(10-29)src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php (1)
SubscriberAttributeDefinitionRepository(12-31)src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (2)
AttributeDefinitionManager(14-113)update(68-97)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (1)
DynamicListAttrManager(12-263)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (1)
DynamicListAttrTablesManager(13-54)src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (3)
getType(52-55)getTableName(72-75)setName(78-82)
tests/Unit/Domain/Subscription/Validator/AttributeTypeValidatorTest.php (1)
src/Domain/Subscription/Validator/AttributeTypeValidator.php (1)
validate(32-52)
src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php (3)
src/Domain/Identity/Model/AdminAttributeDefinition.php (1)
__construct(40-54)src/Domain/Identity/Model/Dto/AdminAttributeDefinitionDto.php (1)
__construct(12-19)src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (1)
__construct(19-32)
src/Domain/Identity/Validator/AttributeTypeValidator.php (4)
src/Domain/Identity/Service/AdminAttributeDefinitionManager.php (1)
__construct(20-28)src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php (1)
__construct(17-28)src/Domain/Identity/Model/AdminAttributeDefinition.php (1)
__construct(40-54)src/Domain/Identity/Model/Dto/AdminAttributeDefinitionDto.php (1)
__construct(12-19)
src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (3)
src/Domain/Subscription/Message/DynamicTableMessage.php (2)
DynamicTableMessage(7-20)__construct(11-14)src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php (2)
SubscriberAttributeDefinitionRepository(12-31)existsByTableName(21-30)src/Domain/Common/Model/AttributeTypeEnum.php (1)
isMultiValued(28-37)
tests/Unit/Domain/Identity/Service/AdminAttributeDefinitionManagerTest.php (2)
src/Domain/Subscription/Validator/AttributeTypeValidator.php (1)
AttributeTypeValidator(14-80)src/Domain/Identity/Validator/AttributeTypeValidator.php (1)
AttributeTypeValidator(15-74)
src/Domain/Identity/Service/AdminAttributeDefinitionManager.php (3)
src/Domain/Subscription/Validator/AttributeTypeValidator.php (1)
AttributeTypeValidator(14-80)src/Domain/Identity/Validator/AttributeTypeValidator.php (1)
AttributeTypeValidator(15-74)src/Domain/Identity/Model/AdminAttributeDefinition.php (1)
setDefaultValue(112-117)
src/Domain/Subscription/Repository/DynamicListAttrRepository.php (2)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (2)
DynamicListAttrDto(10-33)__construct(19-32)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (1)
__construct(17-24)
🪛 GitHub Actions: phpList Core Build
src/Domain/Subscription/Validator/AttributeTypeValidator.php
[error] 76-76: Test 'Throws exception for non string value' failed: expected InvalidArgumentException but Symfony\Component\Validator\Exception\ValidatorException was thrown. Message: 'Value must be an AttributeTypeEnum or string.'
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Domain/Subscription/Repository/DynamicListAttrRepository.php(4 hunks)src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php(2 hunks)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php(1 hunks)tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php(1 hunks)tests/Support/DBAL/FakeDriverException.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/Domain/**
⚙️ CodeRabbit configuration file
src/Domain/**: You are reviewing PHP domain-layer code. Enforce strict domain purity:
- ❌ Do not allow persistence or transaction side effects here.
- Flag ANY usage of Doctrine persistence APIs, especially:
$entityManager->flush(...),$this->entityManager->flush(...)$em->persist(...),$em->remove(...)$em->beginTransaction(),$em->commit(),$em->rollback()- ✅ Accessing Doctrine metadata, schema manager, or read-only schema info is acceptable
as long as it does not modify state or perform writes.⚠️ If code is invoking actual table-creation, DDL execution, or schema synchronization,
then request moving that to the Infrastructure or Application layer (e.g. MessageHandler).- Also flag repositories in Domain that invoke flush/transactional logic;
Domain repositories should be abstractions without side effects.- Encourage using domain events or return-values to signal intent to write,
leaving persistence orchestration to Commands/Jobs.
Files:
src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.phpsrc/Domain/Subscription/Service/Manager/DynamicListAttrManager.phpsrc/Domain/Subscription/Repository/DynamicListAttrRepository.php
🧬 Code graph analysis (4)
src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php (8)
src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (4)
DynamicListAttrManager(12-263)__construct(14-18)insertOptions(23-64)syncOptions(66-143)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (4)
DynamicListAttrTablesManager(13-54)__construct(17-24)resolveTableName(26-46)createOptionsTableIfNotExists(48-53)src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php (2)
__construct(17-28)AttributeDefinitionDto(10-29)src/Domain/Subscription/Message/DynamicTableMessage.php (2)
__construct(11-14)getTableName(16-19)src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (1)
__construct(19-32)src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php (2)
SubscriberAttributeDefinitionRepository(12-31)findOneByName(16-19)src/Domain/Subscription/Model/SubscriberAttributeDefinition.php (7)
setName(78-82)setType(84-98)setListOrder(100-104)setRequired(112-116)setDefaultValue(106-110)setTableName(118-122)getTableName(72-75)src/Domain/Common/Model/AttributeTypeEnum.php (1)
isMultiValued(28-37)
tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php (6)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (1)
DynamicListAttrDto(10-33)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (4)
DynamicListAttrRepository(16-202)getAll(186-201)fetchOptionNames(114-133)fetchSingleOptionName(135-152)src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.php (1)
SubscriberAttributeValueRepository(16-93)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (2)
DynamicListAttrManager(12-263)insertOptions(23-64)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (2)
DynamicListAttrTablesManager(13-54)createOptionsTableIfNotExists(48-53)src/TestingSupport/Traits/DatabaseTestTrait.php (1)
loadSchema(81-92)
src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (3)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (2)
DynamicListAttrDto(10-33)__construct(19-32)src/Domain/Subscription/Repository/DynamicListAttrRepository.php (9)
DynamicListAttrRepository(16-202)__construct(20-27)transactional(29-40)insertMany(73-80)updateById(82-89)deleteById(91-98)getAll(186-201)insertOne(55-67)isNameTakenByOtherRecord(154-180)src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.php (2)
SubscriberAttributeValueRepository(16-93)existsByAttributeAndValue(80-92)
src/Domain/Subscription/Repository/DynamicListAttrRepository.php (5)
src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php (2)
DynamicListAttrDto(10-33)__construct(19-32)src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php (1)
__construct(14-18)src/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.php (1)
__construct(15-17)src/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.php (1)
__construct(14-16)src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php (1)
__construct(17-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: phpList on PHP 8.1 [Build, Test]
- GitHub Check: phpList on PHP 8.1 [Build, Test]
🔇 Additional comments (4)
tests/Support/DBAL/FakeDriverException.php (4)
1-15: Clean setup and helpful documentation.The imports are spot-on and the docblock clearly explains why this test double exists. Nice work making it obvious this is for testing without real driver dependencies.
16-17: Solid test double implementation.Extending Exception while implementing DriverException is the right approach here. The naming makes it immediately clear this is a fake for testing purposes.
18-28: Constructor looks good.The parameter defaults make this easy to use in tests, and you're properly chaining to the parent Exception while storing the SQL state. Clean implementation.
30-33: Interface method correctly implemented.The getSQLState() method does exactly what it needs to—returns the stored SQL state. Straightforward and correct.
| if ($tableName) { | ||
| $this->dynamicTablesManager->createOptionsTableIfNotExists($tableName); | ||
| $this->dynamicListAttrManager->insertOptions($tableName, $attributeDefinitionDto->options); | ||
| } |
There was a problem hiding this comment.
Move persistence orchestration out of the domain layer.
These calls dispatch table creation and orchestrate transactional inserts directly from the domain manager, which violates the domain-purity guideline. Consider emitting a domain event (e.g., AttributeDefinitionCreated) that an Application layer handler can consume to handle table setup and option insertion, keeping the domain free of persistence side effects.
As per coding guidelines.
🤖 Prompt for AI Agents
In src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php around
lines 60 to 63, the domain manager directly orchestrates persistence (creating
dynamic tables and inserting options) which violates domain-purity; refactor by
removing direct calls to DynamicTablesManager and DynamicListAttrManager from
this class, instead raise a domain event (e.g., AttributeDefinitionCreated with
necessary payload like tableName and options) and let an Application layer event
handler subscribe to that event to perform table creation and option insertion
within the appropriate transactional and infrastructure context.
| if ($attributeDefinitionDto->type && $attributeDefinitionDto->type->isMultiValued()) { | ||
| $tableName = $attributeDefinition->getTableName() ?? $this->dynamicTablesManager | ||
| ->resolveTableName(name: $attributeDefinitionDto->name, type: $attributeDefinitionDto->type); | ||
| $this->dynamicTablesManager->createOptionsTableIfNotExists($tableName); | ||
|
|
||
| $this->dynamicListAttrManager->syncOptions($tableName, $attributeDefinitionDto->options); | ||
| } |
There was a problem hiding this comment.
Move persistence orchestration out of the domain layer.
Similar to create(), this method orchestrates table creation and transactional option synchronization directly in the domain manager. Refactor to emit a domain event (e.g., AttributeDefinitionUpdated) so an Application layer handler can manage the table setup and option sync, preserving domain purity.
As per coding guidelines.
🤖 Prompt for AI Agents
In src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php around
lines 88–94, remove the direct persistence orchestration (calls to
dynamicTablesManager->createOptionsTableIfNotExists and
dynamicListAttrManager->syncOptions) from the domain manager and instead raise a
domain event (e.g., AttributeDefinitionUpdated) carrying the necessary data
(attribute id, name, type, resolved/desired tableName, and options). Replace the
two calls with a single event dispatch; implement an Application layer event
handler to perform table creation and option synchronization (including
transactional handling) so all infrastructure work runs outside the domain layer
per guidelines.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests
Thanks for contributing to phpList!