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

add test cases for symfony forms in symfony 5.0 and 5.3 #547

Closed
wants to merge 1 commit into from

Conversation

alleknalle
Copy link
Contributor

See issue mentioned in #545 (comment)

The test in tests/Set/UpToSymfony53/UpToSymfony53Test.php is failing, but it should pass.

@TomasVotruba
Copy link
Member

TomasVotruba commented Jan 16, 2024

Hi, thanks for the PR.

We'll need a fix coming along with the fixture, so we can merge PR right away and avoid staleing.

Closing then.

@alleknalle
Copy link
Contributor Author

@TomasVotruba I've got the feeling the problem is bigger then just fixing this issue. Can you please check my comment #545 (comment) ?

It looks like there is a problem in the order that things are handled.

@TomasVotruba
Copy link
Member

Currently we do not support config override. The fix would be in configure() method of a rule handling the type addition.

@alleknalle
Copy link
Contributor Author

@TomasVotruba What do you mean with config override?

The issue is as follows:

  1. In Symfony 5.0 the signature of method mapDataToForms() went from mapDataToForms(mixed $viewData, $forms) to mapDataToForms(mixed $viewData, iterable $forms)
  2. In Symfony 5.3 they changed the signature to public function mapDataToForms(mixed $viewData, \Traversable $forms)
  3. There are Rector rules for both defined and they work separately.
  4. When you run the Rector set list SymfonySetList::SYMFONY_50 (which ONLY runs Symfony 5.0), the signature is changed correctly to the 5.0 one.
  5. When you run the Rector set list SymfonySetList::SYMFONY_53 (which ONLY runs Symfony 5.3), the signature is changed correctly to the 5.3 one.
  6. But when you run the Rector level set list SymfonyLevelSetList::UP_TO_SYMFONY_53 (which goes from Symfony 2.5 UP TO Symfony 5.3), the signature is changed to the 5.0 one, while it should be the 5.3 one. Using any other level set list for Symfony higher then 5.3 will still change the signature to one defined in 5.0 and NOT 5.3.

I think that this is the case because there is something wrong in the order the Rector config files (and thus rules) are handled. When I change $rectorConfig->sets([SymfonySetList::SYMFONY_53, SymfonyLevelSetList::UP_TO_SYMFONY_52]); in Rector level set list SymfonyLevelSetList::UP_TO_SYMFONY_53 to $rectorConfig->sets([SymfonyLevelSetList::UP_TO_SYMFONY_52, SymfonySetList::SYMFONY_53]); (so I switch the order of loading the sets) and I run it, the correct rules are applied and the signature changes to 5.3 like it should.

That's why I'm thinking that there is something wrong in the way the sets (or rules) are being handled.
We can update all the UP_TO... and switch the order of the sets, but I don't know if that is the correct way. Maybe there is something happening on a higher level that needs to be changed.
If the order of the sets need to be changed, then I can do it, no problem. I don't know if that's needed then for any other Rector repo besides the Symfony one, but we can check that out.
But if changes are needed somewhere, then I have no idea where to look and that's why I created the issues stated before and added this PR to show you that something is not right.

So, if I can you with anything, please let me know and I will do my best, but can you please take a look at this, since you (and probably some other people around here) know a lot more about Rector then I do ;-)

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

Successfully merging this pull request may close these issues.

2 participants