Add UUID initialization in model save method#26
Add UUID initialization in model save method#26FaboBorgesLima 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
Adds automatic UUID initialization for models using HasUuids by generating missing unique IDs during save().
Changes:
- Introduces an
initialize()helper to populate unique-id columns when empty. - Overrides
save()inHasUuidsto run UUID initialization before delegating to the base model save.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| public function save(array $options = []): bool | ||
| { | ||
| $this->initialize($this->uniqueIds()); |
There was a problem hiding this comment.
Overriding save() to always (re)initialize unique IDs can unintentionally mutate existing records on update. If a model already exists and a unique-id column isn't loaded (or is intentionally set to a falsy value), this will generate a new UUID and mark the attribute dirty, causing an UPDATE that changes a supposedly stable identifier. Consider only initializing unique IDs for new models (e.g., when ! $this->exists) or hooking into the insert/creating path instead of every save.
| $this->initialize($this->uniqueIds()); | |
| if (! $this->exists) { | |
| $this->initialize($this->uniqueIds()); | |
| } |
| 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.
initialize() is a very generic method name in a codebase where models already have an initializeTraits() mechanism and a convention of initialize{TraitName} initializers. Renaming this helper to something more specific (e.g., initializeUniqueIds / initializeUuids) would reduce the risk of name collisions in consuming models/traits and make the call site clearer.
| protected function initialize(array $columns): void | ||
| { | ||
| foreach ($columns as $column) { | ||
| if (in_array($column, $this->uniqueIds()) && ! $this->{$column}) { |
There was a problem hiding this comment.
initialize() is currently called with $this->uniqueIds(), but inside the loop it re-checks in_array($column, $this->uniqueIds()) on every iteration. That condition is redundant (and recomputes uniqueIds() repeatedly); simplifying it would make the intent clearer and avoid unnecessary work.
| if (in_array($column, $this->uniqueIds()) && ! $this->{$column}) { | |
| if (! $this->{$column}) { |
| public function save(array $options = []): bool | ||
| { | ||
| $this->initialize($this->uniqueIds()); | ||
|
|
||
| return parent::save($options); |
There was a problem hiding this comment.
This PR introduces new behavior (UUID assignment during save()), but current tests only assert the format returned by newUniqueId(). Adding a test that saving a new model using HasUuids auto-populates the configured unique-id columns would help prevent regressions, especially around create vs update behavior.
No description provided.