From 9ec658c8e172a8b301875b0a913225efa89f4719 Mon Sep 17 00:00:00 2001 From: brutal-factories <143793262+brutal-factories@users.noreply.github.com> Date: Wed, 8 Nov 2023 18:01:31 +0100 Subject: [PATCH] Better merging of collection types for 2.x (#43) --------- Co-authored-by: David Buchmann --- CHANGELOG.md | 8 ++ src/Metadata/PropertyTypeArray.php | 148 -------------------- src/Metadata/PropertyTypeIterable.php | 62 +++++--- src/TypeParser/JMSTypeParser.php | 21 ++- src/TypeParser/PhpTypeParser.php | 15 +- tests/Metadata/PropertyTypeIterableTest.php | 66 +++------ 6 files changed, 89 insertions(+), 231 deletions(-) delete mode 100644 src/Metadata/PropertyTypeArray.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cbc7c2..6af0861 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ # Version 2.x +# 2.0.0 (unreleased) + +* Removed `PropertyTypeArray`, which is superseeded by `PropertyTypeIterable`. +* Removed the deprecated `PropertyTypeIterable::getCollectionClass`. Use `PropertyTypeIterable::getTraversableClass` +* Removed the deprecated `PropertyTypeIterable::isCollection`. Use `PropertyTypeIterable::isTraversable` +* `JMSTypeParser::getTraversableClass` returns `Traversable::class` instead of `Doctrine\Common\Collections\Collection` for general traversable properties. + + # Version 1.x # 1.2.0 diff --git a/src/Metadata/PropertyTypeArray.php b/src/Metadata/PropertyTypeArray.php deleted file mode 100644 index 5243509..0000000 --- a/src/Metadata/PropertyTypeArray.php +++ /dev/null @@ -1,148 +0,0 @@ -subType = $subType; - $this->hashmap = $hashmap; - $this->isCollection = $isCollection; - } - - public function __toString(): string - { - if ($this->subType instanceof PropertyTypeUnknown) { - return 'array'.($this->isCollection ? '|\\'.Collection::class : ''); - } - - $array = $this->isHashmap() ? '[string]' : '[]'; - if ($this->isCollection) { - $collectionType = $this->isHashmap() ? ', string' : ''; - $array .= sprintf('|\\%s<%s%s>', Collection::class, $this->subType, $collectionType); - } - - return ((string) $this->subType).$array.parent::__toString(); - } - - public function isHashmap(): bool - { - return $this->hashmap; - } - - /** - * Returns the type of the next level, which could be an array or hashmap or another type. - */ - public function getSubType(): PropertyType - { - return $this->subType; - } - - /** - * @deprecated Please prefer using {@link isTraversable} - */ - public function isCollection(): bool - { - return $this->isCollection; - } - - /** - * @deprecated Please prefer using {@link getTraversableClass} - * - * @return class-string|null - */ - public function getCollectionClass(): ?string - { - return $this->isCollection() ? Collection::class : null; - } - - public function isTraversable(): bool - { - return $this->isCollection; - } - - /** - * @return class-string<\Traversable> - */ - public function getTraversableClass(): string - { - if (!$this->isTraversable()) { - throw new \UnexpectedValueException("Iterable type '{$this}' is not traversable."); - } - - return \Traversable::class; - } - - /** - * Goes down the type until it is not an array or hashmap anymore. - */ - public function getLeafType(): PropertyType - { - $type = $this->getSubType(); - while ($type instanceof self) { - $type = $type->getSubType(); - } - - return $type; - } - - public function merge(PropertyType $other): PropertyType - { - $nullable = $this->isNullable() && $other->isNullable(); - - if ($other instanceof PropertyTypeUnknown) { - return new self($this->subType, $this->isHashmap(), $nullable); - } - if ($this->isCollection() && (($other instanceof PropertyTypeClass) && is_a($other->getClassName(), Collection::class, true))) { - return new self($this->getSubType(), $this->isHashmap(), $nullable, true); - } - if (!$other instanceof self) { - throw new \UnexpectedValueException(sprintf('Can\'t merge type %s with %s, they must be the same or unknown', self::class, \get_class($other))); - } - - /* - * We allow converting array to hashmap (but not the other way round). - * - * PHPDoc has no clear definition for hashmaps with string indexes, but JMS Serializer annotations do. - */ - if ($this->isHashmap() && !$other->isHashmap()) { - throw new \UnexpectedValueException(sprintf('Can\'t merge type %s with %s, can\'t change hashmap into plain array', self::class, \get_class($other))); - } - - $hashmap = $this->isHashmap() || $other->isHashmap(); - $isCollection = $this->isCollection || $other->isCollection; - if ($other->getSubType() instanceof PropertyTypeUnknown) { - return new self($this->getSubType(), $hashmap, $nullable, $isCollection); - } - if ($this->getSubType() instanceof PropertyTypeUnknown) { - return new self($other->getSubType(), $hashmap, $nullable, $isCollection); - } - - return new self($this->getSubType()->merge($other->getSubType()), $hashmap, $nullable, $isCollection); - } -} diff --git a/src/Metadata/PropertyTypeIterable.php b/src/Metadata/PropertyTypeIterable.php index 1ef26dd..a0bd1f7 100644 --- a/src/Metadata/PropertyTypeIterable.php +++ b/src/Metadata/PropertyTypeIterable.php @@ -4,16 +4,24 @@ namespace Liip\MetadataParser\Metadata; -use Doctrine\Common\Collections\Collection; - /** - * This property type can be merged with PropertyTypeClass, provided that T is, inherits from, or is a parent class of $this->collectionClass + * This property type can be merged with PropertyTypeClass, provided that T is, inherits from, or is a parent class of {@see PropertyTypeIterable::traversableClass} * This property type can be merged with PropertyTypeIterable, if : * - we're not merging a plain array PropertyTypeIterable into a hashmap one, - * - and the collection classes of each are either not present on both sides, or are the same, or parent-child of one another + * - and the traversable classes of each are either not present on either sides, or are the same, or parent-child of one another */ -final class PropertyTypeIterable extends PropertyTypeArray +final class PropertyTypeIterable extends AbstractPropertyType { + /** + * @var PropertyType + */ + private $subType; + + /** + * @var bool + */ + private $hashmap; + /** * @var string */ @@ -24,8 +32,10 @@ final class PropertyTypeIterable extends PropertyTypeArray */ public function __construct(PropertyType $subType, bool $hashmap, bool $nullable, string $traversableClass = null) { - parent::__construct($subType, $hashmap, $nullable, null != $traversableClass); + parent::__construct($nullable); + $this->subType = $subType; + $this->hashmap = $hashmap; $this->traversableClass = $traversableClass; } @@ -41,25 +51,20 @@ public function __toString(): string $array .= sprintf('|\\%s<%s%s>', $this->traversableClass, $this->subType, $collectionType); } - return ((string) $this->subType).$array.AbstractPropertyType::__toString(); + return ((string) $this->subType).$array.parent::__toString(); } - /** - * @deprecated Please prefer using {@link getTraversableClass} - * - * @return class-string|null - */ - public function getCollectionClass(): ?string + public function isHashmap(): bool { - return $this->isCollection() ? null : $this->traversableClass; + return $this->hashmap; } /** - * @deprecated Please prefer using {@link isTraversable} + * Returns the type of the next level, which could be an array or hashmap or another type. */ - public function isCollection(): bool + public function getSubType(): PropertyType { - return (null != $this->traversableClass) && is_a($this->traversableClass, Collection::class, true); + return $this->subType; } /** @@ -79,18 +84,31 @@ public function isTraversable(): bool return null != $this->traversableClass; } + /** + * Goes down the type until it is not an array or hashmap anymore. + */ + public function getLeafType(): PropertyType + { + $type = $this->getSubType(); + while ($type instanceof self) { + $type = $type->getSubType(); + } + + return $type; + } + public function merge(PropertyType $other): PropertyType { $nullable = $this->isNullable() && $other->isNullable(); $thisTraversableClass = $this->isTraversable() ? $this->getTraversableClass() : null; if ($other instanceof PropertyTypeUnknown) { - return new self($this->subType, $this->isHashmap(), $nullable, $thisTraversableClass); + return new self($this->getSubType(), $this->isHashmap(), $nullable, $thisTraversableClass); } if ($this->isTraversable() && (($other instanceof PropertyTypeClass) && is_a($other->getClassName(), \Traversable::class, true))) { - return new self($this->getSubType(), $this->isHashmap(), $nullable, $this->findCommonCollectionClass($thisTraversableClass, $other->getClassName())); + return new self($this->getSubType(), $this->isHashmap(), $nullable, $this->findCommonTraversableClass($thisTraversableClass, $other->getClassName())); } - if (!$other instanceof parent) { + if (!$other instanceof self) { throw new \UnexpectedValueException(sprintf('Can\'t merge type %s with %s, they must be the same or unknown', self::class, \get_class($other))); } @@ -105,7 +123,7 @@ public function merge(PropertyType $other): PropertyType $otherTraversableClass = $other->isTraversable() ? $other->getTraversableClass() : null; $hashmap = $this->isHashmap() || $other->isHashmap(); - $commonClass = $this->findCommonCollectionClass($thisTraversableClass, $otherTraversableClass); + $commonClass = $this->findCommonTraversableClass($thisTraversableClass, $otherTraversableClass); if ($other->getSubType() instanceof PropertyTypeUnknown) { return new self($this->getSubType(), $hashmap, $nullable, $commonClass); @@ -121,7 +139,7 @@ public function merge(PropertyType $other): PropertyType * Find the most derived class that doesn't deny both class hints, meaning the most derived * between left and right if one is a child of the other */ - private function findCommonCollectionClass(?string $left, ?string $right): ?string + private function findCommonTraversableClass(?string $left, ?string $right): ?string { if (null === $right) { return $left; diff --git a/src/TypeParser/JMSTypeParser.php b/src/TypeParser/JMSTypeParser.php index b58c9cc..32efb19 100644 --- a/src/TypeParser/JMSTypeParser.php +++ b/src/TypeParser/JMSTypeParser.php @@ -5,7 +5,6 @@ namespace Liip\MetadataParser\TypeParser; use Doctrine\Common\Collections\ArrayCollection; -use Doctrine\Common\Collections\Collection; use JMS\Serializer\Type\Parser; use Liip\MetadataParser\Exception\InvalidTypeException; use Liip\MetadataParser\Metadata\DateTimeOptions; @@ -20,6 +19,9 @@ final class JMSTypeParser { private const TYPE_ARRAY = 'array'; private const TYPE_ARRAY_COLLECTION = 'ArrayCollection'; + private const TYPE_GENERATOR = 'Generator'; + private const TYPE_ARRAY_ITERATOR = 'ArrayIterator'; + private const TYPE_ITERATOR = 'Iterator'; private const TYPE_DATETIME_INTERFACE = 'DateTimeInterface'; /** @@ -69,13 +71,13 @@ private function parseType(array $typeInfo, bool $isSubType = false): PropertyTy return new PropertyTypeClass($typeInfo['name'], $nullable); } - $collectionClass = $this->getCollectionClass($typeInfo['name']); - if (self::TYPE_ARRAY === $typeInfo['name'] || $collectionClass) { + $traversableClass = $this->getTraversableClass($typeInfo['name']); + if (self::TYPE_ARRAY === $typeInfo['name'] || $traversableClass) { if (1 === \count($typeInfo['params'])) { - return new PropertyTypeIterable($this->parseType($typeInfo['params'][0], true), false, $nullable, $collectionClass); + return new PropertyTypeIterable($this->parseType($typeInfo['params'][0], true), false, $nullable, $traversableClass); } if (2 === \count($typeInfo['params'])) { - return new PropertyTypeIterable($this->parseType($typeInfo['params'][1], true), true, $nullable, $collectionClass); + return new PropertyTypeIterable($this->parseType($typeInfo['params'][1], true), true, $nullable, $traversableClass); } throw new InvalidTypeException(sprintf('JMS property type array can\'t have more than 2 parameters (%s)', var_export($typeInfo, true))); @@ -105,13 +107,18 @@ private function parseType(array $typeInfo, bool $isSubType = false): PropertyTy throw new InvalidTypeException(sprintf('Unknown JMS property found (%s)', var_export($typeInfo, true))); } - private function getCollectionClass(string $name): ?string + private function getTraversableClass(string $name): ?string { switch ($name) { case self::TYPE_ARRAY_COLLECTION: return ArrayCollection::class; + case self::TYPE_GENERATOR: + return \Generator::class; + case self::TYPE_ARRAY_ITERATOR: + case self::TYPE_ITERATOR: + return \ArrayIterator::class; default: - return is_a($name, Collection::class, true) ? $name : null; + return is_a($name, \Traversable::class, true) ? $name : null; } } } diff --git a/src/TypeParser/PhpTypeParser.php b/src/TypeParser/PhpTypeParser.php index 7d05d31..126bb12 100644 --- a/src/TypeParser/PhpTypeParser.php +++ b/src/TypeParser/PhpTypeParser.php @@ -5,7 +5,6 @@ namespace Liip\MetadataParser\TypeParser; use Doctrine\Common\Annotations\PhpParser; -use Doctrine\Common\Collections\Collection; use Liip\MetadataParser\Exception\InvalidTypeException; use Liip\MetadataParser\Metadata\PropertyType; use Liip\MetadataParser\Metadata\PropertyTypeClass; @@ -57,12 +56,12 @@ public function parseAnnotationType(string $rawType, \ReflectionClass $declaring } } - $collectionClass = null; + $traversableClass = null; $filteredTypes = []; foreach ($types as $type) { $resolvedClass = $this->resolveClass($type, $declaringClass); - if (is_a($resolvedClass, Collection::class, true)) { - $collectionClass = $resolvedClass; + if (is_a($resolvedClass, \Traversable::class, true)) { + $traversableClass = $resolvedClass; } else { $filteredTypes[] = $type; } @@ -75,7 +74,7 @@ public function parseAnnotationType(string $rawType, \ReflectionClass $declaring throw new InvalidTypeException(sprintf('Multiple types are not supported (%s)', $rawType)); } - return $this->createType($filteredTypes[0], $nullable, $declaringClass, $collectionClass); + return $this->createType($filteredTypes[0], $nullable, $declaringClass, $traversableClass); } /** @@ -90,7 +89,7 @@ public function parseReflectionType(\ReflectionType $reflType): PropertyType throw new InvalidTypeException(sprintf('No type information found, got %s but expected %s', \ReflectionType::class, \ReflectionNamedType::class)); } - private function createType(string $rawType, bool $nullable, \ReflectionClass $reflClass = null, string $collectionClass = null): PropertyType + private function createType(string $rawType, bool $nullable, \ReflectionClass $reflClass = null, string $traversableClass = null): PropertyType { if (self::TYPE_ARRAY === $rawType) { return new PropertyTypeIterable(new PropertyTypeUnknown(false), false, $nullable); @@ -99,12 +98,12 @@ private function createType(string $rawType, bool $nullable, \ReflectionClass $r if (self::TYPE_ARRAY_SUFFIX === substr($rawType, -\strlen(self::TYPE_ARRAY_SUFFIX))) { $rawSubType = substr($rawType, 0, \strlen($rawType) - \strlen(self::TYPE_ARRAY_SUFFIX)); - return new PropertyTypeIterable($this->createType($rawSubType, false, $reflClass), false, $nullable, $collectionClass); + return new PropertyTypeIterable($this->createType($rawSubType, false, $reflClass), false, $nullable, $traversableClass); } if (self::TYPE_HASHMAP_SUFFIX === substr($rawType, -\strlen(self::TYPE_HASHMAP_SUFFIX))) { $rawSubType = substr($rawType, 0, \strlen($rawType) - \strlen(self::TYPE_HASHMAP_SUFFIX)); - return new PropertyTypeIterable($this->createType($rawSubType, false, $reflClass), true, $nullable, $collectionClass); + return new PropertyTypeIterable($this->createType($rawSubType, false, $reflClass), true, $nullable, $traversableClass); } if (self::TYPE_RESOURCE === $rawType) { diff --git a/tests/Metadata/PropertyTypeIterableTest.php b/tests/Metadata/PropertyTypeIterableTest.php index 6ed4b77..df48cb0 100644 --- a/tests/Metadata/PropertyTypeIterableTest.php +++ b/tests/Metadata/PropertyTypeIterableTest.php @@ -6,7 +6,6 @@ use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; -use Liip\MetadataParser\Metadata\PropertyTypeArray; use Liip\MetadataParser\Metadata\PropertyTypeClass; use Liip\MetadataParser\Metadata\PropertyTypeIterable; use Liip\MetadataParser\Metadata\PropertyTypePrimitive; @@ -34,37 +33,12 @@ public function testDefaultListIsNotCollection(): void $this->assertFalse($list->isTraversable()); } - /** - * @deprecated This only checks the behaviour of deprecated class {@see PropertyTypeArray} - */ - public function testDefaultTraversableClassIsTraversableInterface(): void - { - $subType = new PropertyTypePrimitive('int', false); - $collection = new PropertyTypeArray($subType, false, false, true); - - $this->assertTrue($collection->isTraversable()); - $this->assertSame(\Traversable::class, $collection->getTraversableClass()); - } - - /** - * @deprecated This only checks the behaviour of deprecated class {@see PropertyTypeArray} - */ - public function testDefaultCollectionClassIsCollectionInterface(): void - { - $subType = new PropertyTypePrimitive('int', false); - $collection = new PropertyTypeArray($subType, false, false, true); - - $this->assertTrue($collection->isCollection()); - $this->assertSame(Collection::class, $collection->getCollectionClass()); - } - - public function testExplicitCollectionClassIsKept(): void + public function testConcreteCollectionClassIsKept(): void { $subType = new PropertyTypePrimitive('int', false); $collection = new PropertyTypeIterable($subType, false, false, ArrayCollection::class); $this->assertTrue($collection->isTraversable()); - $this->assertNotSame(\Traversable::class, $collection->getTraversableClass()); $this->assertSame(ArrayCollection::class, $collection->getTraversableClass()); } @@ -77,27 +51,27 @@ public function testMergeListWithClassCollection(): void $list->merge(new PropertyTypeClass(Collection::class, true)); } - public function testMergeDefaultCollectionListWithClassCollection(): void + public function testMergeInterfaceCollectionListWithClassCollection(): void { $subType = new PropertyTypePrimitive('int', false); $typeHintedCollection = new PropertyTypeClass(Collection::class, true); - $defaultCollection = new PropertyTypeArray($subType, false, false, true); + $defaultCollection = new PropertyTypeIterable($subType, false, false, Collection::class); $result = $defaultCollection->merge($typeHintedCollection); - $this->assertInstanceOf(PropertyTypeArray::class, $result); + $this->assertInstanceOf(PropertyTypeIterable::class, $result); /* @var PropertyTypeIterable $result */ $this->assertTrue($result->isTraversable()); $this->assertSame((string) $defaultCollection->getSubType(), (string) $result->getSubType()); - $this->assertSame(\Traversable::class, $result->getTraversableClass()); + $this->assertSame(Collection::class, $result->getTraversableClass()); } - public function testMergeExplicitCollectionListWithClassCollection(): void + public function testMergeClassInterfaceCollectionWithConcreteCollectionList(): void { $subType = new PropertyTypePrimitive('int', false); $typeHintedCollection = new PropertyTypeClass(Collection::class, true); $explicitCollection = new PropertyTypeIterable($subType, false, false, ArrayCollection::class); - $result = $explicitCollection->merge($typeHintedCollection); + $result = $typeHintedCollection->merge($explicitCollection); $this->assertInstanceOf(PropertyTypeIterable::class, $result); /* @var PropertyTypeIterable $result */ $this->assertTrue($result->isTraversable()); @@ -105,13 +79,13 @@ public function testMergeExplicitCollectionListWithClassCollection(): void $this->assertSame(ArrayCollection::class, $result->getTraversableClass()); } - public function testMergeExplicitCollectionListWithDefaultCollection(): void + public function testMergeConcreteCollectionListWithClassInterfaceCollection(): void { $subType = new PropertyTypePrimitive('int', false); - $defaultCollection = new PropertyTypeArray($subType, false, false, true); + $typeHintedCollection = new PropertyTypeClass(Collection::class, true); $explicitCollection = new PropertyTypeIterable($subType, false, false, ArrayCollection::class); - $result = $explicitCollection->merge($defaultCollection); + $result = $explicitCollection->merge($typeHintedCollection); $this->assertInstanceOf(PropertyTypeIterable::class, $result); /* @var PropertyTypeIterable $result */ $this->assertTrue($result->isTraversable()); @@ -119,27 +93,27 @@ public function testMergeExplicitCollectionListWithDefaultCollection(): void $this->assertSame(ArrayCollection::class, $result->getTraversableClass()); } - public function testMergeDefaultCollectionListWithExplicitCollection(): void + public function testMergeConcreteCollectionListWithInterfaceCollectionList(): void { $subType = new PropertyTypePrimitive('int', false); - $defaultCollection = new PropertyTypeArray($subType, false, false, true); + $defaultCollection = new PropertyTypeIterable($subType, false, false, Collection::class); $explicitCollection = new PropertyTypeIterable($subType, false, false, ArrayCollection::class); - $result = $defaultCollection->merge($explicitCollection); - $this->assertInstanceOf(PropertyTypeArray::class, $result); + $result = $explicitCollection->merge($defaultCollection); + $this->assertInstanceOf(PropertyTypeIterable::class, $result); /* @var PropertyTypeIterable $result */ $this->assertTrue($result->isTraversable()); $this->assertSame((string) $explicitCollection->getSubType(), (string) $result->getSubType()); - $this->assertSame(Collection::class, $result->getCollectionClass()); + $this->assertSame(ArrayCollection::class, $result->getTraversableClass()); } - public function testMergeClassCollectionWithExplicitCollectionList(): void + public function testMergeDefaultCollectionListWithConcreteCollectionList(): void { $subType = new PropertyTypePrimitive('int', false); - $typeHintedCollection = new PropertyTypeClass(Collection::class, true); + $defaultCollection = new PropertyTypeIterable($subType, false, false, Collection::class); $explicitCollection = new PropertyTypeIterable($subType, false, false, ArrayCollection::class); - $result = $typeHintedCollection->merge($explicitCollection); + $result = $defaultCollection->merge($explicitCollection); $this->assertInstanceOf(PropertyTypeIterable::class, $result); /* @var PropertyTypeIterable $result */ $this->assertTrue($result->isTraversable()); @@ -147,7 +121,7 @@ public function testMergeClassCollectionWithExplicitCollectionList(): void $this->assertSame(ArrayCollection::class, $result->getTraversableClass()); } - public function testMergeListAndCollection(): void + public function testMergeListAndInterfaceCollectionList(): void { $subType = new PropertyTypePrimitive('int', false); $list = new PropertyTypeIterable($subType, false, false); @@ -162,7 +136,7 @@ public function testMergeListAndCollection(): void } } - public function testMergeListWithExplicitCollectionClass(): void + public function testMergeListWithConcreteCollectionList(): void { $subType = new PropertyTypePrimitive('int', false); $list = new PropertyTypeIterable($subType, false, false);