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

Additional rectors are ran despite not being referenced in my config #3212

Closed
nesk opened this issue Apr 21, 2020 · 11 comments
Closed

Additional rectors are ran despite not being referenced in my config #3212

nesk opened this issue Apr 21, 2020 · 11 comments

Comments

@nesk
Copy link

nesk commented Apr 21, 2020

Bug Report

Subject Details
Rector version v0.7.16
Installed as composer dependency

First, thank you so much for this awesome project! I'm currently encountering some issues and hope you could provide some help.

I've created the following config:

parameters:
    autoload_paths:
        - bin/.phpunit/phpunit-7.5/vendor/autoload.php
    symfony_container_xml_path: var/cache/dev/App_KernelDevDebugContainer.xml

services:
    Rector\Renaming\Rector\Class_\RenameClassRector:
        $oldToNewClasses:
            App\Security\PermissionResolver: App\Security\Permission\PermissionResolver

If I run vendor/bin/rector show, I get something that seems to be the appropriate result:

Rector v0.7.16
Config file: rector.yaml

 * Rector\PostRector\Rector\UseAddingPostRector
 * Rector\Renaming\Rector\Class_\RenameClassRector
      oldToNewClasses:
          App\Security\PermissionResolver: App\Security\Permission\PermissionResolver

 * Rector\PostRector\Rector\PropertyAddingPostRector
 * Rector\PostRector\Rector\NodeToReplacePostRector
 * Rector\PostRector\Rector\NodeRemovingRector
 * Rector\PostRector\Rector\NodeAddingPostRector
 * Rector\PostRector\Rector\NameImportingPostRector
      importDocBlocks: true

However, when I run vendor/bin/rector process src --dry-run, the outputted diffs contain way more modifications than expected (~12 files):

62 files with changes

Mainly, the additional diffs are avbout code quality (which is good, but I don't want them now), for example:

     /**
      * @var boolean
      *
-     * @ORM\Column(name="devOnly", type="boolean", nullable=false, options={"default"="0"})
+     * @ORM\Column(name="devOnly", type="boolean", nullable=false, options={"default":"0"})
      * @JMS\Groups({"default"})
-     *
      */
     private $devOnly = false;

And sometimes there's some pretty weird changes:

  • why removing the route methods?
 class DefaultController extends AbstractController
 {
     /**
-     * @Route(
-     *     path="/some-endpoint",
-     *     methods = { "POST" }
-     * )
+     * @Route(     path="/some-endpoint" )
      */
     public function someAction(Request $request)
     {
  • why removing joinColumns and inverseJoinColumns?
      * @var ArrayCollection $clusters
      * @JMS\Groups({"details"})
      * @ORM\ManyToMany(targetEntity="Cluster", inversedBy="campaigns")
-     * @ORM\JoinTable(name="PushCampaignCluster",
-     *      joinColumns = {
-     *          @ORM\JoinColumn(name="pushCampaignId", referencedColumnName="id")
-     *      },
-     *      inverseJoinColumns = {
-     *          @ORM\JoinColumn(name="clusterId", referencedColumnName="id")
-     *      }
-     * )
-     *
+     * @ORM\JoinTable(name="PushCampaignCluster"
+     *      )
      */
     private $clusters;

Unfortunately, I'm unable to create a reproducible example, I'm really for this and I know it will not help. 😕

I was hoping maybe you have any idea how this could happen so I can investigate further more? If needed, I can provide more diff examples.

Thank you

@nesk
Copy link
Author

nesk commented Apr 21, 2020

When the expected rule in my config (here Rector\Renaming\Rector\Class_\RenameClassRector) outputs a change, I get a diff like this:

    ---------- begin diff ----------
--- Original
+++ New
@@ -1,6 +1,6 @@
 <?php declare(strict_types = 1);

-namespace App\Security;
+namespace App\Security\Permission;

 use App\Entity\AppUser;
 use App\Entity\Company;
    ----------- end diff -----------

Applied rules:

 * Rector\Renaming\Rector\Class_\RenameClassRector

You can see the applied rule is written.

This brings me to one another thing that could help you: the additional (and unwanted) diffs do not provide the applied rule. 🤔

@nesk
Copy link
Author

nesk commented Apr 21, 2020

My guess is: one of the parser you use with Rector can't parse properly the annotations and drops some data.

@localheinz
Copy link

Also see #2994.

@TomasVotruba
Copy link
Member

TomasVotruba commented Apr 21, 2020

@TomasVotruba
Copy link
Member

There are several issues here and it would be mess to address them in parallel here. Better split them to standalone issues.

PostRectors are meta-Rectors run after all normal Rectors. They handle removing nodes, adding properties etc. So that's expected.

I'll address other issues in newly created tickets.

@nesk
Copy link
Author

nesk commented Apr 22, 2020

Thank you for your answers, I did not find the #2994 issue, I'll subscribe to that one and try to provide some examples.

@TomasVotruba
Copy link
Member

@nesk Better create new issues. It's easier to fix 5 smallers one with 5 PRs, than try to fix 1 big with one huge PR.

@nesk
Copy link
Author

nesk commented Apr 22, 2020

Alright, noted!

TomasVotruba added a commit that referenced this issue Apr 22, 2020
TomasVotruba added a commit that referenced this issue Apr 22, 2020
@TomasVotruba
Copy link
Member

TomasVotruba commented Apr 22, 2020

Fix for Extra Spacing

Rector currently can't understand extra spaces, quotes, commas, double collons and equal sings due to lack of php doc reprinting technology.
It's very similar to this here: nikic/PHP-Parser#344

It might take 3-9 months to get there, but with attributes 2 it annotation will be absolete and doesn't make sense anymore to invest into them: https://wiki.php.net/rfc/attributes_v2

To make Rector work properly and preserve format, the code must be manuyll changed to use standard spacing (e.g. the one used in official documentation):

 /**
  * @Route(
  *     path="/some-endpoint",
- *     methods = { "POST" }
+ *     methods={"POST"}
  * )
 public function someAction(Request $request)

@nesk
Copy link
Author

nesk commented Apr 22, 2020

I will do this, thank for the details!

TomasVotruba added a commit that referenced this issue Apr 22, 2020
@TomasVotruba
Copy link
Member

TomasVotruba commented Apr 22, 2020

@nesk I just tested it and same change is needed for the join table:

     * @ORM\JoinTable(name="PushCampaignCluster",
-    *      joinColumns = {
+    *      joinColumns={
     *          @ORM\JoinColumn(name="pushCampaignId", referencedColumnName="id")
     *      },
-    *      inverseJoinColumns = {
+    *      inverseJoinColumns={
     *          @ORM\JoinColumn(name="clusterId", referencedColumnName="id")
     *      }
     * )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants