8000 [Form] action allows only strings by maxhelias · Pull Request #36297 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] action allows only strings #36297

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 4, 2020
Merged

[Form] action allows only strings #36297

merged 1 commit into from
Apr 4, 2020

Conversation

maxhelias
Copy link
Contributor
@maxhelias maxhelias commented Apr 1, 2020 •
edited
Loading
Q A
Branch? 5.0
Bug fix? no
New feature? no
Deprecations? no
Tickets ...
License MIT
Doc PR ...

On updating an old project that had actions to null it's caused me a type-hint error. With that, we can quickly identify where the problem is

@maxhelias maxhelias requested a review from xabbuh as a code owner April 1, 2020 08:58
@maxhelias maxhelias changed the base branch from master to 5.0 April 1, 2020 08:59
@javiereguiluz
Copy link
Member

Should we also allow null? The action attribute is optional for forms, right? So maybe we could pass null to tell Symfony to not add it.

@maxhelias
Copy link
Contributor Author

I'm not sure because by default we have an empty string : https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/Type/FormType.php#L191-L193
Infact, I had null in one of my forms and it causes a typing error not very clear to identify where the problem comes from.

HeahDude
HeahDude reviewed Apr 1, 2020
View reviewed changes
Copy link
Contributor
@HeahDude HeahDude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this PR 👍, but could you please add a test covering null to prevent future regressions?Thanks!

@maxhelias
Copy link
Contributor Author

@HeahDude done 😉

HeahDude
HeahDude approved these changes Apr 2, 2020
View reviewed changes
Copy link
Contributor
@HeahDude HeahDude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@HeahDude
Copy link
Contributor
HeahDude commented Apr 2, 2020

Side note: this should target 3.4 as bug fix IMO

@maxhelias
Copy link
Contributor Author
maxhelias commented Apr 2, 2020 •
edited
Loading

@HeahDude I don't know, it could be cause a BC no ?

@HeahDude
Copy link
Contributor
HeahDude commented Apr 2, 2020

The BC break would also happen in master, every bug fix is some kind of BC break since it changes a wrong behavior. The question is do we consider it a bug? I do, but I also agree that casting null to string should be ok. Though, targeting master would mean deprecating passing null first in 5.1, and throw an exception in 6.0 only.

@yceruto
Copy link
Member
yceruto commented Apr 2, 2020

+1 for 3.4 as bugfix

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Apr 3, 2020
@fabpot
Copy link
Member
fabpot commented Apr 4, 2020

Merging it into master instead. Breaking code in a patch release is very different from breaking code in a minor release. So, while I agree this is a bug fix, I don't see the need to fix it in 3.4.

@fabpot fabpot changed the base branch from 5.0 to master April 4, 2020 07:23
@fabpot
Copy link
Member
fabpot commented Apr 4, 2020

Thank you @maxhelias.

@fabpot fabpot merged commit 5aeecc2 into symfony:master Apr 4, 2020
@maxhelias maxhelias deleted the allow-action branch April 4, 2020 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HeahDude HeahDude HeahDude approved these changes

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Form Status: Reviewed
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
7 participants
@maxhelias @javiereguiluz @HeahDude @yceruto @fabpot @nicolas-grekas @carsonbot
0