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

[FrameworkBundle] fix type annotation on ControllerTrait::addFlash() #36913

Merged
merged 1 commit into from Jun 10, 2020
Merged

[FrameworkBundle] fix type annotation on ControllerTrait::addFlash() #36913

merged 1 commit into from Jun 10, 2020

Conversation

ThomasLandauer
Copy link
Contributor

@ThomasLandauer ThomasLandauer commented May 22, 2020

Q A
Branch? 3.4
Bug fix? no
New feature? no
Deprecations? no
Tickets Fix #28991 Fix #34645
License MIT
Doc PR not yet, see below

Removing string type-hint of $message at addFlash()

Closes #28991 and #34645

Reasons:

@nicolas-grekas
Copy link
Member

This should be considered on 3.4 first to me, agreeing on changing the annotation.

Note that technically this is a BC break, but the method is @final in 4.4.
That's something to consider when deciding what to do here.

@ThomasLandauer
Copy link
Contributor Author

ThomasLandauer commented May 23, 2020

In 3.4 the type-hint isn't there - so everything's fine: https://github.com/symfony/symfony/blob/3.4/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php#L196

In 4.4 it is there, but the entire method still lives in Symfony\Bundle\FrameworkBundle\Controller\ControllerTrait (which doesn't exist any more): https://github.com/symfony/symfony/blob/4.4/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php#L159
So should I do the change there too?

@ro0NL
Copy link
Contributor

ro0NL commented May 31, 2020

@param string $message should be removed in 3.4 isnt it, which lead to this issue in the first place i believe.

Removing `string` type-hint of $message at addFlash()

Closes #28991 and #34645

Reasons:

* `addFlash()` is just a convenience shortcut for `FlashBagInterface::add()` which doesn't have the type hint: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Flash/FlashBagInterface.php#L28 . So removing it here improves consistency.

* #28991 (comment) is a valid use case for having an object as `$message`.

* Twig doesn't have any rendering helpers for the `message`, see https://symfony.com/doc/current/controller.html#flash-messages . And since users have to take care of displaying the `message` themselves, there's no reason to force a string upon them.
@nicolas-grekas nicolas-grekas changed the base branch from master to 3.4 June 4, 2020 10:40
@nicolas-grekas nicolas-grekas changed the title Update AbstractController.php [FrameworkBundle] fix type annotation on ControllerTrait::addFlash() Jun 4, 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.

(I rebased to target 3.4, the change on upper branches will be applied when merging up)

@fabpot
Copy link
Member

fabpot commented Jun 10, 2020

Thank you @ThomasLandauer.

@fabpot fabpot merged commit f87c993 into symfony:3.4 Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants