Add UUID initialization in model save method#25
Add UUID initialization in model save method#25FaboBorgesLima wants to merge 1 commit intohyperf:masterfrom
Conversation
|
Hi, this is a READ-ONLY repository, please submit your PR on the https://github.com/hyperf/hyperf repository. |
There was a problem hiding this comment.
Pull request overview
This PR updates the HasUuids model concern to automatically populate UUID-based identifier columns when a model is saved, so UUID primary keys (and other uniqueIds) don’t need to be manually assigned by callers.
Changes:
- Added a helper method to assign new UUIDs to configured
uniqueIdscolumns when missing. - Overrode
save()in the UUID trait to run the initialization before delegating to the base model save.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected function initialize(array $columns): void | ||
| { | ||
| foreach ($columns as $column) { | ||
| if (in_array($column, $this->uniqueIds()) && ! $this->{$column}) { | ||
| $this->{$column} = $this->newUniqueId(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The helper method name initialize is very generic and can easily collide with other traits or model methods, and it is also easy to confuse with the model’s trait initializer convention (initialize{TraitName} collected in Model::bootTraits). Renaming this to something UUID-specific (e.g., initializeUniqueIds / setUniqueIdsIfMissing) will reduce the risk of trait method collisions and improve readability.
| foreach ($columns as $column) { | ||
| if (in_array($column, $this->uniqueIds()) && ! $this->{$column}) { |
There was a problem hiding this comment.
Inside initialize(), the in_array($column, $this->uniqueIds()) check is redundant when the caller already passes $this->uniqueIds(), and it also recomputes uniqueIds() on every iteration. Consider either removing the membership check entirely or computing uniqueIds() once before the loop.
| foreach ($columns as $column) { | |
| if (in_array($column, $this->uniqueIds()) && ! $this->{$column}) { | |
| $uniqueIds = $this->uniqueIds(); | |
| foreach ($columns as $column) { | |
| if (in_array($column, $uniqueIds, true) && ! $this->{$column}) { |
| public function save(array $options = []): bool | ||
| { | ||
| $this->initialize($this->uniqueIds()); | ||
|
|
||
| return parent::save($options); |
There was a problem hiding this comment.
This change alters persistence behavior by auto-populating UUID columns during save(), but there are no tests covering the new behavior. Please add/update tests to assert (1) a new model gets a UUID assigned before insert, and (2) saving an existing model does not regenerate or change its UUID/primary key.
|
|
||
| public function save(array $options = []): bool | ||
| { | ||
| $this->initialize($this->uniqueIds()); |
There was a problem hiding this comment.
Overriding save() to assign UUIDs unconditionally can mutate primary keys (or other uniqueIds) during update operations. In particular, if an existing model instance has a falsey/missing key attribute (e.g., loaded with a select that omits the PK), this will generate a new UUID and the subsequent update can target the wrong row (or no row) while still returning true. Consider only generating IDs for inserts (e.g., guard with if (! $this->exists)), or move the logic to the insert path (such as performInsert) so updates never auto-regenerate identifiers.
| $this->initialize($this->uniqueIds()); | |
| if (! $this->exists) { | |
| $this->initialize($this->uniqueIds()); | |
| } |
No description provided.