refactor!: migration logging system #31

larskemper · 2025-08-22T14:38:11Z

larskemper · 2025-08-26T07:57:52Z

.github/actions/setup-magento-plugin/action.yml

          [
            {
              "name": "SwagMigrationAssistant",
+              "branch": "feature/migration-logging-refactor",


fyi: this is needed as long as we haven't released the new SwagMigrationAssistant version

YAML should allow comments, so I would suggest adding something like # Todo: change back to trunk after assistant epic gets merged https://github.com/shopware/shopware/issues/10417

should be switch again before merging this, so I thought a thread is more visible

That also works, just thought this PR is targeting the feature branch in magento and later if the merge that feature branch into trunk again we would need to revert this change 🤷‍♂️ .

But either way, we just have to keep track of this somehow and a comment would at least be obvious during code review 😅

Copilot

Pull Request Overview

This PR refactors migration connection usage to align with a new API structure and implements a comprehensive migration logging refactor. The main purpose is to modernize the migration context handling by removing nullable connection references and updating the logging system to use a new builder pattern.

Key Changes:

Migration Context Parameter Reordering: Updated constructor parameter order to prioritize connection, profile, and gateway, removing nullable connection handling
Logging System Refactor: Replaced direct log instantiation with a builder pattern using SwagMigrationLogBuilder
Connection Access Simplification: Removed nullable connection checks and simplified connection credential access

Reviewed Changes

Copilot reviewed 93 out of 93 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
Multiple test files	Updated MigrationContext constructor calls to match new parameter order
Gateway classes	Updated supports() method to use ProfileInterface instead of MigrationContextInterface
Converter classes	Replaced direct logging with SwagMigrationLogBuilder pattern and removed nullable connection handling
Reader classes	Simplified connection credential access by removing null checks
FileHandleErrorLog.php	Refactored logging class to extend new AbstractSwagMigrationLogEntry

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/Profile/Magento/Converter/SalesChannelConverter.php

src/Profile/Magento2/Converter/Magento2SalesChannelConverter.php

.github/actions/setup-magento-plugin/action.yml

tests/Profile/Magento19/Converter/Magento19CrossSellingConverterTest.php

MalteJanz

I only had a rough look but overall it looks fine to me. I hope we got all logs constructed correctly but otherwise we hopefully will see them as bugs in our developer facing log if we implement the "fixing" correctly (as the fix couldn't be applied for example or still caused a write failure)

MalteJanz · 2025-09-09T15:16:40Z

.github/actions/setup-magento-plugin/action.yml

          [
            {
              "name": "SwagMigrationAssistant",
+              "branch": "feature/migration-logging-refactor",


YAML should allow comments, so I would suggest adding something like # Todo: change back to trunk after assistant epic gets merged https://github.com/shopware/shopware/issues/10417

MalteJanz · 2025-09-09T15:20:27Z

src/Profile/Magento/Converter/CategoryConverter.php

+            $this->loggingService->addLogForEach(
+                $fields,
+                fn (string $key) => SwagMigrationLogBuilder::fromMigrationContext($migrationContext)
+                    ->withEntityName(CategoryDefinition::ENTITY_NAME)
+                    ->withFieldSourcePath($key)
+                    ->withSourceData($data)
+                    ->build(EmptyNecessaryFieldRunLog::class)


I guess this would produce duplicated logs as well with your new validation right?
shopware/SwagMigrationAssistant#51 (comment)

Just something to keep in mind (you can also create a ticket / additional PR for adjusting Magento again, if that's necessary 🙈 )

yep, basically all EmptyNecessaryFieldRunLog Logs are somewhat duplicates

fyi: created a new ticket for checking the compatibility shopware/shopware#12442

MalteJanz · 2025-09-09T15:33:00Z

src/Profile/Magento/Converter/SalesChannelConverter.php

        if (!isset($converted['customerGroupId'])) {
            $this->loggingService->addLogEntry(
-                new AssociationRequiredMissingLog(
-                    $this->runId,
-                    DefaultEntities::CUSTOMER_GROUP,
-                    'default_customer_group',
-                    DefaultEntities::SALES_CHANNEL
-                )
+                SwagMigrationLogBuilder::fromMigrationContext($migrationContext)
+                    ->withEntityName(SalesChannelDefinition::ENTITY_NAME)
+                    ->withFieldName('customerGroupId')
+                    ->withFieldSourcePath('group_id')
+                    ->withSourceData($data)
+                    ->withConvertedData($converted)
+                    ->build(AssociationRequiredMissingLog::class)
            );

            return new ConvertStruct(null, $this->originalData);


Not really related to your change but something we have to keep in mind as well:

previously we just logged an error and returned an empty ConvertStruct which basically meant this data would be have the convert_failure flag set in the swag_migration_data table and the write step would ignore this entry completely.

But with this new approach we have to rewrite all converters to:

still log any issues they encounter

additionally continue and try to extract as much data as possible, so with the later "fixes" we get a complete entity again

This also has another side effect:

since we log each error individually the attached convertedData can still evolve and change to include more in a later log. So maybe it's not that useful in the end, we have to evaluate that. Alternative would be just linking to the actual data (swag_migration_data)

Maybe this will also be something we uncover during our backend PoC / prototype and have to deal with👀

MalteJanz · 2025-09-09T15:35:36Z

src/Profile/Magento2/Converter/Magento2CustomerConverter.php

+                        ->withEntityName(CustomerDefinition::ENTITY_NAME)
+                        ->withFieldName('password_hash')


Isn't the field just called password in SW6?

yeah, it is 🙈

refactor: connection & log usage

d4dedb9

larskemper self-assigned this Aug 22, 2025

larskemper mentioned this pull request Aug 22, 2025

refactor!: migration connection usage shopware/SwagMigrationAssistant#43

Merged

larskemper changed the base branch from trunk to feature/migration-logging-refactor August 26, 2025 07:22

docs: add todos

a1c80c1

larskemper force-pushed the refactor/migration-connection-usage branch from 5812481 to a1c80c1 Compare August 26, 2025 07:43

larskemper requested review from CR0YD, MalteJanz, Copilot, ennasus4sun and jozsefdamokos August 26, 2025 07:53

This comment was marked as outdated.

Sign in to view

ci: fix

20f1be8

larskemper force-pushed the refactor/migration-connection-usage branch from fbe5f78 to 20f1be8 Compare August 26, 2025 07:54

larskemper changed the title ~~refactor: migration connection usage~~ refactor!: migration connection usage Aug 26, 2025

larskemper commented Aug 26, 2025

View reviewed changes

larskemper force-pushed the refactor/migration-connection-usage branch 4 times, most recently from 73382da to d141656 Compare August 26, 2025 09:07

fix: phpunit

c83cec3

larskemper force-pushed the refactor/migration-connection-usage branch from d141656 to c83cec3 Compare August 26, 2025 09:45

larskemper marked this pull request as ready for review August 26, 2025 09:50

larskemper requested a review from Copilot August 26, 2025 09:50

Copilot AI reviewed Aug 26, 2025

View reviewed changes

src/Profile/Magento/Converter/SalesChannelConverter.php Outdated Show resolved Hide resolved

src/Profile/Magento2/Converter/Magento2SalesChannelConverter.php Outdated Show resolved Hide resolved

.github/actions/setup-magento-plugin/action.yml Show resolved Hide resolved

ennasus4sun approved these changes Aug 27, 2025

View reviewed changes

tests/Profile/Magento19/Converter/Magento19CrossSellingConverterTest.php Show resolved Hide resolved

feat: add meaningful data to magento migration logs (#32)

a54284c

larskemper changed the title ~~refactor!: migration connection usage~~ refactor!: migration logging system (step 1) Aug 29, 2025

larskemper changed the title ~~refactor!: migration logging system (step 1)~~ refactor!: migration logging system Sep 2, 2025

larskemper marked this pull request as draft September 2, 2025 06:13

MalteJanz approved these changes Sep 9, 2025

View reviewed changes

larskemper mentioned this pull request Sep 11, 2025

Ensure SwagMigrationMagento compatibility shopware/shopware#12442

Open

larskemper assigned MalteJanz and larskemper and unassigned larskemper Sep 11, 2025

		->withEntityName(CustomerDefinition::ENTITY_NAME)
		->withFieldName('password_hash')

refactor!: migration logging system #31

Are you sure you want to change the base?

refactor!: migration logging system #31

Uh oh!

Conversation

larskemper commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MalteJanz Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes:

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MalteJanz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

larskemper commented Aug 22, 2025 •

edited

Loading

MalteJanz Sep 11, 2025 •

edited

Loading