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

[Session] Changed session.flash_bag service to publicly available#11957

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
linaori wants to merge1 commit intosymfony:masterfromlinaori:feature/flash-bag-service
Closed

[Session] Changed session.flash_bag service to publicly available#11957

linaori wants to merge1 commit intosymfony:masterfromlinaori:feature/flash-bag-service

Conversation

@linaori
Copy link
Contributor

QA
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#11279
LicenseMIT
Doc PR-

Main issue is described in my ticket. The problem is that in order to get the flash bag, you have to either ignore theSessionInterface onRequest or type hintSession instead ofSessionInterface during injection. Neither solutions work nicely with custom implementations of the session.

By definingsession.flash_bag as public service, you can just inject this as it's created bysession.getFlashBag. This makes sure that you always have the right bag, even if you decide to inject a custom flash bag in the session service.

For the default implementation, the old variant is used as injection. I have left this as it is so that people can still choose to omit it when constructing or putting in their own flash bag, with their own key if desired. This PR should be 100% BC.

@jakzal
Copy link
Contributor

I'd think twice before doing this in the core, especially since we're considering to get rid of a session as a service (see#10557). You can always register such a service in your project.

Why do you say that type hinting withSession doesn't work with custom session implementations? Apart from situations that a custom implementation doesn't extend the Session (but that requirement is clearly communicated by a type hint).

@linaori
Copy link
ContributorAuthor

@jakzal there's nothing concrete planned yet and this is an issue many programmers face. I have provided some cases below.

Each case has its own problems, however, the ones coming back are:

  • You always need thesession to get the flash bag because thesession.flash_bag is private
  • You always have to do a strict check to make sure the method is available
  • You always have to inject either the request or session to get the flash bag, no other means available
  • You can typehintSession, but why bother interfacing at all then
  • You can define your custom FlashBag service by overriding the parameter, but you "can't" inject it because it's private

In my PR I have made it possible to inject theFlashBagInterface. This makes it a standalone from the session. The fact that it uses the session in the container is something that can change in 3.0, however, I still want my flash bag to be injected so I get do set() directly without always requiring the session or request (makes unit-testing easier too).

Case 1: plain action issue

publicfunctionsomeAction(Request$request){// just because SessionInterface doesn't have getFlashBagif (($session =$request->getSession())instanceof Session) {$session->getFlashBag()->set('a','b');    }}

Case 2: Injection a session

class SomeService{private$session;publicfunction__construct(SessionInterface$session)    {$this->session =$session;    }publicfunctionsomeMethod()    {// have to do the same check as in case 1 because SessionInterface doesn't have it    }}

Case 3: Injection the session that deviates from the interface, not generic anymore

class SomeService{private$session;publicfunction__construct(MyCustomSession$session)    {$this->session =$session;    }publicfunctionsomeMethod()    {$this->session->getFlashBag()->setSomething();    }}

@stof
Copy link
Member

Injecting a private service in your service is perfectly allowed and valid. What is forbidden for a service marked as private is retrieving it dynamically through$container->get() (because it could have been inlined and dropped)

@linaori
Copy link
ContributorAuthor

@stof correct. However, using that service as injection in its current state is not 100% trustworthy. By default it's passed to the Session. However, it's not guaranteed to be that session as theFlashBag parameter is optional. If it's not passed along, it will be generated inside. Using the service I created catches this corner-case.

Worst case scenario: You put stuff in the FlashBag service, but Session is using a different one thus your values are not stored.

https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Session.php#L55

@stof
Copy link
Member

@iltar in the case of FrameworkBundle, the service is already passed to the session, as it is there in the service definition

@linaori
Copy link
ContributorAuthor

@stof again, correct. However, for custom session services, there is no way of properly defining a custom session other than overwriting the.class and usage of the service can't be guaranteed.

This PR solves the issue ofSessionInterface not havinggetFlashBag and you no longer have a visible dependency on theSession norRequest any more. It also fixes the issue where thegetFlashBag might not return the service but a different object (because session allowsnull as argument). Next to that, by making it public, people can do$container->get('session.flash_bag')->set(...). This is how it's presented in 90% of the documentation but currently not possible.

Because this allows people to use thesession.flash_bag service, it's also made forward compatible with the idea of removing thesession service. This service change won't break for anyone and makes it more robust, sounds like a win-win situation to me.

@linaori
Copy link
ContributorAuthor

ping@stof@fabpot it's been quite a while since I've read anything regarding the redesign of sessions. Is there anything planned for this? I have this PR and#11279 open, I was wondering if I should close them or keep them open + rebase against 2.8.

@Tobion
Copy link
Contributor

I guess it's a topic for the next team meeting.

@linaorilinaori deleted the feature/flash-bag-service branchApril 15, 2016 13:33
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@linaori@jakzal@stof@Tobion

[8]ページ先頭

©2009-2025 Movatter.jp