Skip to content

Update SessionInterface.php #23824

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

Closed
wants to merge 2 commits into from
Closed

Update SessionInterface.php #23824

wants to merge 2 commits into from

Conversation

rwitchell
Copy link

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? -
Fixed tickets
License MIT
Doc PR

IDE is complaining the method does not exist. Added method to SessionInterface. Change can be merged up to master.

@Nyholm
Copy link
Member

Nyholm commented Aug 8, 2017

Thank you for this PR.
I'm afraid it is a BC break to add a method to an interface. You cannot target 2.7 branch for this PR.

Also, this is not a bugfix, it is a new feature.

@Koc
Copy link
Contributor

Koc commented Aug 8, 2017

Duplicates #22395 and many other

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Aug 8, 2017
@nicolas-grekas
Copy link
Member

You can type hint Session instead of the interface - I doubt there is real competition for other implementation of SessionInterface, isn't it?

@sstok
Copy link
Contributor

sstok commented Aug 8, 2017

I think the main reason for typehinting the SessionInterface is the ability to mock or use an adapter for legacy applications.

What if we introduce an interface like SessionWithFlashBagInterface that extends SessionInterface? This would make type hinting possible for and not forcing FlashBag support upon every implementation.

@fabpot
Copy link
Member

fabpot commented Aug 9, 2017

Closing as this has been discussed a lot already and cannot be done for BC reasons. Note that even without BC issues, this addition was rejected back then.

@fabpot fabpot closed this Aug 9, 2017
@rwitchell rwitchell deleted the patch-2 branch February 18, 2018 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants