-
Notifications
You must be signed in to change notification settings - Fork 4
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
Added support for abstract entities #56
Added support for abstract entities #56
Conversation
src/Mapping.php
Outdated
*/ | ||
private $mapping = []; | ||
private $classMetadatas = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the liberty of renaming $this->mapping into $this->classMetadatas as it is a private property containing class metadatas within a "Mapping" class, which was misleading. This can be reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, you were right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can just rename to classMetadataList
instead of classMetadatas
(I find the final "s" not really easy to read)
Great thank you, I will look at it quickly |
src/Mapping.php
Outdated
*/ | ||
private $mapping = []; | ||
private $classMetadatas = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, you were right
src/Mapping.php
Outdated
*/ | ||
private $mapping = []; | ||
private $classMetadatas = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can just rename to classMetadataList
instead of classMetadatas
(I find the final "s" not really easy to read)
src/Model/Serializer.php
Outdated
*/ | ||
private function resolveRealClassName(array $data, $className) | ||
{ | ||
if (!empty($data['@type'])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, this is a very tight to an hydra API and api-platform in specific, from whom I try to separate.
Another problem here is if you have different namespaces but the same "entityShortName", then you will have bugs.
but as a matter of fact, your "todo" is probably a good way to solve this for now. I think we can detect the Classmetadata with an IRI and then detect the real class name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now changed to retrieve the real class name from the IRI (@id field).
Requested changes were applied : we now rely on the IRI to retrieve the real class name. I also renamed $classMetadatas into $classMetadataList. |
tagged as v0.21.0 |
This is an attempt to support abstract entities. Feel free to review, ask for modifications / give recommendations, as I primarily tried to fix the issues I personally encountered with abstract entity graphs.