Map array<K, V> to type: object + additionalProperties#2003
Map array<K, V> to type: object + additionalProperties#2003DerManoMann merged 1 commit intozircote:masterfrom
array<K, V> to type: object + additionalProperties#2003Conversation
…nfoTypeResolver (zircote#2001) Previously all PHP array types were mapped to `type: array` + `items`. Now `array<K, V>` with an explicit key type resolves to `type: object` with `additionalProperties`, while `array<T>`, `T[]` and `list<T>` keep their existing `type: array` + `items` behaviour.
DerManoMann
left a comment
There was a problem hiding this comment.
Looks like it is doing what it should. I do not quite like the UnionType check, but there might be no alternative.
Only think to consider, even thought there seems to be a bit of variation, is wheter there is now a lot of duplication that could be simplified around the creation of nested Schema.
| $this->setSchemaType($schema->items, $type->getCollectionValueType(), $analysis); | ||
| $this->type2ref($schema->items, $analysis); | ||
| } | ||
| if ($type->isList() || $type->getCollectionKeyType() instanceof UnionType) { |
There was a problem hiding this comment.
This is tricky. UnionType means int|string so could be an object, we just don't know. However, from what I understand this happens when no key is specified.
I have to admin I do not quite like this check as it is quite loose, however I do not know if there is a better alternative... hmmm.
|
Tried to come up with an example that might conflict with the union check over the weekend... enum TypeEnum: string
{
case First = 'first';
case Second = 'second';
}
#[OAT\Schema()]
class TypedProperties
{
/**
* A map of stringy to string.
*
* @var \ArrayObject<TypeEnum|string,string> $stringyMap
*/
#[OAT\Property]
public \ArrayObject $stringyMap;
} |
|
Claude suggests to check if all union types are built in types, but that still feels ambiguous to me. What about checking if the union contains Casting rules around array keys are tricky so maybe requiring to be explicit about |
|
Thanks for the detailed feedback and the edge case example! Regarding the In PHP, The key insight is: if someone writes Regarding the PHP array keys only allow Regarding the "check if key contains This would cause |
|
Yeah, I agree this is a bit of a mess. Also agree that my example is a bit far fetched ;) Let's go with this for now. We can always refine this if needed. |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-2003-to-5.x
git worktree add --checkout .worktree/backport-2003-to-5.x backport-2003-to-5.x
cd .worktree/backport-2003-to-5.x
git reset --hard HEAD^
git cherry-pick -x f66289ab9c9c3a1cf70222e0bebbe7c6c7109f2f |
Closes #2001
Summary
TypeInfoTypeResolvernow mapsarray<K, V>(with explicit key type) totype: object+additionalPropertiesinstead oftype: array+itemsarray<T>,T[],list<T>(defaultint|stringkey) keep existingtype: array+itemsbehaviourArrayShapeType(e.g.array{foo:bool}) also correctly resolves totype: object+additionalPropertiesLegacyTypeResolveris not affected — it continues to producetype: array+itemsfor all array typesTest plan
TypeResolverTest— 152 tests pass (both 3.0.0 and 3.1.0, both legacy and type-info resolvers)AugmentPropertiesTest— map assertion merged intotestTypedPropertiesarray<int, T>in union types correctly producesobject+additionalProperties