-
-
Notifications
You must be signed in to change notification settings - Fork 933
fix(openapi): Collection fields VS property metadata #4337
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
Conversation
} | ||
|
||
$restriction['properties'][$field] = $this->mergeConstraintRestrictions($baseConstraint, $propertyMetadata); | ||
$restriction['properties'][$field] = $this->mergeConstraintRestrictions($baseConstraint, new PropertyMetadata()); |
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.
Note: ideally we would extract the nested field type from the Collection property metadata, but I don't know how to do that 😕
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.
Not sure to understand this change, could you explain what it does please?
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.
$propertyMetadata
corresponds to the Collection property, but $baseConstraint
corresponds to the nested $field
.
Related: second commit of the referenced PR
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 I understand but why using a parameter then? Because we need to "hydrate" the property metadata with the nested field 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.
Indeed, we could remove the 2nd parameter of
Line 81 in 0d8a016
private function mergeConstraintRestrictions(Constraint $constraint, PropertyMetadata $propertyMetadata): array |
Line 88 in 0d8a016
if ($restrictionMetadata->supports($nestedConstraint, $propertyMetadata) && !empty($nestedConstraintRestriction = $restrictionMetadata->create($nestedConstraint, $propertyMetadata))) { |
And I was hoping one could eventually find a way to deduce a
$fieldPropertyMetadata
(at least a $fieldType
for new PropertyMetadata($fieldType)
) from $propertyMetadata
+$field
and/or $baseConstraint
(For the record: in my project I'm going to replace the Collection-annotated array with a true class subobject)
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.
(Update: the PR was missing a description, fixed now)
I like this could you rebase plesae? |
@soyuka: Sorry for the delay. Now rebased and updated. Thanks |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
The initial implementation was wrong to pass the collection
$propertyMetadata
for each nested field, so I fixed it to use a new PropertyMetadata (empty, for lack of anything better)