8000 fix ignored session save_path by Tobion · Pull Request #904 · symfony/symfony-standard · GitHub
[go: up one dir, main page]

Skip to content
This repository was archived by the owner on Nov 27, 2020. It is now read-only.

fix ignored session save_path #904

Merged
merged 1 commit into from
Jan 14, 2016
Merged

fix ignored session save_path #904

merged 1 commit into from
Jan 14, 2016

Conversation

Tobion
Copy link
Contributor
@Tobion Tobion commented Dec 8, 2015

fixes symfony/symfony#16898

do not use php.ini session handler but native file to actually use save_path config

When handler_id is null it uses the Native one, but that does not set the save_path. So we need to use https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandler.php instead (which is the default configuration).

@weaverryan
Copy link
Member

@Tobion remind me, was this something that changed in Symfony 3? I always have a hard time wrapping me head around this, because I know (at least previously), that setting this setting to null and not setting it meant two different things.

@Tobion
Copy link
Contributor Author
Tobion commented Dec 8, 2015

Not defining handler_id is the same as handler_id: session.handler.native_file (as now) because its the default value in the config. But IMO specifiying it explicitly is better DX because save_path and handler_id depend on each other.
handler_id: ~ means use handler from php.ini.

@ogizanagi
Copy link
Contributor

What about merging this in 2.3 then ?

EDIT: I've just seen save_path is only present in standard-edition 3.0+ actually, and is part of fabpot@dcf4ead, which was merged in 3.0. So it makes sense actually 👍

@weaverryan
Copy link
Member

@Tobion I agree with your point on DX. So, we used to default to ~, meaning "use php.ini" default. NOW this would default people to file storage. I'm just pointing out that we'll be changing the recommended default setting for projects, agree?

But I think I like it: though what about keeping the comment that setting this to null will cause the php.ini default to be used?

And yes, save_path was added in SE 3.0 only, but it always existed. So if we're going to keep it in 3.0, perhaps we should backport it to 2.3: it is nice to explicitly see why sessions are being stored in a specific location.

@Tobion
Copy link
Contributor Author

@ogizanagi the bug is ony in 3.0

Ok to sum up: In #606 the session handler was changed to use php.ini values. If you use the php.ini session handler, then the framework.session.save_path is irrelevant because it's not used at all.

So either we keep using php.ini settings by default (which means we do not know where sessions are saved and thus the var/session directory makes no sense). Or we want to use var/sessions but then we have to fix the session handler to a file based one as well (because it does not make sense to configure the save_path to var/sessions if you use a redis handler in your php.ini.

@Pierstoval
Copy link
Contributor

var/sessions seems to be the best to me, and it was intended by @fabpot to use this directory, so I don't see why we should propose php.ini default to be used.

@weaverryan
Copy link
Member

In fact, the final solution added by Fabien in 3.0, was suggested by drak (ghost, unfortunately, on the PR) on the linked #606 PR: #606 (comment)... but that was never the right solution (for the exact reasons this issue was opened).

I think we should keep the native, php.ini handler, but with more comments than we have now:

session:
    # handler_id set to null will use the default session handler from php.ini
    handler_id: ~

    # or, you can specify some specific handler and options
    # http://symfony.com/doc/current/reference/configuration/framework.html#handler-id
    # handler_id: session.handler.native_file
    # save_path: "%kernel.root_dir%/../var/sessions/%kernel.environment%"

@Tobion
Copy link
Contributor Author
Tobion commented Dec 8, 2015

But then it does not make sense to preconfigure the directory https://github.com/symfony/symfony-standard/tree/master/var/sessions if it is not used.
And I think it is easier to just reference the documenation: http://symfony.com/doc/current/cookbook/session/sessions_directory.html

@weaverryan
Copy link
Member

@Tobion you're right, so I would vote to remove the var/session directory if we used my proposal.

If we do use my proposal and you think the code is a little to wordy (i.e. too many comments), then yea: we could just link to the docs for how to configure session handlers in general. We should link to the reference docs - the link you posted (unfortunately) only shows you how to do file storage: it doesn't help anyone looking to configure something else, like Redis.

So we're back to what you mentioned @Tobion: do we default to using the php.ini default (as was done in 2.4-3.0 in the SE) or do we default to /var/session file storage? Based on the decision in #606, we previously decided to use the php.ini default.

@ogizanagi
Copy link
Contributor

As I understand it, @fabpot proposal was only about avoiding to have the session.save_path inside the application cache (which is the default), in order to prevent flushing it on a cache:clear.
Not especially about promoting the use of session.handler.native_file (but if used, suggest to store sessions into /var/session)

But then, as @weaverryan expressed, it makes no sense to have the var/session directory in SE if it is not used by default, as well as having the session.save_path path option configured. It should rather be commented.

I would opt for @weaverryan suggestion. Or even remove back the save_path option. Then, simply link the cookbook entry, which already mention why you should use another save_path than the default %kernel.cache_dir%/sessions.

@xabbuh
Copy link
Member
xabbuh commented Dec 9, 2015

Imo having sessions stored in var/sessions is less confusing for users as the Symfony applications would not interfer with other PHP applications on the same server which could lead to hard to debug issues.

@weaverryan
Copy link
Member

The more I think about this, the more I agree with @xabbuh: put things in /var/sessions by default (as this PR does). But, I think at least a link to the docs is useful: this would be a common setting to need to change (and the docs could use some work too....)

session:
    # http://symfony.com/doc/current/reference/configuration/framework.html#handler-id
    handler_id: session.handler.native_file
    save_path: "%kernel.root_dir%/../var/sessions/%kernel.environment%"

👍 as-is, but even more with the docs link comment

@ghost
Copy link
ghost commented Dec 28, 2015

I've got the same issue (symfony/symfony#17157). Please merge this!

@fabpot
Copy link
Member
fabpot commented Jan 14, 2016

Thank you @Tobion.

@fabpot fabpot merged commit 070e53c into symfony:3.0 Jan 14, 2016
fabpot added a commit that referenced this pull request Jan 14, 2016
This PR was merged into the 3.0 branch.

Discussion
----------

fix ignored session save_path

fixes symfony/symfony#16898

do not use php.ini session handler but native file to actually use save_path config

When handler_id is null it uses the Native one, but that does not set the save_path. So we need to use https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandler.php instead (which is the default configuration).

Commits
-------

070e53c do not use php.ini session handler but native file to actually use save_path config
@fabpot
Copy link
Member
fabpot commented Jan 14, 2016

@weaverryan added the doc mention in 44a1fd8

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants
0