Skip to content

Conversation

@DennisGarding
Copy link
Contributor

@DennisGarding DennisGarding commented Oct 30, 2025

Remove all checks, which results in an empty Convert Result. These should be catches by the later entity validation

@DennisGarding DennisGarding marked this pull request as draft October 30, 2025 13:50
@DennisGarding DennisGarding self-assigned this Oct 30, 2025
@DennisGarding DennisGarding marked this pull request as ready for review October 31, 2025 09:08
@larskemper larskemper requested a review from Copilot October 31, 2025 12:18
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the Shopware migration assistant to handle edge cases more gracefully by moving validation logic from converters to writers and making the conversion process more permissive. The key changes include:

  • Removing early returns from converters when optional data is missing, allowing partial conversions to proceed
  • Adding filtering logic to writers to prevent overwriting locked/system-default data
  • Replacing $this->getContainer() with static::getContainer() throughout test files for consistency
  • Updating fixture outputs to reflect new conversion behavior for previously rejected edge cases

Reviewed Changes

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

Show a summary per file
File Description
src/Profile/Shopware6/Writer/ProductSortingWriter.php Adds constructor and writeData method to filter out locked product sortings before writing
src/Profile/Shopware6/Writer/CmsPageWriter.php Adds writeData method to filter out locked CMS pages before writing
src/Profile/Shopware6/Converter/TaxRuleConverter.php Refactors to continue conversion even when optional associations are missing
src/Profile/Shopware6/Converter/SeoUrlConverter.php Removes early return for unmodified SEO URLs, makes foreignKey conversion conditional
src/Profile/Shopware6/Converter/ProductSortingConverter.php Removes locked product sorting check from converter (moved to writer)
src/Profile/Shopware6/Converter/NumberRangeConverter.php Makes type lookup non-blocking, allows conversion without valid type
src/Profile/Shopware6/Converter/MediaFolderConverter.php Restructures logic to handle edge case for digital products media folder
src/Profile/Shopware6/Converter/MailTemplateConverter.php Makes mail template type lookup non-blocking
src/Profile/Shopware6/Converter/DocumentConverter.php Makes document type lookup non-blocking
src/Profile/Shopware6/Converter/DocumentBaseConfigConverter.php Makes document type lookup non-blocking
src/Profile/Shopware6/Converter/CustomerWishlistConverter.php Removes customer and sales channel validation from converter
src/Profile/Shopware6/Converter/CmsPageConverter.php Removes locked CMS page handling from converter (moved to writer)
src/Profile/Shopware6/Converter/CategoryCmsPageAssociationConverter.php Makes CMS page mapping lookup non-blocking
src/Profile/Shopware/Converter/* Multiple converters refactored to remove strict validation and allow partial conversions
src/Migration/Writer/SeoUrlWriter.php Filters out modified SEO URLs before writing
src/Migration/Writer/LanguageWriter.php Adds constructor and writeData to filter existing languages
tests/**/* Updates getContainer() to static::getContainer() and adjusts test expectations
tests/_fixtures/**/* Updates test fixtures to reflect new conversion behavior
src/DependencyInjection/*.xml Adds required service dependencies for new writer logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@MalteJanz
Copy link
Contributor

Please add a short PR description 🙂 . E.g. questions I would like to see answered in it:

  • What type of checks are being removed, e.g. checks for entity fields that are required in SW6 and would produce another log entry otherwise in the later entity validation
  • What other type of checks are removed and what's the reasoning behind it. E.g. does the user still get notified about errors in optional fields? Or are they just silently set to null?

…are/SwagMigrationAssistant into 13124/remove-validation-from-converter
@DennisGarding DennisGarding force-pushed the 13124/remove-validation-from-converter branch from 44cdc50 to 92419d8 Compare November 11, 2025 06:47
Copy link
Contributor

@ennasus4sun ennasus4sun left a comment

Choose a reason for hiding this comment

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

nice 💪🏻

Copy link
Contributor

@MalteJanz MalteJanz left a comment

Choose a reason for hiding this comment

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

Still have to look further (didn't had the time yet for a full review) 🙈 .

And guys, please review and test this very carefully. Because we can easily introduce new bugs here, especially as our acceptance tests aren't running right now in the feature branch

// TODO: fix & update snapshots
// eslint-disable-next-line playwright/no-skipped-test
test.skip();

Copy link
Contributor

@MalteJanz MalteJanz left a comment

Choose a reason for hiding this comment

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

After my open comments are solved: LGTM, nice job, this was quite a task 💪

@DennisGarding DennisGarding merged commit 5d4ec50 into feature/migration-logging-refactor Dec 2, 2025
8 checks passed
@DennisGarding DennisGarding deleted the 13124/remove-validation-from-converter branch December 2, 2025 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants