Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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

Closed
Dirkhuethorst wants to merge5 commits intosymfony:6.2fromCuriousInc:ticket_50789

Conversation

@Dirkhuethorst
Copy link
Contributor

@DirkhuethorstDirkhuethorst commentedJun 27, 2023•
edited by Nyholm
Loading

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

QA
Branch?6.2
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#50789
LicenseMIT
Doc PR

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://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
Copy link

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
Copy link
Member

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

@Nyholm
Copy link
Member

Nyholm commentedJun 27, 2023•
edited
Loading

Thank you for this PR.

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

I suggest to change it toSessionInterface

kaznovac reacted with thumbs up emoji

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

@GromNaNGromNaN left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

TheFlashBagAwareSessionInterface was introduced insymfony/http-foundation: 6.2 by#46491, andsymfony/twig-bridge: 6.2 allowssymfony/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 ofFlashBagAwareSessionInterface orSessionInterface. Changing the return type to this single interface would reduce the functions that could be used in the templates.

@Nyholm
Copy link
Member

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

@NyholmNyholm added Feature and removed Bug labelsJun 27, 2023
@derrabus
Copy link
Member

I think, if we change the return type to?SessionInterface, we can file it as a bugfix. Using a custom implementation of theSessionInterface 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.

GromNaN, wouterj, and kaznovac reacted with thumbs up emoji

Change the return type of AppVariable.php getSession from 'Session' to 'SessionInterface'
@Dirkhuethorst
Copy link
ContributorAuthor

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
Copy link
Member

GromNaN commentedJul 17, 2023•
edited
Loading

You canreturn [] if$session is not an instance ofFlashBagAwareSessionInterface or!method_exists($session, 'getFlashBag').

@Dirkhuethorst
Copy link
ContributorAuthor

You canreturn [] if$session is not an instance ofFlashBagAwareSessionInterface.

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'
@fabpotfabpot removed this from the6.2 milestoneJul 30, 2023
@fabpotfabpot added this to the6.3 milestoneJul 30, 2023
@fabpot
Copy link
Member

Thank you@Dirkhuethorst.

fabpot added a commit that referenced this pull requestSep 10, 2023
…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
Copy link
Member

Merged, but Github didn't get the memo.

@fabpotfabpot closed thisSep 10, 2023
chalasr added a commit that referenced this pull requestSep 11, 2023
…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/ACommits-------3bd4e60 [TwigBridge] Remove duck typing from AppVariable::getFlashes()
@fabpotfabpot mentioned this pull requestSep 30, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@GromNaNGromNaNGromNaN approved these changes

@derrabusderrabusderrabus left review comments

@fabpotfabpotfabpot approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

6.3

Development

Successfully merging this pull request may close these issues.

6 participants

@Dirkhuethorst@carsonbot@derrabus@Nyholm@GromNaN@fabpot
[8]ページ先頭
©2009-2025 Movatter.jp