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

[ANNOTATIONS_TO_ATTRIBUTES] Upgrading Assert\Range leads to error #8300

Closed
rvanlaak opened this issue Nov 9, 2023 · 12 comments
Closed

[ANNOTATIONS_TO_ATTRIBUTES] Upgrading Assert\Range leads to error #8300

rvanlaak opened this issue Nov 9, 2023 · 12 comments
Labels

Comments

@rvanlaak
Copy link

rvanlaak commented Nov 9, 2023

Bug Report

Subject Details
Rector version v0.18.6

The Symfony / Sensio annotation to attribute upgrade works great, except for one BC break. It is up for debate whether annotations should be 100% BC with attributes.

Minimal PHP Code Causing Issue

## entity file

/*
  * @Assert\Range(
  *      min = 0,
  *      max = 100,
  *      minMessage = "Average discount must be at least {{ limit }}",
  *      maxMessage = "Average discount must be less than {{ limit }}"
  * )
  */
<?php

declare(strict_types=1);

use Rector\Config\RectorConfig;
use Rector\Doctrine\Set\DoctrineSetList;
use Rector\Symfony\Set\SymfonySetList;
use Rector\Symfony\Set\SensiolabsSetList;

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->paths([
        __DIR__ . '/config',
        __DIR__ . '/public',
        __DIR__ . '/src',
        __DIR__ . '/tests',
    ]);

    $rectorConfig->sets([
        DoctrineSetList::ANNOTATIONS_TO_ATTRIBUTES,
        SymfonySetList::ANNOTATIONS_TO_ATTRIBUTES,
        SensiolabsSetList::ANNOTATIONS_TO_ATTRIBUTES,
    ]);
};

Actual Behaviour

The parameters are kept intact

#[Assert\Range(minMessage: 'Average discount must be at least {{ limit }}', maxMessage: 'Average discount must be less than {{ limit }}', min: 0, max: 100)]

Expected Behaviour

But should be changed to notInRangeMessage instead. Without changing it, an exception will be thrown.

Maybe we can consider

  1. always remove the values
  2. merge the two messages to one message (replace {{limit}} with min or max
  3. ask the developer on rector execution what to do
#[Assert\Range(notInRangeMessage: 'Average discount should be between {{ min }} and {{ max }}.', min: 0, max: 100)]
@rvanlaak rvanlaak added the bug label Nov 9, 2023
@TomasVotruba
Copy link
Member

TomasVotruba commented Dec 3, 2023

Thank you for your report!

We'll need an isolated failing demo link from: http://getrector.com/demo,
that way we can reproduce the bug.

The smaller demo the better 👍

@mindaugasvcs
Copy link

ANNOTATIONS_TO_ATTRIBUTES is working fine until Doctrine annotations gets involved, it then just hangs forever and the --debug option does not provide any message.
I found that this line in my Entity class caused problem:
use Doctrine\ORM\Mapping\Index as Index;
When I removed ' as Index' it suddenly started working fine.
Not sure if it is a bug.

@TomasVotruba
Copy link
Member

Thank you for your report!

We'll still need an isolated failing demo link from: http://getrector.com/demo,
that way we can reproduce the bug. Closing for now to avoid piling up more issue without demo link.

Feel free to create a new one with demo link, so we can see the issue ourselves and fix it 👍

@mindaugasvcs
Copy link

Thank you for your report!

We'll still need an isolated failing demo link from: http://getrector.com/demo, that way we can reproduce the bug. Closing for now to avoid piling up more issue without demo link.

Feel free to create a new one with demo link, so we can see the issue ourselves and fix it 👍

Recreating the issue is not possible on your test site as it depends on the libs it uses, still the output is not quite right, it just spits out 'use Doctrine\ORM\Mapping\Index;' on the top while leaving 'use Doctrine\ORM\Mapping\Index as Index;' as it is.

@TomasVotruba
Copy link
Member

Could you share the demo link with minimal PHP code that is causing the troubles? Only that way we can start digging

@mindaugasvcs
Copy link

Could you share the demo link with minimal PHP code that is causing the troubles? Only that way we can start digging

https://getrector.com/demo/8669e371-05c9-477f-b32a-34ef1400bb99

@TomasVotruba
Copy link
Member

Thanks 👍

@samsonasik Could you look into this one? Seems use import is being added despite already existing one.

@TomasVotruba TomasVotruba reopened this Dec 26, 2023
@samsonasik
Copy link
Member

The issue seems unrelated with import.

Alias name should be different with last name import in the namespace, it like doing useless alias:

namespace App;

use stdClass as stdClass;

which should not happen on the first place.

@mindaugasvcs we need reproducible repo that show the issue, with before after expected output

@samsonasik
Copy link
Member

I think the solution is you can just simply remove the useless alias:

-use Doctrine\ORM\Mapping\Index as Index;
+use Doctrine\ORM\Mapping\Index;

as that useless, so I guess it is not a bug.

@samsonasik
Copy link
Member

I am closing it as it is not a bug, the alias name is same with last name, which should not happen on the first place.

@samsonasik
Copy link
Member

I created new special rule to remove useless alias in use statement as same with last name:

@TomasVotruba
Copy link
Member

@samsonasik Even better, thanks for the new rule 👍

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