Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: make aggregate id an interface #566

Merged
merged 14 commits into from
Oct 6, 2023
Merged

Conversation

mateuszsip
Copy link
Contributor

No description provided.

@mateuszsip
Copy link
Contributor Author

1) Class AggregateIdTest cannot be found in /home/runner/work/aggregate/aggregate/tests/unit/AggregateIdTest.php

It seems that phpunit has a problem with trait approach, we do the same in our services and it works fine 🤔

tests/unit/AggregateIdTest.php Outdated Show resolved Hide resolved
src/UuidV7AggregateIdTrait.php Show resolved Hide resolved
src/UuidAggregateIdTrait.php Outdated Show resolved Hide resolved
src/AggregateId.php Outdated Show resolved Hide resolved

public function equals(AggregateId $other): bool
{
return $this->uuid->toString() === $other->toString();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return $this->uuid->toString() === $other->toString();
return $this->uuid->equals($other->uuid);

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively

Suggested change
return $this->uuid->toString() === $other->toString();
return $other instanceof $this && $this->uuid->equals($other->uuid);

But this comes down to semantics of if FooId and BarId with the same underlying UUID are equal? I would say they are not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if I can easily make use of equals.
Check is done from trait, so I can't have access to field, we have to expose uuid instance on the interface to make it work?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like this:
91d6c6b

Copy link
Member

Choose a reason for hiding this comment

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

As the trait is mixed in to the actual class it has full private member access of other instances of the same type, so it works for me (tests pass) with this change locally:

    public function equals(AggregateId $other): bool
    {
        return $other instanceof $this
            && $this->uuid->equals($other->uuid);
    }

So we can drop exposing toUuid() unless I'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was sure that phpstan had a problem that make sense yesterday, but maybe I something else was wrong 🤔
Passes now, happy to change

@ben-challis
Copy link
Member

1) Class AggregateIdTest cannot be found in /home/runner/work/aggregate/aggregate/tests/unit/AggregateIdTest.php

It seems that phpunit has a problem with trait approach, we do the same in our services and it works fine 🤔

Probably just the *Test suffix as PHPUnit's class loading is pretty simple from my understanding?

@mateuszsip mateuszsip marked this pull request as ready for review October 5, 2023 21:04
@mateuszsip mateuszsip requested a review from a team as a code owner October 5, 2023 21:04
Copy link
Member

@ben-challis ben-challis left a comment

Choose a reason for hiding this comment

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

Just minor things, LGTM

tests/unit/AggregateIdTestSpecification.php Outdated Show resolved Hide resolved
tests/unit/AggregateIdTestSpecification.php Outdated Show resolved Hide resolved
tests/unit/AggregateIdTestSpecification.php Outdated Show resolved Hide resolved
tests/unit/AggregateIdTestSpecification.php Outdated Show resolved Hide resolved
tests/unit/AggregateIdTestSpecification.php Outdated Show resolved Hide resolved
tests/unit/AggregateIdTestSpecification.php Outdated Show resolved Hide resolved
src/AggregateId.php Show resolved Hide resolved
@ben-challis ben-challis added this pull request to the merge queue Oct 6, 2023
Merged via the queue into main with commit a802d90 Oct 6, 2023
6 checks passed
@ben-challis ben-challis deleted the aggregate-id-interface branch October 6, 2023 10:16
@akondas
Copy link
Member

akondas commented Oct 6, 2023

what is motivation/driver for this change? ignore me 😅 I should review this pr first

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.

None yet

3 participants