Skip to content

Rename classes/functions #369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Rename classes/functions #369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions config/services/mappers.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
2 changes: 1 addition & 1 deletion ...ubscription/Service/CsvRowToDtoMapper.php → ...Subscription/Service/ArrayToDtoMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down
6 changes: 3 additions & 3 deletions ...main/Subscription/Service/CsvImporter.php → ...Subscription/Service/CsvToDtoImporter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
) {
Expand All @@ -25,7 +25,7 @@ public function __construct(
* @return array{valid: ImportSubscriberDto[], errors: array<int, array<string>>}
* @throws CsvException
*/
public function import(string $csvFilePath): array
public function parseAndValidate(string $csvFilePath): array
{
$reader = Reader::createFromPath($csvFilePath, 'r');
$reader->setHeaderOffset(0);
Expand Down
23 changes: 14 additions & 9 deletions src/Domain/Subscription/Service/SubscriberCsvImporter.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Comment on lines +26 to +31
Copy link

@coderabbitai coderabbitai bot Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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

* @SuppressWarnings("CouplingBetweenObjects")
* @SuppressWarnings("ExcessiveParameterList")
*/
Expand All @@ -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;
Expand All @@ -46,7 +49,7 @@ public function __construct(
SubscriberAttributeManager $attributeManager,
SubscriptionManager $subscriptionManager,
SubscriberRepository $subscriberRepository,
CsvImporter $csvImporter,
CsvToDtoImporter $csvToDtoImporter,
EntityManagerInterface $entityManager,
TranslatorInterface $translator,
MessageBusInterface $messageBus,
Expand All @@ -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;
Expand All @@ -78,6 +81,7 @@ public function importFromCsv(
'updated' => 0,
'skipped' => 0,
'blacklisted' => 0,
'invalid_email' => 0,
'errors' => [],
];

Expand All @@ -89,7 +93,7 @@ public function importFromCsv(
);
}

$result = $this->csvImporter->import($path);
$result = $this->csvToDtoImporter->parseAndValidate($path);

foreach ($result['valid'] as $dto) {
try {
Expand Down Expand Up @@ -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;
}
Expand Down
20 changes: 10 additions & 10 deletions tests/Unit/Domain/Subscription/Service/SubscriberCsvImporterTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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);

Expand All @@ -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),
Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -173,7 +173,7 @@ public function testImportFromCsvUpdatesExistingSubscribers(): void
$importDto->extraAttributes = [];

$this->csvImporterMock
->method('import')
->method('parseAndValidate')
->with($tempFile)
->willReturn([
'valid' => [$importDto],
Expand Down Expand Up @@ -244,7 +244,7 @@ public function testImportResolvesByForeignKeyWhenProvidedAndMatches(): void
);

$this->csvImporterMock
->method('import')
->method('parseAndValidate')
->with($tempFile)
->willReturn([
'valid' => [$dto],
Expand Down Expand Up @@ -309,7 +309,7 @@ public function testImportConflictWhenEmailAndForeignKeyReferToDifferentSubscrib
);

$this->csvImporterMock
->method('import')
->method('parseAndValidate')
->with($tempFile)
->willReturn([
'valid' => [$dto],
Expand Down Expand Up @@ -369,7 +369,7 @@ public function testImportResolvesByEmailWhenForeignKeyNotFound(): void
);

$this->csvImporterMock
->method('import')
->method('parseAndValidate')
->with($tempFile)
->willReturn([
'valid' => [$dto],
Expand Down Expand Up @@ -429,7 +429,7 @@ public function testImportCreatesNewWhenNeitherEmailNorForeignKeyFound(): void
);

$this->csvImporterMock
->method('import')
->method('parseAndValidate')
->with($tempFile)
->willReturn([
'valid' => [$dto],
Expand Down
Loading