-
Notifications
You must be signed in to change notification settings - Fork 95
Request > Return Session instead of SessionInterface
#233
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
Conversation
Symfony 6 removed the `Session` and `FlashBagInterface` services. That means that the only way to get the FlashBag (to add flashes) it to use `$request->getSession()->getFlashBag()`. But the problem is, `getFlashBag` is not defined on the `SessionInterface`: ``` Call to an undefined method Symfony\Component\HttpFoundation\Session\SessionInterface::getFlashBag(). ``` To make everyone's life easier, let's change the return type to `Sesssion` instead. This is what's returned in a default Symfony application.
@ondrejmirtes Does this make sense? Or should it be done differently?
It makes sense, but still, it's technically straight up lying about the return type. I don't really like it.
But it means that all code needs to do an assertion check. For example, right now I have 40 errors because getFlashBag does not exist on session on request.
I thought these extensions should solve friction like this.
Can we somehow find out about the real return type from the service map? EDIT Right, we can't, because they removed the service. Hmm.
I think the suggested change makes a lot of sense because:
- this is a symfony plugin;
- almost all symfony users have the normal session configuration, meaning request session returns the session object.
For a small set of users it could mean that phpstan now falsely allows getFlashBag on the request session, because they have a custom session implementation. They prob didn't call the flashbag anyway, as it doesn't work. And if they implemented it, great. If needed, and those complain, we can add a toggle to disable it for them.
You know what, after some digging in Symfony's code, I think you are right. Lets merge this.
Hi, I reused your test. Since the return type wasn't really dynamic, the same result can be achieved with stub files: 012305d
Thank you.
Interesting, that was my first try but it didn't seem to pick it up. I read the code as : if request class does not exist then use stub
Nope, that's not how stub files work. They're purely for overriding PHPDocs, nothing else.
@ondrejmirtes Hi, I'm curious ; is it possible to make this return type a Benevolent<Session|SessionInterface> with phpdoc ?
If Session implements SessionInterface, then Session|SessionInterface is just SessionInterface. Thanks to https://phpstan.org/developing-extensions/type-system#type-normalization.
If Session implements SessionInterface, then
Session|SessionInterfaceis justSessionInterface. Thanks to https://phpstan.org/developing-extensions/type-system#type-normalization.
Oh I see...
So there is no way to add phpdoc in a way that
$session->getFlashbag()is not reported because it exists in Session$session instanceof Sessionis not reported as always true because it can be a SessionInterface.
Both can't be true at the same time.
Symfony 6 removed the
SessionandFlashBagInterfaceservices.That means that the only way to get the FlashBag (to add flashes) it to use
$request->getSession()->getFlashBag().But the problem is,
getFlashBagis not defined on theSessionInterface:To make everyone's life easier, let's change the return type to
Sesssioninstead.This is what's returned in a default Symfony application.