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

Use normal Collection<ClassName> when property has indexBy #359

Merged
merged 3 commits into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
<?php

namespace Rector\Doctrine\Tests\CodeQuality\Rector\Class_\AddReturnDocBlockToCollectionPropertyGetterByToManyAnnotationRector\Fixture\OneToMany;

use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping as ORM;
use Rector\Doctrine\Tests\CodeQuality\Rector\Class_\AddReturnDocBlockToCollectionPropertyGetterByToManyAnnotationRector\Source\Training;

#[ORM\Entity()]
class Trainer
{
/**
* @var Collection|Training[]
*/
#[ORM\OneToMany(targetEntity: Training::class, indexBy: "id", mappedBy: "trainer")]
private $trainings = [];

public function getTrainings()
{
return $this->trainings;
}

public function setTrainings($trainings)
{
$this->trainings = $trainings;
}
}

?>
-----
<?php

namespace Rector\Doctrine\Tests\CodeQuality\Rector\Class_\AddReturnDocBlockToCollectionPropertyGetterByToManyAnnotationRector\Fixture\OneToMany;

use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping as ORM;
use Rector\Doctrine\Tests\CodeQuality\Rector\Class_\AddReturnDocBlockToCollectionPropertyGetterByToManyAnnotationRector\Source\Training;

#[ORM\Entity()]
class Trainer
{
/**
* @var Collection|Training[]
*/
#[ORM\OneToMany(targetEntity: Training::class, indexBy: "id", mappedBy: "trainer")]
private $trainings = [];

/**
* @return \Doctrine\Common\Collections\Collection<\Rector\Doctrine\Tests\CodeQuality\Rector\Class_\AddReturnDocBlockToCollectionPropertyGetterByToManyAnnotationRector\Source\Training>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both values have to filled Collection<key, value>, as first is key, next is value, see:

https://github.com/doctrine/collections/blob/2790cb57e6510e4404873bdca6688e447550d720/src/Collection.php#L29-L30

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per @AlexeyKosov at issue rectorphp/rector#8791 (comment)

the key is not necessarily need to be defined, we use \Doctrine\Common\Collections\Collection<ClassName> too in our project, and working well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't respect the parent interface contract:

should have the Collection<string, Training> type rather than Collection<int, Training>.

Screenshot from 2024-12-21 09-49-00

Both key and value should be included.

Copy link
Member Author

@samsonasik samsonasik Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for win win solution, skip php doc change is better if there is indexBy key, since lookup target class -> property type defined by indexBy may or may not work.

I will create new PR

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*/
public function getTrainings()
{
return $this->trainings;
}

public function setTrainings($trainings)
{
$this->trainings = $trainings;
}
}

?>
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@

namespace Rector\Doctrine\Tests\CodeQuality\Rector\Class_\AddReturnDocBlockToCollectionPropertyGetterByToManyAnnotationRector\Source;

use Doctrine\ORM\Mapping as ORM;

final class Training
{

#[ORM\Column(name: 'id', type: 'string')]
#[ORM\Id]
private string $id;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php

namespace Rector\Doctrine\Tests\CodeQuality\Rector\Property\ImproveDoctrineCollectionDocTypeInEntityRector\Fixture\OneToMany;

use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping as ORM;
use Rector\Doctrine\Tests\CodeQuality\Rector\Property\ImproveDoctrineCollectionDocTypeInEntityRector\Source\Training;

/**
* @ORM\Entity
*/
class WithIndexBy
{
/**
* @ORM\OneToMany(targetEntity=Training::class, mappedBy="trainer", "indexBy"="id")
* @var Collection<int, string>|Training[]
*/
private $trainings = [];
}

?>
-----
<?php

namespace Rector\Doctrine\Tests\CodeQuality\Rector\Property\ImproveDoctrineCollectionDocTypeInEntityRector\Fixture\OneToMany;

use Doctrine\Common\Collections\Collection;
use Doctrine\ORM\Mapping as ORM;
use Rector\Doctrine\Tests\CodeQuality\Rector\Property\ImproveDoctrineCollectionDocTypeInEntityRector\Source\Training;

/**
* @ORM\Entity
*/
class WithIndexBy
{
/**
* @ORM\OneToMany(targetEntity=Training::class, mappedBy="trainer", "indexBy"="id")
* @var \Doctrine\Common\Collections\Collection<\Rector\Doctrine\Tests\CodeQuality\Rector\Property\ImproveDoctrineCollectionDocTypeInEntityRector\Source\Training>
*/
private $trainings = [];
}

?>
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,10 @@ public function refactor(Node $node): ?Node

// update docblock with known collection type
$phpDocInfo = $this->phpDocInfoFactory->createFromNodeOrEmpty($classMethod);
$newVarType = $this->collectionTypeFactory->createType($collectionObjectType);
$newVarType = $this->collectionTypeFactory->createType(
$collectionObjectType,
$this->collectionTypeResolver->hasIndexBy($property)
);
$this->phpDocTypeChanger->changeReturnType($classMethod, $phpDocInfo, $newVarType);
$hasChanged = true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,15 +208,21 @@ private function refactorPropertyPhpDocInfo(Property $property, PhpDocInfo $phpD
return null;
}

$newVarType = $this->collectionTypeFactory->createType($collectionObjectType);
$newVarType = $this->collectionTypeFactory->createType(
$collectionObjectType,
$this->collectionTypeResolver->hasIndexBy($property)
);
$this->phpDocTypeChanger->changeVarType($property, $phpDocInfo, $newVarType);
} else {
$collectionObjectType = $this->collectionTypeResolver->resolveFromToManyProperty($property);
if (! $collectionObjectType instanceof FullyQualifiedObjectType) {
return null;
}

$newVarType = $this->collectionTypeFactory->createType($collectionObjectType);
$newVarType = $this->collectionTypeFactory->createType(
$collectionObjectType,
$this->collectionTypeResolver->hasIndexBy($property)
);
$this->phpDocTypeChanger->changeVarType($property, $phpDocInfo, $newVarType);
}

Expand Down Expand Up @@ -249,7 +255,10 @@ private function refactorAttribute(Expr $expr, PhpDocInfo $phpDocInfo, Property

$fullyQualifiedObjectType = new FullyQualifiedObjectType($targetEntityClassName);

$genericObjectType = $this->collectionTypeFactory->createType($fullyQualifiedObjectType);
$genericObjectType = $this->collectionTypeFactory->createType(
$fullyQualifiedObjectType,
$this->collectionTypeResolver->hasIndexBy($property)
);
$this->phpDocTypeChanger->changeVarType($property, $phpDocInfo, $genericObjectType);

return $property;
Expand Down
9 changes: 7 additions & 2 deletions rules/CodeQuality/SetterCollectionResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use PHPStan\Type\UnionType;
use Rector\Doctrine\CodeQuality\Enum\DoctrineClass;
use Rector\Doctrine\TypeAnalyzer\CollectionTypeFactory;
use Rector\Doctrine\TypeAnalyzer\CollectionTypeResolver;
use Rector\Doctrine\TypeAnalyzer\CollectionVarTagValueNodeResolver;
use Rector\NodeManipulator\AssignManipulator;
use Rector\NodeNameResolver\NodeNameResolver;
Expand All @@ -31,7 +32,8 @@ public function __construct(
private NodeNameResolver $nodeNameResolver,
private CollectionVarTagValueNodeResolver $collectionVarTagValueNodeResolver,
private StaticTypeMapper $staticTypeMapper,
private CollectionTypeFactory $collectionTypeFactory
private CollectionTypeFactory $collectionTypeFactory,
private CollectionTypeResolver $collectionTypeResolver
) {
}

Expand Down Expand Up @@ -80,7 +82,10 @@ public function resolveAssignedGenericCollectionType(Class_ $class, ClassMethod
if (count($nonCollectionTypes) === 1) {
$soleType = $nonCollectionTypes[0];
if ($soleType instanceof ArrayType && $soleType->getItemType() instanceof ObjectType) {
return $this->collectionTypeFactory->createType($soleType->getItemType());
return $this->collectionTypeFactory->createType(
$soleType->getItemType(),
$this->collectionTypeResolver->hasIndexBy($property)
);
}
}
}
Expand Down
7 changes: 6 additions & 1 deletion src/NodeManipulator/ToManyRelationPropertyTypeResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
use Rector\Doctrine\NodeAnalyzer\AttributeFinder;
use Rector\Doctrine\PhpDoc\ShortClassExpander;
use Rector\Doctrine\TypeAnalyzer\CollectionTypeFactory;
use Rector\Doctrine\TypeAnalyzer\CollectionTypeResolver;
use Rector\PhpParser\Node\Value\ValueResolver;
use Rector\StaticTypeMapper\ValueObject\Type\FullyQualifiedObjectType;

Expand All @@ -30,6 +31,7 @@ public function __construct(
private AttributeFinder $attributeFinder,
private ValueResolver $valueResolver,
private CollectionTypeFactory $collectionTypeFactory,
private CollectionTypeResolver $collectionTypeResolver
) {
}

Expand Down Expand Up @@ -92,6 +94,9 @@ private function resolveTypeFromTargetEntity(Expr|string $targetEntity, Property
$entityFullyQualifiedClass = $this->shortClassExpander->resolveFqnTargetEntity($targetEntity, $property);
$fullyQualifiedObjectType = new FullyQualifiedObjectType($entityFullyQualifiedClass);

return $this->collectionTypeFactory->createType($fullyQualifiedObjectType);
return $this->collectionTypeFactory->createType(
$fullyQualifiedObjectType,
$this->collectionTypeResolver->hasIndexBy($property)
);
}
}
5 changes: 3 additions & 2 deletions src/TypeAnalyzer/CollectionTypeFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@

final class CollectionTypeFactory
{
public function createType(ObjectType $objectType): GenericObjectType
public function createType(ObjectType $objectType, bool $withIndexBy): GenericObjectType
{
$genericTypes = [new IntegerType(), $objectType];
$genericTypes = $withIndexBy ? [$objectType] : [new IntegerType(), $objectType];

return new GenericObjectType('Doctrine\Common\Collections\Collection', $genericTypes);
}
}
32 changes: 32 additions & 0 deletions src/TypeAnalyzer/CollectionTypeResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,25 @@
use PhpParser\Node\Expr;
use PhpParser\Node\Expr\ClassConstFetch;
use PhpParser\Node\Identifier;
use PhpParser\Node\Param;
use PhpParser\Node\Stmt\Property;
use PHPStan\PhpDocParser\Ast\NodeTraverser;
use PHPStan\PhpDocParser\Ast\Type\ArrayTypeNode;
use PHPStan\PhpDocParser\Ast\Type\IdentifierTypeNode;
use PHPStan\PhpDocParser\Ast\Type\TypeNode;
use PHPStan\PhpDocParser\Ast\Type\UnionTypeNode;
use Rector\BetterPhpDocParser\PhpDoc\ArrayItemNode;
use Rector\BetterPhpDocParser\PhpDoc\DoctrineAnnotationTagValueNode;
use Rector\BetterPhpDocParser\PhpDoc\StringNode;
use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfo;
use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfoFactory;
use Rector\Doctrine\CodeQuality\Enum\CollectionMapping;
use Rector\Doctrine\CodeQuality\Enum\EntityMappingKey;
use Rector\Doctrine\CodeQuality\Enum\OdmMappingKey;
use Rector\Doctrine\NodeAnalyzer\AttrinationFinder;
use Rector\Doctrine\NodeAnalyzer\TargetEntityResolver;
use Rector\Doctrine\PhpDoc\ShortClassExpander;
use Rector\PhpDocParser\NodeTraverser\SimpleCallableNodeTraverser;
use Rector\StaticTypeMapper\Naming\NameScopeFactory;
use Rector\StaticTypeMapper\ValueObject\Type\FullyQualifiedObjectType;

Expand All @@ -39,6 +44,8 @@ public function __construct(
private ShortClassExpander $shortClassExpander,
private AttrinationFinder $attrinationFinder,
private TargetEntityResolver $targetEntityResolver,
private PhpDocInfoFactory $phpDocInfoFactory,
private SimpleCallableNodeTraverser $simpleCallableNodeTraverser
) {
}

Expand All @@ -62,6 +69,31 @@ public function resolveFromTypeNode(TypeNode $typeNode, Node $node): ?FullyQuali
return null;
}

public function hasIndexBy(Property|Param $property): bool
{
$phpDocInfo = $this->phpDocInfoFactory->createFromNode($property);
if ($phpDocInfo instanceof PhpDocInfo && str_contains((string) $phpDocInfo->getPhpDocNode(), 'indexBy')) {
return true;
}

$attrGroups = $property->attrGroups;
$hasIndexBy = false;

$this->simpleCallableNodeTraverser->traverseNodesWithCallable(
$attrGroups,
function (Node $node) use (&$hasIndexBy): ?int {
if ($node instanceof Arg && $node->name instanceof Identifier && $node->name->toString() === 'indexBy') {
$hasIndexBy = true;
return NodeTraverser::STOP_TRAVERSAL;
}

return null;
}
);

return $hasIndexBy;
}

public function resolveFromToManyProperty(Property $property): ?FullyQualifiedObjectType
{
$doctrineAnnotationTagValueNodeOrAttribute = $this->attrinationFinder->getByMany(
Expand Down
Loading