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

AnnotationToAttributeRector removes empty lines #5447

Closed
stephanvierkant opened this issue Feb 7, 2021 · 20 comments
Closed

AnnotationToAttributeRector removes empty lines #5447

stephanvierkant opened this issue Feb 7, 2021 · 20 comments
Labels

Comments

@stephanvierkant
Copy link
Contributor

Bug Report

Subject Details
Rector version last dev-master
Installed as composer dependency

Minimal PHP Code Causing Issue

See https://getrector.org/demo/edb98391-fcaf-4f4b-941c-db7afb2a6127

<?php
use Symfony\Component\Routing\Annotation\Route;

final class SomeController
{
    /**
     * @Route(
     *     path = "/foo",
     *     name = "foo",
     *     methods = {"POST"}
     * )
     */
    public function foo()
    {
        $foo = true;
        
        $bar = false;
    }
}

Responsible rules

  • ArgumentAdderRector

  • AnnotationToAttributeRector

Expected Behavior

Rector removes an empty line:

-        $foo = true;
-        
-        $bar = false;
+        $foo = true;
+        $bar = false;

This shouldn't be happening.

@TomasVotruba
Copy link
Member

Hi, this is expected behavior. Coding style must be handled by your coding standard tool.

See
https://github.com/rectorphp/rector#how-to-apply-coding-standards

@stephanvierkant
Copy link
Contributor Author

Sorry, I don't understand what you mean. Why does Rector remove those lines?

@TomasVotruba
Copy link
Member

@stephanvierkant
Copy link
Contributor Author

stephanvierkant commented Feb 7, 2021

I've read that and I'm using ECS. But ECS can't add those removed lines, isn't it?

And in this case, it's related to AnnotationToAttributeRector, because without this it doesn't remove the empty line: https://getrector.org/demo/6aa8ac68-b891-4f84-8604-ca8b8226e975

@TomasVotruba
Copy link
Member

If it's not in ECS yet, you need to provide your own rule.
There is not much I can add over the link.

@stephanvierkant
Copy link
Contributor Author

stephanvierkant commented Feb 7, 2021

Sorry, I'm afraid you don't understand what I mean.

With AnnotationToAttributeRector, an (unrelated!) empty line is removed. I don't see why. It has nothing to do with attributes/annotations and if other rectors are applied instead, the line stays untouched.

@TomasVotruba
Copy link
Member

I understand that. It's a side effect of AST changes. The reprint to the original content is not always 100 %. It's limitation of technology itself. This is in the linked content.

This will happen accidentaly for many changes in AST nodes, unrelated to any specific rule.

You can read more about it here: nikic/PHP-Parser#344

@stephanvierkant
Copy link
Contributor Author

Ok, thanks for clarifying!

It didn't expect the body to be touched in the first place, since the body seems to be unrelated to this rector . That's why I thought it might be another problem.

@svitiashchuk
Copy link

I believe a lot of developers use empty lines inside methods to separate some logic. I decided to move from Symfony @Route annotation to attributes. And after running rector those blank lines have gone.

public function listAction(PaginationRequest $request): Response
{
    $before = $request->query->getInt('before');
    $limit = $request->query->getInt('limit');
-removed-
    $conversations = $this->entityManager->getRepository(Conversation::class);
-removed-
    // some heavy-logic block
    //....

Reverting changes line by line is pretty annoying. So, here is how I managed it (hope this could help someone).
All actions described below expect that you have committed changes. Otherwise you will lose them!

# Step 1. First of all run `rector` 
vendor/bin/rector process src/Controller

# Step 2. Generate patch using `git diff` without context lines, only changes (`-U0`). 
# pipe it to perl to replace in place empty lines (-0 to operate multiline)
git diff -U0 | perl -0 -pwe "s/@@.+\n\-\n//g" > no-removed-blank-lines.patch

# Step 3. Reset changes made by rector
git reset --hard HEAD

# Step 4. Finally apply patch and it is done
git apply -v --unidiff-zero no-removed-blank-lines.patch

So what happens in Step 3, you just get rid of blank lines (like below) in your patch. In the end you just apply necessary changes.

@@ -60 +52,0 @@ class ConversationsController
-

P.S. other changes like whitespace in method signature could be fixed using your code-style tools 😉

@TomasVotruba
Copy link
Member

@svitiashchuk Thank you for sharing this exact solution. I'll definitely use it soon in refactoring that I make with Rector 👍

@algimantas
Copy link

It would be really nice if AnnotationToAttributeRector would not touch function body.

@samsonasik
Copy link
Member

@algimantas most of the line removal from AnnotationToAttributeRector is solvable by NewlineAfterStatementRector rule by add new line on most common usage of new line after stmts registered

https://github.com/rectorphp/rector-src/blob/93030d3ef7bdf94769d3ea5c8b7d15fe9fb99658/rules/CodingStyle/Rector/Stmt/NewlineAfterStatementRector.php#L43-L59

@smilesrg
Copy link

smilesrg commented May 9, 2022

@samsonasik could you please modify the AnnotationToAttributeRector to not touch empty lines to not mess with solutions like this? #5447 (comment)

@samsonasik
Copy link
Member

Using git diff means it not using rector, the only way to reduce it with rector is by another rule, like NewlineAfterStatementRector ref https://getrector.org/demo/4838247f-608a-4200-aa0f-ba4dcdb993cf

@raziel057
Copy link

raziel057 commented May 23, 2022

@svitiashchuk I tried the solution proposed here #5447 (comment) but the patch (git apply -v --unidiff-zero no-removed-blank-lines.patch) only applies on the first route of each class.

Sample of class before change (using annotation):

/**
 * Brand Import controller.
 *
 * @Route("/secured/transport/brand/import")
 */
class BrandController extends ImportController
{
    /**
     * @Route("/get/fields", name="brand_import_get_field_definitions", methods={"POST"})
     */
    public function getFieldDefinitionsAction(Request $request): JsonResponse
    {
        return $this->getFieldDefintions($request);
    }

    /**
     * @Route("/get/import/file", name="brand_import_get_file", methods={"POST"})
     */
    public function getImportFileAction(Request $request): Response
    {
        return $this->getImportFile($request, 'brand');
    }
...

Sample of class before change (using attributes - after vendor/bin/rector process src)

/**
  * Brand Import controller.
  *
- * @Route("/secured/transport/brand/import")
  */
+#[Route(path: '/secured/transport/brand/import')]
 class BrandController extends ImportController
 {
-    /**
-     * @Route("/get/fields", name="brand_import_get_field_definitions", methods={"POST"})
-     */
+    #[Route(path: '/get/fields', name: 'brand_import_get_field_definitions', methods: ['POST'])]
     public function getFieldDefinitionsAction(Request $request): JsonResponse
     {
         return $this->getFieldDefintions($request);
     }
-
-    /**
-     * @Route("/get/import/file", name="brand_import_get_file", methods={"POST"})
-     */
+    #[Route(path: '/get/import/file', name: 'brand_import_get_file', methods: ['POST'])]
     public function getImportFileAction(Request $request): Response
     {
         return $this->getImportFile($request, 'brand');
     }

What is generated in file no-removed-blank-lines.patch:

diff -U0 | perl -0 -pwe "s/@@.+\n\-\n//g" > no-removed-blank-lines.patch
diff --git a/src/PTC/AdminBundle/Controller/Import/BrandController.php b/src/PTC/AdminBundle/Controller/Import/BrandController.php
index 433a3879..9a8ed902 100644
--- a/src/PTC/AdminBundle/Controller/Import/BrandController.php
+++ b/src/PTC/AdminBundle/Controller/Import/BrandController.php
@@ -13 +12,0 @@ use Symfony\Component\Routing\Annotation\Route;
- * @Route("/secured/transport/brand/import")
@@ -14,0 +14 @@ use Symfony\Component\Routing\Annotation\Route;
+#[Route(path: '/secured/transport/brand/import')]
@@ -17,3 +17 @@ class BrandController extends ImportController
-    /**
-     * @Route("/get/fields", name="brand_import_get_field_definitions", methods={"POST"})
-     */
+    #[Route(path: '/get/fields', name: 'brand_import_get_field_definitions', methods: ['POST'])]
-    /**
-     * @Route("/get/import/file", name="brand_import_get_file", methods={"POST"})
-     */
+    #[Route(path: '/get/import/file', name: 'brand_import_get_file', methods: ['POST'])

@TomasVotruba
Copy link
Member

@raziel057 Hi, could you share minimal reproducible repository? I'll see if there is some other way to handle this.

@raziel057
Copy link

@svitiashchuk It's cause by the perl transformation as the different @@ -24,4 +22 @@ class BrandController extends ImportController are lost (maybe related to my version)

perl -v

This is perl 5, version 26, subversion 3 (v5.26.3) built for i686-cygwin-threads-64int-multi
(with 7 registered patches, see perl -V for more detail)

@raziel057
Copy link

If I remove the perl part, I have the following result:

diff --git a/src/PTC/AdminBundle/Controller/Import/BrandController.php b/src/PTC/AdminBundle/Controller/Import/BrandController.php
index 433a3879..9a8ed902 100644
--- a/src/PTC/AdminBundle/Controller/Import/BrandController.php
+++ b/src/PTC/AdminBundle/Controller/Import/BrandController.php
@@ -13 +12,0 @@ use Symfony\Component\Routing\Annotation\Route;
- * @Route("/secured/transport/brand/import")
@@ -14,0 +14 @@ use Symfony\Component\Routing\Annotation\Route;
+#[Route(path: '/secured/transport/brand/import')]
@@ -17,3 +17 @@ class BrandController extends ImportController
-    /**
-     * @Route("/get/fields", name="brand_import_get_field_definitions", methods={"POST"})
-     */
+    #[Route(path: '/get/fields', name: 'brand_import_get_field_definitions', methods: ['POST'])]
@@ -24,4 +22 @@ class BrandController extends ImportController
-
-    /**
-     * @Route("/get/import/file", name="brand_import_get_file", methods={"POST"})
-     */
+    #[Route(path: '/get/import/file', name: 'brand_import_get_file', methods: ['POST'])]
@@ -32,4 +27 @@ class BrandController extends ImportController

@raziel057
Copy link

As a minimal reproducer, you can just get the file in the following zip and try to run the perl command:

no-removed-blank-lines.zip

cat no-removed-blank-lines.patch | perl -0 -pwe "s/@@.+\n\-\n//g" > test.diff

@TomasVotruba
Copy link
Member

Repository would be better, as we'll need to make it work online first. Otherwise we can get to "works on my machine" situation.

TomasVotruba added a commit that referenced this issue Jan 8, 2024

Verified

This commit was signed with the committer’s verified signature.
gsmet Guillaume Smet
rectorphp/rector-src@e3ccf17 Fix scoped e2e to load rector/rector:dev-main (#5447)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants