Skip to content

[TwigBridge] allow null for $message of filter method trans #37941

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

Merged
merged 1 commit into from
Aug 26, 2020
Merged

[TwigBridge] allow null for $message of filter method trans #37941

merged 1 commit into from
Aug 26, 2020

Conversation

Flinsch
Copy link
Contributor

@Flinsch Flinsch commented Aug 25, 2020

Q A
Branch? 5.0
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #37931
License MIT

With Symfony 5.0, filter method trans of Symfony Twig Bridge does not allow null values for $message parameter anymore, breaking backward compatibility. See also #37931. The included commit provides a fix to this BC break by allowing null values again.

@fabpot
Copy link
Member

fabpot commented Aug 25, 2020

Closing as explained in the issue.

@fabpot fabpot closed this Aug 25, 2020
@Flinsch
Copy link
Contributor Author

Flinsch commented Aug 25, 2020

Closing as explained in the issue.

Have you even read the issue in full, @fabpot? Sorry, I don't mean to be rude, but I can't understand how you can just close this without even considering.

@fabpot
Copy link
Member

fabpot commented Aug 26, 2020

Reopening

@fabpot fabpot reopened this Aug 26, 2020
@nicolas-grekas nicolas-grekas added this to the 5.1 milestone Aug 26, 2020
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Works for me, we did this already in the Translator class.
Please add a test case.

@nicolas-grekas nicolas-grekas changed the base branch from 5.0 to 5.1 August 26, 2020 12:22
@nicolas-grekas nicolas-grekas changed the base branch from 5.1 to 5.0 August 26, 2020 12:22
@nicolas-grekas
Copy link
Member

Can you please rebase and target 5.1 also?

@Flinsch
Copy link
Contributor Author

Flinsch commented Aug 26, 2020

Will do. Thank you for your consideration.

@nicolas-grekas nicolas-grekas changed the title [TwigBridge] Fix #37931: BC break where filter method trans did not allow null values for $message parameter anymore [TwigBridge] allow null values for $message of filter method trans Aug 26, 2020
@nicolas-grekas nicolas-grekas changed the title [TwigBridge] allow null values for $message of filter method trans [TwigBridge] allow null for $message of filter method trans Aug 26, 2020
@Flinsch
Copy link
Contributor Author

Flinsch commented Aug 26, 2020

Can you please rebase and target 5.1 also?

@nicolas-grekas, the source branch cannot be changed for a pull request. I can rebase 5.0 onto 5.1 but the source branch will still read 5.0 which might lead to confusions. What should I do in your opinion? Should we stick to the name 5.0 for the source branch or should I replace the pull request with a new one having target/base branch and source branch both set to 5.1?

The Symfony guidelines on how to create pull requests say that "bug fixes must be submitted against the lowest maintained branch where they apply (lowest branches are regularly merged to upper ones so they get the fixes too.)" That's why I have chosen 5.0 in the first place. Just as a remark. I'm sorry if that was wrong.

@nicolas-grekas
Copy link
Member

You can rebase your branch 5.0 on 5.1 yes, that'd be fine on our side. You could then delete it on your fork, 5.0 is not maintained anymore anyway.

@nicolas-grekas nicolas-grekas changed the base branch from 5.0 to 5.1 August 26, 2020 13:44
@Flinsch
Copy link
Contributor Author

Flinsch commented Aug 26, 2020

Please add a test case.

Done.

fabbot.io still has comlaints about "Common Typos" and "Merge Commits" which don't seem to have any relations to my changes/commits however.

@Flinsch Flinsch requested a review from nicolas-grekas August 26, 2020 14:08
@fabpot
Copy link
Member

fabpot commented Aug 26, 2020

Looks like you forgot to rebase your changes on 5.1 (many unrelated commits). When done, push it again, that should be enough.

… allow null values for `$message` parameter anymore
@nicolas-grekas
Copy link
Member

Thank you @Flinsch.

@nicolas-grekas nicolas-grekas merged commit 9c86cd2 into symfony:5.1 Aug 26, 2020
@Flinsch
Copy link
Contributor Author

Flinsch commented Aug 26, 2020

Thank you @Flinsch.

I can only return my thanks, @nicolas-grekas.

@fabpot fabpot mentioned this pull request Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants