[2.3][Session] Give greater control over how and when session starts #7576

Merged
merged 6 commits into from Apr 18, 2013

Projects

None yet

4 participants

@ghost
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets na
License MIT
Doc PR symfony/symfony-docs#2475

Refs #6036

Gives control over how start on demand works: allowing to turn it on or off and to allow bag access when session is off.

Drak added some commits Apr 6, 2013
Drak [HttpFoundation] Give control over how session start on demand.
This allows control over how the session start on demand works

0: no start on demand when bags are accessed
1: start session if bags are accessed
2: no start on demand when bags are accessed but still return bag contents
8fc2397
Drak [FrameworkBundle] Add configuration to allow control over session sta…
…rt on demand.

1. Gives user control over session start on demand mode.
2. Re-introduce flag to allow session listener to manually start session.
af0a140
@ghost

note: The tests pass, but there is an error with Travis on 5.3 build segfault.

@stof stof and 1 other commented on an outdated diff Apr 6, 2013
...FrameworkBundle/DependencyInjection/Configuration.php
@@ -184,6 +184,11 @@ private function addSessionSection(ArrayNodeDefinition $rootNode)
->info('session configuration')
->canBeUnset()
->children()
+ ->booleanNode('auto_start')->info('Flag for SessionListener to start session')
+ ->defaultValue(true)->end()
@stof
stof Apr 6, 2013

you should use defaultTrue().

And the indentation should be 4 spaces per level. If you find the line too long, format it like this:

    ->booleanNode('auto_start')
        ->defaultTrue()
        ->info('Flag for SessionListener to start session') 
    ->end()
@stof stof and 1 other commented on an outdated diff Apr 6, 2013
...FrameworkBundle/DependencyInjection/Configuration.php
@@ -184,6 +184,11 @@ private function addSessionSection(ArrayNodeDefinition $rootNode)
->info('session configuration')
->canBeUnset()
->children()
+ ->booleanNode('auto_start')->info('Flag for SessionListener to start session')
+ ->defaultValue(true)->end()
+ ->scalarNode('on_demand_mode')->info('Start session on demand (on access), 0 - off, 1 - on (strict), 2 - off (lax)')
+ ->defaultValue(1)->end()
@stof
stof Apr 6, 2013

you should validate the value, using an enumNode

@stof
stof Apr 6, 2013

and instead of 0, 1or2, you should let the user use a meaningful string (lax,strict, ...) and transform it inbeforeNormalization` to keep the config readable.

@stof stof commented on the diff Apr 6, 2013
...ameworkBundle/Resources/config/schema/symfony-1.0.xsd
@@ -77,6 +77,8 @@
</xsd:complexType>
<xsd:complexType name="session">
+ <xsd:attribute name="auto-start" type="xsd:boolean" />
+ <xsd:attribute name="on-demand-mode" type="xsd:string" />
@stof
stof Apr 6, 2013

mock-name is missing

@stof
Symfony member

Your link to the doc PR is wrong. It is linking to a doc issue, not to a doc PR.

@ghost

@stof - does it have to be a PR rather than an issue? I thought the idea was so that a feature will not be forgotten. the eventual PR will reference that ticket. I'd prefer not to write docs until it's merged.

@fabpot
Symfony member

@stof is right: we don't merge new features anymore without proper documentation.

@ghost

@fabpot - OK, that's fine. If the PR is acceptable now then I'll do the docs.

@fabpot fabpot and 1 other commented on an outdated diff Apr 6, 2013
src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
@@ -9,6 +9,9 @@ CHANGELOG
* added `TimedPhpEngine`
* added `--clean` option the the `translation:update` command
* added `http_method_override` option
+ * Reintroduce `auto_start` session config flag which control to `SessionListener` to manually start sessions
@fabpot
fabpot Apr 6, 2013

This sentence looks weird.

@ghost
ghost Apr 6, 2013

It would appear working with PHP sessions for too long is rewiring my brain. I'll fix it.

@fabpot fabpot and 1 other commented on an outdated diff Apr 6, 2013
...oundation/Session/Storage/SessionStorageInterface.php
@@ -25,6 +25,22 @@
interface SessionStorageInterface
{
/**
+ * Do not start session on demand and throw exception if attempt
+ * is make to read or write bag data.
+ */
+ const NO_START_ON_DEMAND_STRICT = 0;
@fabpot
fabpot Apr 6, 2013

what about using on, off, and off_lax to be consistent with the configuration options?

@ghost
ghost Apr 6, 2013

Do you mean something like START_ON_DEMAND_OFF, START_ON_DEMAND_ON and START_ON_DEMAND_OFF_LAX? or something else?

@fabpot
fabpot Apr 6, 2013

I mean: const NO_START_ON_DEMAND_STRICT = 'off'. So using on, off, and off_lax for values instead of 0, 1, and 2.

@fabpot fabpot and 2 others commented on an outdated diff Apr 6, 2013
...dle/FrameworkBundle/EventListener/SessionListener.php
- public function __construct(ContainerInterface $container)
+ public function __construct(ContainerInterface $container, $autoStart)
@fabpot
fabpot Apr 6, 2013

I would set the default value to be the one that was implicitly used in 2.2

@ghost
ghost Apr 6, 2013

OK, so technically that is off and we rely on start on demand.

@stof
stof Apr 6, 2013

it does not seem to be done (or at least not pushed)

@ghost
ghost Apr 6, 2013

I changed the default in the DependencyInjection/Configuration.php for the param.

@fabpot
fabpot Apr 6, 2013

You should also add it here (for a better BC).

@fabpot fabpot and 1 other commented on an outdated diff Apr 6, 2013
...oundation/Session/Storage/MockArraySessionStorage.php
* Constructor.
*
- * @param string $name Session name
+ * @param string $name Session name.
@fabpot
fabpot Apr 6, 2013

we don't use dots at the end of argument docs.

@ghost
ghost Apr 6, 2013

Fix for all Session/* folder

@fabpot fabpot and 1 other commented on an outdated diff Apr 6, 2013
...oundation/Session/Storage/MockArraySessionStorage.php
@@ -197,11 +204,13 @@ public function registerBag(SessionBagInterface $bag)
public function getBag($name)
{
if (!isset($this->bags[$name])) {
- throw new \InvalidArgumentException(sprintf('The SessionBagInterface %s is not registered.', $name));
+ throw new \InvalidArgumentException(sprintf('The SessionBagInterface %s is not registered', $name));
@fabpot
fabpot Apr 6, 2013

we do use dots at the end of exception messages.

@ghost
ghost Apr 6, 2013

Fixed.throughout session code.

@fabpot fabpot and 1 other commented on an outdated diff Apr 6, 2013
...Tests/Session/Storage/MockArraySessionStorageTest.php
*/
public function testUnstartedSave()
{
$this->storage->save();
}
+
+ /**
+ * @expectedException \RuntimeException
+ */
+ public function testtStartOnDemandException()
@fabpot
fabpot Apr 6, 2013

small typo: there is a double t testtStart...

@ghost
ghost Apr 6, 2013

Fixed (in three placed)

@stof stof and 1 other commented on an outdated diff Apr 6, 2013
src/Symfony/Component/HttpFoundation/CHANGELOG.md
@@ -4,6 +4,7 @@ CHANGELOG
2.3.0
* `UploadedFile::isValid` now returns false if the file was not uploaded via HTTP (in a non-test mode)
+ * addded control for session start on demand.
Drak [HttpFoundation][FrameworkBundle] Keep save auto_start behaviour as i…
…n 2.2 and make component values consistent with FrameworkBundle's configuration options.
2583c26
@ghost Unknown referenced this pull request in symfony/symfony-docs Apr 6, 2013
Merged

Document start on demand feature. #2475

@ghost

@fabpot - Documentation PR completed

@TerjeBr TerjeBr and 1 other commented on an outdated diff Apr 11, 2013
...tpFoundation/Session/Storage/NativeSessionStorage.php
$this->start();
+ } elseif (!$this->started && self::NO_START_ON_DEMAND_STRICT === $this->mode) {
+ throw new \RuntimeException('Cannot access session bags because the session has been started.');
@TerjeBr
TerjeBr Apr 11, 2013

I think the message should be 'Cannot access session bags because the session has not been started.'
You forgot a 'not' in there. Otherwise the messages does not make sense.

@ghost
ghost Apr 11, 2013

Woops! Fixed!

@TerjeBr TerjeBr commented on an outdated diff Apr 11, 2013
...oundation/Session/Storage/MockArraySessionStorage.php
$this->start();
+ } elseif (!$this->started && self::NO_START_ON_DEMAND_STRICT === $this->mode) {
+ throw new \RuntimeException('Cannot access session bags because the session has been started.');
@TerjeBr
TerjeBr Apr 11, 2013

Same here as I noted below. I think the message should be 'Cannot access session bags because the session has not been started.'
You forgot a 'not' in there. Otherwise the messages does not make sense.

Drak Coding standards
As requested by @fabpot
1f521d8
@pborreli pborreli commented on the diff Apr 11, 2013
...ttpFoundation/Session/Storage/Proxy/AbstractProxy.php
@@ -36,7 +36,7 @@
protected $saveHandlerName;
/**
- * Gets the session.save_handler name.
+ * Gets the session.save_handler name
@pborreli
pborreli Apr 11, 2013

why removing trailing dot here ?

@ghost
ghost Apr 11, 2013

It was an error, I was removing them from the @param blocks. I think it doesn't matter though

@fabpot
fabpot Apr 18, 2013

Consistency does matter. You should add them back. Sorry for the trouble.

@fabpot fabpot added a commit that referenced this pull request Apr 18, 2013
@fabpot fabpot merged branch drak/start_on_demand (PR #7576)
This PR was merged into the master branch.

Discussion
----------

[2.3][Session] Give greater control over how and when session starts

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | na
| License       | MIT
| Doc PR        | symfony/symfony-docs#2475

Refs #6036

Gives control over how start on demand works: allowing to turn it on or off and to allow bag access when session is off.

Commits
-------

f431cb0 Fix tests
1f521d8 Coding standards
2583c26 [HttpFoundation][FrameworkBundle] Keep save auto_start behaviour as in 2.2 and make component values consistent with FrameworkBundle's configuration options.
ceaf69b [FrameworkBundle] Use more sophisticated validation and configuration.
af0a140 [FrameworkBundle] Add configuration to allow control over session start on demand.
8fc2397 [HttpFoundation] Give control over how session start on demand.
7aa0681
@fabpot fabpot merged commit f431cb0 into symfony:master Apr 18, 2013

1 check passed

Details default Scrutinizer: No Comments — Travis: Passed
@fabpot fabpot added a commit that referenced this pull request Apr 18, 2013
@fabpot fabpot Revert "merged branch drak/start_on_demand (PR #7576)"
This reverts commit 7aa0681, reversing
changes made to 7bf8933.
5a3428d
@fabpot
Symfony member

Re-reading the comments on #6036 (#6036 (comment)), I'm not sure that this change is wise. So, I've reverted it for now.

@ghost Unknown added a commit that referenced this pull request Apr 18, 2013
Drak Revert "Revert "merged branch drak/start_on_demand (PR #7576)""
This reverts commit 5a3428d.
0758afc
@ghost Unknown added a commit that referenced this pull request Apr 20, 2013
Drak Revert "Revert "merged branch drak/start_on_demand (PR #7576)""
This reverts commit 5a3428d.
eaf3f75
@hackzilla hackzilla pushed a commit that referenced this pull request Dec 10, 2014
@fabpot fabpot merged branch drak/start_on_demand (PR #7576)
This PR was merged into the master branch.

Discussion
----------

[2.3][Session] Give greater control over how and when session starts

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | na
| License       | MIT
| Doc PR        | symfony/symfony-docs#2475

Refs #6036

Gives control over how start on demand works: allowing to turn it on or off and to allow bag access when session is off.

Commits
-------

f431cb0 Fix tests
1f521d8 Coding standards
2583c26 [HttpFoundation][FrameworkBundle] Keep save auto_start behaviour as in 2.2 and make component values consistent with FrameworkBundle's configuration options.
ceaf69b [FrameworkBundle] Use more sophisticated validation and configuration.
af0a140 [FrameworkBundle] Add configuration to allow control over session start on demand.
8fc2397 [HttpFoundation] Give control over how session start on demand.
c21c53a
@hackzilla hackzilla pushed a commit that referenced this pull request Dec 10, 2014
@fabpot fabpot Revert "merged branch drak/start_on_demand (PR #7576)"
This reverts commit 7aa0681, reversing
changes made to 7bf8933.
f7778ac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment