[TwigBridge] Change return type of Symfony\Bridge\Twig\AppVariable::getSession() #50794

Dirkhuethorst · 2023-06-27T10:26:58Z

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

carsonbot · 2023-06-27T10:27:04Z

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:

Always add tests
Keep backward compatibility (see https://symfony.com/bc).
Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
Features and deprecations must be submitted against the 6.4 branch.

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!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

carsonbot · 2023-06-27T10:27:06Z

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

Change the return type of AppVariable.php getSession from 'Session' to 'FlashBagAwareSessionInterface'

derrabus · 2023-06-27T11:07:59Z

The failing tests on the low-deps run look related.

Nyholm · 2023-06-27T20:05:32Z

Thank you for this PR.

I see you replaced the return type from class Session to interface FlashBagAwareSessionInterface. This is not always correct. It could be a Session that do not implement FlashBag feature.

I suggest to change it to SessionInterface

GromNaN

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.

src/Symfony/Bridge/Twig/AppVariable.php

Nyholm · 2023-06-27T20:18:21Z

I was hesitating if this was a bugfix or not. But with @GromNaN arguments, I agree. This is a feature.

derrabus · 2023-06-28T13:25:17Z

I think, if we change the return type to ?SessionInterface, we can file it as a bugfix. Using a custom implementation of the SessionInterface should've worked in Symfony 5 and it stopped working because of a too narrow native return type in Symfony 6. I believe this breaking change was unintended.

Change the return type of AppVariable.php getSession from 'Session' to 'SessionInterface'

Dirkhuethorst · 2023-07-17T08:26:13Z

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.

src/Symfony/Bridge/Twig/AppVariable.php:147:30: UndefinedInterfaceMethod: Method Symfony\Component\HttpFoundation\Session\SessionInterface::getFlashBag does not exist (see https://psalm.dev/181)

I am not sure how to proceed with this issue

GromNaN · 2023-07-17T08:38:15Z

You can return [] if $session is not an instance of FlashBagAwareSessionInterface or !method_exists($session, 'getFlashBag').

Dirkhuethorst · 2023-07-17T08:41:47Z

You can return [] if $session is not an instance of FlashBagAwareSessionInterface.

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'

src/Symfony/Bridge/Twig/AppVariable.php

Change the return type of AppVariable.php getSession from 'Session' to 'SessionInterface'

fabpot · 2023-09-10T08:51:24Z

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()

fabpot · 2023-09-10T08:52:49Z

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()

carsonbot added Status: Needs Review Bug TwigBridge labels Jun 27, 2023

carsonbot added this to the 6.2 milestone Jun 27, 2023

[TwigBridge] Fix for GitHub ticket 50789

aafaf89

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

Dirkhuethorst force-pushed the ticket_50789 branch from 2f61869 to aafaf89 Compare June 27, 2023 10:38

[TwigBridge] Fix for GitHub ticket 50789

7e04f58

Change the return type of AppVariable.php getSession from 'Session' to 'FlashBagAwareSessionInterface'

derrabus added Status: Needs Work and removed Status: Needs Review labels Jun 27, 2023

Nyholm changed the title ~~[TwigBridge] Fix for GitHub ticket 50789~~ [TwigBridge] Change return type of Symfony\Bridge\Twig\AppVariable::getSession() Jun 27, 2023

GromNaN requested changes Jun 27, 2023

View reviewed changes

src/Symfony/Bridge/Twig/AppVariable.php Outdated Show resolved Hide resolved

Nyholm added Feature and removed Bug labels Jun 27, 2023

[TwigBridge] Fix for GitHub ticket 50789

0d1d50b

Change the return type of AppVariable.php getSession from 'Session' to 'SessionInterface'

GromNaN approved these changes Jul 17, 2023

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Jul 17, 2023

[TwigBridge] Fix for GitHub ticket 50789

10fd572

Change the return type of AppVariable.php getSession from 'Session' to 'SessionInterface'

GromNaN reviewed Jul 17, 2023

View reviewed changes

src/Symfony/Bridge/Twig/AppVariable.php Show resolved Hide resolved

[TwigBridge] Fix for GitHub ticket 50789

97fc164

Change the return type of AppVariable.php getSession from 'Session' to 'SessionInterface'

fabpot removed this from the 6.2 milestone Jul 30, 2023

fabpot added this to the 6.3 milestone Jul 30, 2023

fabpot approved these changes Sep 10, 2023

View reviewed changes

fabpot closed this Sep 10, 2023

derrabus mentioned this pull request Sep 10, 2023

[TwigBridge] Remove duck typing from AppVariable::getFlashes() #51619

Merged

fabpot mentioned this pull request Sep 30, 2023

Release v6.3.5 #51788

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TwigBridge] Change return type of Symfony\Bridge\Twig\AppVariable::getSession() #50794

[TwigBridge] Change return type of Symfony\Bridge\Twig\AppVariable::getSession() #50794

Dirkhuethorst commented Jun 27, 2023 •

edited by Nyholm

Loading

carsonbot commented Jun 27, 2023

carsonbot commented Jun 27, 2023

derrabus commented Jun 27, 2023

Nyholm commented Jun 27, 2023 •

edited

Loading

GromNaN left a comment •

edited

Loading

Nyholm commented Jun 27, 2023

derrabus commented Jun 28, 2023

Dirkhuethorst commented Jul 17, 2023

GromNaN commented Jul 17, 2023 •

edited

Loading

Dirkhuethorst commented Jul 17, 2023

fabpot commented Sep 10, 2023

fabpot commented Sep 10, 2023

[TwigBridge] Change return type of Symfony\Bridge\Twig\AppVariable::getSession() #50794

[TwigBridge] Change return type of Symfony\Bridge\Twig\AppVariable::getSession() #50794

Conversation

Dirkhuethorst commented Jun 27, 2023 • edited by Nyholm Loading

carsonbot commented Jun 27, 2023

carsonbot commented Jun 27, 2023

derrabus commented Jun 27, 2023

Nyholm commented Jun 27, 2023 • edited Loading

GromNaN left a comment • edited Loading

Choose a reason for hiding this comment

Nyholm commented Jun 27, 2023

derrabus commented Jun 28, 2023

Dirkhuethorst commented Jul 17, 2023

GromNaN commented Jul 17, 2023 • edited Loading

Dirkhuethorst commented Jul 17, 2023

fabpot commented Sep 10, 2023

fabpot commented Sep 10, 2023

Dirkhuethorst commented Jun 27, 2023 •

edited by Nyholm

Loading

Nyholm commented Jun 27, 2023 •

edited

Loading

GromNaN left a comment •

edited

Loading

GromNaN commented Jul 17, 2023 •

edited

Loading