Skip to content

Conversation

@huangdijia
Copy link
Member

Summary

This PR extracts common methods from HasUuids and HasUlids traits into a new HasUniqueStringIds trait to improve code maintainability and reduce duplication.

Changes Made

  • Created HasUniqueStringIds trait with shared logic:

    • uniqueIds() - Returns columns that should receive unique identifiers
    • getKeyType() - Determines the key type based on unique IDs
    • getIncrementing() - Determines if IDs are auto-incrementing
    • newUniqueId() - Abstract method to be implemented by using traits
  • Modified HasUuids trait:

    • Now uses HasUniqueStringIds trait
    • Removed duplicate methods (moved to HasUniqueStringIds)
    • Only implements newUniqueId() for UUID generation
  • Modified HasUlids trait:

    • Now uses HasUniqueStringIds trait
    • Removed duplicate methods (moved to HasUniqueStringIds)
    • Only implements newUniqueId() for ULID generation

Benefits

  • DRY Principle: Eliminates 76 lines of duplicate code
  • Maintainability: Changes to shared logic only need to be made in one place
  • Extensibility: Future unique ID types can easily use the same shared trait
  • No Breaking Changes: Maintains the same public API and behavior

Test plan

  • Verify existing tests pass for models using HasUuids trait
  • Verify existing tests pass for models using HasUlids trait
  • Confirm that models with UUIDs/ULIDs still generate and handle IDs correctly
  • Check that getKeyType() returns 'string' for unique ID columns
  • Check that getIncrementing() returns false for unique ID columns

…uplication

Extracted common methods from HasUuids and HasUlids traits into a new HasUniqueStringIds trait to improve code maintainability and reduce duplication. The new trait contains shared logic for:
- uniqueIds() method to get columns receiving unique identifiers
- getKeyType() method to determine the key type based on unique IDs
- getIncrementing() method to determine if IDs are auto-incrementing

This refactoring makes the codebase more DRY and easier to maintain.
@huangdijia huangdijia added this to the v3.2 milestone Nov 1, 2025
@huangdijia huangdijia marked this pull request as draft November 1, 2025 13:13
@huangdijia huangdijia marked this pull request as ready for review November 1, 2025 13:23
@huangdijia huangdijia requested a review from Copilot November 1, 2025 13:23
Copy link

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

This PR refactors the HasUuids and HasUlids traits to extract common functionality into a new shared HasUniqueStringIds trait, reducing code duplication and improving maintainability.

  • Extracts shared methods (uniqueIds, getKeyType, getIncrementing) into HasUniqueStringIds trait
  • Adds route binding validation with resolveRouteBindingQuery and handleInvalidUniqueId methods
  • Both HasUuids and HasUlids now only implement their specific validation logic via isValidUniqueId

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/database/src/Model/Concerns/HasUniqueStringIds.php New trait containing shared functionality for string-based unique IDs including validation, route binding, and key type management
src/database/src/Model/Concerns/HasUuids.php Refactored to use HasUniqueStringIds trait and only implement UUID-specific validation
src/database/src/Model/Concerns/HasUlids.php Refactored to use HasUniqueStringIds trait and only implement ULID-specific validation

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

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@limingxinleo limingxinleo merged commit f04922d into hyperf:3.2 Nov 3, 2025
76 checks passed
@huangdijia huangdijia deleted the refactor/extract-unique-string-ids-trait branch November 3, 2025 09:40
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.

2 participants