Refactor attribute type system to use enums and manage dynamic list attributes table names #369
Conversation
📝 WalkthroughWalkthroughAdds enum-backed attribute types and a new exception; introduces dynamic-list DTOs, repositories, managers and tests for creating/syncing option tables; renames CSV mapper/importer classes and updates call sites; adjusts DI/config and multiple tests to the new APIs. (49 words) Changes
Sequence Diagram(s)mermaid mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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.
| * Handles full subscriber import workflow, including DB transactions and message dispatching. | ||
| * | ||
| * Note: Although this lives in the Service namespace, it acts as an *application service* — | ||
| * it orchestrates multiple domain services and manages transactions/flushes directly. | ||
| * This is an intentional exception for this complex import use case. | ||
| * |
There was a problem hiding this comment.
Domain layer purity concern: persistence operations in Domain.
While the docblock acknowledges this service uses transactions and flush() calls, the coding guidelines for src/Domain/** are strict: domain-layer code should not contain infrastructure persistence side effects like $entityManager->flush(), beginTransaction(), commit(), or rollback().
The guidelines recommend moving transaction orchestration and flush operations to application-layer Command handlers or background Jobs, with the Domain layer signaling write-intent via domain events, outbox patterns, or return values.
Consider refactoring this as an application-layer service that coordinates Domain services, rather than housing transaction management within the Domain namespace.
As per coding guidelines
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
| public function __construct( | ||
| private readonly SubscriberAttributeDefinitionRepository $definitionRepository, | ||
| private readonly Connection $connection, | ||
| string $dbPrefix = 'phplist_', | ||
| string $dynamicListTablePrefix = 'listattr_', | ||
| ) { | ||
| $this->prefix = $dbPrefix . $dynamicListTablePrefix; | ||
| } | ||
|
|
||
| public function resolveTableName(string $name, ?AttributeTypeEnum $type): ?string | ||
| { | ||
| if ($type === null) { | ||
| return null; | ||
| } | ||
|
|
||
| $shouldHaveBackingTable = match ($type) { | ||
| AttributeTypeEnum::Select, | ||
| AttributeTypeEnum::Checkbox, | ||
| AttributeTypeEnum::MultiSelect, | ||
| AttributeTypeEnum::CheckboxGroup => true, | ||
| default => false, | ||
| }; | ||
|
|
||
| if (!$shouldHaveBackingTable) { | ||
| return null; | ||
| } | ||
|
|
||
| $base = u($name)->snake()->toString(); | ||
| $candidate = $base; | ||
| $index = 1; | ||
| while ($this->definitionRepository->existsByTableName($candidate)) { | ||
| $suffix = $index; | ||
| $candidate = $base . $suffix; | ||
| $index++; | ||
| } | ||
|
|
||
| return $candidate; | ||
| } | ||
|
|
||
| /** | ||
| * @throws InvalidArgumentException | ||
| */ | ||
| public function createOptionsTableIfNotExists(string $listTable): void | ||
| { | ||
| $schemaManager = $this->connection->createSchemaManager(); | ||
| $fullTableName = $this->prefix . $listTable; | ||
|
|
||
| if ($schemaManager->tablesExist([$fullTableName])) { | ||
| return; | ||
| } | ||
|
|
||
| if (!preg_match('/^[A-Za-z0-9_]+$/', $listTable)) { | ||
| throw new InvalidArgumentException('Invalid list table name: ' . $listTable); | ||
| } | ||
|
|
||
| $table = new Table($fullTableName); | ||
| $table->addColumn('id', 'integer', ['autoincrement' => true, 'notnull' => true]); | ||
| $table->addColumn('name', 'string', ['length' => 255, 'notnull' => false]); | ||
| $table->addColumn('listorder', 'integer', ['notnull' => false, 'default' => 0]); | ||
| $table->setPrimaryKey(['id']); | ||
| $table->addUniqueIndex(['name'], 'uniq_' . $fullTableName . '_name'); | ||
|
|
||
| $schemaManager->createTable($table); | ||
| } |
There was a problem hiding this comment.
Domain layer must not manage persistence or schema.
This manager lives under src/Domain/**, yet it opens Doctrine connections, runs transactions, and creates tables. That’s infrastructure work and violates the domain-purity rule we’re enforcing (no persistence side effects, no Doctrine APIs). Please push table-creation and DB wiring into an application/infrastructure service instead, leaving the domain to express intent only.
As per coding guidelines
🤖 Prompt for AI Agents
src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php lines
18-81: This class in the Domain layer is performing infrastructure work
(Doctrine Connection, SchemaManager, table creation). Remove all
persistence/schema code from this domain class: drop the Connection dependency
and the createOptionsTableIfNotExists method (or make it non-side-effecting),
keep resolveTableName as a pure function that only computes/returns a valid
table name candidate, and instead define a small Domain-level intent interface
(e.g. DynamicListTableCreator or a Domain Event) that describes "ensure options
table exists for X". Implement the actual table-creation logic in the
application/infrastructure layer by adding a concrete service that depends on
Doctrine and implements that interface (or listens to the event) and performs
validation/sanitization and SchemaManager calls there.
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.
| PhpList\Core\Domain\Subscription\Service\Manager\DynamicListAttrTablesManager: | ||
| autowire: true | ||
| autoconfigure: true | ||
| public: true | ||
| arguments: | ||
| $dbPrefix: '%database_prefix%' | ||
| $dynamicListTablePrefix: '%list_table_prefix%' |
There was a problem hiding this comment.
Avoid Doctrine schema management in the Domain layer.
Wiring DynamicListAttrTablesManager with Doctrine’s AbstractSchemaManager means the Domain service (src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager) is now creating tables directly. That’s a persistence side effect inside src/Domain/**, which our coding guidelines explicitly prohibit. Please move the schema creation logic into an application/infrastructure service and inject a domain-friendly abstraction so the Domain layer stays pure. As per coding guidelines
| if ($cur->name !== $dto->name) { | ||
| $nameExists = $this->dynamicListAttrRepository->existsByName($listTable, $dto->name); | ||
| if ($nameExists) { | ||
| throw new RuntimeException('Option name ' . $dto->name . ' already exists.'); | ||
| } | ||
| $updates['name'] = $dto->name; | ||
| } | ||
| if ($cur->listOrder !== $dto->listOrder) { | ||
| $updates['listorder'] = $dto->listOrder; | ||
| } | ||
|
|
||
| if ($updates) { | ||
| $this->dynamicListAttrRepository->updateById($listTable, (int)$dto->id, $updates); | ||
| } | ||
| $result[] = $dto; |
There was a problem hiding this comment.
Allow case-only renames without tripping the uniqueness check.
When updateRowsById() compares $cur->name and $dto->name, a case-only rename (e.g. “Option” → “OPTION”) hits existsByName() for the same row, sees it already “exists”, and throws. We should skip the uniqueness lookup when only casing changes, or otherwise exclude the current id from the check, so legitimate case adjustments don’t explode.
A quick fix is to compare on lowercase before calling existsByName():
- if ($cur->name !== $dto->name) {
+ if (mb_strtolower($cur->name) !== mb_strtolower($dto->name)) {
$nameExists = $this->dynamicListAttrRepository->existsByName($listTable, $dto->name);
if ($nameExists) {
throw new RuntimeException('Option name ' . $dto->name . ' already exists.');
}
$updates['name'] = $dto->name;
}🤖 Prompt for AI Agents
In src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php around
lines 224 to 238, the uniqueness check incorrectly blocks case-only renames
because existsByName() finds the same row; modify the logic to skip the
existsByName() call when the only change is casing (e.g. compare
strtolower($cur->name) !== strtolower($dto->name) before invoking
existsByName()), or if the repository supports it, call an
existsByNameExcludingId($listTable, $dto->name, (int)$dto->id) so the current
row is excluded from the uniqueness check; keep the rest of the update flow the
same (set $updates['name'] and call updateById when needed).
| $updated = $manager->update(attributeDefinition: $attribute, attributeDefinitionDto: $dto); | ||
|
|
||
| $this->assertSame('New', $updated->getName()); | ||
| $this->assertSame('text', $updated->getType()); | ||
| $this->assertSame(AttributeTypeEnum::Text, $updated->getType()); | ||
| $this->assertSame(5, $updated->getListOrder()); | ||
| $this->assertSame('Canada', $updated->getDefaultValue()); | ||
| $this->assertFalse($updated->isRequired()); | ||
| $this->assertSame('custom_attrs', $updated->getTableName()); | ||
| $this->assertSame(null, $updated->getTableName()); | ||
| } |
There was a problem hiding this comment.
Handle type upgrades to multi-valued attributes.
AttributeDefinitionManager::update() only syncs options when a table name already exists. If an existing attribute (tableName null) is changed to a multi-valued enum, we never resolve a table, create it, or insert the incoming options—the update quietly drops them. Please extend the update path to provision the table and seed options whenever the new type is multi-valued but no table exists yet. A sketch:
- if ($attributeDefinition->getTableName()) {
- $this->dynamicListAttrManager
- ->syncOptions($attributeDefinition->getTableName(), $attributeDefinitionDto->options);
- }
+ $currentTable = $attributeDefinition->getTableName();
+ if ($currentTable) {
+ $this->dynamicListAttrManager
+ ->syncOptions($currentTable, $attributeDefinitionDto->options);
+ } elseif ($attributeDefinitionDto->type?->isMultiValued()) {
+ $tableName = $this->dynamicTablesManager
+ ->resolveTableName($attributeDefinition->getName(), $attributeDefinitionDto->type);
+ $attributeDefinition->setTableName($tableName);
+ $this->dynamicTablesManager->createOptionsTableIfNotExists($tableName);
+ $this->dynamicListAttrManager->insertOptions($tableName, $attributeDefinitionDto->options);
+ }This keeps updates functional when admins promote a field to a multi-valued type.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php
around lines 128 to 136, the test exposes a bug where
AttributeDefinitionManager::update() does not provision a table or seed options
when an attribute with tableName == null is upgraded to a multi-valued enum
type; update() currently only syncs options if a table already exists. Modify
the update flow so that when the incoming attribute type is multi-valued (enum
or multi-select) and the current attribute has no tableName, the manager
creates/allocates a new table name, persists it to the attribute definition,
creates the physical options table (or calls the repository method that
provisions it), and inserts the provided options before returning the updated
entity; ensure this new branch handles transactions/errors consistently with the
existing table-sync path and add/adjust tests to cover promoting null-table
attributes to multi-valued with seeding.
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
| } elseif ($attributeDefinitionDto->type->isMultiValued()) { | ||
| $tableName = $this->dynamicTablesManager |
There was a problem hiding this comment.
Guard against null attribute types before calling methods.
When AttributeDefinitionDto::$type is null (which the DTO allows), this branch will throw on ->isMultiValued(). That’s a hard crash during updates. Please null-check first.
- } elseif ($attributeDefinitionDto->type->isMultiValued()) {
+ } elseif ($attributeDefinitionDto->type !== null && $attributeDefinitionDto->type->isMultiValued()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } elseif ($attributeDefinitionDto->type->isMultiValued()) { | |
| $tableName = $this->dynamicTablesManager | |
| } elseif ($attributeDefinitionDto->type !== null && $attributeDefinitionDto->type->isMultiValued()) { | |
| $tableName = $this->dynamicTablesManager |
🤖 Prompt for AI Agents
In src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php around
lines 91-92, the code calls $attributeDefinitionDto->type->isMultiValued()
without guarding against a null type; update the condition to check that
$attributeDefinitionDto->type is not null before invoking isMultiValued() (e.g.
use an explicit null check or short-circuiting like if
($attributeDefinitionDto->type &&
$attributeDefinitionDto->type->isMultiValued()) ), and handle the null case
appropriately (skip the multi-valued branch or follow the intended fallback
behavior).
| $tableName = $this->dynamicTablesManager | ||
| ->resolveTableName(name: $attributeDefinitionDto->name, type: $attributeDefinitionDto->type); | ||
| $this->dynamicListAttrManager->insertOptions($tableName, $attributeDefinitionDto->options); | ||
| } |
There was a problem hiding this comment.
Set up the dynamic table before inserting options.
If we hit this branch, we resolve a table name and immediately try to insert into it without ever assigning the table to the entity or creating the table. The insert will fail because the table doesn’t exist, and later lookups still see a null tableName. Let’s wire in both steps before inserting.
- } elseif ($attributeDefinitionDto->type->isMultiValued()) {
- $tableName = $this->dynamicTablesManager
- ->resolveTableName(name: $attributeDefinitionDto->name, type: $attributeDefinitionDto->type);
- $this->dynamicListAttrManager->insertOptions($tableName, $attributeDefinitionDto->options);
+ } elseif ($attributeDefinitionDto->type !== null && $attributeDefinitionDto->type->isMultiValued()) {
+ $tableName = $this->dynamicTablesManager
+ ->resolveTableName(name: $attributeDefinitionDto->name, type: $attributeDefinitionDto->type);
+ if ($tableName !== null) {
+ $attributeDefinition->setTableName($tableName);
+ $this->dynamicTablesManager->createOptionsTableIfNotExists($tableName);
+ $this->dynamicListAttrManager->insertOptions($tableName, $attributeDefinitionDto->options);
+ }
}🤖 Prompt for AI Agents
In src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php around
lines 92-95, the code resolves a dynamic table name and immediately calls
insertOptions which will fail because the table hasn't been created nor assigned
to the attribute entity; fix by after resolving $tableName: (1) assign it to the
attribute definition (e.g. $attributeDefinition->setTableName($tableName) or
$attributeDefinitionDto->tableName = $tableName), (2) ensure the table exists by
invoking the dynamicTablesManager create/ensure method (e.g.
createTableIfNotExists($tableName) or createTable($tableName,
$attributeDefinition->schema)), and then (3) call
$this->dynamicListAttrManager->insertOptions($tableName,
$attributeDefinitionDto->options); persist/save the updated attribute definition
if required.
| $this->manager = new DynamicListAttrManager( | ||
| dynamicListAttrRepository: $this->dynamicListAttrRepo, | ||
| subscriberAttributeValueRepository: $subscriberAttributeValueRepo | ||
| ); |
There was a problem hiding this comment.
Use the constructor’s actual parameter name here too.
The integration test hits the same fatal as the unit test because subscriberAttributeValueRepository isn’t a recognised named argument. PHPUnit never gets past setUp. Please line this up with the constructor signature (attributeValueRepo) so the test can exercise the code.
$this->manager = new DynamicListAttrManager(
dynamicListAttrRepository: $this->dynamicListAttrRepo,
- subscriberAttributeValueRepository: $subscriberAttributeValueRepo
+ attributeValueRepo: $subscriberAttributeValueRepo
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->manager = new DynamicListAttrManager( | |
| dynamicListAttrRepository: $this->dynamicListAttrRepo, | |
| subscriberAttributeValueRepository: $subscriberAttributeValueRepo | |
| ); | |
| $this->manager = new DynamicListAttrManager( | |
| dynamicListAttrRepository: $this->dynamicListAttrRepo, | |
| attributeValueRepo: $subscriberAttributeValueRepo | |
| ); |
🤖 Prompt for AI Agents
In
tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php
around lines 54-57, the test uses an unknown named constructor argument
subscriberAttributeValueRepository causing a fatal; change that named argument
to match the constructor signature (attributeValueRepo) so the
DynamicListAttrManager is instantiated with the correct named parameter and
setUp can complete.
| return new DynamicListAttrManager( | ||
| dynamicListAttrRepository: $this->listAttrRepo, | ||
| subscriberAttributeValueRepository: $this->subscriberAttributeValueRepo | ||
| ); |
There was a problem hiding this comment.
Fix the named parameter to match the constructor.
DynamicListAttrManager::__construct() takes attributeValueRepo, so this named argument triggers the “Unknown named parameter” fatal that’s currently killing the PHPUnit run. Swap to the right parameter name so the partials can even be instantiated.
return new DynamicListAttrManager(
dynamicListAttrRepository: $this->listAttrRepo,
- subscriberAttributeValueRepository: $this->subscriberAttributeValueRepo
+ attributeValueRepo: $this->subscriberAttributeValueRepo
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return new DynamicListAttrManager( | |
| dynamicListAttrRepository: $this->listAttrRepo, | |
| subscriberAttributeValueRepository: $this->subscriberAttributeValueRepo | |
| ); | |
| return new DynamicListAttrManager( | |
| dynamicListAttrRepository: $this->listAttrRepo, | |
| attributeValueRepo: $this->subscriberAttributeValueRepo | |
| ); |
🧰 Tools
🪛 GitHub Actions: phpList Core Build
[error] 30-30: Unknown named parameter $subscriberAttributeValueRepository. Command: vendor/bin/phpunit tests/Unit/ --testdox
🤖 Prompt for AI Agents
In tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php
around lines 28 to 31, the named constructor argument
subscriberAttributeValueRepository doesn't match
DynamicListAttrManager::__construct() which expects attributeValueRepo; replace
the named parameter with attributeValueRepo while keeping the same repository
variable (e.g. attributeValueRepo: $this->subscriberAttributeValueRepo) so the
partial can be instantiated without the "Unknown named parameter" fatal.
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Chores
Tests
Thanks for contributing to phpList!