Hi Davey and all,
I was planning to fix session_start() behaviors by PHP 7.1, but I
forgot to do this completely. Partial fix is merged currently.
Following PR makes session_start() return FALSE when it cannot start
session always.
https://github.com/php/php-src/pull/2167
In short, this patch fixes number of session_start()'s insane behaviors.
Original session_start() is designed to continue execution as much as
it can. This design caused a lot of issues including number of crash
bugs. We've removed most issues caused by this design, but
session_start() behaves insane way. e.g. Return TRUE and initializes
$_SESSION array for useless session, improper error messages, memory
leak, etc. (Please verify phpt changes how this patch makes
session_start() behave sane way)
This fix may change app behavior. However, it changes behavior only
when there is useless session which is fatal anyway. Therefore, it
could be applied to PHP 7.1. IMO.
What do you think?
Regards,
P.S. Sorry for late important bug fix.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Davey,
I was planning to fix
session_start()behaviors by PHP 7.1, but I
forgot to do this completely. Partial fix is merged currently.Following PR makes
session_start()returnFALSEwhen it cannot start
session always.https://github.com/php/php-src/pull/2167
In short, this patch fixes number of
session_start()'s insane behaviors.Original
session_start()is designed to continue execution as much as
it can. This design caused a lot of issues including number of crash
bugs. We've removed most issues caused by this design, but
session_start()behaves insane way. e.g. ReturnTRUEand initializes
$_SESSION array for useless session, improper error messages, memory
leak, etc. (Please verify phpt changes how this patch makes
session_start()behave sane way)This fix may change app behavior. However, it changes behavior only
when there is useless session which is fatal anyway. Therefore, it
could be applied to PHP 7.1. IMO.What do you think?
Since session_start() is made to set proper session status finally,
many bugs and inconsistencies can be fixed altogether.
I pushed patch fixes number of nonsense/inconsistent session function
behaviors. The additional patch is pushed so that it's easy to cherry
pick minimum fixes. The last push is the additional fixes.
- Disallow nonsense function usage.
. Do not allow to change cookie parameters when it has no effects.
session_set_cookie_params()session_cache_limiter()
. Dn not allow to change INI parameters when it cannot be changed.
session_name()session_module_name()session_save_path()
session_set_save_handler()session_cache_expire()
. Do not allow parameter for void parameter functions.
session_unset()session_write_close()session_commit()
session_abort()session_reset() - Return function status as it should.
.session_abort()session_flush()session_commit()
session_write_close()session_reset()session_destroy() - Raise proper errors.
. Many functions.
Session module allows insane usage currently. This will fix most of them.
I need to address nonsense INI usage. There will be one more push at
least for this.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
I pushed patch fixes number of nonsense/inconsistent session function
behaviors. The additional patch is pushed so that it's easy to cherry
pick minimum fixes. The last push is the additional fixes.
These changes look like a reasonable cleanup. I'm not a big fan of
zend_parse_parameters_none additions, since I don't see any useful
function for them, but they don't hurt either. I left some notes on the
patch though, please look there.
I'm not sure about 7.1 - while I don't see anything that would break any
reasonable code, except issues noted on the pull, the patch is pretty
big and I might have missed something. I guess it's for RM to decide. No
objection for 7.2.
I notice that most test changes deal with scenarios that were covered
before, are there tests that test the new things - the scenarios for
which handling has changed? I couldn't find tests for some cases, like
INI modification, would be nice to add them.
Also, a reminder that you'd have to update all the documentation in the
manual to reflect the change in the return values. And UPGRADING too.
Stas Malyshev
smalyshev@gmail.com
On Tue, Oct 18, 2016 at 11:08 PM, Stanislav Malyshev smalyshev@gmail.com
wrote:
Hi!
I pushed patch fixes number of nonsense/inconsistent session function
behaviors. The additional patch is pushed so that it's easy to cherry
pick minimum fixes. The last push is the additional fixes.These changes look like a reasonable cleanup. I'm not a big fan of
zend_parse_parameters_none additions, since I don't see any useful
function for them, but they don't hurt either. I left some notes on the
patch though, please look there.I'm not sure about 7.1 - while I don't see anything that would break any
reasonable code, except issues noted on the pull, the patch is pretty
big and I might have missed something. I guess it's for RM to decide. No
objection for 7.2.I notice that most test changes deal with scenarios that were covered
before, are there tests that test the new things - the scenarios for
which handling has changed? I couldn't find tests for some cases, like
INI modification, would be nice to add them.Also, a reminder that you'd have to update all the documentation in the
manual to reflect the change in the return values. And UPGRADING too.
I was planning to fix
session_start()behaviors by PHP 7.1, but I
forgot to do this completely. Partial fix is merged currently.
Yasuo, assuming "partial fix" doesn't mean "broken fix" but instead "it
doesn't do everything I planned" then I do not want this in 7.1. As others
have pointed out, it's not a small change and sessions are a fundamental
part of PHP — we are simply too far into the release cycle to include this.
Please target 7.2/master.
Thanks,
- Davey
Hi Davey,
Yasuo, assuming "partial fix" doesn't mean "broken fix" but instead "it
doesn't do everything I planned" then I do not want this in 7.1. As others
have pointed out, it's not a small change and sessions are a fundamental
part of PHP — we are simply too far into the release cycle to include this.Please target 7.2/master.
OK. I'll merge this to master.
Partial fix is done so that it does not break internal API signatures.
It's fixed as plain bug fix. PHP 7.1's session_start() sets a little
better session status and 'FALSE' than older PHP, but it's far from
correct/reliable. I'll update UPGRADING comment.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Stas,
I pushed patch fixes number of nonsense/inconsistent session function
behaviors. The additional patch is pushed so that it's easy to cherry
pick minimum fixes. The last push is the additional fixes.These changes look like a reasonable cleanup. I'm not a big fan of
zend_parse_parameters_none additions, since I don't see any useful
function for them, but they don't hurt either. I left some notes on the
patch though, please look there.I'm not sure about 7.1 - while I don't see anything that would break any
reasonable code, except issues noted on the pull, the patch is pretty
big and I might have missed something. I guess it's for RM to decide. No
objection for 7.2.I notice that most test changes deal with scenarios that were covered
before, are there tests that test the new things - the scenarios for
which handling has changed? I couldn't find tests for some cases, like
INI modification, would be nice to add them.Also, a reminder that you'd have to update all the documentation in the
manual to reflect the change in the return values. And UPGRADING too.
Thank you.
I replied to your comments.
There is one debatable comment regarding session_cache_limiter().
We may delay error check at 'headers sent'. I suppose almost all usage
is misuse.
Only valid use case is
<?php
ob_start();
session_start();
session_set_cache_limiter('public'); // <== Call this between
session_start() and session_regenerate_id()
session_regenerate_id();
?>
Other than this, all calls after session_start() is invalid. Valid use
case is very limited and unlikely. If users have to change cache
limiter, they can session_commit(), then session_start(). So I would
like to keep as it is now.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Stas,
Only valid use case is
<?php
ob_start();
session_start();
session_set_cache_limiter('public'); // <== Call this between
session_start()andsession_regenerate_id()
session_regenerate_id();
?>Other than this, all calls after
session_start()is invalid. Valid use
case is very limited and unlikely. If users have to change cache
limiter, they cansession_commit(), thensession_start(). So I would
like to keep as it is now.
I thought php_session_reset_id() sends all headers, but it sends only
session ID cookie header. So current code is good. Sorry for
confusion.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Davey,
I was planning to fix
session_start()behaviors by PHP 7.1, but I
forgot to do this completely. Partial fix is merged currently.Following PR makes
session_start()returnFALSEwhen it cannot start
session always.https://github.com/php/php-src/pull/2167
In short, this patch fixes number of
session_start()'s insane behaviors.
I think I've finished the patch for now.
There are many other places that I would like to improve/fix error
handlings. e.g. Session save handlers. This patch is too large
already. I'll fix them in master.
The patch could be divided into "session_start() error handling fix
only" and "other session core error handling issues". It may be good
to include "session_start() error handling fix only" for PHP 7.1, but
other fixes won't bother correct PHP scripts. Please note that
session_start() fix in PHP-7.1 and master branch is partial fix. I
suggest to merge "session_start() error handling fix only" at least,
but it's your choice.
Please let me know when you decide what to do or merge appropriate
patch from the PR.
BTW, I expect UnitTest failures, framework/etc, that test session
module functionalities. They have tests for failure cases. Even if
there are minor BC issues (UnitTests report error, bad code emit
errors), this fix must be done at some point anyway.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Yasuo,
Hi Davey,
I was planning to fix
session_start()behaviors by PHP 7.1, but I
forgot to do this completely. Partial fix is merged currently.Following PR makes
session_start()returnFALSEwhen it cannot start
session always.https://github.com/php/php-src/pull/2167
In short, this patch fixes number of
session_start()'s insane behaviors.I think I've finished the patch for now.
There are many other places that I would like to improve/fix error
handlings. e.g. Session save handlers. This patch is too large
already. I'll fix them in master.The patch could be divided into "session_start() error handling fix
only" and "other session core error handling issues". It may be good
to include "session_start() error handling fix only" for PHP 7.1, but
other fixes won't bother correct PHP scripts. Please note that
session_start()fix in PHP-7.1 and master branch is partial fix. I
suggest to merge "session_start() error handling fix only" at least,
but it's your choice.Please let me know when you decide what to do or merge appropriate
patch from the PR.BTW, I expect UnitTest failures, framework/etc, that test session
module functionalities. They have tests for failure cases. Even if
there are minor BC issues (UnitTests report error, bad code emit
errors), this fix must be done at some point anyway.
I like this change as it adds some cleanup and common sense to this
API. However 7.1 is in RC, this patch, while covering logical changes,
affect behaviors and is quite big (lot of similar changes but still).
I would feel much more comfortable to push it after 7.1 (as in, for
7.2 and later).
Cheers,
Pierre
@pierrejoye | http://www.libgd.org