[2.1][FrameworkBundle] Allow configuration of session garbage collection #3659

ghost · 2012-03-21T03:34:42Z

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2171
Todo: -

…or session 'keep-alive'.

vicb · 2012-03-21T06:00:07Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd

@@ -90,6 +90,9 @@
        <!-- end of deprecated attributes -->
        <xsd:attribute name="cache-limiter" type="xsd:string" />
        <xsd:attribute name="auto-start" type="xsd:boolean" />
+        <xsd:attribute name="gc-maxlifetime" type="xsd:integer" />


I think this should xsd:string (to allow %lifetime%)

I'm sorry, but I dont follow. Also please note this is very important that this lifetime is not confused with other lifetimes (like cookie lifetime) for example.

The question is: do we want to allow integers only or would parameters be fine also ("lifetime" is a generic name above)

It's kind of terrible because it seems PHP's configuration ini takes strings and casts them as required which is kind of nasty. If you set something boolean when you retrieve the values you get back string integers on ini_get(). So the question is how much validation do we want, since these would have to be "integer strings" if you catch my drift.

I think if Symfony2 doesnt allow %string% replacements for integer fields, then that's something that should be looked at quite honestly.

I don't really get your point here. My concern is not with .ini or sf2 but with the xsd only.

Sure, but it would mean for string replacements, everything has to be marked as string, which means you lose a big part of the validation.

@Drak The validation is already performed properly by the Configuration class, using the processed value (meaning that you can validate as an integer there and use %lifetime% to use a parameter). The XSD validation occurs way too early to be able to be strict (which is also why we cannot use required in the XSD for required params for instance because they are required in one config file, not in each config file)

OK so basically all fields can/should be xsd:string and it's sorted out in the configuration class?

yeah, XSD files are not used to perform the actual validation as the Config system is far more powerful for this. The main goal of providing an XSD is for IDE auto-completion for guys choosing XML for their app-level config (and they are probably a minority). Thus, if an error occurs during the XSD validation, the exception thrown to the user is pretty cryptic which is generally a WTF ("what does this stuff mean ?")
I'm wondering if it makes sense to continue maintaining them.

@stof - thanks for the clarification, I'll make the necessary changes this PR.

…args.

ghost · 2012-03-21T21:56:20Z

@fabpot - this PR is ready for merge. It basically allows configuration of some session ini values that are necessary in controlling the session behaviour.

dlsniper · 2012-03-21T22:57:18Z

@Drak shouldn't all the options here: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php#L266 be available for configuration, or am I just reading the source wrong and they already are?

In this case should I make a separate PR to cover the rest or could you do it in this one?

fabpot · 2012-03-23T14:56:22Z

@Drak: the discussion is the ticket is very interesting and I think it should be part of a cookbook in the documentation. Can you take care of that before I merge this PR? Thanks.

ghost · 2012-03-25T15:32:59Z

@fabpot - yes - it's on the todo list. Will update this PR when done.

ghost · 2012-03-26T19:45:13Z

@fabpot - this is ready for merging, the documentation is done (the PR is in but I'll tweak it, but no need to wait to merge this PR). I will also add something extra to cookbook (I wrote docs for the component).

@fabpot

Commits ------- cdba4cf [FrameworkBundle] Change XSD to allow string replacements on session args. 52f7955 [FrameworkBundle] Remove default from gc_* session configuration keys. 749593d [FrameworkBundle] Allow configuration of session garbage collection for session 'keep-alive'. Discussion ---------- [2.1][FrameworkBundle] Allow configuration of session garbage collection Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: #2171 Todo: - --------------------------------------------------------------------------- by drak at 2012-03-21T21:56:20Z @fabpot - this PR is ready for merge. It basically allows configuration of some session ini values that are necessary in controlling the session behaviour. --------------------------------------------------------------------------- by dlsniper at 2012-03-21T22:57:18Z @Drak shouldn't all the options here: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Storage/NativeSessionStorage.php#L266 be available for configuration, or am I just reading the source wrong and they already are? In this case should I make a separate PR to cover the rest or could you do it in this one? --------------------------------------------------------------------------- by fabpot at 2012-03-23T14:56:22Z @Drak: the discussion is the ticket is very interesting and I think it should be part of a cookbook in the documentation. Can you take care of that before I merge this PR? Thanks. --------------------------------------------------------------------------- by drak at 2012-03-25T15:32:59Z @fabpot - yes - it's on the todo list. Will update this PR when done. --------------------------------------------------------------------------- by drak at 2012-03-26T19:45:13Z @fabpot - this is ready for merging, the documentation is done (the PR is in but I'll tweak it, but no need to wait to merge this PR). I will also add something extra to cookbook (I wrote docs for the component).

dlsniper · 2012-03-26T20:09:28Z

@Drak please see my above comment, #3659 (comment) , I can start working on a PR for this if you want, should be fairly straight forward.

What do you think?

ghost · 2012-03-28T06:29:58Z

@dlsniper I have no strong objections, except that some of the options, in the context of FrameworkBundle do not make sense to configure - for example, auto_start really does need to be switched off at all times - the reason is, we only want to explicitly start sessions. use_cookies and always_use_cookies (from memory sorry if it's wrong), really always need to be on. Remember, these configuration option in the Component make sense for standalone use, but it's within the context of FrameworkBundle where options may not work.

Sorry for my late reply, just had a lot on my plate.

dlsniper · 2012-03-28T08:26:43Z

@Drak yeah, no worries. I know for example we were forced to use auto_start in our application because of strange behavior in the previous implementation of session (before your changes) hence my asking out it.

Thank you again for your time and the great contributions you've made for Sf2 so far :)

[FrameworkBundle] Allow configuration of session garbage collection f…

749593d

…or session 'keep-alive'.

vicb reviewed Mar 21, 2012
View reviewed changes

This was referenced Mar 21, 2012

Make it possible to set keep alive sessions #2171

Closed

[WIP][Session] Added auto-extend capability #3654

Closed

Drak added 2 commits March 21, 2012 12:53

[FrameworkBundle] Remove default from gc_* session configuration keys.

52f7955

[FrameworkBundle] Change XSD to allow string replacements on session …

cdba4cf

…args.

fabpot merged commit cdba4cf into symfony:master Mar 26, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2.1][FrameworkBundle] Allow configuration of session garbage collection #3659

[2.1][FrameworkBundle] Allow configuration of session garbage collection #3659

ghost commented Mar 21, 2012

vicb Mar 21, 2012

ghost Mar 21, 2012

vicb Mar 21, 2012

ghost Mar 21, 2012

vicb Mar 21, 2012

ghost Mar 21, 2012

stof Mar 21, 2012

ghost Mar 21, 2012

stof Mar 21, 2012

ghost Mar 21, 2012

ghost commented Mar 21, 2012

dlsniper commented Mar 21, 2012

fabpot commented Mar 23, 2012

ghost commented Mar 25, 2012

ghost commented Mar 26, 2012

dlsniper commented Mar 26, 2012

ghost commented Mar 28, 2012

dlsniper commented Mar 28, 2012

[2.1][FrameworkBundle] Allow configuration of session garbage collection #3659

[2.1][FrameworkBundle] Allow configuration of session garbage collection #3659

Conversation

ghost commented Mar 21, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 21, 2012

dlsniper commented Mar 21, 2012

fabpot commented Mar 23, 2012

ghost commented Mar 25, 2012

ghost commented Mar 26, 2012

dlsniper commented Mar 26, 2012

ghost commented Mar 28, 2012

dlsniper commented Mar 28, 2012