Skip to content

Add initialization for unique identifiers in HasUuids#7727

Open
FaboBorgesLima wants to merge 4 commits intohyperf:masterfrom
FaboBorgesLima:patch-2
Open

Add initialization for unique identifiers in HasUuids#7727
FaboBorgesLima wants to merge 4 commits intohyperf:masterfrom
FaboBorgesLima:patch-2

Conversation

@FaboBorgesLima
Copy link
Copy Markdown

No description provided.

Copilot AI review requested due to automatic review settings March 26, 2026 05:46
Copy link
Copy Markdown

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

Adds trait-level initialization logic so models using HasUuids can have their unique identifier columns pre-populated automatically during model construction.

Changes:

  • Added initializeHasUuids() trait initializer to set UUIDs on columns returned by uniqueIds() when empty.
  • Generates UUIDs via existing newUniqueId() (UUIDv7) during initialization.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

{
$columns = $this->uniqueIds();
foreach ($columns as $column) {
if (in_array($column, $this->uniqueIds()) && ($this->{$column} === null || $this->{$column} === '')) {
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside the foreach ($columns as $column) loop, in_array($column, $this->uniqueIds()) is redundant (the loop already iterates over uniqueIds()), and calling uniqueIds() again inside the loop adds unnecessary work and could behave inconsistently if uniqueIds() is overridden with non-deterministic logic. Use the $columns variable (or remove the membership check entirely) and keep the emptiness check only.

Suggested change
if (in_array($column, $this->uniqueIds()) && ($this->{$column} === null || $this->{$column} === '')) {
if ($this->{$column} === null || $this->{$column} === '') {

Copilot uses AI. Check for mistakes.
Comment on lines +72 to +79
protected function initializeHasUuids(): void
{
$columns = $this->uniqueIds();
foreach ($columns as $column) {
if (in_array($column, $this->uniqueIds()) && ($this->{$column} === null || $this->{$column} === '')) {
$this->{$column} = $this->newUniqueId();
}
}
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change adds new runtime behavior (auto-populating unique ID columns during model initialization), but there is no test covering it. Consider adding/expanding a unit test (e.g., using ModelStubWithUuid) to assert that a new model instance gets a non-empty UUID assigned for the key column, and that providing an explicit key value is not overwritten.

Copilot uses AI. Check for mistakes.
FaboBorgesLima and others added 2 commits March 26, 2026 02:53
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Refactor UUID initialization logic to use the initializeHasUUID method for better clarity and efficiency.
@limingxinleo
Copy link
Copy Markdown
Member

应该不需要这个吧

Now it only triggers when inserting
@FaboBorgesLima
Copy link
Copy Markdown
Author

应该不需要这个吧

You were completely right. Using initialize generates UUIDs on every instantiation (like hydrating select queries), causing unnecessary CPU overhead.
I refactored the Trait to override performInsert directly. Now, it only generates the UUID right before the database insertion, and then calls parent::performInsert(). This isolates the logic within the Trait and reduces the overhead to zero.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants