[TwigBridge] Change return type of Symfony\Bridge\Twig\AppVariable::getSession() #50794
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
It looks like you unchecked the "Allow edits from maintainer" box. That is fine, but please note that if you have multiple commits, you'll need to squash your commits into one before this can be merged. Or, you can check the "Allow edits from maintainers" box and the maintainer can squash for you. Cheers! Carsonbot |
Change the return type of AppVariable.php getSession from 'Session' to 'SessionInterface', to be in line with the return type of getSession in http-foundation/Request.php
2f61869 to
aafaf89
Compare
Change the return type of AppVariable.php getSession from 'Session' to 'FlashBagAwareSessionInterface'
|
The failing tests on the low-deps run look related. |
|
Thank you for this PR. I see you replaced the return type from class I suggest to change it to |
There was a problem hiding this comment.
The FlashBagAwareSessionInterface was introduced in symfony/http-foundation: 6.2 by #46491, and symfony/twig-bridge: 6.2 allows symfony/http-foundation: 5.4 in which the interface doesn't exit.
We usually don't update the dependencies constraints in maintenance branches except for security reasons. I don't think we should accept this change as a bugfix, it looks more like a new feature that would target the next version (6.4 ATM).
Also, Symfony\Component\HttpFoundation\Session\Session implements \IteratorAggregate and \Countable on top of FlashBagAwareSessionInterface or SessionInterface. Changing the return type to this single interface would reduce the functions that could be used in the templates.
|
I was hesitating if this was a bugfix or not. But with @GromNaN arguments, I agree. This is a feature. |
|
I think, if we change the return type to |
Change the return type of AppVariable.php getSession from 'Session' to 'SessionInterface'
|
I've changed the return type to SessionInterface, but now the Psalm check fails because in the AppVariable class the Flashbag of the session is accessed.
I am not sure how to proceed with this issue |
|
You can |
Won't that break backwards compatibility with http-foundation 5.4 where the FlashBagAwareSessionInterface does not exist? |
Change the return type of AppVariable.php getSession from 'Session' to 'SessionInterface'
Change the return type of AppVariable.php getSession from 'Session' to 'SessionInterface'
|
Thank you @Dirkhuethorst. |
…Variable::getSession() (Dirkhuethorst) This PR was submitted for the 6.2 branch but it was squashed and merged into the 6.3 branch instead. Discussion ---------- [TwigBridge] Change return type of Symfony\Bridge\Twig\AppVariable::getSession() Change the return type of AppVariable.php getSession from 'Session' to 'SessionInterface', to be in line with the return type of getSession in http-foundation/Request.php | Q | A | ------------- | --- | Branch? | 6.2 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix #50789 | License | MIT | Doc PR | Commits ------- 9dafdc1 [TwigBridge] Change return type of Symfony\Bridge\Twig\AppVariable::getSession()
|
Merged, but Github didn't get the memo. |
…ashes()` (derrabus) This PR was merged into the 7.0 branch. Discussion ---------- [TwigBridge] Remove duck typing from `AppVariable::getFlashes()` | Q | A | ------------- | --- | Branch? | 7.0 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Follows #50794 | License | MIT | Doc PR | N/A Commits ------- 3bd4e60 [TwigBridge] Remove duck typing from AppVariable::getFlashes()
Change the return type of AppVariable.php getSession from 'Session' to 'SessionInterface', to be in line with the return type of getSession in http-foundation/Request.php