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.

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

_ #673

Closed
ghost opened this issue Aug 28, 2014 · 21 comments
Closed

_ #673

ghost opened this issue Aug 28, 2014 · 21 comments

Comments

@ghost
Copy link

ghost commented Aug 28, 2014

_

@boussou
Copy link

boussou commented Aug 28, 2014

I laready proposed this change (with a centralized function, maybe it would be better to use a class). refused.
(see my pull request)

@ikkez
Copy link
Collaborator

ikkez commented Aug 28, 2014

👍
seems like you are not the only one who had problems with that, as it was also mentioned in #628.
I vote for adding this enhancement.

@bcosca
Copy link
Owner

bcosca commented Sep 4, 2014

@stehlo It is possible that $_SESSION is empty because a session variable has not been defined yet, but session_start() may have already been invoked. !isset($_SESSION) and empty($_SESSION) will thus fail. Even a check on $_COOKIE[session_name()] is not reliable either.

@bcosca
Copy link
Owner

bcosca commented Sep 4, 2014

session_status() is the only way to go but it works only in PHP5.4.

@bcosca
Copy link
Owner

bcosca commented Sep 4, 2014

session_start();
// Do whatever needs to be done with the session
session_commit();
var_dump($_SESSION);
var_dump(session_id()); // session has ended?!
var_dump(session_status()===PHP_SESSION_ACTIVE);
var_dump($_COOKIE[session_name()]);
session_start(); // will not cause any error

@bcosca
Copy link
Owner

bcosca commented Sep 4, 2014

A session_id() still exists even when a session_commit() has already been issued, which makes this unreliable:

if (session_id()=='')
     session_start();

An empty $_SESSION is not also indicative of a non-existent session because a session variable may not have been stored yet, even when the session is already active.

@bcosca
Copy link
Owner

bcosca commented Sep 4, 2014

The @ suppression operator in the framework code is essentially a workaround on a pre-PHP 5.4 bug: https://bugs.php.net/bug.php?id=52982 which didn't have a userland function for determining the session state; an oversight by the PHP devs.

@boussou
Copy link

boussou commented Sep 4, 2014

A session_id() still exists even when a session_commit()

it is less important in my humble opinion. What we need is to start a session when necessary (lazily) & avoid multiple calls to session_start.

@ChrisFrench
Copy link

I don't think it is insane to move to php 5.4 we have all be running f3 on
php 5.5 for several months now, everything works swimmingly and you have
amazing features, of php auth methods, traits, etc.

On Thu, Sep 4, 2014 at 1:41 AM, boussou notifications@github.com wrote:

A session_id() still exists even when a session_commit()
it is less important in my humble opinion. What we need is to start a
session when necessary (lazily) & avoid multiple calls to session_start.


Reply to this email directly or view it on GitHub
#673 (comment).

@ikkez
Copy link
Collaborator

ikkez commented Sep 4, 2014

yeah php 5.4 is nice.
we still got a customer that run on php 5.2 😒
but i think most of the hosters and especially shared free hosters are still on 5.3.x
But having 5.4 as ground base would be nice. And the deep nested escaping only works 100% reliable for php >= 5.4 in Base->dupe method. So i would also recommend 5.4 for any bigger ambitious project.

@boussou: well that is indeed a good point.

@boussou
Copy link

boussou commented Sep 4, 2014

Frankly, there is no urge to stop beeing compatible with 5.3.
Who need traits?
I see only new array syntax & small perf improvement that would be worth doing it.

@ChrisFrench
Copy link

You kind of just contradicted yourself, but obviously I just said I use
traits, and you will probably too when you have the option.

php 5.3 is probably going to be standard for another year at least, so I
doubt f3 will move away from it.

On Sep 4, 2014 10:25 AM, "boussou" notifications@github.com wrote:

Frankly, there is no urge to stop beeing compatible with 5.3.
Who need traits?
I see only new array syntax & small perf improvement that would be worth
doing it.


Reply to this email directly or view it on GitHub
#673 (comment).

@bcosca
Copy link
Owner

bcosca commented Sep 5, 2014

Considering the end of PHP 5.3's lifecycle has been announced, we can start planning for development of a 5.4 branch and backport whatever is feasible.

@xfra35
Copy link
Collaborator

xfra35 commented Sep 8, 2014

In the meanwhile, maybe we could replace every call to @session_start() with $f3->session_start() where the framework session_start() function would look like:

function session_start() {
  if (PHP_VERSION_ID < 50400 ? session_id() == '' : session_status() == PHP_SESSION_NONE)
    session_start();
}

@ikkez
Copy link
Collaborator

ikkez commented Sep 8, 2014

👍

@eazuka
Copy link

eazuka commented Sep 14, 2014

👍 for >= PHP 5.4

@sgpinkus
Copy link

sgpinkus commented Jan 8, 2015

"cause premature interruption of this database operation, due to the error being thrown". @stehlo Can you explain further how exactly this causes an Error? Docs and my testing show multiple calls to session_start() causing non fatal E_NOTICE errors (unless of course you have PHP configured such that E_NOTICE is fatal), and returns true if a session exists or a new sessions is started, or false if not.

@sgpinkus
Copy link

sgpinkus commented Jan 8, 2015

@stehlo Your right I don't understand. Could you please explain the error the change you requested fixes more clearly? Your response is appreciated.

@bcosca
Copy link
Owner

bcosca commented Jan 8, 2015

Guys, let's keep this discussion civil.

This request has been considered, but it is tabled for version 4 of the framework due to the PHP5.4 feature that is known to settle this issue once and for all. The suggested fix in my opinion is really just a workaround due to PHP5.3's language deficiency.

@xfra35
Copy link
Collaborator

xfra35 commented Jan 8, 2015

@sam-at-github to answer your question, I can see that the error reporting changed from 3.3.0 to 3.4.0:

//3.3.0
error_reporting(E_ALL|E_STRICT);
//3.4.0
error_reporting((E_ALL|E_STRICT)&~E_NOTICE);

That probably explains the difference (E_NOTICE ignored in 3.4.0).

@Rayne
Copy link
Contributor

Rayne commented Sep 9, 2016

As @bcosca introduced PHP 5.4 features to fatfree-core this issue is now resolvable.

bcosca added a commit to f3-factory/fatfree-core that referenced this issue Sep 15, 2016
@bcosca bcosca closed this as completed Sep 15, 2016
@ghost ghost changed the title Serious issues caused by an attempt to start new session, when it has already been started _ Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants