Skip to content

feat: error resolution#41

Merged
Malte Janz (MalteJanz) merged 83 commits intotrunkfrom
feature/migration-logging-refactor
Feb 27, 2026
Merged

feat: error resolution#41
Malte Janz (MalteJanz) merged 83 commits intotrunkfrom
feature/migration-logging-refactor

Conversation

@larskemper
Copy link
Member

@larskemper Lars Kemper (larskemper) commented Aug 19, 2025

@larskemper Lars Kemper (larskemper) force-pushed the feature/migration-logging-refactor branch from 45c0a95 to 4a5b659 Compare August 28, 2025 08:06
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 represents the first step in refactoring the migration logging system. The changes focus on standardizing log entry creation and data collection while maintaining backward compatibility.

  • Introduces a new SwagMigrationLogBuilder for consistent log entry creation across the system
  • Refactors MigrationContext constructor to improve parameter order and make connection a required parameter
  • Simplifies fixture files by removing redundant source ID tracking and verbose log parameters
  • Updates test files to use the modernized logging system and parameter structures

Reviewed Changes

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

Show a summary per file
File Description
Various test snapshots Updated to reflect simplified log structure without source ID tracking
Test files across Profile/ Updated MigrationContext constructor calls with reordered parameters
Converter test files Simplified assertion patterns for log codes without parameter validation
Gateway and media processing Updated to use new logging builder pattern and simplified connection handling
Logging classes Refactored to extend AbstractSwagMigrationLogEntry with simplified structure
Comments suppressed due to low confidence (1)

tests/Profile/Shopware55/Converter/TranslationConverterTest.php:1

  • This assertion is duplicated on line 437. The second assertion should likely check $logs[3]['code'] instead of $logs[2]['code'] again, or verify a different aspect of the log entry.
<?php declare(strict_types=1);

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

@MalteJanz
Copy link
Contributor

Malte Janz (MalteJanz) commented Feb 26, 2026

Fyi: I'm doing an interactive rebase to rename these commits to follow our conventional commit naming and force push that:

r 001ed93 # 12367/add entity classes (#76)
r d6af580 # 13248/change swag migration fix table (#81)
r 8a16844 # Chore: 13247/change table swag migration logging (#80)
r abde712 # prettier

Unfortunately that didn't work out and we tried a few different approaches, all led to conflicts and differences in the files afterwards (likely because of the previous merges and conflict resolution). So we decided to stick to this history

@MalteJanz Malte Janz (MalteJanz) marked this pull request as ready for review February 26, 2026 13:41
@MalteJanz
Copy link
Contributor

Looks like our acceptance test went red. Turns out it's not a good idea to include exception trace in our snapshot test, if changes to unrelated code can make them fail 🙈 . We should remove them before comparing the snapshot, as in the actual log they are really useful.

Screenshot 2026-02-26 at 15 22 19

@MalteJanz Malte Janz (MalteJanz) merged commit dc865e6 into trunk Feb 27, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment