rectorphp/rector-doctrine

ShouldNotHappenException entity not found with MoveRepositoryFromParentToConstructorRector

Closed this issue · 3 comments

ToshY commented

Problem

Adding the rule MoveRepositoryFromParentToConstructorRector results in a ShouldNotHappenException with the message An entity was not found for ....

Stacktrace

[file] module/App/src/Repository/TestRepository.php
[rule] Rector\Doctrine\Rector\Class_\MoveRepositoryFromParentToConstructorRector
PHP Fatal error:  Uncaught Rector\Core\Exception\ShouldNotHappenException: An entity was not found for "App\Repository\TestRepository" repository. in /app/vendor/rector/rector/vendor/rector/rector-doctrine/src/NodeFactory/RepositoryAssignFactory.php:48
Stack trace:
#0 /app/vendor/rector/rector/vendor/rector/rector-doctrine/src/Rector/Class_/MoveRepositoryFromParentToConstructorRector.php(105): Rector\Doctrine\NodeFactory\RepositoryAssignFactory->create()
#1 /app/vendor/rector/rector/src/Rector/AbstractRector.php(209): Rector\Doctrine\Rector\Class_\MoveRepositoryFromParentToConstructorRector->refactor()
#2 /app/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php(176): Rector\Core\Rector\AbstractRector->enterNode()
#3 /app/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php(105): PhpParser\NodeTraverser->traverseArray()
#4 /app/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php(196): PhpParser\NodeTraverser->traverseNode()
#5 /app/vendor/rector/rector/vendor/nikic/php-parser/lib/PhpParser/NodeTraverser.php(85): PhpParser\NodeTraverser->traverseArray()
#6 /app/vendor/rector/rector/src/PhpParser/NodeTraverser/RectorNodeTraverser.php(43): PhpParser\NodeTraverser->traverse()
#7 /app/vendor/rector/rector/src/Application/FileProcessor.php(44): Rector\Core\PhpParser\NodeTraverser\RectorNodeTraverser->traverse()
#8 /app/vendor/rector/rector/src/Application/FileProcessor/PhpFileProcessor.php(115): Rector\Core\Application\FileProcessor->refactor()
#9 /app/vendor/rector/rector/src/Application/ApplicationFileProcessor.php(162): Rector\Core\Application\FileProcessor\PhpFileProcessor->process()
#10 /app/vendor/rector/rector/src/Application/ApplicationFileProcessor.php(132): Rector\Core\Application\ApplicationFileProcessor->processFiles()
#11 /app/vendor/rector/rector/src/Console/Command/ProcessCommand.php(123): Rector\Core\Application\ApplicationFileProcessor->run()
#12 /app/vendor/rector/rector/vendor/symfony/console/Command/Command.php(325): Rector\Core\Console\Command\ProcessCommand->execute()
#13 /app/vendor/rector/rector/vendor/symfony/console/Application.php(944): RectorPrefix202306\Symfony\Component\Console\Command\Command->run()
#14 /app/vendor/rector/rector/vendor/symfony/console/Application.php(326): RectorPrefix202306\Symfony\Component\Console\Application->doRunCommand()
#15 /app/vendor/rector/rector/src/Console/ConsoleApplication.php(49): RectorPrefix202306\Symfony\Component\Console\Application->doRun()
#16 /app/vendor/rector/rector/vendor/symfony/console/Application.php(212): Rector\Core\Console\ConsoleApplication->doRun()
#17 /app/vendor/rector/rector/bin/rector.php(132): RectorPrefix202306\Symfony\Component\Console\Application->run()
#18 /app/vendor/rector/rector/bin/rector(5): require_once('...')
#19 /app/vendor/bin/rector(120): include('...')
#20 {main}

Reproduction

Can not get it reproduced on getrector/demo, so I'll give an example.

rector.php

<?php

declare(strict_types=1);

use Rector\Config\RectorConfig;
use Rector\Core\ValueObject\PhpVersion;
use Rector\Doctrine\Rector\Class_\MoveRepositoryFromParentToConstructorRector;

return static function (RectorConfig $rectorConfig): void
{
    $rectorConfig->phpVersion(phpVersion: PhpVersion::PHP_82);
    $rectorConfig->importNames();
    $rectorConfig->importShortClasses();
    $rectorConfig->disableParallel();
    $rectorConfig->autoloadPaths(autoloadPaths: [
        __DIR__ . '/vendor/autoload.php',
    ]);
    $rectorConfig->paths(paths: [
        __DIR__ . '/module/App/src/Repository/TestRepository.php',
    ]);
    $rectorConfig->rules([
        MoveRepositoryFromParentToConstructorRector::class,
    ]);
};

module/App/src/Repository/TestRepository.php

<?php

declare(strict_types=1);

namespace App\Repository;

use App\Entity\Test;
use Doctrine\ORM\EntityRepository;

final class TestRepository extends EntityRepository
{
    public function findAllTest(): array {
        $qb = $this->getEntityManager()->createQueryBuilder();
        $qb->select('t')
           ->from(Test::class, 't');

        return $qb->getQuery()->getResult();
    }
}

Run vendor/bin/rector process --clear-cache --debug gives stacktrace above as result.

After looking at the resolveFromRepositoryClass I added a method with a phpdoc having a return type.

module/App/src/Repository/TestRepository.php

<?php

declare(strict_types=1);

namespace App\Repository;

use App\Entity\Test;
use Doctrine\ORM\EntityRepository;

final class TestRepository extends EntityRepository
{
    public function findAllTest(): array {
        $qb = $this->getEntityManager()->createQueryBuilder();
        $qb->select('t')
           ->from(Test::class, 't');

        return $qb->getQuery()->getResult();
    }

    /**
     * @return Test
     */
    public function findSingle(): Test {
        $qb = $this->getEntityManager()->createQueryBuilder();
        $qb->select('t')
            ->from(Test::class, 't')
            ->setMaxResults(1);

        return $qb->getQuery()->getSingleResult();
    }
}

After running rector I get the following result:

module/App/src/Repository/TestRepository.php

<?php

declare(strict_types=1);

namespace App\Repository;

use App\Entity\Test;
use Doctrine\ORM\EntityRepository;

final class TestRepository
{
    /**
     * @var EntityRepository<Test>
     */
    private EntityRepository $repository;
    public function __construct(EntityManagerInterface $em, ClassMetadata $class, EntityManagerInterface $entityManager)
    {
        $this->repository = $entityManager->getRepository(\Test::class);
        parent::__construct($em, $class);
    }
    public function findAllTest(): array {
        $qb = $this->getEntityManager()->createQueryBuilder();
        $qb->select('t')
           ->from(Test::class, 't');

        return $qb->getQuery()->getResult();
    }

    /**
     * @return Test
     */
    public function findSingle(): Test {
        $qb = $this->getEntityManager()->createQueryBuilder();
        $qb->select('t')
            ->from(Test::class, 't')
            ->setMaxResults(1);

        return $qb->getQuery()->getSingleResult();
    }
}

Now it created the constructor but it has atleast 3 issues and does not reflect the documentation:

  1. Multiple EntityManagerInterface in the constructor.
  2. Addition of ClassMetadata in constructor, not stated in the docs.
  3. Should not be $entityManager->getRepository(\Test::class) but instead $entityManager->getRepository(Test::class)

Conclusion

The example/explanation in the documentation does not reflect what is actually required for this rule to work. With the current multitude of issues it produces it currently does not seem to work correctly.

TomasVotruba commented

Thanks for reporting 👍

It seems the App\Entity\Test annotation exists outside of Rector's autoloading, so it cannot find it.
How do you run Rector?

It should be always run inside the project itself:

vendor/bin/rector

Also this set must be used a whole, not rule-by-rule, or the migration will change only parts of the repository file:

// rector.php
use Rector\Config\RectorConfig;
use Rector\Core\ValueObject\PhpVersion;

return static function (RectorConfig $rectorConfig): void
{
    $rectorConfig->phpVersion(phpVersion: PhpVersion::PHP_82);
    $rectorConfig->paths([
        __DIR__ . '/module/App/src',
    ]);

    $rectorConfig->sets([
        \Rector\Doctrine\Set\DoctrineSetList::DOCTRINE_REPOSITORY_AS_SERVICE
    ]);
};
TomasVotruba commented

Reproducible Github repository would be very hepful, so we can solve it directly 👍

TomasVotruba commented

I was thinking about this for a long time. This set is rather experimental and not suitable for generic sets like rector-doctrine, as it could create lot of confusion like this :) this is not a first confused report about this set.

That's why I've decided to withdraw the sets from this repository and let developers handle this themselves (if ever needed):
#205