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

No Rector applied, but Symfony Rout annotation modified in wrong way #3932

Closed
Aerendir opened this issue Aug 8, 2020 · 14 comments
Closed

No Rector applied, but Symfony Rout annotation modified in wrong way #3932

Aerendir opened this issue Aug 8, 2020 · 14 comments
Labels

Comments

@Aerendir
Copy link
Contributor

Aerendir commented Aug 8, 2020

Bug Report

Subject Details
Rector version v0.7.63
Installed as composer dependency
    ---------- begin diff ----------
--- Original
+++ New
@@ -68,7 +68,7 @@
     }

     /**
-     * @Route(defaults={"reclaim" = null}, name="domain_reclaim")
+     * @Route(defaults={reclaim=null}, name="domain_reclaim")
      * @ParamConverter("reclaim", class="App:EmailReclaim")
      *
      * @param Request $request
    ----------- end diff -----------

Unfortunately I'm not able to reproduce the issue in the online demo.

I tried, but it seems that online all works well.

Last try: https://getrector.org/demo/7089f262-3fe2-41a2-ab7e-8da49a6b6759#result

Using the full controller code causes an error on the demo that says I cannot use the mail() function.

The problem is caused only when the Route annotation has the defaults key set for ParamConverter: on all other Route annotations works while it fails on the four that has the defaults key.

@TomasVotruba
Copy link
Member

I think there is already issue opened for this

@Aerendir
Copy link
Contributor Author

@TomasVotruba , can you link it?

@Aerendir
Copy link
Contributor Author

Do you refer to this?

#3927

It is not the same issue: in 3927, only the formatting of the annotation is changed (and this maybe OK: Rector is not a formatting tool).

In the case I'm reporting, instead, Rector is removing the " (quotes), breaking the code itself, not the style checks.

@TomasVotruba
Copy link
Member

Yes, that's one of them. Different input code, but same issue.
Closing as duplicated.

@Aerendir
Copy link
Contributor Author

Hey @TomasVotruba , I was fixing in my app the issue corresponding to this issue.

Currently the bug is still present.

You closed this as duplicate of #3927, then closed #3927 because

Unfortunatelly, there is now no way to preserve annotation format correctly, as described in README:

But, the point is that what I'm signaling here is actually a bug, not a formatting issue.

Unfortunately I'm still not able to reproduce the issue on the online demo, but I've found the issue: spaces!

If I remove the spaces from the Route annotation, Rector doesn't change it:

    ---------- begin diff ----------
--- Original
+++ New
@@ -68,7 +68,7 @@
     }

     /**
-     * @Route(defaults={"reclaim" = null}, name="domain_reclaim")
+     * @Route(defaults={"reclaim"=null}, name="domain_reclaim")
      * @ParamConverter("reclaim", class="App:EmailReclaim")
      *
      * @param Request $request
    ----------- end diff -----------

So, maybe it is related to the way the comments are formatted, but we are not speaking of a visual issue but of a syntax issue: removing the quotes, as Rector currently does on certain, unfortunately unknown, circumstances, actually breaks the code, not simply a style checker: it causes a real bug, something that makes the app unable to work, that triggers errors.

Anyway, I hope that mentioning the fact that the issue is caused by the presence of the spaces can actually help you to try to guess which the issue could be.

As said, unfortunately, I'm not able to reproduce the issue.

Anyway, this is my last try https://getrector.org/demo/b177da4e-c1af-4495-b16c-de659a6505ab

Cheers...

@Padam87
Copy link

Padam87 commented Sep 30, 2020

Same here as @Aerendir

@TomasVotruba TomasVotruba reopened this Sep 30, 2020
@TomasVotruba
Copy link
Member

Re-opening to keep track on it.

We need failing test case to have it covered.

@Aerendir
Copy link
Contributor Author

@TomasVotruba , unfortunately it is not reproducible :(

I know the failing test cases are required, but in every try I did, I was not able to reproduce it on the online demo 🤬

@TomasVotruba
Copy link
Member

In that case, GitHub repository should be good enough. I'll be able to create fixture from it

@Aerendir
Copy link
Contributor Author

Aerendir commented Oct 1, 2020

@TomasVotruba , it is a private repos, is an app with a lot of private dependencies... It is not so simple to give you access to the repo 😓 It seems like an empasse 🤔

@TomasVotruba
Copy link
Member

No need for full repository. Those 5 lines you've shared + composer deps of Doctrine will be enough.

@Aerendir
Copy link
Contributor Author

Aerendir commented Oct 7, 2020

Do you intend this?

    "require": {
        "php": "^7.4",
        "ext-ctype": "*",
        "ext-curl": "*",
        "ext-exif": "*",
        "ext-gd": "*",
        "ext-iconv": "*",
        "ext-intl": "*",
        "ext-json": "*",
        ...
        "babdev/pagerfanta-bundle": "^2.5",
        "doctrine/doctrine-bundle": "2.0.10",
        "doctrine/doctrine-migrations-bundle": "^3.0",
        "easycorp/easyadmin-bundle": "^3.1",
        "ekino/newrelic-bundle": "^2.2",
        "geoip2/geoip2": "^2.10",
        "hackzilla/password-generator": "^1.5",
        "knplabs/doctrine-behaviors": "^2.0",
        "knplabs/knp-gaufrette-bundle": "^0.7.1",
        "liip/imagine-bundle": "^2.3",
        "pagerfanta/pagerfanta": "^2.4",
        "php-translation/symfony-bundle": "^0.12.1",
        "ramsey/uuid": "^4.1",
        "scienta/doctrine-json-functions": "^4.1",
        "sensiolabs/ansi-to-html": "^1.2",
        "sentry/sentry-symfony": "^3.5",
        "serendipity_hq/bundle-aws-ses-monitor": "dev-dev",
        "serendipity_hq/bundle-features": "^0.12",
        "serendipity_hq/bundle-stripe": "dev-dev",
        "serendipity_hq/bundle-users": "dev-dev",
        "serendipity_hq/component-console-styles": "dev-dev",
        "serendipity_hq/component-stopwatch": "dev-dev",
        "serendipity_hq/component-text-matrix": "dev-dev",
        "serendipity_hq/component-then-when": "dev-dev",
        "serendipity_hq/component-value-objects": "^7.0",
        "symfony/amazon-mailer": "5.1.*",
        "symfony/apache-pack": "^1.0",
        "symfony/console": "5.1.*",
        "symfony/doctrine-messenger": "5.1.*",
        "symfony/dotenv": "5.1.*",
        "symfony/expression-language": "5.1.*",
        "symfony/flex": "^1.3.1",
        "symfony/framework-bundle": "5.1.*",
        "symfony/mailer": "5.1.*",
        "symfony/messenger": "5.1.*",
        "symfony/monolog-bundle": "^3.5",
        "symfony/security-bundle": "5.1.*",
        "symfony/twig-bundle": "5.1.*",
        "symfony/webpack-encore-bundle": "^1.7",
        "symfony/workflow": "5.1.*",
        "symfony/yaml": "5.1.*",
        "thecodingmachine/safe": "^1.1",
        "twig/extensions": "^1.5",
        "twig/inky-extra": "^3.0"
    },
    "require-dev": {
        "bamarni/composer-bin-plugin": "^1.4",
        "dg/bypass-finals": "^1.2",
        "doctrine/doctrine-fixtures-bundle": "^3.3",
        "heroku/heroku-buildpack-php": "^178",
        "roave/security-advisories": "dev-master",
        "serendipity_hq/component-var-dumper-f": "^2.0",
        "symfony/google-mailer": "5.1.*",
        "symfony/http-client": "5.1.*",
        "symfony/maker-bundle": "^1.20",
        "symfony/stopwatch": "^5.1",
        "symfony/web-profiler-bundle": "^5.0"
    },

@TomasVotruba
Copy link
Member

That's quite a lot of interevening packages.
Rather somethine like:

{
    "require": {
        "php": ">=7.2",
        "doctrine/orm": "^2.9"
    }
}

@TomasVotruba TomasVotruba changed the title No rector applied, but Symfony Rout annotation modified in wrong way No Rector applied, but Symfony Rout annotation modified in wrong way Oct 9, 2020
@TomasVotruba
Copy link
Member

Sub-problem of #4334

TomasVotruba added a commit that referenced this issue May 22, 2023
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

4 participants