Skip to content

FEATURE: Introduce RoleId and RoleIds value objects#3415

Open
bwaidelich wants to merge 7 commits into9.0from
feature/3414-roleIds-value-object
Open

FEATURE: Introduce RoleId and RoleIds value objects#3415
bwaidelich wants to merge 7 commits into9.0from
feature/3414-roleIds-value-object

Conversation

@bwaidelich
Copy link
Copy Markdown
Member

Resolves: #3414

$this->description = $description;
$this->parentRoles = $parentRoles;
}

Copy link
Copy Markdown
Member

@kitsunet kitsunet Nov 12, 2024

Choose a reason for hiding this comment

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

Suggested change
public static function fromRoleId(RoleId $id, array $parentRoles = [], string $label = '', string $description = ''): self
{
return new ($id->value, array $parentRoles = [], string $label = '', string $description = '');
}
public static function fromIdentifierString(string $identifier, array $parentRoles = [], string $label = '', string $description = ''): self
{
return new ($identifier, array $parentRoles = [], string $label = '', string $description = '');
}

And then deprecate the __construct as it is so we can at least open a window for refactoring internally?

Copy link
Copy Markdown
Member Author

@bwaidelich bwaidelich Nov 12, 2024

Choose a reason for hiding this comment

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

@kitsunet mh, Role::fromRoleId() and Role::fromIdentifierString() sounds wrong to me.. But what do you think of

public function __construct(RoleId|string $id, array $parentRoles = [], string $label = '', string $description = '') {
  if (is_string($id)) {
    $id = RoleId::fromString($id);
  }
  // ...
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sure, sounds good! My thought train was not finished really. I was thinking / hoping how we can refactor towards something like RoleIds $parentRoles or somthing, but the static constructors would not necessarily make that easier :D

Copy link
Copy Markdown
Member Author

@bwaidelich bwaidelich Nov 13, 2024

Choose a reason for hiding this comment

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

I agree – there is a lot of potential to improve things, but I wanted to avoid another rabbit hole (like #3410)
Adjusted with 32bbd2d

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, totally fair

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Introduce RoleIds value object

2 participants