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

add test for object as primary key #927

Closed
wants to merge 2 commits into from
Closed

add test for object as primary key #927

wants to merge 2 commits into from

Conversation

fbourigault
Copy link

Q A
Bug fix? not yet
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets n/a
License MIT
Doc PR n/a

I think it's a simpler reproduction of #924. The issue seems to be about using objects (Ramsey\Uuid\Uuid, DateTime) as primary key. Stack traces looks the same.

@dunglas
Copy link
Member

dunglas commented Feb 2, 2017

I've pushed the start of a fix. It doesn't work for DateTime because it cannot be casted to a string.
I suggest to prevent using a non-resource as an identifier. Supporting this edge case introduces a lot of complexity for no real benefit. WDYT @api-platform/core-team

@teohhanhui
Copy link
Contributor

Yes, I don't think we should support this.

@fbourigault
Copy link
Author

I talked about primary keys with @dunglas on slack and agreed to not support objects as primary key. In my real case (using ramsey\uuid), I could easly revert to use of strings. Feel free to close this PR.

@teohhanhui teohhanhui closed this Feb 2, 2017
@fbourigault fbourigault deleted the object-as-primary-key branch April 6, 2017 11:16
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