diff --git a/.coderabbit.yaml b/.coderabbit.yaml index e6be6bb2..0bfc6829 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -9,32 +9,40 @@ reviews: high_level_summary_in_walkthrough: false poem: false path_instructions: - - path: "src/Domain/**" - instructions: | - 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. + - path: "src/Domain/**" + instructions: | + 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. - - path: "src/**/Command/**" - instructions: | - Application layer (Commands/Handlers) is the right place to coordinate persistence. - - ✅ It is acceptable to call $entityManager->flush() here. - - Check that flush is used atomically (once per unit of work) after all domain operations. - - Ensure no domain entity or domain service is calling flush; only the handler orchestrates it. - - Prefer $em->transactional(...) or explicit try/catch with rollback on failure. + - path: "src/**/Command/**" + instructions: | + Application layer (Commands/Handlers) is the right place to coordinate persistence. + - ✅ It is acceptable to call $entityManager->flush() here. + - Check that flush is used atomically (once per unit of work) after all domain operations. + - Ensure no domain entity or domain service is calling flush; only the handler orchestrates it. + - Prefer $em->transactional(...) or explicit try/catch with rollback on failure. - - path: "src/**/MessageHandler/**" - instructions: | - 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. + - path: "src/**/MessageHandler/**" + instructions: | + Background jobs/workers may perform persistence and schema management. + - ✅ Allow `$entityManager->flush()` when the job is the orchestration boundary. + - ✅ Allow table creation, migration, or schema synchronization (e.g. via Doctrine SchemaTool or SchemaManager), + as this is considered infrastructure-level orchestration. + - Verify idempotency for schema operations — e.g., check if a table exists before creating. + - Ensure domain-layer code invoked by the job remains free of persistence calls. + - Batch flush operations where practical. auto_review: enabled: true diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index aad3619f..0c6c6983 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,3 +1,3 @@ -"@coderabbitai summary" +@coderabbitai summary Thanks for contributing to phpList! diff --git a/composer.json b/composer.json index 4ea09327..821b5c99 100644 --- a/composer.json +++ b/composer.json @@ -77,7 +77,8 @@ "symfony/lock": "^6.4", "webklex/php-imap": "^6.2", "ext-imap": "*", - "tatevikgr/rss-feed": "dev-main" + "tatevikgr/rss-feed": "dev-main", + "ext-pdo": "*" }, "require-dev": { "phpunit/phpunit": "^9.5", diff --git a/config/PHPMD/rules.xml b/config/PHPMD/rules.xml index b8b0c3df..b3b8a8d4 100644 --- a/config/PHPMD/rules.xml +++ b/config/PHPMD/rules.xml @@ -8,22 +8,6 @@ - - - - - - diff --git a/config/packages/messenger.yaml b/config/packages/messenger.yaml index 38a75a1b..4193c501 100644 --- a/config/packages/messenger.yaml +++ b/config/packages/messenger.yaml @@ -30,4 +30,5 @@ framework: 'PhpList\Core\Domain\Messaging\Message\PasswordResetMessage': async_email 'PhpList\Core\Domain\Messaging\Message\CampaignProcessorMessage': async_email 'PhpList\Core\Domain\Messaging\Message\SyncCampaignProcessorMessage': sync + 'PhpList\Core\Domain\Subscription\Message\DynamicTableMessage': sync diff --git a/config/parameters.yml.dist b/config/parameters.yml.dist index 4e9e0cff..88f0b2b2 100644 --- a/config/parameters.yml.dist +++ b/config/parameters.yml.dist @@ -23,6 +23,8 @@ parameters: env(PHPLIST_DATABASE_PASSWORD): 'phplist' database_prefix: '%%env(DATABASE_PREFIX)%%' env(DATABASE_PREFIX): 'phplist_' + list_table_prefix: '%%env(LIST_TABLE_PREFIX)%%' + env(LIST_TABLE_PREFIX): 'listattr_' # Email configuration app.mailer_from: '%%env(MAILER_FROM)%%' diff --git a/config/services/managers.yml b/config/services/managers.yml index 2f72bd7e..282f5e1d 100644 --- a/config/services/managers.yml +++ b/config/services/managers.yml @@ -48,6 +48,21 @@ services: autowire: true autoconfigure: true + PhpList\Core\Domain\Subscription\Service\Manager\DynamicListAttrManager: + autowire: true + autoconfigure: true + + Doctrine\DBAL\Schema\AbstractSchemaManager: + factory: ['@doctrine.dbal.default_connection', 'createSchemaManager'] + + PhpList\Core\Domain\Subscription\Service\Manager\DynamicListAttrTablesManager: + autowire: true + autoconfigure: true + public: true + arguments: + $dbPrefix: '%database_prefix%' + $dynamicListTablePrefix: '%list_table_prefix%' + PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager: autowire: true autoconfigure: true diff --git a/config/services/mappers.yml b/config/services/mappers.yml index f84bd717..b0df7b58 100644 --- a/config/services/mappers.yml +++ b/config/services/mappers.yml @@ -4,10 +4,10 @@ services: autoconfigure: true public: false - PhpList\Core\Domain\Subscription\Service\CsvRowToDtoMapper: + PhpList\Core\Domain\Subscription\Service\ArrayToDtoMapper: autowire: true autoconfigure: true - PhpList\Core\Domain\Subscription\Service\CsvImporter: + PhpList\Core\Domain\Subscription\Service\CsvToDtoImporter: autowire: true autoconfigure: true diff --git a/config/services/messenger.yml b/config/services/messenger.yml index 214a4c86..79076663 100644 --- a/config/services/messenger.yml +++ b/config/services/messenger.yml @@ -29,3 +29,7 @@ services: autoconfigure: true tags: [ 'messenger.message_handler' ] + PhpList\Core\Domain\Subscription\MessageHandler\DynamicTableMessageHandler: + autowire: true + autoconfigure: true + tags: [ 'messenger.message_handler' ] diff --git a/config/services/repositories.yml b/config/services/repositories.yml index e9d4d8c6..bab5c2d4 100644 --- a/config/services/repositories.yml +++ b/config/services/repositories.yml @@ -70,7 +70,8 @@ services: PhpList\Core\Domain\Subscription\Repository\DynamicListAttrRepository: autowire: true arguments: - $prefix: '%database_prefix%' + $dbPrefix: '%database_prefix%' + $dynamicListTablePrefix: '%list_table_prefix%' PhpList\Core\Domain\Subscription\Repository\SubscriberHistoryRepository: parent: PhpList\Core\Domain\Common\Repository\AbstractRepository arguments: diff --git a/resources/translations/messages.en.xlf b/resources/translations/messages.en.xlf index efeb8ad2..02ca7140 100644 --- a/resources/translations/messages.en.xlf +++ b/resources/translations/messages.en.xlf @@ -342,10 +342,6 @@ Subscription not found for this subscriber and list. Subscription not found for this subscriber and list. - - Attribute definition already exists - Attribute definition already exists - Another attribute with this name already exists. Another attribute with this name already exists. @@ -734,6 +730,14 @@ Thank you. Conflict: email and foreign key refer to different subscribers. __Conflict: email and foreign key refer to different subscribers. + + Invalid email: %email% + __Invalid email: %email% + + + Value must be an AttributeTypeEnum or string. + __Value must be an AttributeTypeEnum or string. + diff --git a/src/Core/Bootstrap.php b/src/Core/Bootstrap.php index c56b6e4a..82ddb28f 100644 --- a/src/Core/Bootstrap.php +++ b/src/Core/Bootstrap.php @@ -168,8 +168,6 @@ private function assertConfigureHasBeenCalled(): void /** * Dispatches the current HTTP request (if there is any). * - * @SuppressWarnings("PHPMD.StaticAccess") - * * @return null * * @throws RuntimeException if configure has not been called before diff --git a/src/Domain/Common/Model/AttributeTypeEnum.php b/src/Domain/Common/Model/AttributeTypeEnum.php new file mode 100644 index 00000000..b76f5208 --- /dev/null +++ b/src/Domain/Common/Model/AttributeTypeEnum.php @@ -0,0 +1,58 @@ + true, + default => false, + }; + } + + public function canTransitionTo(self $toType): bool + { + if ($this === $toType) { + return true; + } + + if ($this->isMultiValued() !== $toType->isMultiValued()) { + return false; + } + + return true; + } + + public function assertTransitionAllowed(self $toType): void + { + if (!$this->canTransitionTo($toType)) { + throw new AttributeTypeChangeNotAllowed($this->value, $toType->value); + } + } +} diff --git a/src/Domain/Common/SystemInfoCollector.php b/src/Domain/Common/SystemInfoCollector.php index 56b30579..d6a5118c 100644 --- a/src/Domain/Common/SystemInfoCollector.php +++ b/src/Domain/Common/SystemInfoCollector.php @@ -24,7 +24,6 @@ public function __construct(RequestStack $requestStack, array $configuredKeys = /** * Return key=>value pairs (already sanitized for safe logging/HTML display). - * @SuppressWarnings(PHPMD.StaticAccess) * @return array */ public function collect(): array diff --git a/src/Domain/Configuration/Service/Provider/ConfigProvider.php b/src/Domain/Configuration/Service/Provider/ConfigProvider.php index a1db70fc..9dd24b08 100644 --- a/src/Domain/Configuration/Service/Provider/ConfigProvider.php +++ b/src/Domain/Configuration/Service/Provider/ConfigProvider.php @@ -24,7 +24,6 @@ public function __construct( } /** - * @SuppressWarnings(PHPMD.StaticAccess) * @throws InvalidArgumentException */ public function isEnabled(ConfigOption $key): bool @@ -43,7 +42,6 @@ public function isEnabled(ConfigOption $key): bool /** * Get configuration value by its key, from settings or default configs or default value (if provided) - * @SuppressWarnings(PHPMD.StaticAccess) * @throws InvalidArgumentException */ public function getValue(ConfigOption $key): ?string @@ -65,7 +63,6 @@ public function getValue(ConfigOption $key): ?string return $this->defaultConfigs->has($key->value) ? $this->defaultConfigs->get($key->value)['value'] : null; } - /** @SuppressWarnings(PHPMD.StaticAccess) */ public function getValueWithNamespace(ConfigOption $key): ?string { $full = $this->getValue($key); diff --git a/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php b/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php index bbe14a46..e97ba32e 100644 --- a/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php +++ b/src/Domain/Configuration/Service/Provider/DefaultConfigProvider.php @@ -7,7 +7,6 @@ use Symfony\Contracts\Translation\TranslatorInterface; // phpcs:disable Generic.Files.LineLength -/** @SuppressWarnings(PHPMD.StaticAccess) */ class DefaultConfigProvider { /** diff --git a/src/Domain/Identity/Command/ImportDefaultsCommand.php b/src/Domain/Identity/Command/ImportDefaultsCommand.php index a1ff627e..cf36e765 100644 --- a/src/Domain/Identity/Command/ImportDefaultsCommand.php +++ b/src/Domain/Identity/Command/ImportDefaultsCommand.php @@ -87,7 +87,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int /** * @return array - * @SuppressWarnings(PHPMD.StaticAccess) */ private function allPrivilegesGranted(): array { diff --git a/src/Domain/Identity/Model/AdminAttributeDefinition.php b/src/Domain/Identity/Model/AdminAttributeDefinition.php index b9a70346..055a4cf0 100644 --- a/src/Domain/Identity/Model/AdminAttributeDefinition.php +++ b/src/Domain/Identity/Model/AdminAttributeDefinition.php @@ -122,11 +122,4 @@ public function setRequired(?bool $required): self return $this; } - - public function setTableName(?string $tableName): self - { - $this->tableName = $tableName; - - return $this; - } } diff --git a/src/Domain/Identity/Model/Administrator.php b/src/Domain/Identity/Model/Administrator.php index 09ba46e0..9829c96e 100644 --- a/src/Domain/Identity/Model/Administrator.php +++ b/src/Domain/Identity/Model/Administrator.php @@ -167,8 +167,6 @@ public function setPrivileges(Privileges $privileges): self /** * @throws InvalidArgumentException - * - * @SuppressWarnings(PHPMD.StaticAccess) */ public function setPrivilegesFromArray(array $privilegesData): void { @@ -183,7 +181,6 @@ public function setPrivilegesFromArray(array $privilegesData): void $this->setPrivileges($privileges); } - /** @SuppressWarnings(PHPMD.StaticAccess) */ public function getPrivileges(): Privileges { return Privileges::fromSerialized($this->privileges); diff --git a/src/Domain/Identity/Model/Dto/AdminAttributeDefinitionDto.php b/src/Domain/Identity/Model/Dto/AdminAttributeDefinitionDto.php index 429faccb..114e0306 100644 --- a/src/Domain/Identity/Model/Dto/AdminAttributeDefinitionDto.php +++ b/src/Domain/Identity/Model/Dto/AdminAttributeDefinitionDto.php @@ -15,7 +15,6 @@ public function __construct( public readonly ?int $listOrder = null, public readonly ?string $defaultValue = null, public readonly ?bool $required = false, - public readonly ?string $tableName = null, ) { } } diff --git a/src/Domain/Identity/Model/Privileges.php b/src/Domain/Identity/Model/Privileges.php index 0b53de69..a0ec74c1 100644 --- a/src/Domain/Identity/Model/Privileges.php +++ b/src/Domain/Identity/Model/Privileges.php @@ -14,9 +14,6 @@ class Privileges */ private array $flags = []; - /** - * @SuppressWarnings(PHPMD.StaticAccess) - */ public function __construct(?array $flags = []) { foreach (PrivilegeFlag::cases() as $flag) { diff --git a/src/Domain/Identity/Service/AdminAttributeDefinitionManager.php b/src/Domain/Identity/Service/AdminAttributeDefinitionManager.php index e868cbc4..50a58bd0 100644 --- a/src/Domain/Identity/Service/AdminAttributeDefinitionManager.php +++ b/src/Domain/Identity/Service/AdminAttributeDefinitionManager.php @@ -8,7 +8,7 @@ use PhpList\Core\Domain\Identity\Model\Dto\AdminAttributeDefinitionDto; use PhpList\Core\Domain\Identity\Repository\AdminAttributeDefinitionRepository; use PhpList\Core\Domain\Identity\Exception\AttributeDefinitionCreationException; -use PhpList\Core\Domain\Subscription\Validator\AttributeTypeValidator; +use PhpList\Core\Domain\Identity\Validator\AttributeTypeValidator; use Symfony\Contracts\Translation\TranslatorInterface; class AdminAttributeDefinitionManager @@ -42,8 +42,7 @@ public function create(AdminAttributeDefinitionDto $attributeDefinitionDto): Adm ->setType($attributeDefinitionDto->type) ->setListOrder($attributeDefinitionDto->listOrder) ->setRequired($attributeDefinitionDto->required) - ->setDefaultValue($attributeDefinitionDto->defaultValue) - ->setTableName($attributeDefinitionDto->tableName); + ->setDefaultValue($attributeDefinitionDto->defaultValue); $this->definitionRepository->persist($attributeDefinition); @@ -65,8 +64,7 @@ public function update( ->setType($attributeDefinitionDto->type) ->setListOrder($attributeDefinitionDto->listOrder) ->setRequired($attributeDefinitionDto->required) - ->setDefaultValue($attributeDefinitionDto->defaultValue) - ->setTableName($attributeDefinitionDto->tableName); + ->setDefaultValue($attributeDefinitionDto->defaultValue); return $attributeDefinition; } diff --git a/src/Domain/Identity/Validator/AttributeTypeValidator.php b/src/Domain/Identity/Validator/AttributeTypeValidator.php new file mode 100644 index 00000000..c7bb3853 --- /dev/null +++ b/src/Domain/Identity/Validator/AttributeTypeValidator.php @@ -0,0 +1,74 @@ +normalizeToEnum($value); + + if (!in_array($enum, self::VALID_TYPES, true)) { + $validList = implode(', ', array_map( + static fn(AttributeTypeEnum $enum) => $enum->value, + self::VALID_TYPES + )); + + $message = $this->translator->trans( + 'Invalid attribute type: "%type%". Valid types are: %valid_types%', + [ + '%type%' => $enum->value, + '%valid_types%' => $validList, + ] + ); + + throw new ValidatorException($message); + } + } + + /** + * @throws InvalidArgumentException if value cannot be converted to AttributeTypeEnum + */ + private function normalizeToEnum(mixed $value): AttributeTypeEnum + { + if ($value instanceof AttributeTypeEnum) { + return $value; + } + + if (is_string($value)) { + try { + return AttributeTypeEnum::from($value); + } catch (Throwable) { + $lower = strtolower($value); + foreach (AttributeTypeEnum::cases() as $case) { + if ($case->value === $lower) { + return $case; + } + } + } + } + + throw new InvalidArgumentException( + $this->translator->trans('Value must be an AttributeTypeEnum or string.') + ); + } +} diff --git a/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php b/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php index 82e1e664..f0e4b5b1 100644 --- a/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php +++ b/src/Domain/Messaging/MessageHandler/CampaignProcessorMessageHandler.php @@ -27,7 +27,6 @@ /** * @SuppressWarnings(PHPMD.CouplingBetweenObjects) - * @SuppressWarnings(PHPMD.StaticAccess) * @SuppressWarnings(PHPMD.ExcessiveParameterList) */ #[AsMessageHandler] diff --git a/src/Domain/Messaging/Model/Message/MessageMetadata.php b/src/Domain/Messaging/Model/Message/MessageMetadata.php index e2c43c72..0d817c2e 100644 --- a/src/Domain/Messaging/Model/Message/MessageMetadata.php +++ b/src/Domain/Messaging/Model/Message/MessageMetadata.php @@ -49,9 +49,6 @@ public function __construct( $this->sendStart = $sendStart; } - /** - * @SuppressWarnings("PHPMD.StaticAccess") - */ public function getStatus(): ?MessageStatus { return MessageStatus::from($this->status); diff --git a/src/Domain/Messaging/Model/UserMessage.php b/src/Domain/Messaging/Model/UserMessage.php index cdb6b7c2..87150cd7 100644 --- a/src/Domain/Messaging/Model/UserMessage.php +++ b/src/Domain/Messaging/Model/UserMessage.php @@ -66,9 +66,6 @@ public function getViewed(): ?DateTime return $this->viewed; } - /** - * @SuppressWarnings("PHPMD.StaticAccess") - */ public function getStatus(): ?UserMessageStatus { return UserMessageStatus::from($this->status); diff --git a/src/Domain/Subscription/Exception/AttributeTypeChangeNotAllowed.php b/src/Domain/Subscription/Exception/AttributeTypeChangeNotAllowed.php new file mode 100644 index 00000000..60a466ae --- /dev/null +++ b/src/Domain/Subscription/Exception/AttributeTypeChangeNotAllowed.php @@ -0,0 +1,19 @@ +%s', + $oldType, + $newType + )); + } +} diff --git a/src/Domain/Subscription/Message/DynamicTableMessage.php b/src/Domain/Subscription/Message/DynamicTableMessage.php new file mode 100644 index 00000000..709bb710 --- /dev/null +++ b/src/Domain/Subscription/Message/DynamicTableMessage.php @@ -0,0 +1,20 @@ +tableName = $tableName; + } + + public function getTableName(): string + { + return $this->tableName; + } +} diff --git a/src/Domain/Subscription/MessageHandler/DynamicTableMessageHandler.php b/src/Domain/Subscription/MessageHandler/DynamicTableMessageHandler.php new file mode 100644 index 00000000..05d5e955 --- /dev/null +++ b/src/Domain/Subscription/MessageHandler/DynamicTableMessageHandler.php @@ -0,0 +1,50 @@ +schemaManager->tablesExist([$message->getTableName()])) { + return; + } + + if (!preg_match('/^[A-Za-z0-9_]+$/', $message->getTableName())) { + throw new InvalidArgumentException('Invalid list table name: ' . $message->getTableName()); + } + + $table = new Table($message->getTableName()); + $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_' . $message->getTableName() . '_name'); + + try { + $this->schemaManager->createTable($table); + } catch (TableExistsException $e) { + // Table was created by a concurrent process or a previous test run. + // Since this method is idempotent by contract, swallow the exception. + return; + } + } +} diff --git a/src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php b/src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php index 4907e7c0..73df4b93 100644 --- a/src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php +++ b/src/Domain/Subscription/Model/Dto/AttributeDefinitionDto.php @@ -4,18 +4,26 @@ namespace PhpList\Core\Domain\Subscription\Model\Dto; +use InvalidArgumentException; +use PhpList\Core\Domain\Common\Model\AttributeTypeEnum; + class AttributeDefinitionDto { /** * @SuppressWarnings("BooleanArgumentFlag") + * @param DynamicListAttrDto[] $options + * @throws InvalidArgumentException */ public function __construct( public readonly string $name, - public readonly ?string $type = null, + public readonly ?AttributeTypeEnum $type = null, public readonly ?int $listOrder = null, public readonly ?string $defaultValue = null, public readonly ?bool $required = false, - public readonly ?string $tableName = null, + public readonly array $options = [], ) { + if (trim($this->name) === '') { + throw new InvalidArgumentException('Name cannot be empty'); + } } } diff --git a/src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php b/src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php new file mode 100644 index 00000000..3e4625a3 --- /dev/null +++ b/src/Domain/Subscription/Model/Dto/DynamicListAttrDto.php @@ -0,0 +1,33 @@ +id = $id; + $this->name = $trimmed; + $this->listOrder = $listOrder; + } +} diff --git a/src/Domain/Subscription/Model/SubscriberAttributeDefinition.php b/src/Domain/Subscription/Model/SubscriberAttributeDefinition.php index 7b943b5c..54240c08 100644 --- a/src/Domain/Subscription/Model/SubscriberAttributeDefinition.php +++ b/src/Domain/Subscription/Model/SubscriberAttributeDefinition.php @@ -5,6 +5,7 @@ namespace PhpList\Core\Domain\Subscription\Model; use Doctrine\ORM\Mapping as ORM; +use PhpList\Core\Domain\Common\Model\AttributeTypeEnum; use PhpList\Core\Domain\Common\Model\Interfaces\DomainModel; use PhpList\Core\Domain\Common\Model\Interfaces\Identity; use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeDefinitionRepository; @@ -48,9 +49,9 @@ public function getName(): string return $this->name; } - public function getType(): ?string + public function getType(): ?AttributeTypeEnum { - return $this->type; + return $this->type === null ? null : AttributeTypeEnum::from($this->type); } public function getListOrder(): ?int @@ -80,9 +81,19 @@ public function setName(string $name): self return $this; } - public function setType(?string $type): self + public function setType(?AttributeTypeEnum $type): self { - $this->type = $type; + if ($type === null) { + $this->type = null; + return $this; + } + + if ($this->type !== null) { + $currentType = AttributeTypeEnum::from($this->type); + $currentType->assertTransitionAllowed($type); + } + $this->type = $type->value; + return $this; } diff --git a/src/Domain/Subscription/Repository/DynamicListAttrRepository.php b/src/Domain/Subscription/Repository/DynamicListAttrRepository.php index 104938b0..8b61caf0 100644 --- a/src/Domain/Subscription/Repository/DynamicListAttrRepository.php +++ b/src/Domain/Subscription/Repository/DynamicListAttrRepository.php @@ -7,13 +7,104 @@ use Doctrine\DBAL\ArrayParameterType; use Doctrine\DBAL\Connection; use InvalidArgumentException; +use PDO; +use PhpList\Core\Domain\Subscription\Model\Dto\DynamicListAttrDto; +use Symfony\Component\Serializer\Exception\ExceptionInterface; +use Symfony\Component\Serializer\Normalizer\DenormalizerInterface; +use Throwable; class DynamicListAttrRepository { + private string $fullTablePrefix; + public function __construct( private readonly Connection $connection, - private readonly string $prefix = 'phplist_' + private readonly DenormalizerInterface $serializer, + string $dbPrefix = 'phplist_', + string $dynamicListTablePrefix = 'listattr_', ) { + $this->fullTablePrefix = $dbPrefix . $dynamicListTablePrefix; + } + + public function transactional(callable $callback): mixed + { + $this->connection->beginTransaction(); + try { + $result = $callback(); + $this->connection->commit(); + return $result; + } catch (Throwable $e) { + $this->connection->rollBack(); + throw $e; + } + } + + /** + * @param array> $rows + * @return DynamicListAttrDto[] + * @throws ExceptionInterface + */ + private function denormalizeAll(array $rows): array + { + return array_map( + fn(array $row) => $this->serializer->denormalize($row, DynamicListAttrDto::class), + $rows + ); + } + + public function insertOne(string $listTable, DynamicListAttrDto $dto): DynamicListAttrDto + { + if (!preg_match('/^[A-Za-z0-9_]+$/', $listTable)) { + throw new InvalidArgumentException('Invalid list table'); + } + $table = $this->fullTablePrefix . $listTable; + $this->connection->insert($table, [ + 'name' => $dto->name, + 'listorder' => $dto->listOrder ?? 0, + ]); + $id = (int)$this->connection->lastInsertId(); + return new DynamicListAttrDto(id: $id, name: $dto->name, listOrder: $dto->listOrder ?? 0); + } + + /** + * @param DynamicListAttrDto[] $dtos + * @return DynamicListAttrDto[] + */ + public function insertMany(string $listTable, array $dtos): array + { + $result = []; + foreach ($dtos as $dto) { + $result[] = $this->insertOne($listTable, $dto); + } + return $result; + } + + public function updateById(string $listTable, int $id, array $updates): void + { + if (!preg_match('/^[A-Za-z0-9_]+$/', $listTable)) { + throw new InvalidArgumentException('Invalid list table'); + } + $table = $this->fullTablePrefix . $listTable; + $this->connection->update($table, $updates, ['id' => $id]); + } + + public function deleteById(string $listTable, int $id): void + { + if (!preg_match('/^[A-Za-z0-9_]+$/', $listTable)) { + throw new InvalidArgumentException('Invalid list table'); + } + $table = $this->fullTablePrefix . $listTable; + $this->connection->delete($table, ['id' => $id], ['integer']); + } + + public function existsById(string $listTable, int $id): bool + { + if (!preg_match('/^[A-Za-z0-9_]+$/', $listTable)) { + throw new InvalidArgumentException('Invalid list table'); + } + $table = $this->fullTablePrefix . $listTable; + $stmt = $this->connection->executeQuery('SELECT COUNT(*) FROM ' . $table . ' WHERE id = :id', ['id' => $id]); + return (bool)$stmt->fetchOne(); } /** @@ -30,7 +121,7 @@ public function fetchOptionNames(string $listTable, array $ids): array throw new InvalidArgumentException('Invalid list table'); } - $table = $this->prefix . 'listattr_' . $listTable; + $table = $this->fullTablePrefix . $listTable; $queryBuilder = $this->connection->createQueryBuilder(); $queryBuilder->select('name') @@ -47,7 +138,7 @@ public function fetchSingleOptionName(string $listTable, int $id): ?string throw new InvalidArgumentException('Invalid list table'); } - $table = $this->prefix . 'listattr_' . $listTable; + $table = $this->fullTablePrefix . $listTable; $queryBuilder = $this->connection->createQueryBuilder(); $queryBuilder->select('name') @@ -59,4 +150,53 @@ public function fetchSingleOptionName(string $listTable, int $id): ?string return $val === false ? null : (string) $val; } + + public function existsByName(string $listTable, DynamicListAttrDto $dto): bool + { + if (!preg_match('/^[A-Za-z0-9_]+$/', $listTable)) { + throw new InvalidArgumentException('Invalid list table'); + } + + $table = $this->fullTablePrefix . $listTable; + + $sql = 'SELECT 1 FROM ' . $table . ' WHERE LOWER(name) = LOWER(:name)'; + $params = ['name' => $dto->name]; + $types = ['name' => PDO::PARAM_STR]; + + if ($dto->id !== null) { + $sql .= ' AND id <> :excludeId'; + $params['excludeId'] = $dto->id; + $types['excludeId'] = PDO::PARAM_INT; + } + + $sql .= ' LIMIT 1'; + + try { + $result = $this->connection->fetchOne($sql, $params, $types); + return $result !== false; + } catch (Throwable $e) { + throw $e; + } + } + + /** + * @throws InvalidArgumentException + * @return DynamicListAttrDto[] + */ + public function getAll(string $listTable): array + { + if (!preg_match('/^[A-Za-z0-9_]+$/', $listTable)) { + throw new InvalidArgumentException('Invalid list table'); + } + + $table = $this->fullTablePrefix . $listTable; + + $rows = $this->connection->createQueryBuilder() + ->select('id', 'name', 'listorder') + ->from($table) + ->executeQuery() + ->fetchAllAssociative(); + + return $this->denormalizeAll($rows); + } } diff --git a/src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php b/src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php index 1db2e0e6..1720d44d 100644 --- a/src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php +++ b/src/Domain/Subscription/Repository/SubscriberAttributeDefinitionRepository.php @@ -17,4 +17,15 @@ public function findOneByName(string $name): ?SubscriberAttributeDefinition { return $this->findOneBy(['name' => $name]); } + + public function existsByTableName(string $tableName): bool + { + return (bool) $this->createQueryBuilder('s') + ->select('COUNT(s.id)') + ->where('s.tableName IS NOT NULL') + ->andWhere('s.tableName = :tableName') + ->setParameter('tableName', $tableName) + ->getQuery() + ->getSingleScalarResult(); + } } diff --git a/src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.php b/src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.php index 6da037fd..49f15ab2 100644 --- a/src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.php +++ b/src/Domain/Subscription/Repository/SubscriberAttributeValueRepository.php @@ -76,4 +76,18 @@ public function getForSubscriber(Subscriber $subscriber): array ->getQuery() ->getResult(); } + + public function existsByAttributeAndValue(string $tableName, int $optionId): bool + { + $row = $this->createQueryBuilder('sa') + ->join('sa.attributeDefinition', 'ad') + ->andWhere('ad.tableName = :tableName') + ->setParameter('tableName', $tableName) + ->andWhere('sa.value = :value') + ->setParameter('value', $optionId) + ->getQuery() + ->getOneOrNullResult(); + + return $row !== null; + } } diff --git a/src/Domain/Subscription/Service/CsvRowToDtoMapper.php b/src/Domain/Subscription/Service/ArrayToDtoMapper.php similarity index 98% rename from src/Domain/Subscription/Service/CsvRowToDtoMapper.php rename to src/Domain/Subscription/Service/ArrayToDtoMapper.php index 0657c538..dcd2f6e8 100644 --- a/src/Domain/Subscription/Service/CsvRowToDtoMapper.php +++ b/src/Domain/Subscription/Service/ArrayToDtoMapper.php @@ -6,7 +6,7 @@ use PhpList\Core\Domain\Subscription\Model\Dto\ImportSubscriberDto; -class CsvRowToDtoMapper +class ArrayToDtoMapper { private const FK_HEADER = 'foreignkey'; private const KNOWN_HEADERS = [ diff --git a/src/Domain/Subscription/Service/CsvImporter.php b/src/Domain/Subscription/Service/CsvToDtoImporter.php similarity index 92% rename from src/Domain/Subscription/Service/CsvImporter.php rename to src/Domain/Subscription/Service/CsvToDtoImporter.php index 01fb51ea..0ab88ad3 100644 --- a/src/Domain/Subscription/Service/CsvImporter.php +++ b/src/Domain/Subscription/Service/CsvToDtoImporter.php @@ -11,10 +11,10 @@ use Symfony\Contracts\Translation\TranslatorInterface; use Throwable; -class CsvImporter +class CsvToDtoImporter { public function __construct( - private readonly CsvRowToDtoMapper $rowMapper, + private readonly ArrayToDtoMapper $rowMapper, private readonly ValidatorInterface $validator, private readonly TranslatorInterface $translator, ) { @@ -25,7 +25,7 @@ public function __construct( * @return array{valid: ImportSubscriberDto[], errors: array>} * @throws CsvException */ - public function import(string $csvFilePath): array + public function parseAndValidate(string $csvFilePath): array { $reader = Reader::createFromPath($csvFilePath, 'r'); $reader->setHeaderOffset(0); diff --git a/src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php b/src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php index 5f95640d..70aaa0e8 100644 --- a/src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php +++ b/src/Domain/Subscription/Service/Manager/AttributeDefinitionManager.php @@ -16,15 +16,21 @@ class AttributeDefinitionManager private SubscriberAttributeDefinitionRepository $definitionRepository; private AttributeTypeValidator $attributeTypeValidator; private TranslatorInterface $translator; + private DynamicListAttrManager $dynamicListAttrManager; + private DynamicListAttrTablesManager $dynamicTablesManager; public function __construct( SubscriberAttributeDefinitionRepository $definitionRepository, AttributeTypeValidator $attributeTypeValidator, TranslatorInterface $translator, + DynamicListAttrManager $dynamicListAttrManager, + DynamicListAttrTablesManager $dynamicTablesManager, ) { $this->definitionRepository = $definitionRepository; $this->attributeTypeValidator = $attributeTypeValidator; $this->translator = $translator; + $this->dynamicListAttrManager = $dynamicListAttrManager; + $this->dynamicTablesManager = $dynamicTablesManager; } public function create(AttributeDefinitionDto $attributeDefinitionDto): SubscriberAttributeDefinition @@ -32,22 +38,30 @@ public function create(AttributeDefinitionDto $attributeDefinitionDto): Subscrib $existingAttribute = $this->definitionRepository->findOneByName($attributeDefinitionDto->name); if ($existingAttribute) { throw new AttributeDefinitionCreationException( - message: $this->translator->trans('Attribute definition already exists'), + message: $this->translator->trans('Attribute definition already exists.'), statusCode: 409 ); } $this->attributeTypeValidator->validate($attributeDefinitionDto->type); + $tableName = $this->dynamicTablesManager + ->resolveTableName(name: $attributeDefinitionDto->name, type: $attributeDefinitionDto->type); + $attributeDefinition = (new SubscriberAttributeDefinition()) ->setName($attributeDefinitionDto->name) ->setType($attributeDefinitionDto->type) ->setListOrder($attributeDefinitionDto->listOrder) ->setRequired($attributeDefinitionDto->required) ->setDefaultValue($attributeDefinitionDto->defaultValue) - ->setTableName($attributeDefinitionDto->tableName); + ->setTableName($tableName); $this->definitionRepository->persist($attributeDefinition); + if ($tableName) { + $this->dynamicTablesManager->createOptionsTableIfNotExists($tableName); + $this->dynamicListAttrManager->insertOptions($tableName, $attributeDefinitionDto->options); + } + return $attributeDefinition; } @@ -69,8 +83,15 @@ public function update( ->setType($attributeDefinitionDto->type) ->setListOrder($attributeDefinitionDto->listOrder) ->setRequired($attributeDefinitionDto->required) - ->setDefaultValue($attributeDefinitionDto->defaultValue) - ->setTableName($attributeDefinitionDto->tableName); + ->setDefaultValue($attributeDefinitionDto->defaultValue); + + 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); + } return $attributeDefinition; } diff --git a/src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php b/src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php new file mode 100644 index 00000000..dad3da8a --- /dev/null +++ b/src/Domain/Subscription/Service/Manager/DynamicListAttrManager.php @@ -0,0 +1,263 @@ +listOrder !== null) { + $order = $opt->listOrder; + } else { + do { + $index++; + } while (isset($usedOrders[$index])); + + $order = $index; + $usedOrders[$order] = true; + } + $options[] = new DynamicListAttrDto(id: null, name: $opt->name, listOrder: $order); + } + + $seen = []; + $unique = []; + foreach ($options as $opt) { + $lowercaseName = mb_strtolower($opt->name); + if (isset($seen[$lowercaseName])) { + continue; + } + $seen[$lowercaseName] = true; + $unique[] = $opt; + } + + if ($unique === []) { + return $result; + } + + return $this->dynamicListAttrRepository->transactional(function () use ($listTable, $unique) { + return $this->dynamicListAttrRepository->insertMany($listTable, $unique); + }); + } + + public function syncOptions(string $getTableName, array $options): array + { + $result = []; + $usedOrders = $this->getSetListOrders($options); + [$incomingById, $incomingNew] = $this->splitOptionsSet($options, $usedOrders); + + return $this->dynamicListAttrRepository->transactional(function () use ( + $getTableName, + $incomingById, + $incomingNew, + $usedOrders, + &$result + ) { + [$currentById, $currentByName] = $this->splitCurrentSet($getTableName); + + // Track all lowercase names that should remain after sync (to avoid accidental pruning) + $keptByLowerName = []; + + // 1) Updates for items with id and inserts for id-missing-but-present ones + $result = array_merge( + $result, + $this->updateRowsById( + incomingById: $incomingById, + currentById: $currentById, + listTable: $getTableName + ) + ); + foreach ($incomingById as $dto) { + // Keep target names (case-insensitive) + $keptByLowerName[mb_strtolower($dto->name)] = true; + } + + // Handle incoming items without id but matching an existing row by case-insensitive name + foreach ($incomingNew as $key => $dto) { + // $key is already lowercase (set in splitOptionsSet) + if (isset($currentByName[$key])) { + $existing = $currentByName[$key]; + $this->dynamicListAttrRepository->updateById( + listTable: $getTableName, + id: (int)$existing->id, + updates: ['name' => $dto->name, 'listorder' => $dto->listOrder] + ); + $result[] = new DynamicListAttrDto(id: $existing->id, name: $dto->name, listOrder: $dto->listOrder); + // Mark as kept and remove from incomingNew so it won't be re-inserted + $keptByLowerName[$key] = true; + unset($incomingNew[$key]); + } + } + + // 2) Inserts for truly new items (no id and no existing match) + // Mark remaining new keys as kept, then insert them + foreach (array_keys($incomingNew) as $lowerKey) { + $keptByLowerName[$lowerKey] = true; + } + $result = array_merge( + $result, + $this->insertOptions( + listTable: $getTableName, + rawOptions: $incomingNew, + usedOrders: $usedOrders + ) + ); + + // 3) Prune: rows not present in the intended final set (case-insensitive) + foreach ($currentByName as $lowerKey => $row) { + if (!isset($keptByLowerName[$lowerKey])) { + // This row is not in desired input → consider removal + if (!$this->optionHasReferences($getTableName, $row->id)) { + $this->dynamicListAttrRepository->deleteById($getTableName, (int)$row->id); + } else { + $result[] = $row; + } + } + } + + return $result; + }); + } + + private function optionHasReferences(string $listTable, int $optionId): bool + { + return $this->attributeValueRepo + ->existsByAttributeAndValue(tableName: $listTable, optionId: $optionId); + } + + private function getSetListOrders(array $options): array + { + $nonNullListOrders = array_values( + array_filter( + array_map(fn($opt) => $opt->listOrder ?? null, $options), + fn($order) => $order !== null + ) + ); + return array_flip($nonNullListOrders); + } + + /** + * Splits the dynamic list attribute set into lookup maps. + * + * @return array{ + * 0: array, + * 1: array + * } + */ + private function splitOptionsSet(array $options, array &$usedOrders): array + { + $incomingById = []; + $incomingNew = []; + $index = 0; + + foreach ($options as $opt) { + if ($opt->listOrder !== null) { + $order = $opt->listOrder; + } else { + do { + $index++; + } while (isset($usedOrders[$index])); + + $order = $index; + $usedOrders[$order] = true; + } + if ($opt->id !== null) { + $incomingById[(int)$opt->id] = new DynamicListAttrDto( + id: $opt->id, + name: $opt->name, + listOrder: $order, + ); + } else { + $key = mb_strtolower($opt->name); + if (!isset($incomingNew[$key])) { + $incomingNew[$key] = new DynamicListAttrDto(id: null, name: $opt->name, listOrder: $order); + } + } + } + + return [$incomingById, $incomingNew]; + } + + /** + * Splits the current dynamic list attribute set into lookup maps. + * + * @param string $listTable + * @return array{ + * 0: array, + * 1: array + * } + */ + private function splitCurrentSet(string $listTable): array + { + $currentById = []; + $currentByName = []; + + $rows = $this->dynamicListAttrRepository->getAll($listTable); + foreach ($rows as $listAttrDto) { + $currentById[$listAttrDto->id] = $listAttrDto; + $currentByName[mb_strtolower($listAttrDto->name)] = $listAttrDto; + } + + return [$currentById, $currentByName]; + } + + private function updateRowsById(array $incomingById, array $currentById, string $listTable): array + { + $result = []; + + /** @var DynamicListAttrDto $dto */ + foreach ($incomingById as $dto) { + if (!isset($currentById[$dto->id])) { + // Unexpected: incoming has id but the current table does not — insert a new row + $inserted = $this->dynamicListAttrRepository->insertOne( + listTable: $listTable, + dto: new DynamicListAttrDto(id: null, name: $dto->name, listOrder: $dto->listOrder) + ); + $result[] = $inserted; + } else { + $cur = $currentById[$dto->id]; + $updates = []; + if ($cur->name !== $dto->name) { + $nameExists = $this->dynamicListAttrRepository->existsByName($listTable, $dto); + 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; + } + } + + return $result; + } +} diff --git a/src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php b/src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php new file mode 100644 index 00000000..ff579a2c --- /dev/null +++ b/src/Domain/Subscription/Service/Manager/DynamicListAttrTablesManager.php @@ -0,0 +1,54 @@ +prefix = $dbPrefix . $dynamicListTablePrefix; + } + + public function resolveTableName(string $name, ?AttributeTypeEnum $type): ?string + { + if ($type === null) { + return null; + } + + if (!$type->isMultiValued()) { + return null; + } + + $base = u($name)->snake()->toString(); + $candidate = $base; + $index = 1; + while ($this->definitionRepository->existsByTableName($candidate)) { + $suffix = $index; + $candidate = $base . $suffix; + $index++; + } + + return $candidate; + } + + public function createOptionsTableIfNotExists(string $listTable): void + { + $fullTableName = $this->prefix . $listTable; + + $this->messageBus->dispatch(new DynamicTableMessage($fullTableName)); + } +} diff --git a/src/Domain/Subscription/Service/Manager/SubscriberManager.php b/src/Domain/Subscription/Service/Manager/SubscriberManager.php index e0b7a3dd..e7eaba09 100644 --- a/src/Domain/Subscription/Service/Manager/SubscriberManager.php +++ b/src/Domain/Subscription/Service/Manager/SubscriberManager.php @@ -58,7 +58,6 @@ public function getSubscriberById(int $subscriberId): ?Subscriber return $this->subscriberRepository->find($subscriberId); } - /** @SuppressWarnings(PHPMD.StaticAccess) */ public function updateSubscriber(UpdateSubscriberDto $subscriberDto, Administrator $admin): Subscriber { /** @var Subscriber $subscriber */ @@ -123,7 +122,6 @@ public function createFromImport(ImportSubscriberDto $subscriberDto): Subscriber return $subscriber; } - /** @SuppressWarnings(PHPMD.StaticAccess) */ public function updateFromImport(Subscriber $existingSubscriber, ImportSubscriberDto $subscriberDto): ChangeSetDto { $existingSubscriber->setEmail($subscriberDto->email); diff --git a/src/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.php b/src/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.php index 3b33f629..8a715ba6 100644 --- a/src/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.php +++ b/src/Domain/Subscription/Service/Provider/CheckboxGroupValueProvider.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Domain\Subscription\Service\Provider; +use PhpList\Core\Domain\Common\Model\AttributeTypeEnum; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeValue; use PhpList\Core\Domain\Subscription\Repository\DynamicListAttrRepository; @@ -16,9 +17,7 @@ public function __construct(private readonly DynamicListAttrRepository $repo) public function supports(SubscriberAttributeDefinition $attribute): bool { - // phpcs:ignore Generic.Commenting.Todo - // @todo: check what real types exist in the database - return $attribute->getType() === 'checkboxgroup'; + return $attribute->getType() === AttributeTypeEnum::CheckboxGroup; } public function getValue(SubscriberAttributeDefinition $attribute, SubscriberAttributeValue $userValue): string diff --git a/src/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.php b/src/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.php index 22b3ab4e..70eab744 100644 --- a/src/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.php +++ b/src/Domain/Subscription/Service/Provider/SelectOrRadioValueProvider.php @@ -4,9 +4,11 @@ namespace PhpList\Core\Domain\Subscription\Service\Provider; +use PhpList\Core\Domain\Common\Model\AttributeTypeEnum; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeValue; use PhpList\Core\Domain\Subscription\Repository\DynamicListAttrRepository; +use function in_array; class SelectOrRadioValueProvider implements AttributeValueProvider { @@ -16,7 +18,7 @@ public function __construct(private readonly DynamicListAttrRepository $repo) public function supports(SubscriberAttributeDefinition $attribute): bool { - return \in_array($attribute->getType(), ['select','radio'], true); + return in_array($attribute->getType(), [AttributeTypeEnum::Select, AttributeTypeEnum::Radio], true); } public function getValue(SubscriberAttributeDefinition $attribute, SubscriberAttributeValue $userValue): string diff --git a/src/Domain/Subscription/Service/SubscriberCsvImporter.php b/src/Domain/Subscription/Service/SubscriberCsvImporter.php index 6873a03a..8440d11e 100644 --- a/src/Domain/Subscription/Service/SubscriberCsvImporter.php +++ b/src/Domain/Subscription/Service/SubscriberCsvImporter.php @@ -23,9 +23,12 @@ use Throwable; /** - * phpcs:ignore Generic.Commenting.Todo - * @todo: check if dryRun will work (some function flush) - * Service for importing subscribers from a CSV file. + * 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. + * * @SuppressWarnings("CouplingBetweenObjects") * @SuppressWarnings("ExcessiveParameterList") */ @@ -35,7 +38,7 @@ class SubscriberCsvImporter private SubscriberAttributeManager $attributeManager; private SubscriptionManager $subscriptionManager; private SubscriberRepository $subscriberRepository; - private CsvImporter $csvImporter; + private CsvToDtoImporter $csvToDtoImporter; private EntityManagerInterface $entityManager; private TranslatorInterface $translator; private MessageBusInterface $messageBus; @@ -46,7 +49,7 @@ public function __construct( SubscriberAttributeManager $attributeManager, SubscriptionManager $subscriptionManager, SubscriberRepository $subscriberRepository, - CsvImporter $csvImporter, + CsvToDtoImporter $csvToDtoImporter, EntityManagerInterface $entityManager, TranslatorInterface $translator, MessageBusInterface $messageBus, @@ -56,7 +59,7 @@ public function __construct( $this->attributeManager = $attributeManager; $this->subscriptionManager = $subscriptionManager; $this->subscriberRepository = $subscriberRepository; - $this->csvImporter = $csvImporter; + $this->csvToDtoImporter = $csvToDtoImporter; $this->entityManager = $entityManager; $this->translator = $translator; $this->messageBus = $messageBus; @@ -78,6 +81,7 @@ public function importFromCsv( 'updated' => 0, 'skipped' => 0, 'blacklisted' => 0, + 'invalid_email' => 0, 'errors' => [], ]; @@ -89,7 +93,7 @@ public function importFromCsv( ); } - $result = $this->csvImporter->import($path); + $result = $this->csvToDtoImporter->parseAndValidate($path); foreach ($result['valid'] as $dto) { try { @@ -244,11 +248,12 @@ private function handleInvalidEmail( if (!filter_var($dto->email, FILTER_VALIDATE_EMAIL)) { if ($options->skipInvalidEmail) { $stats['skipped']++; + $stats['invalid_email']++; + $stats['errors'][] = $this->translator->trans('Invalid email: %email%', ['%email%' => $dto->email]); return true; } - // phpcs:ignore Generic.Commenting.Todo - // @todo: check + $dto->email = 'invalid_' . $dto->email; $dto->sendConfirmation = false; } diff --git a/src/Domain/Subscription/Validator/AttributeTypeValidator.php b/src/Domain/Subscription/Validator/AttributeTypeValidator.php index 36bcd45d..d4ededff 100644 --- a/src/Domain/Subscription/Validator/AttributeTypeValidator.php +++ b/src/Domain/Subscription/Validator/AttributeTypeValidator.php @@ -4,11 +4,12 @@ namespace PhpList\Core\Domain\Subscription\Validator; -use InvalidArgumentException; +use PhpList\Core\Domain\Common\Model\AttributeTypeEnum; use PhpList\Core\Domain\Common\Model\ValidationContext; use PhpList\Core\Domain\Common\Validator\ValidatorInterface; use Symfony\Component\Validator\Exception\ValidatorException; use Symfony\Contracts\Translation\TranslatorInterface; +use Throwable; class AttributeTypeValidator implements ValidatorInterface { @@ -17,35 +18,63 @@ public function __construct(private readonly TranslatorInterface $translator) } private const VALID_TYPES = [ - 'textline', - 'checkbox', - 'checkboxgroup', - 'radio', - 'select', - 'hidden', - 'textarea', - 'date', + AttributeTypeEnum::TextLine, + AttributeTypeEnum::Hidden, + AttributeTypeEnum::CreditCardNo, + AttributeTypeEnum::Select, + AttributeTypeEnum::Date, + AttributeTypeEnum::Checkbox, + AttributeTypeEnum::TextArea, + AttributeTypeEnum::Radio, + AttributeTypeEnum::CheckboxGroup, ]; public function validate(mixed $value, ValidationContext $context = null): void { - if (!is_string($value)) { - throw new InvalidArgumentException($this->translator->trans('Value must be a string.')); - } + $enum = $this->normalizeToEnum($value); + + if (!in_array($enum, self::VALID_TYPES, true)) { + $validList = implode(', ', array_map( + static fn(AttributeTypeEnum $enum) => $enum->value, + self::VALID_TYPES + )); - $errors = []; - if (!in_array($value, self::VALID_TYPES, true)) { - $errors[] = $this->translator->trans( + $message = $this->translator->trans( 'Invalid attribute type: "%type%". Valid types are: %valid_types%', [ - '%type%' => $value, - '%valid_types%' => implode(', ', self::VALID_TYPES), + '%type%' => $enum->value, + '%valid_types%' => $validList, ] ); + + throw new ValidatorException($message); + } + } + + /** + * @throws ValidatorException if value cannot be converted to AttributeTypeEnum + */ + private function normalizeToEnum(mixed $value): AttributeTypeEnum + { + if ($value instanceof AttributeTypeEnum) { + return $value; } - if (!empty($errors)) { - throw new ValidatorException(implode("\n", $errors)); + if (is_string($value)) { + try { + return AttributeTypeEnum::from($value); + } catch (Throwable) { + $lower = strtolower($value); + foreach (AttributeTypeEnum::cases() as $case) { + if ($case->value === $lower) { + return $case; + } + } + } } + + throw new ValidatorException( + $this->translator->trans('Value must be an AttributeTypeEnum or string.') + ); } } diff --git a/tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php b/tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php new file mode 100644 index 00000000..0e267565 --- /dev/null +++ b/tests/Integration/Domain/Subscription/Service/DynamicListAttrManagerFunctionalTest.php @@ -0,0 +1,116 @@ +loadSchema(); + + $connection = $this->entityManager->getConnection(); + $serializer = self::getContainer()->get('serializer'); + + // Use real repository and manager services (but we construct them explicitly + // to be independent from service wiring changes). + $this->dynamicListAttrRepo = new DynamicListAttrRepository( + connection: $connection, + serializer: $serializer, + dbPrefix: 'phplist_', + dynamicListTablePrefix: 'listattr_' + ); + + $subscriberAttributeValueRepo = null; + $subscriberAttributeValueRepo = self::getContainer()->get(SubscriberAttributeValueRepository::class); + + // Get tables manager from container for creating/ensuring dynamic tables + $this->tablesManager = self::getContainer()->get(DynamicListAttrTablesManager::class); + + // Create manager with actual constructor signature + $this->manager = new DynamicListAttrManager( + dynamicListAttrRepository: $this->dynamicListAttrRepo, + attributeValueRepo: $subscriberAttributeValueRepo + ); + } + + public function testCreateInsertAndFetchOptionsEndToEnd(): void + { + // Create the dynamic options table for logical name "colors" + $this->tablesManager->createOptionsTableIfNotExists('colours'); + + // Insert options (including a duplicate name differing by case) + $inserted = $this->manager->insertOptions('colours', [ + new DynamicListAttrDto(id: null, name: 'Red'), + new DynamicListAttrDto(id: null, name: 'Blue'), + // case-insensitive duplicate -> should be skipped + new DynamicListAttrDto(id: null, name: 'red'), + ]); + + // We expect exactly 2 distinct rows inserted + Assert::assertCount(2, $inserted); + $names = array_map(fn(DynamicListAttrDto $d) => $d->name, $inserted); + sort($names); + Assert::assertSame(['Blue', 'Red'], $names); + + // Now fetch through the repository + $all = $this->dynamicListAttrRepo->getAll('colours'); + Assert::assertCount(2, $all); + + // Collect ids to test fetchOptionNames/fetchSingleOptionName + $ids = array_map(fn(DynamicListAttrDto $d) => (int)$d->id, $inserted); + sort($ids); + + $fetchedNames = $this->dynamicListAttrRepo->fetchOptionNames('colours', $ids); + sort($fetchedNames); + Assert::assertSame(['Blue', 'Red'], $fetchedNames); + + // Single fetch + $oneName = $this->dynamicListAttrRepo->fetchSingleOptionName('colours', $ids[0]); + Assert::assertNotNull($oneName); + Assert::assertTrue(in_array($oneName, ['Blue', 'Red'], true)); + } + + protected function tearDown(): void + { + try { + if ($this->entityManager !== null) { + $connection = $this->entityManager->getConnection(); + $fullTable = 'phplist_listattr_colours'; + // Use raw SQL for cleanup to avoid relying on SchemaManager in tests + $connection->executeStatement('DROP TABLE IF EXISTS ' . $fullTable); + } + // phpcs:ignore Generic.CodeAnalysis.EmptyStatement.DetectedCatch + } catch (Throwable $e) { + // Ignore cleanup failures to not mask test results + } finally { + $this->dynamicListAttrRepo = null; + $this->manager = null; + $this->tablesManager = null; + parent::tearDown(); + } + } +} diff --git a/tests/Support/DBAL/FakeDriverException.php b/tests/Support/DBAL/FakeDriverException.php new file mode 100644 index 00000000..9ad972dc --- /dev/null +++ b/tests/Support/DBAL/FakeDriverException.php @@ -0,0 +1,34 @@ +sqlState = $sqlState; + } + + public function getSQLState(): ?string + { + return $this->sqlState; + } +} diff --git a/tests/Unit/Domain/Identity/Model/AdminAttributeDefinitionTest.php b/tests/Unit/Domain/Identity/Model/AdminAttributeDefinitionTest.php index 0343d179..2d2edd71 100644 --- a/tests/Unit/Domain/Identity/Model/AdminAttributeDefinitionTest.php +++ b/tests/Unit/Domain/Identity/Model/AdminAttributeDefinitionTest.php @@ -186,26 +186,4 @@ public function testGetTableNameReturnsTableName(): void { self::assertSame($this->tableName, $this->subject->getTableName()); } - - public function testSetTableNameSetsTableName(): void - { - $newTableName = 'new_table'; - $this->subject->setTableName($newTableName); - - self::assertSame($newTableName, $this->subject->getTableName()); - } - - public function testSetTableNameReturnsInstance(): void - { - $result = $this->subject->setTableName('new_table'); - - self::assertSame($this->subject, $result); - } - - public function testSetTableNameCanSetNull(): void - { - $this->subject->setTableName(null); - - self::assertNull($this->subject->getTableName()); - } } diff --git a/tests/Unit/Domain/Identity/Service/AdminAttributeDefinitionManagerTest.php b/tests/Unit/Domain/Identity/Service/AdminAttributeDefinitionManagerTest.php index 3c3356d7..edf16d37 100644 --- a/tests/Unit/Domain/Identity/Service/AdminAttributeDefinitionManagerTest.php +++ b/tests/Unit/Domain/Identity/Service/AdminAttributeDefinitionManagerTest.php @@ -9,7 +9,7 @@ use PhpList\Core\Domain\Identity\Repository\AdminAttributeDefinitionRepository; use PhpList\Core\Domain\Identity\Service\AdminAttributeDefinitionManager; use PhpList\Core\Domain\Identity\Exception\AttributeDefinitionCreationException; -use PhpList\Core\Domain\Subscription\Validator\AttributeTypeValidator; +use PhpList\Core\Domain\Identity\Validator\AttributeTypeValidator; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Symfony\Contracts\Translation\TranslatorInterface; @@ -40,7 +40,6 @@ public function testCreateCreatesNewAttributeDefinition(): void listOrder: 10, defaultValue: 'default', required: true, - tableName: 'test_table' ); $this->repository->expects($this->once()) @@ -55,8 +54,7 @@ public function testCreateCreatesNewAttributeDefinition(): void && $definition->getType() === $dto->type && $definition->getListOrder() === $dto->listOrder && $definition->getDefaultValue() === $dto->defaultValue - && $definition->isRequired() === $dto->required - && $definition->getTableName() === $dto->tableName; + && $definition->isRequired() === $dto->required; })); $result = $this->subject->create($dto); @@ -67,7 +65,6 @@ public function testCreateCreatesNewAttributeDefinition(): void $this->assertEquals($dto->listOrder, $result->getListOrder()); $this->assertEquals($dto->defaultValue, $result->getDefaultValue()); $this->assertEquals($dto->required, $result->isRequired()); - $this->assertEquals($dto->tableName, $result->getTableName()); } public function testCreateThrowsExceptionIfAttributeAlreadyExists(): void @@ -105,7 +102,6 @@ public function testUpdateUpdatesAttributeDefinition(): void listOrder: 20, defaultValue: 'new-default', required: false, - tableName: 'new_table' ); $this->repository->expects($this->once()) @@ -138,11 +134,6 @@ public function testUpdateUpdatesAttributeDefinition(): void ->with(false) ->willReturnSelf(); - $attributeDefinition->expects($this->once()) - ->method('setTableName') - ->with('new_table') - ->willReturnSelf(); - $result = $this->subject->update($attributeDefinition, $dto); $this->assertSame($attributeDefinition, $result); diff --git a/tests/Unit/Domain/Subscription/MessageHandler/DynamicTableMessageHandlerTest.php b/tests/Unit/Domain/Subscription/MessageHandler/DynamicTableMessageHandlerTest.php new file mode 100644 index 00000000..8139e492 --- /dev/null +++ b/tests/Unit/Domain/Subscription/MessageHandler/DynamicTableMessageHandlerTest.php @@ -0,0 +1,156 @@ +schemaManager = $this->createMock(AbstractSchemaManager::class); + } + + public function testInvokeCreatesTableWhenNotExists(): void + { + $tableName = 'phplist_listattr_sizes'; + $message = new DynamicTableMessage($tableName); + + $capturedTable = null; + + $this->schemaManager + ->expects($this->once()) + ->method('tablesExist') + ->with([$tableName]) + ->willReturn(false); + + $this->schemaManager + ->expects($this->once()) + ->method('createTable') + ->with($this->callback(function (Table $table) use (&$capturedTable, $tableName) { + $capturedTable = $table; + // Basic checks + $this->assertSame($tableName, $table->getName()); + $this->assertTrue($table->hasColumn('id')); + $this->assertTrue($table->hasColumn('name')); + $this->assertTrue($table->hasColumn('listorder')); + + // id column + $idCol = $table->getColumn('id'); + $this->assertSame('integer', $idCol->getType()->getName()); + $this->assertTrue($idCol->getAutoincrement()); + $this->assertTrue($idCol->getNotnull()); + + // name column + $nameCol = $table->getColumn('name'); + $this->assertSame('string', $nameCol->getType()->getName()); + $this->assertSame(255, $nameCol->getLength()); + $this->assertFalse($nameCol->getNotnull()); + + // listorder column + $orderCol = $table->getColumn('listorder'); + $this->assertSame('integer', $orderCol->getType()->getName()); + $this->assertFalse($orderCol->getNotnull()); + $this->assertSame(0, $orderCol->getDefault()); + + // Primary key + $this->assertSame(['id'], $table->getPrimaryKey()?->getColumns()); + + // Unique index on name + $indexName = 'uniq_' . $tableName . '_name'; + $this->assertTrue($table->hasIndex($indexName)); + $idx = $table->getIndex($indexName); + $this->assertTrue($idx->isUnique()); + $this->assertSame(['name'], $idx->getColumns()); + + return true; + })) + ->willReturnCallback(function (Table $table) { + // no-op; we just want the assertions in the callback + }); + + $handler = new DynamicTableMessageHandler($this->schemaManager); + $handler($message); + + $this->assertInstanceOf(Table::class, $capturedTable); + } + + public function testInvokeDoesNothingWhenTableAlreadyExists(): void + { + $tableName = 'phplist_listattr_sizes'; + $message = new DynamicTableMessage($tableName); + + $this->schemaManager + ->expects($this->once()) + ->method('tablesExist') + ->with([$tableName]) + ->willReturn(true); + + $this->schemaManager + ->expects($this->never()) + ->method('createTable'); + + $handler = new DynamicTableMessageHandler($this->schemaManager); + $handler($message); + // reached without creating a table + $this->assertTrue(true); + } + + public function testInvokeThrowsForInvalidTableName(): void + { + $invalidName = 'invalid-name!'; + $message = new DynamicTableMessage($invalidName); + + // tablesExist is consulted before validating the name + $this->schemaManager + ->expects($this->once()) + ->method('tablesExist') + ->with([$invalidName]) + ->willReturn(false); + + $handler = new DynamicTableMessageHandler($this->schemaManager); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Invalid list table name: ' . $invalidName); + $handler($message); + $this->assertTrue(true); + } + + public function testInvokeSwallowsTableExistsRace(): void + { + $tableName = 'phplist_listattr_colors'; + $message = new DynamicTableMessage($tableName); + + $this->schemaManager + ->expects($this->once()) + ->method('tablesExist') + ->with([$tableName]) + ->willReturn(false); + + $this->schemaManager + ->expects($this->once()) + ->method('createTable') + ->willThrowException(new TableExistsException( + new FakeDriverException('already exists', '42P07'), + null + )); + + $handler = new DynamicTableMessageHandler($this->schemaManager); + + // Should not throw despite the TableExistsException + $handler($message); + $this->assertTrue(true); + } +} diff --git a/tests/Unit/Domain/Subscription/Repository/DynamicListAttrRepositoryTest.php b/tests/Unit/Domain/Subscription/Repository/DynamicListAttrRepositoryTest.php index 948c8347..fee2b8ed 100644 --- a/tests/Unit/Domain/Subscription/Repository/DynamicListAttrRepositoryTest.php +++ b/tests/Unit/Domain/Subscription/Repository/DynamicListAttrRepositoryTest.php @@ -11,13 +11,17 @@ use InvalidArgumentException; use PhpList\Core\Domain\Subscription\Repository\DynamicListAttrRepository; use PHPUnit\Framework\TestCase; +use Symfony\Component\Serializer\Normalizer\DenormalizerInterface; class DynamicListAttrRepositoryTest extends TestCase { public function testFetchOptionNamesReturnsEmptyForEmptyIds(): void { $conn = $this->createMock(Connection::class); - $repo = new DynamicListAttrRepository($conn, 'phplist_'); + $repo = new DynamicListAttrRepository( + $conn, + $this->createMock(DenormalizerInterface::class), + ); $this->assertSame([], $repo->fetchOptionNames('valid_table', [])); $this->assertSame([], $repo->fetchOptionNames('valid_table', [])); @@ -26,7 +30,10 @@ public function testFetchOptionNamesReturnsEmptyForEmptyIds(): void public function testFetchOptionNamesThrowsOnInvalidTable(): void { $conn = $this->createMock(Connection::class); - $repo = new DynamicListAttrRepository($conn, 'phplist_'); + $repo = new DynamicListAttrRepository( + $conn, + $this->createMock(DenormalizerInterface::class), + ); $this->expectException(InvalidArgumentException::class); $this->expectExceptionMessage('Invalid list table'); @@ -80,7 +87,10 @@ public function testFetchOptionNamesReturnsNames(): void $conn->method('createQueryBuilder')->willReturn($qb); - $repo = new DynamicListAttrRepository($conn, 'phplist_'); + $repo = new DynamicListAttrRepository( + $conn, + $this->createMock(DenormalizerInterface::class), + ); $names = $repo->fetchOptionNames('users', [1, '2', 3]); $this->assertSame(['alpha', 'beta', 'gamma'], $names); @@ -89,7 +99,10 @@ public function testFetchOptionNamesReturnsNames(): void public function testFetchSingleOptionNameThrowsOnInvalidTable(): void { $conn = $this->createMock(Connection::class); - $repo = new DynamicListAttrRepository($conn, 'phplist_'); + $repo = new DynamicListAttrRepository( + $conn, + $this->createMock(DenormalizerInterface::class), + ); $this->expectException(InvalidArgumentException::class); $this->expectExceptionMessage('Invalid list table'); @@ -117,7 +130,10 @@ public function testFetchSingleOptionNameReturnsString(): void $qb->expects($this->once())->method('executeQuery')->willReturn($result); $conn->method('createQueryBuilder')->willReturn($qb); - $repo = new DynamicListAttrRepository($conn, 'phplist_'); + $repo = new DynamicListAttrRepository( + $conn, + $this->createMock(DenormalizerInterface::class), + ); $this->assertSame('Bradford', $repo->fetchSingleOptionName('ukcountries', 42)); } @@ -141,7 +157,10 @@ public function testFetchSingleOptionNameReturnsNullWhenNotFound(): void $qb->method('executeQuery')->willReturn($result); $conn->method('createQueryBuilder')->willReturn($qb); - $repo = new DynamicListAttrRepository($conn, 'phplist_'); + $repo = new DynamicListAttrRepository( + $conn, + $this->createMock(DenormalizerInterface::class), + ); $this->assertNull($repo->fetchSingleOptionName('termsofservices', 999)); } } diff --git a/tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php index 6a047051..f109f1c4 100644 --- a/tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php +++ b/tests/Unit/Domain/Subscription/Service/Manager/AttributeDefinitionManagerTest.php @@ -4,13 +4,18 @@ namespace PhpList\Core\Tests\Unit\Domain\Subscription\Service\Manager; +use PhpList\Core\Domain\Common\Model\AttributeTypeEnum; use PhpList\Core\Domain\Subscription\Exception\AttributeDefinitionCreationException; use PhpList\Core\Domain\Subscription\Model\Dto\AttributeDefinitionDto; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition; use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeDefinitionRepository; use PhpList\Core\Domain\Subscription\Service\Manager\AttributeDefinitionManager; +use PhpList\Core\Domain\Subscription\Service\Manager\DynamicListAttrManager; +use PhpList\Core\Domain\Subscription\Service\Manager\DynamicListAttrTablesManager; use PhpList\Core\Domain\Subscription\Validator\AttributeTypeValidator; use PHPUnit\Framework\TestCase; +use Symfony\Component\Messenger\Envelope; +use Symfony\Component\Messenger\MessageBusInterface; use Symfony\Component\Translation\Translator; class AttributeDefinitionManagerTest extends TestCase @@ -19,19 +24,28 @@ public function testCreateAttributeDefinition(): void { $repository = $this->createMock(SubscriberAttributeDefinitionRepository::class); $validator = $this->createMock(AttributeTypeValidator::class); + $bus = $this->createMock(MessageBusInterface::class); + $bus->method('dispatch')->willReturnCallback(function ($message) { + return new Envelope($message); + }); + $dynamicTablesManager = new DynamicListAttrTablesManager( + definitionRepository: $this->createMock(SubscriberAttributeDefinitionRepository::class), + messageBus: $bus, + ); $manager = new AttributeDefinitionManager( definitionRepository: $repository, attributeTypeValidator: $validator, - translator: new Translator('en') + translator: new Translator('en'), + dynamicListAttrManager: $this->createMock(DynamicListAttrManager::class), + dynamicTablesManager: $dynamicTablesManager, ); $dto = new AttributeDefinitionDto( name: 'Country', - type: 'checkbox', + type: AttributeTypeEnum::Checkbox, listOrder: 1, defaultValue: 'US', required: true, - tableName: 'user_attribute' ); $repository->expects($this->once()) @@ -45,11 +59,11 @@ public function testCreateAttributeDefinition(): void $this->assertInstanceOf(SubscriberAttributeDefinition::class, $attribute); $this->assertSame('Country', $attribute->getName()); - $this->assertSame('checkbox', $attribute->getType()); + $this->assertSame(AttributeTypeEnum::Checkbox, $attribute->getType()); $this->assertSame(1, $attribute->getListOrder()); $this->assertSame('US', $attribute->getDefaultValue()); $this->assertTrue($attribute->isRequired()); - $this->assertSame('user_attribute', $attribute->getTableName()); + $this->assertSame('country', $attribute->getTableName()); } public function testCreateThrowsWhenAttributeAlreadyExists(): void @@ -60,15 +74,16 @@ public function testCreateThrowsWhenAttributeAlreadyExists(): void definitionRepository: $repository, attributeTypeValidator: $validator, translator: new Translator('en'), + dynamicListAttrManager: $this->createMock(DynamicListAttrManager::class), + dynamicTablesManager: $this->createMock(DynamicListAttrTablesManager::class), ); $dto = new AttributeDefinitionDto( name: 'Country', - type: 'checkbox', + type: AttributeTypeEnum::Checkbox, listOrder: 1, defaultValue: 'US', required: true, - tableName: 'user_attribute' ); $existing = $this->createMock(SubscriberAttributeDefinition::class); @@ -91,6 +106,8 @@ public function testUpdateAttributeDefinition(): void definitionRepository: $repository, attributeTypeValidator: $validator, translator: new Translator('en'), + dynamicListAttrManager: $this->createMock(DynamicListAttrManager::class), + dynamicTablesManager: $this->createMock(DynamicListAttrTablesManager::class), ); $attribute = new SubscriberAttributeDefinition(); @@ -98,11 +115,10 @@ public function testUpdateAttributeDefinition(): void $dto = new AttributeDefinitionDto( name: 'New', - type: 'text', + type: AttributeTypeEnum::Text, listOrder: 5, defaultValue: 'Canada', required: false, - tableName: 'custom_attrs' ); $repository->expects($this->once()) @@ -110,14 +126,14 @@ public function testUpdateAttributeDefinition(): void ->with('New') ->willReturn(null); - $updated = $manager->update($attribute, $dto); + $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()); } public function testUpdateThrowsWhenAnotherAttributeWithSameNameExists(): void @@ -128,15 +144,16 @@ public function testUpdateThrowsWhenAnotherAttributeWithSameNameExists(): void definitionRepository: $repository, attributeTypeValidator: $validator, translator: new Translator('en'), + dynamicListAttrManager: $this->createMock(DynamicListAttrManager::class), + dynamicTablesManager: $this->createMock(DynamicListAttrTablesManager::class), ); $dto = new AttributeDefinitionDto( name: 'Existing', - type: 'text', + type: AttributeTypeEnum::Text, listOrder: 5, defaultValue: 'Canada', required: false, - tableName: 'custom_attrs' ); $current = new SubscriberAttributeDefinition(); @@ -152,7 +169,7 @@ public function testUpdateThrowsWhenAnotherAttributeWithSameNameExists(): void $this->expectException(AttributeDefinitionCreationException::class); - $manager->update($current, $dto); + $manager->update(attributeDefinition: $current, attributeDefinitionDto: $dto); } public function testDeleteAttributeDefinition(): void @@ -163,6 +180,8 @@ public function testDeleteAttributeDefinition(): void definitionRepository: $repository, attributeTypeValidator: $validator, translator: new Translator('en'), + dynamicListAttrManager: $this->createMock(DynamicListAttrManager::class), + dynamicTablesManager: $this->createMock(DynamicListAttrTablesManager::class), ); $attribute = new SubscriberAttributeDefinition(); diff --git a/tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php new file mode 100644 index 00000000..c51cb81d --- /dev/null +++ b/tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrManagerTest.php @@ -0,0 +1,114 @@ +subscriberAttributeValueRepo = $this->createMock(SubscriberAttributeValueRepository::class); + $this->listAttrRepo = $this->createMock(DynamicListAttrRepository::class); + } + + private function makeManager(): DynamicListAttrManager + { + return new DynamicListAttrManager( + dynamicListAttrRepository: $this->listAttrRepo, + attributeValueRepo: $this->subscriberAttributeValueRepo + ); + } + + private function makePartialManager(array $methods): DynamicListAttrManager|MockObject + { + return $this->getMockBuilder(DynamicListAttrManager::class) + ->setConstructorArgs([ + $this->listAttrRepo, + $this->subscriberAttributeValueRepo, + ]) + ->onlyMethods($methods) + ->getMock(); + } + + public function testInsertOptionsSkipsEmpty(): void + { + $manager = $this->makeManager(); + + // Empty array should be a no-op (no DB calls) + $this->listAttrRepo->expects($this->never())->method('transactional'); + $manager->insertOptions('colors', []); + // if we got here, expectations were met + $this->assertTrue(true); + } + + public function testInsertOptionsSkipsDuplicatesAndAssignsOrder(): void + { + $manager = $this->makeManager(); + + $this->listAttrRepo->expects($this->once()) + ->method('transactional') + ->willReturnCallback(function (callable $cb) { + // The callback should call insertMany with one unique row + $this->listAttrRepo->expects($this->once()) + ->method('insertMany') + ->with('colors', $this->callback(function ($arr) { + return is_array($arr) && count($arr) === 1 && $arr[0] instanceof DynamicListAttrDto; + })) + ->willReturn([new DynamicListAttrDto(id: 1, name: 'Red', listOrder: 1)]); + return $cb(); + }); + + $opts = [ + new DynamicListAttrDto(id: null, name: 'Red'), + // duplicate by case-insensitive compare -> skipped + new DynamicListAttrDto(id: null, name: 'red'), + ]; + + $result = $manager->insertOptions('colors', $opts); + $this->assertCount(1, $result); + $this->assertSame('Red', $result[0]->name); + } + + public function testSyncOptionsInsertsAndPrunesWithReferences(): void + { + $this->listAttrRepo->expects($this->once()) + ->method('transactional') + ->willReturnCallback(function (callable $cb) { + return $cb(); + }); + + $this->listAttrRepo->expects($this->once()) + ->method('getAll') + ->with('colors') + ->willReturn([]); + + // Mock insertOptions to avoid dealing with SQL internals + $manager = $this->makePartialManager(['insertOptions']); + $manager->expects($this->once()) + ->method('insertOptions') + ->with('colors', $this->callback(function ($arr) { + // Should get two unique items + return is_array($arr) && count($arr) === 2; + }), $this->anything()) + ->willReturn([]); + + $result = $manager->syncOptions('colors', [ + new DynamicListAttrDto(id: null, name: 'Red'), + new DynamicListAttrDto(id: null, name: 'Blue'), + ]); + + $this->assertCount(0, $result); + } +} diff --git a/tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrTablesManagerTest.php b/tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrTablesManagerTest.php new file mode 100644 index 00000000..a9166799 --- /dev/null +++ b/tests/Unit/Domain/Subscription/Service/Manager/DynamicListAttrTablesManagerTest.php @@ -0,0 +1,83 @@ +definitionRepo = $this->createMock(SubscriberAttributeDefinitionRepository::class); + $this->bus = $this->createMock(MessageBusInterface::class); + $this->bus->method('dispatch')->willReturnCallback(function ($message) { + return new Envelope($message); + }); + } + + private function makeManager(): DynamicListAttrTablesManager + { + return new DynamicListAttrTablesManager( + definitionRepository: $this->definitionRepo, + messageBus: $this->bus, + dbPrefix: 'phplist_', + dynamicListTablePrefix: 'listattr_' + ); + } + + public function testResolveTableNameReturnsNullWhenTypeIsNullOrUnsupported(): void + { + $manager = $this->makeManager(); + + $this->assertNull($manager->resolveTableName('My Name', null)); + + // For unsupported type (e.g., Text) should be null + $this->assertNull($manager->resolveTableName('My Name', AttributeTypeEnum::Text)); + } + + public function testResolveTableNameSnakeCasesAndEnsuresUniqueness(): void + { + // First two candidates exist, third is unique + $this->definitionRepo + ->method('existsByTableName') + ->willReturnOnConsecutiveCalls(true, true, false); + + $manager = $this->makeManager(); + $name = $manager->resolveTableName('Fancy Label', AttributeTypeEnum::Select); + + // "Fancy Label" -> "fancy_label" with numeric suffix appended after collisions + $this->assertSame('fancy_label2', $name); + } + + public function testCreateOptionsTableIfNotExistsDispatchesMessage(): void + { + $this->bus + ->expects($this->exactly(2)) + ->method('dispatch') + ->with($this->callback(function ($message) { + $this->assertInstanceOf(DynamicTableMessage::class, $message); + $this->assertSame('phplist_listattr_sizes', $message->getTableName()); + return true; + })) + ->willReturnCallback(function ($message) { + return new Envelope($message); + }); + + $manager = $this->makeManager(); + $manager->createOptionsTableIfNotExists('sizes'); + $manager->createOptionsTableIfNotExists('sizes'); + $this->assertTrue(true); + } +} diff --git a/tests/Unit/Domain/Subscription/Service/Provider/CheckboxGroupValueProviderTest.php b/tests/Unit/Domain/Subscription/Service/Provider/CheckboxGroupValueProviderTest.php index b5be5650..c01ae3b1 100644 --- a/tests/Unit/Domain/Subscription/Service/Provider/CheckboxGroupValueProviderTest.php +++ b/tests/Unit/Domain/Subscription/Service/Provider/CheckboxGroupValueProviderTest.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Tests\Unit\Domain\Subscription\Service\Provider; +use PhpList\Core\Domain\Common\Model\AttributeTypeEnum; use PhpList\Core\Domain\Subscription\Model\Subscriber; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeValue; @@ -30,7 +31,9 @@ private function createAttribute( ?string $tableName = 'colors' ): SubscriberAttributeDefinition { $attr = new SubscriberAttributeDefinition(); - $attr->setName('prefs')->setType($type)->setTableName($tableName); + $attr->setName('prefs') + ->setType(AttributeTypeEnum::from($type)) + ->setTableName($tableName); return $attr; } @@ -52,7 +55,7 @@ public function testSupportsReturnsTrueForCheckboxgroup(): void public function testSupportsReturnsFalseForOtherTypes(): void { - $attr = $this->createAttribute('textline'); + $attr = $this->createAttribute('text'); self::assertFalse($this->subject->supports($attr)); } diff --git a/tests/Unit/Domain/Subscription/Service/Provider/ScalarValueProviderTest.php b/tests/Unit/Domain/Subscription/Service/Provider/ScalarValueProviderTest.php index 2b28c295..1a1e67eb 100644 --- a/tests/Unit/Domain/Subscription/Service/Provider/ScalarValueProviderTest.php +++ b/tests/Unit/Domain/Subscription/Service/Provider/ScalarValueProviderTest.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Tests\Unit\Domain\Subscription\Service\Provider; +use PhpList\Core\Domain\Common\Model\AttributeTypeEnum; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeValue; use PhpList\Core\Domain\Subscription\Service\Provider\ScalarValueProvider; @@ -26,7 +27,7 @@ public function testSupportsReturnsFalseWhenTypeIsNotNull(): void $provider = new ScalarValueProvider(); $attr = $this->createMock(SubscriberAttributeDefinition::class); - $attr->method('getType')->willReturn('checkbox'); + $attr->method('getType')->willReturn(AttributeTypeEnum::Checkbox); self::assertFalse($provider->supports($attr)); } diff --git a/tests/Unit/Domain/Subscription/Service/Provider/SelectOrRadioValueProviderTest.php b/tests/Unit/Domain/Subscription/Service/Provider/SelectOrRadioValueProviderTest.php index 38849fd7..465f5f78 100644 --- a/tests/Unit/Domain/Subscription/Service/Provider/SelectOrRadioValueProviderTest.php +++ b/tests/Unit/Domain/Subscription/Service/Provider/SelectOrRadioValueProviderTest.php @@ -4,6 +4,7 @@ namespace PhpList\Core\Tests\Unit\Domain\Subscription\Service\Provider; +use PhpList\Core\Domain\Common\Model\AttributeTypeEnum; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition; use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeValue; use PhpList\Core\Domain\Subscription\Repository\DynamicListAttrRepository; @@ -18,11 +19,11 @@ public function testSupportsReturnsTrueForSelectAndRadio(): void $provider = new SelectOrRadioValueProvider($repo); $attrSelect = $this->createMock(SubscriberAttributeDefinition::class); - $attrSelect->method('getType')->willReturn('select'); + $attrSelect->method('getType')->willReturn(AttributeTypeEnum::Select); self::assertTrue($provider->supports($attrSelect)); $attrRadio = $this->createMock(SubscriberAttributeDefinition::class); - $attrRadio->method('getType')->willReturn('radio'); + $attrRadio->method('getType')->willReturn(AttributeTypeEnum::Radio); self::assertTrue($provider->supports($attrRadio)); } @@ -32,7 +33,7 @@ public function testSupportsReturnsFalseForOtherTypes(): void $provider = new SelectOrRadioValueProvider($repo); $attr = $this->createMock(SubscriberAttributeDefinition::class); - $attr->method('getType')->willReturn('checkboxgroup'); + $attr->method('getType')->willReturn(AttributeTypeEnum::CheckboxGroup); self::assertFalse($provider->supports($attr)); } diff --git a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php index 884bcc7e..ef48183f 100644 --- a/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php +++ b/tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php @@ -12,7 +12,7 @@ use PhpList\Core\Domain\Subscription\Model\SubscriberAttributeDefinition; use PhpList\Core\Domain\Subscription\Repository\SubscriberAttributeDefinitionRepository; use PhpList\Core\Domain\Subscription\Repository\SubscriberRepository; -use PhpList\Core\Domain\Subscription\Service\CsvImporter; +use PhpList\Core\Domain\Subscription\Service\CsvToDtoImporter; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberAttributeManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberHistoryManager; use PhpList\Core\Domain\Subscription\Service\Manager\SubscriberManager; @@ -29,7 +29,7 @@ class SubscriberCsvImporterTest extends TestCase private SubscriberManager&MockObject $subscriberManagerMock; private SubscriberAttributeManager&MockObject $attributeManagerMock; private SubscriberRepository&MockObject $subscriberRepositoryMock; - private CsvImporter&MockObject $csvImporterMock; + private CsvToDtoImporter&MockObject $csvImporterMock; private SubscriberAttributeDefinitionRepository&MockObject $attributeDefinitionRepositoryMock; private SubscriberCsvImporter $subject; @@ -39,7 +39,7 @@ protected function setUp(): void $this->attributeManagerMock = $this->createMock(SubscriberAttributeManager::class); $subscriptionManagerMock = $this->createMock(SubscriptionManager::class); $this->subscriberRepositoryMock = $this->createMock(SubscriberRepository::class); - $this->csvImporterMock = $this->createMock(CsvImporter::class); + $this->csvImporterMock = $this->createMock(CsvToDtoImporter::class); $this->attributeDefinitionRepositoryMock = $this->createMock(SubscriberAttributeDefinitionRepository::class); $entityManager = $this->createMock(EntityManagerInterface::class); @@ -48,7 +48,7 @@ protected function setUp(): void attributeManager: $this->attributeManagerMock, subscriptionManager: $subscriptionManagerMock, subscriberRepository: $this->subscriberRepositoryMock, - csvImporter: $this->csvImporterMock, + csvToDtoImporter: $this->csvImporterMock, entityManager: $entityManager, translator: new Translator('en'), messageBus: $this->createMock(MessageBusInterface::class), @@ -108,7 +108,7 @@ public function testImportFromCsvCreatesNewSubscribers(): void $importDto2->extraAttributes = ['first_name' => 'Jane']; $this->csvImporterMock - ->method('import') + ->method('parseAndValidate') ->with($tempFile) ->willReturn([ 'valid' => [$importDto1, $importDto2], @@ -173,7 +173,7 @@ public function testImportFromCsvUpdatesExistingSubscribers(): void $importDto->extraAttributes = []; $this->csvImporterMock - ->method('import') + ->method('parseAndValidate') ->with($tempFile) ->willReturn([ 'valid' => [$importDto], @@ -244,7 +244,7 @@ public function testImportResolvesByForeignKeyWhenProvidedAndMatches(): void ); $this->csvImporterMock - ->method('import') + ->method('parseAndValidate') ->with($tempFile) ->willReturn([ 'valid' => [$dto], @@ -309,7 +309,7 @@ public function testImportConflictWhenEmailAndForeignKeyReferToDifferentSubscrib ); $this->csvImporterMock - ->method('import') + ->method('parseAndValidate') ->with($tempFile) ->willReturn([ 'valid' => [$dto], @@ -369,7 +369,7 @@ public function testImportResolvesByEmailWhenForeignKeyNotFound(): void ); $this->csvImporterMock - ->method('import') + ->method('parseAndValidate') ->with($tempFile) ->willReturn([ 'valid' => [$dto], @@ -429,7 +429,7 @@ public function testImportCreatesNewWhenNeitherEmailNorForeignKeyFound(): void ); $this->csvImporterMock - ->method('import') + ->method('parseAndValidate') ->with($tempFile) ->willReturn([ 'valid' => [$dto], diff --git a/tests/Unit/Domain/Subscription/Validator/AttributeTypeValidatorTest.php b/tests/Unit/Domain/Subscription/Validator/AttributeTypeValidatorTest.php index cf691324..d7b5a928 100644 --- a/tests/Unit/Domain/Subscription/Validator/AttributeTypeValidatorTest.php +++ b/tests/Unit/Domain/Subscription/Validator/AttributeTypeValidatorTest.php @@ -4,7 +4,6 @@ namespace PhpList\Core\Tests\Unit\Domain\Subscription\Validator; -use InvalidArgumentException; use PhpList\Core\Domain\Subscription\Validator\AttributeTypeValidator; use PHPUnit\Framework\TestCase; use Symfony\Component\Translation\Translator; @@ -31,16 +30,14 @@ public function testValidatesValidType(): void public function testThrowsExceptionForInvalidType(): void { $this->expectException(ValidatorException::class); - $this->expectExceptionMessage('Invalid attribute type: "invalid_type"'); - + $this->validator->validate('invalid_type'); } public function testThrowsExceptionForNonStringValue(): void { - $this->expectException(InvalidArgumentException::class); - $this->expectExceptionMessage('Value must be a string.'); - + $this->expectException(ValidatorException::class); + $this->validator->validate(123); } }