Skip to content
This repository was archived by the owner on Aug 22, 2023. It is now read-only.

PHPORM-35 Add various tests on Model _id types #22

Merged
merged 4 commits into from
Aug 2, 2023
Merged

Conversation

GromNaN
Copy link
Owner

@GromNaN GromNaN commented Jul 26, 2023

Fix PHPORM-35

Adding tests on _id with various types.

int and UTCDateTime are not supported for query because the value is automatically converted to string by Laravel\Database\Eloquent\Builder.

@GromNaN GromNaN requested a review from alcaeus July 26, 2023 16:55
Copy link
Collaborator

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

The tests are what I had in mind, but I am somewhat confused about the test failures.


public static function provideId(): iterable
{
yield 'int' => [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would also make sense to check the expected value of the identifier, not only the type. This is relevant as the default behaviour for binary UUIDs is to cast it to a string, which is completely different from what the cast does.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Updated the tests to validate the expected value. Using lose assertEquals because the ObjectId instance is recreated when the "model" is hydrated from the database.

@alcaeus alcaeus merged commit 9f1f8f3 into master Aug 2, 2023
@alcaeus alcaeus deleted the PHPORM-35 branch August 2, 2023 14:03
GromNaN added a commit that referenced this pull request Aug 22, 2023
* PHPORM-35 Add various tests on Model _id

* Add assertion on expected value

* Test _id as array and object

* Remove tests for arrays and objects as identifiers when keyType is string

---------

Co-authored-by: Andreas Braun <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants