NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:3.4fromThomasLandauer:patch-1

Jun 10, 2020

Merged

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

fabpot merged 1 commit intosymfony:3.4fromThomasLandauer:patch-1

Jun 10, 2020

Conversation

Copy link

Contributor

ThomasLandauer commentedMay 22, 2020•
edited by nicolas-grekas
Loading

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

Removingstring type-hint of $message at addFlash()

Closes#28991 and#34645

Reasons:

addFlash() is just a convenience shortcut forFlashBagInterface::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.
Flash message behavior changed #28991 (comment) is a valid use case for having an object as$message.
Twig doesn't have any rendering helpers for themessage, seehttps://symfony.com/doc/current/controller.html#flash-messages . And since users have to take care of displaying themessage themselves, there's no reason to force a string upon them.
This isn't a real new feature, but it isn't a bugfix either ;-)
I didn't updatesrc/**/CHANGELOG.md yet.
I'm not sure if it's necessary to update the docs. Maybe a short notehttps://symfony.com/doc/current/controller.html#flash-messages ?

carsonbot added Status: Needs Review Feature labels

May 22, 2020

nicolas-grekas added this to thenext milestone

May 23, 2020

Copy link

Member

nicolas-grekas commentedMay 23, 2020

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.

Copy link

ContributorAuthor

ThomasLandauer commentedMay 23, 2020•
edited
Loading

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 inSymfony\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?

Copy link

Contributor

ro0NL commentedMay 31, 2020

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

ThomasLandauer mentioned this pull request

May 31, 2020

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

Closed

nicolas-grekas removed the Feature label

Jun 4, 2020

nicolas-grekas modified the milestones:next,3.4

Jun 4, 2020

Update AbstractController.php

dfb4614

Removing `string` type-hint of $message at addFlash()Closes#28991 and#34645Reasons:* `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`, seehttps://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.