Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[symfony/framework-bundle] Enable session support by default #333

Merged
1 commit merged into from
Jan 8, 2018
Merged

[symfony/framework-bundle] Enable session support by default #333

1 commit merged into from
Jan 8, 2018

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Jan 6, 2018 •
edited
Loading

Q A
License MIT

Enabling session by default has only one practical downside: some classes/services are loaded but never used. Not a big deal.
STILL, this is something that is going to be fixed on Symfony by symfony/symfony#25699, where the session is made extra-lazy.

Thus, this is my proposal to fix #262.

ghost
ghost approved these changes Jan 6, 2018
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

This was referenced Jan 6, 2018
Closed
Merged
ghost
ghost approved these changes Jan 6, 2018
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

@nicolas-grekas
Copy link
Member Author

This can be merged without waiting for symfony/symfony#25699.

@Nyholm
Copy link
Member

Nyholm commented Jan 6, 2018 •
edited
Loading

Recipes should have default values that fit 80% of the use cases.

Looking at 2017 survey Im not sure 80% of the users would like to have a session.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Jan 6, 2018 •
edited
Loading

@Nyholm which part of the survey do you link to?
Anyway, this doesn't really matter: there is no downside to enable sessions - ie, it does nothing unless you actually use sessions (in which case they have to be enabled.)

sroze
sroze reviewed Jan 6, 2018
View reviewed changes
symfony/framework-bundle/3.3/config/packages/framework.yaml Outdated
#session:
# # With this config, PHP's native session handling is used
# handler_id: ~
session:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add a comment here to explain that it doesn't harm their application's performance.

Copy link
Member

Choose a reason for hiding this comment

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

And also perhaps a note about how they could remove this to explicitly disable sessions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Any suggestions?

Copy link
Contributor

@sroze sroze Jan 7, 2018 •
edited by weaverryan
Loading

Choose a reason for hiding this comment

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

# Enables session support. Note that the session will ONLY be started if you read or write from it.
# Remove or comment this section to explicitly disable session support.
session:

Copy link
Member

Choose a reason for hiding this comment

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

👍 I rudely made a few minor tweaks to your suggestion - hope you don't mind @sroze ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

That’s perfect. I’ll survive it 😜

Copy link
Member Author

Choose a reason for hiding this comment

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

updated, thanks

@Nyholm
Copy link
Member

Nyholm commented Jan 6, 2018

Page 3.

Okey, fair.

weaverryan
weaverryan reviewed Jan 6, 2018
View reviewed changes
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

This is awesome. This is currently the biggest DX issue, along with csrf_protection, which I believe we could also “fix” after this, since it’s complicated due to session maybe not being available.

@weaverryan weaverryan mentioned this pull request Jan 6, 2018
Closed
@weaverryan
Copy link
Member

I opened a docs issue for this

@Toflar
Copy link

Toflar commented Jan 8, 2018

This is awesome. This is currently the biggest DX issue, along with csrf_protection, which I believe we could also “fix” after this, since it’s complicated due to session maybe not being available.

@weaverryan We've switched from session based CSRF protection to the Double Submit Cookie strategy to avoid relying on the session just for CSRF protection. Just FYI, maybe that would be a good addition to SF in a future version 😄

@covex-nn covex-nn mentioned this pull request Jan 8, 2018
Closed
ghost
ghost approved these changes Jan 8, 2018
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Pull request passes validation.

weaverryan
weaverryan approved these changes Jan 8, 2018
View reviewed changes
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

This is good for me!

@ghost ghost merged commit adf2c2d into symfony:master Jan 8, 2018
ghost pushed a commit that referenced this pull request Jan 8, 2018
Merge pull request #333
43f48db
fabpot added a commit to symfony/symfony that referenced this pull request Jan 10, 2018
@fabpot
bug #25699 [HttpKernel] Fix session handling: decouple "save" from se…
0cbd417 
…tting response "private" (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpKernel] Fix session handling: decouple "save" from setting response "private"

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

Fixes #25583 (comment) from @Tobion, and provides extra laziness for the "session" service, related to symfony/recipes#333.

(deps=high failure will be fixed by merging to upper branches.)

Commits
-------

f8727b8 [HttpKernel] Fix session handling: decouple "save" from setting response "private"
symfony-splitter pushed a commit to symfony/http-kernel that referenced this pull request Jan 10, 2018
@fabpot
bug #25699 [HttpKernel] Fix session handling: decouple "save" from se…
2841b6c 
…tting response "private" (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpKernel] Fix session handling: decouple "save" from setting response "private"

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

Fixes symfony/symfony#25583 (comment) from @Tobion, and provides extra laziness for the "session" service, related to symfony/recipes#333.

(deps=high failure will be fixed by merging to upper branches.)

Commits
-------

f8727b8 [HttpKernel] Fix session handling: decouple "save" from setting response "private"
symfony-splitter pushed a commit to symfony/http-foundation that referenced this pull request Jan 10, 2018
@fabpot
bug #25699 [HttpKernel] Fix session handling: decouple "save" from se…
bff4b20 
…tting response "private" (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpKernel] Fix session handling: decouple "save" from setting response "private"

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

Fixes symfony/symfony#25583 (comment) from @Tobion, and provides extra laziness for the "session" service, related to symfony/recipes#333.

(deps=high failure will be fixed by merging to upper branches.)

Commits
-------

f8727b8827 [HttpKernel] Fix session handling: decouple "save" from setting response "private"
@covex-nn covex-nn mentioned this pull request Jan 10, 2018
Open
@xabbuh xabbuh mentioned this pull request Jan 22, 2018
Closed
@nicolas-grekas nicolas-grekas deleted the session branch May 23, 2018 10:54
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sroze sroze sroze left review comments

@weaverryan weaverryan weaverryan approved these changes

@fabpot fabpot fabpot approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

What to do if a bundle (recipe) needs session?
6 participants
@nicolas-grekas @Nyholm @weaverryan @Toflar @fabpot @sroze