[HttpFoundation] update phpdoc of FlashBagInterface::add() #27765
Conversation
.idea/vcs.xml
Outdated
| @@ -0,0 +1,6 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
this file should be removed
| @@ -24,7 +24,7 @@ | ||
| * Adds a flash message for type. | ||
| * | ||
| * @param string $type | ||
| * @param string $message | ||
| * @param mixed $message |
There was a problem hiding this comment.
IIRC the rationale behind this type hint is that Symfony does not (did not?) want to take the responsability of serializing this properly. So technically, it works, but that might generate invalid support.
There was a problem hiding this comment.
If this still holds true, maybe you could add a comment right here, explaining just that.
There was a problem hiding this comment.
not sure how this applies: you are already allowed to put any serializable in the session, do we have special comments about this?
There was a problem hiding this comment.
I can't find the source of this discussion I saw, but if we go this way, I'd suggest adding a message indicating that $message should be serializable, and what that means (it doesn't crash when given to \serialize()? is the symfony serializer supported? What about a custom serializer service?)
There was a problem hiding this comment.
Also, a docs PR should probably be done too.
There was a problem hiding this comment.
FlashBagInterface::add should have exactly the same support as Session::set (it's basicaly a specialized Session::set).
Session::set doesn't say anything about what can be stored in the session (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/SessionInterface.php#L121) so I gess it's the responsibility of the implementation to decide what can be stored (all the implementations in symfony are based on serialize but some libs could extend it to support something else).
There was a problem hiding this comment.
Forget everything I said then, it would make no sense to add that comment here and not in the session interface.
120d624 to
9135e18
Compare
|
Thank you @sir-kain. |
…() (sir-kain) This PR was squashed before being merged into the 2.8 branch (closes #27765). Discussion ---------- [HttpFoundation] update phpdoc of FlashBagInterface::add() | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - **Reason why I propose to change the docblock like this: ** The `FlashBagInterface::add()` function does not work only with the `string` type in second parameter Commits ------- 9135e18 [HttpFoundation] update phpdoc of FlashBagInterface::add()
|
Congrats on your first PR!!! |
|
Good Job ! @sir-kain |
**Reason why I propose to change the docblock like this: **
The
FlashBagInterface::add()function does not work only with thestringtype in second parameter