8000 [WIP][HttpFoundation] Trap fatal configuration condition. · Pull Request #4255 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[WIP][HttpFoundation] Trap fatal configuration condition. #4255

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
Closed
wants to merge 2 commits into from
Closed

[WIP][HttpFoundation] Trap fatal configuration condition. #4255

wants to merge 2 commits into from

Conversation

ghost
Copy link
@ghost ghost commented May 11, 2012

Bug fix: no
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: -
Todo: -
License of the code: MIT

It seems three is an unexpected behaviour in PHP - but basically the ini directive session.auto_start actually starts a session the instant PHP initialises as opposed to "on-demand" when PHP actually needs a session.

This means two things:

  1. There is no point in setting this ini directive in userland since if it's set to 1 the session will have already started, and if it was 0, then setting it to 1 will have zero effect.
  2. To use Symfony2 sessions, we require to start the session ourselves, so if the directive is set to 1 we cannot use the session.

It seems like this runtime exception is wise. Fortunately, the default in php.ini is for auto_start = 0.

[HttpFoundation] Trap fatal configuration condition.
7411e0f 
session.auto_start actually starts a session the instant PHP starts
using the php.ini defined session.save_handler and thus makes the
entire PHP process incompatible with Symfony2 sessions.

The setting also appears to be useless if set at runtime also.
@travisbot
Copy link

This pull request passes (merged 7411e0f into 554e073).

@vicb
Copy link
Contributor
vicb commented May 11, 2012

To use Symfony2 sessions, we require to start the session ourselves, so if the directive is set to 1 we cannot use the session.

Why ? What would prevent us from using the Session ? (Not speaking about the current code but technically ?)

@Seldaek
Copy link
Member
Seldaek commented May 11, 2012

I'm really not sure this is a good idea. It would be better as part of the config.php/check.php scripts IMO, to give you a warning that your system may be misconfigured, but enforcing it like that may put people in trouble if they can not disable the option.

@ghost
Copy link
Author
ghost commented May 11, 2012

@vicd - once a session has started it means PHP has bound itself to the session handlers (whatever was defined at PHP init - usually files).

@Seldaek - in Symfony2 Full Stack framework world maybe it's OK, but as a standalone component you may not have a checks.php. The fact is, if this (rare) condition is met, it is a runtime error. The point is quite simple, if auto_start = 1 the session will have been started and PHP will not be using Symfony2 sessions - that is a clear runtime exception.

@Seldaek
Copy link
Member
Seldaek commented May 11, 2012

@Drak what about if you detect that doing session_write_close()? Then configuring it and calling session_start() should do the right thing no?

@vicb
Copy link
Contributor
vicb commented May 11, 2012

@Drak (I am vicb) And ? Are you saying that the config should be moved to php.ini ? (related to #4208).

@ghost
Copy link
Author
ghost commented May 11, 2012

@vicb - It's not related to #4208. What I am saying is that session.auto_start does something completely different. Rather than starting a session on demand, it starts a session when PHP starts-up (before userland code starts execution). It means session.auto_start basically has no effect beyond php.ini even though you can set it during runtime (making it somewhat a bug since the scope should not be PHP_INI_ALL - but that's a PHP core issue).

@Seldaek Maybe - I'll switch this to [WIP] status ad see, but it might be a bit too hacky since we're talking about environment.

@ghost
Copy link
Author
ghost commented May 11, 2012

For reference, FrameworkBundle session auto_start and the session.auto_start configurations are different.

@vicb
Copy link
Contributor
vicb commented May 11, 2012

@Drak I think it is related - if the handler configuration is done in php.ini (rather than in the native handlers) then the Session component could use the PHP session no matter if it has already been (session.auto_)started.

(PHP_INI_ALL implies PHP_INI_USER which allows updates from PHP code (ini_set()) and windows registry. There seems to be no way in PHP to allow updates from windows registry but not from code)

@ghost
Copy link
Author
ghost commented May 11, 2012

Session component could use the PHP session no matter if it has already been (session.auto_)started.

You cannot configure custom session handlers from php.ini, it can only be done in user-land after PHP has started. session.auto_start has nothing to do with configuring the sessions, it simply starts a session before PHP runs any of our code.

You can still/already configure native save handlers from php.ini and the Symfony2 handlers do not overwrite those settings. Look at the constructors of the Native*Handler classes which protects the session.save_path from being overwritten when $path=null.

This particular class performs housekeeping/management around the sessions data. Ini directives are not forced by default, except two values (use_cookies and cache_limiter which are sensible defaults). We give complete freedom to the developer as everything is explicitly set by the dev, not Symfony2 - this was discussed at length in some FrameworkBundle issues/PRs.

[HttpFoundation] Remove references to session.auto_start
a52faea 
This code has no relevence as session.auto_start's effect is pre
userland runtime.
@travisbot
Copy link

This pull request passes (merged a52faea into 554e073).

@vicb
Copy link
Contributor
vicb commented May 11, 2012

You cannot configure custom session handlers from php.ini

We agree here, the linked PR is about native handlers which is why it is related

session.auto_start has nothing to do with configuring the sessions

When speaking about configuration, I mean the settings mentioned in the related PR (and not session.auto_start)

Coming back to my initial question: Is there anything that technically prevent supporting Sf sessions when auto_start is on ? (This PR proposes not to support this case).

@ghost
Copy link
Author
ghost commented May 11, 2012

Is there anything that technically prevent supporting Sf sessions when auto_start is on ? (This PR proposes not to support this case).

Symfony sessions have never supported this case and technically it's challenging (read not possible).

Overall, this PR does not change anything - it simply highlights a condition where PHP would invisibly not use the save handler you have configured. I had assumed the setting would auto start sessions on demand - that would make sense and that is what I had coded for.

@mvrhov
Copy link
mvrhov commented May 11, 2012

I would not throw an exception but try to log an notice.

@ghost ghost closed this May 13, 2012
@vicb
Copy link
Contributor
vicb commented May 14, 2012

@Drak what do you mean by "read not possible" when PHP sessions are auto-started ?

@vicb
Copy link
Contributor
vicb commented May 14, 2012

What we do today (when using a native handler)

  1. When the "session" service is accessed:
    • configuration (ini_set())
    • start the session (session_start())
    • use the session

What should work (related to 4208)

  1. Configure the session (php.ini)
  2. When the "session" service is accessed:
    • start the session (session_start())
    • use the session

Then why the following can't work:

  1. Configure the session (php.ini)
  2. Auto start a session on incoming request (auto_start = 1)
  3. When the "session" service is accessed:
    • start the Sf2 session
    • use the session

@ghost
Copy link
Author
ghost commented May 15, 2012

Here is what I have been talking about https://bugs.php.net/bug.php?id=62015

Let's say you are using the PDO handler - by the time Symfony2 is ready to use the session it's already using the internal files handler. session.auto_start is absolutely 100% incompatible and the code should throw a runtime exception. Currently I have it as a PHP notice, but this is wrong, it should be an exception.

Basically, the scope of session.auto_start is going to be changed so it cannot be set after runtime (PHP_INI_ALL) because it should never have been set at this level and cannot work at runtime.

@fabpot
Copy link
Member
fabpot commented May 15, 2012

@Drak: Why did you close this PR if you think this is the way to go?

@vicb
Copy link
Contributor
vicb commented May 15, 2012

@Drak I should have been more explicit to make it clear: I was speaking using auto_start with native handlers (which is what the related PR is about). It is of course not possible to use a custom handler as they are configured by code (session_set_save_handler()) and it is not possible to execute code before the session is started when auto_start is enabled.

However if a native handler is configured in the ini file (as in the related PR) there should be nothing preventing to the use of auto_start = 1.

Changing PHP_INI_ALL will prevent windows users to update the setti 8000 ngs in the registry - still it is a good change not to be allowed to change it from a ini_set()

@ghost
Copy link
Author
ghost commented May 15, 2012

@vicb - Yes indeed, but anything that starts the session except $session->start() may not necessarily work as intended (although I've handles as many edge cases as possible).

I believe session.auto_start is a real relic from the old days of PHP when was more about convenience - there is clearly no good present day use case or excuse not to use session_start() (in any general application). I really see no reason to even try to support it at all (which is impossible to do reliably).

The PHP.ini default is also off, which is a clear indication that it is not a sensible default any more. Interestingly even Zend_Session expressly forbids session.auto_start to be active as well as forbids the use of session_start(). Presumably they also have similar objections.

I further assert that if one is using Sf2 HttpFoundation in the first place, one can surely expect sessions to be used as we have documented. As documented we dont use session_regenerate_id(), session_start(), session_id() or session_name(), but instead use our APIs - this ensures uniform behaviour in all circumstances, including during testing.

@vicb
Copy link
Contributor
vicb commented May 15, 2012

Yes indeed, but anything that starts the session except $session->start() may not necessarily work as intended (although I've handles as many edge cases as possible).

Could you detail what may not work as intended ?

If supporting autostart is only a matter of wrapping session_start() in and if{}, I see not reason why we could not do that.
(1) This would make sense only if 4208 gets merged. (2) We should also raise an exception in the fmkBundle config if autostart is on and a custom handler is configured. (3) I wouldn't use autostart = 1 but if there is no reason why we could not support it, let's support it

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@travisbot @vicb @Seldaek @mvrhov @fabpot
0