"php_serialize" session serialize handler

11 years ago by Yasuo Ohgaki — view source

unread

Hi all,

Current session module has a few limitations due to "register_globals"
support which is now obsolete. One of them is numeric key indexed session
data. i.e. $_SESSION[1] = $var is not allowed now and it raises error at
R_SHUTDOWN with useless message for debugging.

https://bugs.php.net/bug.php?id=65359

Following patch (diff against 5.5 branch) add php_serialize session
serialize handler and removes the limitation. To test patch, set
"session.serialize_handler = php_serialize" in php.ini file or set it via
ini_set().

https://gist.github.com/yohgaki/6171628
(All tests and my new test for new serializer passed with Valgrind)

It's possible to remove unneeded limitation from current serializers, but
it causes BC. (e.g. AFAIK, perl and python have serializer/unserializer for
PHP session data. If we simply remove limitation, session data decode may
fail.) Therefore, adding new serializer would be better.

Since it uses plain serialize(), users may simply unserialize() to inspect
session data if it is needed. Users may setup initial $_SESSION array from
serialize()ed data. i.e. session_decode($serialized_data) These would be
useful for some users.

I would like to add this new serializer to 5.5(optional) and
master(default).

Any comments?

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Stas Malyshev — view source

unread

Hi!

Current session module has a few limitations due to "register_globals"
support which is now obsolete. One of them is numeric key indexed session
data. i.e. $_SESSION[1] = $var is not allowed now and it raises error at
R_SHUTDOWN with useless message for debugging.

Why is it useful to do this? I don't see how using numerical indexes in
global namespace (and SESSION is one of global namespaces in PHP) is a
good idea. Could you explain the use case here?

This seems to be to be of a very limited use and introducing new
serializer format with all resulting BC problems and confusion doesn't
seem to be worth it.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

11 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

On Thu, Aug 8, 2013 at 5:34 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:

Why is it useful to do this? I don't see how using numerical indexes in
global namespace (and SESSION is one of global namespaces in PHP) is a
good idea. Could you explain the use case here?

Personally, I would not use numeric index.
However, users are expecting it to work and there is no way to raise
informative error message where is wrong. IIRC, there were description
about "register_globals" originated limitations in our documents, but it
seems it was gone already.

This seems to be to be of a very limited use and introducing new
serializer format with all resulting BC problems and confusion doesn't
seem to be worth it.

The limitation is come from register_globals and it has nasty problem.
i.e. Error occurred at unknown script line 0.
This kind of behavior is better to be removed and it's worth it. IMHO.

Most users don't care what serializer does and users who may have
BC issue can use old serializers anytime.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Stas Malyshev — view source

unread

Hi!

Personally, I would not use numeric index.
However, users are expecting it to work and there is no way to raise

I don't think it is reasonable to expect it to work, it never worked, it
never was documented to work and there's no real use case for it.

The limitation is come from register_globals and it has nasty problem.

I don't see how is it a nasty problem if there's actually no good reason
to do this. If there is a good reason to do it, please tell what it is.

Most users don't care what serializer does and users who may have
BC issue can use old serializers anytime.

Using old serializer is work which we would make users do for
improvement that practically nobody needs and that does not provide any
actual improvement that can not easily be achieved otherwise. Breaking
BC just because somebody somewhere needs an exotic feature that can
easily be worked around doesn't look like a good idea to me.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

11 years ago by Yasuo Ohgaki — view source

unread

Hi Stas

On Thu, Aug 8, 2013 at 10:03 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:

Personally, I would not use numeric index.
However, users are expecting it to work and there is no way to raise

I don't think it is reasonable to expect it to work, it never worked, it
never was documented to work and there's no real use case for it.

I also think this limitation was documented, but it seems it gone (or
I missed)

The limitation is come from register_globals and it has nasty problem.

I don't see how is it a nasty problem if there's actually no good reason
to do this. If there is a good reason to do it, please tell what it is.

PHP Notice: Unknown: Skipping numeric key 1 in Unknown on line 0
https://bugs.php.net/bug.php?id=65359

This error occurs at shutdown. Removing unneeded limitation is good reason
IMHO. If there is

$_SESSION[$key] = $var;

It's very hard to find what is and where is wrong.

Most users don't care what serializer does and users who may have
BC issue can use old serializers anytime.

Using old serializer is work which we would make users do for
improvement that practically nobody needs and that does not provide any
actual improvement that can not easily be achieved otherwise. Breaking
BC just because somebody somewhere needs an exotic feature that can
easily be worked around doesn't look like a good idea to me.

I should have mentioned that new serializer not only allow numeric index,
but
also allow special characters due to the implementation. i.e. ! and | may
be
used with the new serializer.

Removing unneeded limitations, rather than forcing them to users, is user
friendly and the way to go. IMHO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Stas Malyshev — view source

unread

Hi!

Removing unneeded limitations, rather than forcing them to users, is user
friendly and the way to go. IMHO.

If we wrote it from scratch, sure. But if we already have existing and
working one, having people to deal with migrating data and
incompatibilities that arise IMHO is not worth the additional benefit of
having session variable named "!", without which one can live quite well.
So I'm not against having option for new serializer that does it if
somebody needs it, but I think changing the default for this and the
disruption potentially caused by it is not a very good idea.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

11 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

On Thu, Aug 8, 2013 at 11:03 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:

Removing unneeded limitations, rather than forcing them to users, is user
friendly and the way to go. IMHO.

If we wrote it from scratch, sure. But if we already have existing and
working one, having people to deal with migrating data and
incompatibilities that arise IMHO is not worth the additional benefit of
having session variable named "!", without which one can live quite well.
So I'm not against having option for new serializer that does it if
somebody needs it, but I think changing the default for this and the
disruption potentially caused by it is not a very good idea.

I agree. Breaking existing apps w/o good reason must be avoided. Therefore,
I proposed that introduce new serializer for 5.5 as optional. Do you
agree to have new serializer for 5.6 as optional and make it default
for future releases?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

On Thu, Aug 8, 2013 at 11:03 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:

Removing unneeded limitations, rather than forcing them to users, is
user
friendly and the way to go. IMHO.

If we wrote it from scratch, sure. But if we already have existing and
working one, having people to deal with migrating data and
incompatibilities that arise IMHO is not worth the additional benefit of
having session variable named "!", without which one can live quite well.
So I'm not against having option for new serializer that does it if
somebody needs it, but I think changing the default for this and the
disruption potentially caused by it is not a very good idea.

I agree. Breaking existing apps w/o good reason must be avoided. Therefore,
I proposed that introduce new serializer for 5.5 as optional. Do you
agree to have new serializer for 5.6 as optional and make it default
for future releases?

Or make it optional for 5.5 and 5.6, write full documentation about
serializer
and make new serializer default for future releases.

It would work better. There would be enough time for users may have BC
issue. If they do, it would be simple task to adopt because it is plain
serialize().

What do you think?

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Lester Caine — view source

unread

Yasuo Ohgaki wrote:

Or make it optional for 5.5 and 5.6, write full documentation about
serializer
and make new serializer default for future releases.

It would work better. There would be enough time for users may have BC
issue. If they do, it would be simple task to adopt because it is plain
serialize().

What do you think?

The one thing that PHP used to have was consistency. Applications that were
designed on PHP4 worked on PHP5, and those developed on PHP5 could be made
backwards compatible. This allowed sites to run and run without any major
reworking. Then E_STRICT came along, and while it can be 'switched off',
ignoring it creates many more problems. At what point will these fixes simply be
dropped altogether, and the remaining legacy code HAS to be converted? If one
was starting again today would things be done the same way? Obviously not, but
this continual picking away at core functionality is a further distraction from
keeping sites working? I'm currently battling with owncloud which works
perfectly on Apache 2.2, but crashes without ANY error messages on Apache 2.4.
I've asked for help on the Apache list and basically been told to 'piss off' as
I often am here! But the people who keep pushing all these little changes 'just
to make things better' need to start helping fix the breaks. phpdocs is still
not producing good documentation for many of the namespace changes and other
extras included in PHP5.3 let alone 5.4, and now other changes are being
proposed that would require further rework on phpdocs and Reflections? The
current builds of phpdocs do not actually yet produce output that matches the
previous version following a complete rework of that which now requires many
days of work simply to understand the new code base :(

We need to help get the rest of the infrastructure working with PHP5.4 before
starting work on 5.6 ... let alone 5.5 ...

--
Lester Caine - G8HFL

Contact - http://lsces.co.uk/wiki/?page=contact
L.S.Caine Electronic Services - http://lsces.co.uk
EnquirySolve - http://enquirysolve.com/
Model Engineers Digital Workshop - http://medw.co.uk
Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

11 years ago by Yasuo Ohgaki — view source

unread

Hi Lester,

We need to help get the rest of the infrastructure working with PHP5.4
before starting work on 5.6 ... let alone 5.5 ...

Since 5.5 is just released, some fixes may be good to add.
However, I agree that released version is better to keep the
feature sets as it is released.

I'll just commit this to master branch and document it as 5.6
feature.

Any more comments?

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Leigh — view source

unread

Sorry I failed at reply to all.

Hi Leigh,

Are you keeping it as optional for 5.6 and not the default? If you're
making it default don't we need to vote on BC breaks?

I suppose affected users are other language users, but there are
codes that outside of our control. I'm not going to remove old
serializers. BC issue is matter of php.ini setting for them.

If many of us feel vote is needed, I don't mind at all writing RFC.

Well a change to default behaviour is a BC break. In my opinion its fine to
add a new serialise handler, but I'm not sure if it should be default
behaviour.

11 years ago by Yasuo Ohgaki — view source

unread

Hi Leigh,

but I'm not sure if it should be default behaviour.

What's the point of having old serialzers that is bonded to
"register_globals"?
This kind of change should have done when 5.4.0 was released, IMO.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Leigh — view source

unread

What's the point of having old serialzers that is bonded to
"register_globals"?
This kind of change should have done when 5.4.0 was released, IMO.

Because somebodies code will depend on functionality not changing, and we
shouldn't break their code without a very good reason.

I can't judge the scope of the break, or how many users will be affected. I
just want to say that breaks without a good reason shouldn't happen.

11 years ago by Lester Caine — view source

unread

Leigh wrote:

What's the point of having old serialzers that is bonded to
"register_globals"?
This kind of change should have done when 5.4.0 was released, IMO.

Because somebodies code will depend on functionality not changing, and we
shouldn't break their code without a very good reason.

I can't judge the scope of the break, or how many users will be affected. I
just want to say that breaks without a good reason shouldn't happen.

Seconded.

In hindsight I personally would have preferred to remain with clean PHP5.2 code
and should have carried on with that. I've wasted many many months reworking
everything for PHP5.4 and so I'm now at a stage where my real answer is probably
"Do what you like, I'll stick with and maintain PHP5.4 now". The problem is that
ISP's like Go Daddy will still switch versions without any regard to users as
they have just done with Apache 2.4 so we all end up having to live with other
peoples decisions :( I still have sites where 'register_globals' is on simply
because I've not had any time to rework perfectly functional systems and I can
avoid anybody else changing the infrastructure! Nobody will pay for changing
those sites ...

--
Lester Caine - G8HFL

11 years ago by Yasuo Ohgaki — view source

unread

Hi,

Leigh wrote:

I can't judge the scope of the break, or how many users will be affected.
I
just want to say that breaks without a good reason shouldn't happen.

Seconded.

In hindsight I personally would have preferred to remain with clean PHP5.2
code and should have carried on with that. I've wasted many many months
reworking everything for PHP5.4 and so I'm now at a stage where my real
answer is probably "Do what you like, I'll stick with and maintain PHP5.4
now". The problem is that ISP's like Go Daddy will still switch versions
without any regard to users as they have just done with Apache 2.4 so we
all end up having to live with other peoples decisions :( I still have
sites where 'register_globals' is on simply because I've not had any time
to rework perfectly functional systems and I can avoid anybody else
changing the infrastructure! Nobody will pay for changing those sites ...

I think you are confused.
How php_serialize would cause BC issues for PHP users?

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Leigh — view source

unread

How php_serialize would cause BC issues for PHP users?

Not everyone uses PHP in the way you would expect. Just how many sites out
there do you think use PHPs session functionality? I'd go for hundreds of
millions, and that's a pretty big target to hit.

If you session_encode() something on 5.x with default settings and 5.x+1
cannot session_decode() it with default settings, that is a BC break.

11 years ago by Yasuo Ohgaki — view source

unread

Hi Leigh,

How php_serialize would cause BC issues for PHP users?

Not everyone uses PHP in the way you would expect. Just how many sites out
there do you think use PHPs session functionality? I'd go for hundreds of
millions, and that's a pretty big target to hit.

If you session_encode() something on 5.x with default settings and 5.x+1
cannot session_decode() it with default settings, that is a BC break.

Mixing versions with shared data should be carefully handled for almost all
apps.
Even if this is the case, users may use old serializer so no BC.

Anyway, we also have to consider number of bug/request reports that it
solves.
There are countless bug reports that were closed as "won't fix"/"not a bug"
because of the register_globals support in session module.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Lester Caine — view source

unread

Leigh wrote:

How php_serialize would cause BC issues for PHP users?
Not everyone uses PHP in the way you would expect. Just how many sites out there
do you think use PHPs session functionality? I'd go for hundreds of millions,
and that's a pretty big target to hit.

If you session_encode() something on 5.x with default settings and 5.x+1 cannot
session_decode() it with default settings, that is a BC break.

My own concerns with much of this are due to third party libraries using some
'new approach' which requires a particular setup such as changing a default, but
another library still requires the old way of working. THAT is the problem.

--
Lester Caine - G8HFL

11 years ago by Yasuo Ohgaki — view source

unread

Hi Lester,

Leigh wrote:
How php_serialize would cause BC issues for PHP users?
Not everyone uses PHP in the way you would expect. Just how many sites
out there
do you think use PHPs session functionality? I'd go for hundreds of
millions,
and that's a pretty big target to hit.

If you session_encode() something on 5.x with default settings and 5.x+1
cannot
session_decode() it with default settings, that is a BC break.
My own concerns with much of this are due to third party libraries using
some 'new approach' which requires a particular setup such as changing a
default, but another library still requires the old way of working. THAT is
the problem.

Mixing versions with shared data should be carefully handled for almost
all apps.
Even if this is the case, users may use old serializer so no BC.

Anyway, we also have to consider number of bug/request reports that it
solves.
There are countless bug reports that were closed as "won't fix"/"not a
bug"
because of the register_globals support in session module.

As I wrote, there is no real BC issue as it's just a matter of ini setting.

It's sounded like denial of improvement and removal of misdesigned
component.
I'm not going to remove old one, though. Search bug db, there are many
bug reports/feature requests that this patch solves.

If users aren't reading release note of minor version up, then it is users'
fault,
not ours. If it is, we cannot release new PHP.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Lester Caine — view source

unread

Yasuo Ohgaki wrote:

If users aren't reading release note of minor version up, then it is users' fault,
not ours. If it is, we cannot release new PHP.

MANY users don't even know that their hosting has been changed until their sites
stop working ... This happens a lot more often now than it ever used to!
Silly little things like '<?=' stopping working took a lot of time to address
because the 'problem' was not easy to spot. Blaming the users is simply not
acceptable!

--
Lester Caine - G8HFL

11 years ago by Levi Morrison — view source

unread

Blaming the users is simply not acceptable!

That actually depends on the organization's focus. I, for one, will gladly
blame users when they don't care when their software is deprecated. Using
deprecated software is their risk, not ours, especially when they are not
keeping in close contact with their hosting providers.

11 years ago by Stas Malyshev — view source

unread

Hi!

As I wrote, there is no real BC issue as it's just a matter of ini setting.

There is a BC issue if you need an ini setting. You keep dismissing it
but if it affected the app your business relates on it'd be a major
problem if on upgrade your app breaks and you need to scour release
notes to figure out all settings you need to change for each upgrade. So
we try to minimize such cases.
In some instances, it's unavoidable - but in this case, the support for
numerics in _SESSION is an exotic feature that will be of little use for
99% of PHP users. So introducing additional upgrade headaches for this
is IMHO unnecessary.

I'm not going to remove old one, though. Search bug db, there are many
bug reports/feature requests that this patch solves.

How many bug reports are there requesting numeric index support in session?

If users aren't reading release note of minor version up, then it is users'
fault,
not ours. If it is, we cannot release new PHP.

We can. We can also take some responsibility for making it easier to
upgrade and not just say "we had it all in the fine print, it's your own
fault for not reading everything". Vendors that do that are usually
disliked by their customers.

--
Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

11 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

On Sat, Aug 10, 2013 at 1:53 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:

As I wrote, there is no real BC issue as it's just a matter of ini
setting.

There is a BC issue if you need an ini setting. You keep dismissing it
but if it affected the app your business relates on it'd be a major
problem if on upgrade your app breaks and you need to scour release
notes to figure out all settings you need to change for each upgrade. So
we try to minimize such cases.
In some instances, it's unavoidable - but in this case, the support for
numerics in _SESSION is an exotic feature that will be of little use for
99% of PHP users. So introducing additional upgrade headaches for this
is IMHO unnecessary.

I think we are not talking about newbie users. Users who share session data
among web servers are advanced users and they would not have problem
reading release notes before upgrading.

Most of users don't have care at all. If there is old session data, it's
just discarded.

I'm not going to remove old one, though. Search bug db, there are many

bug reports/feature requests that this patch solves.

How many bug reports are there requesting numeric index support in session?

It's one type of bug report that relates to this patch. There types
bug/requests.
Most of them are old one I suppose. IIRC, there were

Session fails to save if it contains special chars
Want unserialize session from session data, not from $_SESSION
Need function generates initial session data via dession_decode
Session should use plain serialize

These are occasionally pops up and closed as "not a bug"/"wont fix". I
remember
since I was one of them closed as "not a bug"/"wont fix".

If users aren't reading release note of minor version up, then it is
users'
fault,
not ours. If it is, we cannot release new PHP.

We can. We can also take some responsibility for making it easier to
upgrade and not just say "we had it all in the fine print, it's your own
fault for not reading everything". Vendors that do that are usually
disliked by their customers.

Even if there are users do not reading release notes, it does not break apps
as it creates new session data.

Users don't have clue, they would not have problem.
Users may have affected, they should be able to read release notes.
There wouldn't be upgrade headache.

We don't want to keep misdesigned component due to "register_globals",
don't we? Especially when the solution does not have real BC issue.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Stas Malyshev — view source

unread

Hi!

Session fails to save if it contains special chars

Session data or session variable names? If the former, it is a strange
bug but certainly needs fixing. If it's the latter, it is the same
exotic issue, which I suspect very small number of people actually have.
Still, I'm ok with adding option to support them, as long as it does not
make problems for the majority.

Want unserialize session from session data, not from $_SESSION

Need function generates initial session data via dession_decode

These functions can be added without changing session format, but the
mere existence of these requests shows us that

Session should use plain serialize

This is not a valid request, there's nothing mandating session using
plain serialize.

We don't want to keep misdesigned component due to "register_globals",
don't we? Especially when the solution does not have real BC issue.

We're walking in circles here. I have a number of times existence of the
BC issue, yet you plainly dismiss it as "not real", and continue to talk
about register_globals, which has nothing to do with it. So, to not
prolong pointless discussion, here's my opinion:

Adding php_serializer option for session is fine for 5.5, please make
a pull req for it.
Making it default - as long as it is not backwards-compatible - is
not OK, at least before PHP 6. If you feel strongly that it must be
done, the vote must be held on this subject since this change has BC
implications.

With this, I think I'm done discussing this subject.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

11 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

On Sun, Aug 11, 2013 at 7:54 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:

Session fails to save if it contains special chars

Session data or session variable names? If the former, it is a strange
bug but certainly needs fixing. If it's the latter, it is the same
exotic issue, which I suspect very small number of people actually have.
Still, I'm ok with adding option to support them, as long as it does not
make problems for the majority.

IIRC. It is name.

Want unserialize session from session data, not from $_SESSION

Need function generates initial session data via dession_decode

These functions can be added without changing session format, but the
mere existence of these requests shows us that

Session should use plain serialize

This is not a valid request, there's nothing mandating session using
plain serialize.

This kind of request were closed as bogus/won't fix since there were
register_globals. We can make the serializer, but it's not worth it due
to the limitations at least for me.

We don't want to keep misdesigned component due to "register_globals",
don't we? Especially when the solution does not have real BC issue.

We're walking in circles here. I have a number of times existence of the
BC issue, yet you plainly dismiss it as "not real", and continue to talk
about register_globals, which has nothing to do with it. So, to not
prolong pointless discussion, here's my opinion:

Adding php_serializer option for session is fine for 5.5, please make
a pull req for it.

Making it default - as long as it is not backwards-compatible - is
not OK, at least before PHP 6. If you feel strongly that it must be
done, the vote must be held on this subject since this change has BC
implications.

With this, I think I'm done discussing this subject.

OK. I'm fine with this, I'll send PR later with tests.
I still don't understand why we should care so much, instead of
removing "some error occurred in some file line 0", though.

Anyway, this would be end of discussion and I'm glad that
I can close some of bugs/requests assigned to me.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

11 years ago by Yasuo Ohgaki — view source

unread

Hi Stas,

On Sat, Aug 10, 2013 at 1:53 AM, Stas Malyshev smalyshev@sugarcrm.comwrote:

We can also take some responsibility for making it easier to
upgrade

That's the reason why I proposed adding "php_serialize" to 5.5.
Even if there are very few users may have problem with new serializer,
it would be good to provide enough time to play with it.
Adding "php_serialize" does not affect users at all unless they explicitly
use it.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.netHi

"php_serialize" session serialize handler

This seems to be to be of a very limited use and introducing new serializer format with all resulting BC problems and confusion doesn't seem to be worth it.

-- Lester Caine - G8HFL

-- Lester Caine - G8HFL

-- Lester Caine - G8HFL

-- Lester Caine - G8HFL

With this, I think I'm done discussing this subject.

This seems to be to be of a very limited use and introducing new
serializer format with all resulting BC problems and confusion doesn't
seem to be worth it.

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL

--
Lester Caine - G8HFL