-
Notifications
You must be signed in to change notification settings - Fork 25
feat: add migration logging required fields #38
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.
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
feat: add migration logging required fields #38
Conversation
23e60b2 to
b256199
Compare
4e9147e to
b3ac25f
Compare
| static::assertCount(20, $data); | ||
| static::assertSame('attr1', $data[0]['name']); | ||
| static::assertSame('text', $data[0]['type']); | ||
| static::assertCount(21, $data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MalteJanz I'm not quite sure why these changed/failed now with my changes and not earlier. Comparing them to sw55.sql, they seem fine, but I don't fully understand the details to be certain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strange, maybe this change in your MR caused this?
SwagMigrationAssistant/src/Profile/Shopware/Gateway/Local/Reader/AttributeReader.php
Line 191 in 1ebff3d
| $fks[] = $foreignKey->getReferencingColumnNames(); |
On trunk it looks like this:
SwagMigrationAssistant/src/Profile/Shopware/Gateway/Local/Reader/AttributeReader.php
Line 191 in f41e6ab
| $fks[] = $foreignKey->getLocalColumns(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good one, that's it. I changed it because getLocalColumns() was deprecated and replaced with getReferencingColumnNames(). For some reason, the previous method didn’t return the "first" column, so I think this behavior is correct now.
e5673e1 to
a3a33d9
Compare
a3a33d9 to
e581615
Compare
0a01359 to
1ebff3d
Compare
e0d0cd5
into
feature/migration-logging-refactor
This reverts commit e0d0cd5.
There was a problem hiding this 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 migration logging to require profile name and gateway name fields instead of entity and source ID, consolidating logging messages and simplifying the logging structure. This change addresses issue #11810 by standardizing the required fields for migration logging.
Key Changes:
- Refactored all logging classes to use
profileNameandgatewayNameinstead ofentityandsourceId - Simplified log entry interface by removing detailed description/title methods and consolidating code patterns
- Updated logging database schema and entity to match new required fields structure
Reviewed Changes
Copilot reviewed 118 out of 120 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Migration/Logging/Log/*.php |
Updated all logging classes to use new constructor pattern with profile/gateway names |
src/Migration/Logging/SwagMigrationLoggingDefinition.php |
Modified database schema to use new required fields |
src/Migration/Logging/SwagMigrationLoggingEntity.php |
Updated entity properties to match new schema |
src/Migration/MigrationContextInterface.php |
Changed connection getter to non-nullable |
tests/ |
Updated test files to use new logging patterns and file existence checks |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
resolves #11810