[SOLID] Rector\SOLID\Rector\Class_\FinalizeClassesWithoutChildrenRector: should not change Doctrine Entities

[gnutix]

When using the level solid, it will turn every non-final class to final, including Doctrine entities.
But as stated in Doctrine's documentation :

An entity is a lightweight, persistent domain object. An entity can be any regular PHP class observing the following restrictions: - An entity class must not be final or contain final methods.

So it would be nice if there was a way for Rector to detect @ORM\Entity annotation and not process these classes.

[gnutix]

Sorry, mistake from my side... It's handled correctly already.

[gnutix]

I'm encountering this issue again :

docker run --rm -v $(pwd):/app rector_phar process --ansi --autoload-file /app/vendor/autoload.php --config /app/rector.yaml /app/src /app/tests --dry-run
Rector 0.6.x-dev@9a330e2
Config file: app/rector.yaml

 1062/1062 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

1 file with changes
===================

1) app/src/SomeNamespace/ExpenseTag.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -12,7 +12,7 @@
  * @ORM\Table(name="expense_tag")
  * @ORM\Entity()
  */
-class ExpenseTag
+final class ExpenseTag
 {
     /**
      * @var Expense
    ----------- end diff -----------

Applied rules:

 * Rector\SOLID\Rector\Class_\FinalizeClassesWithoutChildrenRector

Here's the file :

<?php

declare(strict_types=1);

namespace App\SomeNamespace;

use Doctrine\ORM\Mapping as ORM;
use SomeVendor\Snowflake\Snowflake;

/**
 * @ORM\HasLifecycleCallbacks
 * @ORM\Table(name="expense_tag")
 * @ORM\Entity()
 */
class ExpenseTag
{
    /**
     * @var Expense
     *
     * @ORM\Id
     * @ORM\ManyToOne(targetEntity="Expense", inversedBy="expenseTags")
     */
    private $expense;

    /**
     * @var Snowflake
     *
     * @ORM\Id
     * @ORM\Column(name="tag_id", type="snowflake")
     */
    private $tagId;

    public function __construct(Expense $expense, Snowflake $tagId)
    {
        $this->expense = $expense;
        $this->tagId = $tagId;
    }

    public function tagId(): Snowflake
    {
        return $this->tagId;
    }
}

I've tried adding this file "as is" in packages/SOLID/tests/Rector/Class_/FinalizeClassesWithoutChildrenRector/Fixture/custom_entity.php.inc and ran the tests with phpunit packages/SOLID/tests/Rector/Class_/FinalizeClassesWithoutChildrenRector/FinalizeClassesWithoutChildrenRectorTest.php, but they passed... :/ so I don't really understand why Rector tries to change it in my code base. Any thoughts @TomasVotruba ?

Could it be because it's running within Docker or with the phar ? (and Doctrine's annotation is not properly recognized ?)

[TomasVotruba]

Could it be because it's running within Docker or with the phar ? (and Doctrine's annotation is not properly recognized ?)

It probably is.

It would be great if you'd add this test case to https://github.com/rectorphp/rector-prefixed e2e test. I think it will be related to prefixed version

If you run it with --debug, what files are autoloaded?

[gnutix]

I fixed it by running Rector from the root of the project (/app) instead of the root of the Docker container (/), see the 2nd point of my comment here

[TomasVotruba]

Could you update README for Docker to prevent that?

[gnutix]

Not sure it applies to the rector/rector container (there seem to be some "COPY" and "WORKDIR" things in there already).

I'm not using the standard Rector container because I need to freeze Rector to a specific commit - so I'm using the PhpStan container in which I require rector instead, like this :

# Dockerfile
FROM phpstan/phpstan:0.12
RUN apk --update --progress --no-cache --repository http://dl-cdn.alpinelinux.org/alpine/v3.9/community add \
    php7-dom \
    php7-xmlwriter \
    && composer global require \
        rector/rector:"dev-master#2c3724b" #rector from December 30th 2019
WORKDIR /app
ENTRYPOINT ["rector"]

The WORKDIR /app is the part that made the difference for me. And I mount the volume with the project's root as /app (though that's not a "generic" solution code-wise, it's quite fragile...).

[TomasVotruba]

Ok 👍

[gnutix]

False hope... the issue is not resolved. Now that I managed to run Rector without issues with our custom phpstan.neon (by renaming it), I can get rid of Docker and run rector.phar directly (which is much more robust / fast - at least on my machine). But the issue is back.

I managed to simplify it that much :

$ php rector.phar process --rule "Rector\SOLID\Rector\Class_\FinalizeClassesWithoutChildrenRector" --dry-run src/Domain/Expenses/ExpenseTag.php 
Rector 0.6.x-dev@f4bfaeb
Config file: rector.yaml

 3/3 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

1 file with changes
===================

1) src/Domain/Expenses/ExpenseTag.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -12,7 +12,7 @@
  * @ORM\Table(name="expense_tag")
  * @ORM\Entity()
  */
-class ExpenseTag
+final class ExpenseTag
 {
     /**
      * @var Expense
    ----------- end diff -----------

Applied rules:

 * Rector\SOLID\Rector\Class_\FinalizeClassesWithoutChildrenRector

I've tried to set the same thing in the playground repo, but it passed there :

Rector dev-master@d83ea25
Config file: rector.yaml

[parsing] src/Domain/Expenses/ExpenseTag.php
[refactoring] src/Domain/Expenses/ExpenseTag.php
[printing] src/Domain/Expenses/ExpenseTag.php

 [OK] Rector is done! 0 changed files                                                             

Any ideas ? :(

[TomasVotruba]

I'd debug FinalizeClassesWithoutChildrenRector

This method should return true:

rector/packages/SOLID/src/Rector/Class_/FinalizeClassesWithoutChildrenRector.php

Line 81 in d83ea25

if ($this->isDoctrineEntityClass($node)) {

This is the method:

rector/packages/Doctrine/src/PhpDocParser/DoctrineDocBlockResolver.php

Lines 75 to 92 in 7a497d9

	public function isDoctrineEntityClassWithIdProperty(Class_ $class): bool
	{
	if (! $this->isDoctrineEntityClass($class)) {
	return false;
	}
	foreach ($class->stmts as $classStmt) {
	if (! $classStmt instanceof Property) {
	continue;
	}
	if ($this->hasPropertyDoctrineIdTag($classStmt)) {
	return true;
	}
	}
	return false;
	}

[gnutix]

@TomasVotruba I took some time to investigate. Minimal code is :

# src/Whatever/Task.php
<?php

namespace Rector\Whatever;

use Doctrine\ORM\Mapping as ORM;

/**
 * @ORM\Entity
 */
class Task
{
}

Then I added dump($phpDocNode); after line $phpDocNode = $this->phpDocParser->parse($tokenIterator);, which gave :

cd bin && php rector process --dry-run --set solid ../src/Whatever/Task.php

  -phpDocNode: Rector\BetterPhpDocParser\Attributes\Ast\PhpDoc\AttributeAwarePhpDocNode^ {#6325
    +children: array:1 [
      0 => Rector\BetterPhpDocParser\Attributes\Ast\PhpDoc\AttributeAwarePhpDocTagNode^ {#6324
        +name: "@ORM\Entity"
        +value: Rector\BetterPhpDocParser\PhpDocNode\Doctrine\Class_\EntityTagValueNode^ {#6315
          -repositoryClass: null
          -readOnly: false
          #orderedVisibleItems: []
          #hasNewlineBeforeClosing: false
          #hasNewlineAfterOpening: false
          -attributes: []
        }

Rector 0.6.x-dev@9ae9807

 3/3 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
                                                                                                                     
 [OK] Rector is done! 0 changed files                                                                                   

cd tmp && php rector.phar process --dry-run --set solid ../src/Whatever/Task.php

  -phpDocNode: Rector\BetterPhpDocParser\Attributes\Ast\PhpDoc\AttributeAwarePhpDocNode^ {#7834
    +children: array:1 [
      0 => Rector\BetterPhpDocParser\Attributes\Ast\PhpDoc\AttributeAwarePhpDocTagNode^ {#7833
        +name: "@ORM"
        +value: Rector\BetterPhpDocParser\Attributes\Ast\PhpDoc\AttributeAwareGenericTagValueNode^ {#7832
          +value: "\Entity"
          -attributes: []
        }

Rector 0.6.x-dev@9ae9807

 3/3 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

1 file with changes
===================

1) ../src/Whatever/Task.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -7,6 +7,6 @@
 /**
  * @ORM\Entity
  */
-class Task
+final class Task
 {
 }
    ----------- end diff -----------

Applied rules:
 * Rector\SOLID\Rector\Class_\FinalizeClassesWithoutChildrenRector
                                                                                                                        
 [OK] Rector is done! 1 changed files                                                                                   

So \PHPStan\PhpDocParser\Parser\PhpDocParser::parse() returns a different result, which then breaks your code.

What should I do ? Open a new issue on PHPStan's repo ? Invite ondrejmirtes to discuss it here ? Do you have any clue why this might happen ?

[TomasVotruba]

I have a theory... It might related to removed stubs, where are annotations located.

Without those, Rector doesn't know about these classes and cannot parse it as an annotation.

[gnutix]

I don't think it could, as I reopened this issue 11 days ago and you merged the removal of the stubs/ folder from rector.phar two days ago.

Or do you mean the removal of specific stub classes, like the annotations ones ?

[gnutix]

Now we know that the same issue happens with ECS's PHAR version and PhpCsFixer\Fixer\ClassNotation\FinalClassFixer.

[TomasVotruba]

Any update on this? Just fixed partially-reated embeddables here: #2921

If still on, we'll need failing Github Action from you to confirm. Without that, we'll have to close it as wontfix

[TomasVotruba]

Demo link is needed: https://getrector.org/demo

[gnutix]

I've tried running the current master version, and still get these :

10) src/Domain/Expenses/ExpenseTag.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -12,7 +12,7 @@
  * @ORM\Table()
  * @ORM\Entity
  */
-class ExpenseTag
+final class ExpenseTag
 {
     /**
      * @var Expense
    ----------- end diff -----------

Applied rules:

 * Rector\SOLID\Rector\Class_\FinalizeClassesWithoutChildrenRector

And the demo does the same! 👍
https://getrector.org/demo/ebc84c7a-c901-46bf-9857-14b226cedd75

[TomasVotruba]

When you remove the HasLifecycle annotatoin (fixed in master), the CI works correctly: https://getrector.org/demo/bf923428-7655-4f3b-8678-1aa5155fbd05#result

[gnutix]

@TomasVotruba I upgraded to v0.7.6 and this issue still persists using rector.phar. I won't reopen, as I can't produce a demo nor a failing test, however I thought you should know. I've ignored this Rector in my code base, as ECS is already catching them too.