551 ramsey UUID en ramsey UUID doctrine#434
Merged
ilaria-orlando merged 6 commits intomasterfrom Mar 23, 2026
Merged
Conversation
Reviewer's GuideReplaces ramsey/uuid and its Doctrine integration with Symfony Uid-based UUIDs across MediaLibrary domain objects, introduces a custom Doctrine DBAL UUID type, and updates JSON/form serialization and configuration to work with Symfony\Component\Uid\Uuid while removing the ramsey dependencies. Class diagram for updated MediaGroup and MediaGroupMediaItem UUID handlingclassDiagram
class MediaGroup {
- Uuid id
- Type type
+ private __construct(Uuid id, Type type)
+ static create(Type type) MediaGroup
+ static createFromId(Uuid id, Type type) MediaGroup
+ getId() Uuid
+ getType() Type
+ jsonSerialize() array
}
class MediaGroupMediaItem {
- Uuid id
- MediaGroup group
- MediaItem item
- DateTime createdOn
- int sequence
+ private __construct(MediaGroup group, MediaItem item, int sequence)
+ static create(MediaGroup group, MediaItem item, int sequence) MediaGroupMediaItem
+ jsonSerialize() array
+ getId() Uuid
}
class MediaGroupDataTransferObject {
- Uuid id
}
class MediaGroupType {
- MessageBusSupportingMiddleware bus
+ private getMediaGroupFromMediaGroupData(array mediaGroupData) MediaGroup
+ private getMediaGroupTransformFunction() callable
}
class SaveMediaGroupHandler {
- MediaItemRepository mediaItemRepository
+ __construct(MediaItemRepository mediaItemRepository)
}
class MediaGallery {
- MediaGroup mediaGroup
}
class Uuid {
+ static v4() Uuid
+ static fromString(string value) Uuid
+ static isValid(string value) bool
+ toRfc4122() string
}
MediaGroup "1" o-- "*" MediaGroupMediaItem : contains
MediaGroupMediaItem "1" --> "1" MediaGroup : group
MediaGroupMediaItem "1" --> "1" MediaItem : item
MediaGallery "1" --> "1" MediaGroup : references
MediaGroupDataTransferObject "1" --> "1" Uuid : id
MediaGroupType "1" --> "1" MediaGroup : builds
SaveMediaGroupHandler "1" --> "1" MediaItemRepository : uses
MediaGroup "1" --> "1" Uuid : id
MediaGroupMediaItem "1" --> "1" Uuid : id
Class diagram for new Common Doctrine UuidTypeclassDiagram
class GuidType {
}
class UuidType {
+ const NAME
+ convertToPHPValue(mixed value, AbstractPlatform platform) Uuid
+ convertToDatabaseValue(mixed value, AbstractPlatform platform) string
+ getName() string
+ requiresSQLCommentHint(AbstractPlatform platform) bool
+ getMappedDatabaseTypes(AbstractPlatform platform) string[]
}
class Uuid {
+ static fromString(string value) Uuid
+ static isValid(string value) bool
+ toRfc4122() string
}
class AbstractPlatform {
}
class ConversionException {
+ static conversionFailed(mixed value, string type) ConversionException
}
UuidType --|> GuidType
UuidType --> Uuid : converts
UuidType --> AbstractPlatform : db platform
UuidType --> ConversionException : throws
Flow diagram for UuidType database and PHP value conversionflowchart LR
subgraph Database
DBValue["UUID column value (string or NULL)"]
end
subgraph "PHP\nDoctrine DBAL"
UuidTypeInstance["Common\\Doctrine\\Type\\UuidType"]
PHPValue["PHP value (Symfony Uuid or null)"]
end
DBValue -->|read from DB| UuidTypeInstance
UuidTypeInstance -->|convertToPHPValue\n- if value is Uuid: return as is\n- if empty or non string: return null\n- else Uuid from string or throw ConversionException| PHPValue
PHPValue -->|write to DB| UuidTypeInstance
UuidTypeInstance -->|convertToDatabaseValue\n- if null or empty: null\n- if Uuid: toRfc4122\n- if string or stringable and Uuid is valid: string\n- else: ConversionException| DBValue
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
MediaGroupMediaItem, consider adding aUuidtyped property for$id(as done inMediaGroup) instead of relying only on a docblock, to keep UUID handling consistent and benefit from static analysis. - In
Common\Doctrine\Type\UuidType::convertToPHPValue, returningnullfor non-string, non-null values may mask data issues; consider throwing aConversionExceptionin that branch so invalid database values are surfaced explicitly. - The
symfony/uiddependency is currently required as"*"incomposer.json; tightening this to a specific major/minor constraint (e.g.^6.0or^7.0) will reduce the risk of unexpected breaking changes on update.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MediaGroupMediaItem`, consider adding a `Uuid` typed property for `$id` (as done in `MediaGroup`) instead of relying only on a docblock, to keep UUID handling consistent and benefit from static analysis.
- In `Common\Doctrine\Type\UuidType::convertToPHPValue`, returning `null` for non-string, non-null values may mask data issues; consider throwing a `ConversionException` in that branch so invalid database values are surfaced explicitly.
- The `symfony/uid` dependency is currently required as `"*"` in `composer.json`; tightening this to a specific major/minor constraint (e.g. `^6.0` or `^7.0`) will reduce the risk of unexpected breaking changes on update.
## Individual Comments
### Comment 1
<location path="src/Common/Doctrine/Type/UuidType.php" line_range="32-37" />
<code_context>
+
+ public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string
+ {
+ if ($value === null || $value === '') {
+ return null;
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Treating empty-string values as null in convertToDatabaseValue may mask invalid inputs.
Because `convertToDatabaseValue` maps both `null` and `''` to `null`, an empty string ID from the application will be stored as `NULL` instead of failing conversion. This can hide invalid input and violate NOT NULL expectations. Consider only treating `null` as `null` and letting `''` go through normal validation so it raises a `ConversionException` if it’s not a valid UUID.
```suggestion
public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string
{
if ($value === null) {
return null;
}
```
</issue_to_address>
### Comment 2
<location path="composer.json" line_range="62-63" />
<code_context>
"twig/string-extra": "^3.23",
- "twig/inky-extra": "^3.23"
+ "twig/inky-extra": "^3.23",
+ "symfony/uid": "*"
},
"require-dev": {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use a bounded version constraint for symfony/uid instead of a wildcard.
Using "*" for `symfony/uid` makes installs non-deterministic and risks unexpected breaking changes when a new major version is released. Please align this with the rest of the Symfony stack by using a bounded constraint (e.g. `^6.0`).
```suggestion
"twig/inky-extra": "^3.23",
"symfony/uid": "^6.0"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+32
to
+37
| public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string | ||
| { | ||
| if ($value === null || $value === '') { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
suggestion (bug_risk): Treating empty-string values as null in convertToDatabaseValue may mask invalid inputs.
Because convertToDatabaseValue maps both null and '' to null, an empty string ID from the application will be stored as NULL instead of failing conversion. This can hide invalid input and violate NOT NULL expectations. Consider only treating null as null and letting '' go through normal validation so it raises a ConversionException if it’s not a valid UUID.
Suggested change
| public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string | |
| { | |
| if ($value === null || $value === '') { | |
| return null; | |
| } | |
| public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string | |
| { | |
| if ($value === null) { | |
| return null; | |
| } | |
tijsverkoyen
approved these changes
Mar 19, 2026
tijsverkoyen
approved these changes
Mar 19, 2026
jonasdekeukelaere
approved these changes
Mar 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Type
Pull request description
Replace ramsey/uuid and ramsey/doctrine/uuid by symfony/uuid for MediaLibrary, MediaGallery, MediaGroupMediaItem and MediaGroup
Summary by Sourcery
Migrate MediaLibrary and related domains from ramsey/uuid to Symfony's UID UUID implementation and centralize UUID database handling.
Enhancements:
Build: