# "php_serialize" session serialize handler - Thread: https://externals.io/message/68394 - Original archive: https://news-web.php.net/php.internals/68394 - Started by Yasuo Ohgaki on 2013-08-07T06:59:14+00:00 - Messages: 26 - Page 1 of 2: messages 1 to 25 (25 per page, add `?page=N` to the URL) - Next page: https://externals.io/message/68394.md?page=2 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #68394 — Yasuo Ohgaki — 2013-08-07T06:59:14+00:00 URL: https://externals.io/message/68394#68394 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 --- ## #68404 — Stas Malyshev — 2013-08-07T20:34:26+00:00 URL: https://externals.io/message/68394#68404 · In reply to: #68394 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 --- ## #68408 — Yasuo Ohgaki — 2013-08-08T00:58:11+00:00 URL: https://externals.io/message/68394#68408 · In reply to: #68404 Hi Stas, On Thu, Aug 8, 2013 at 5:34 AM, Stas Malyshev wrote: > 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 --- ## #68411 — Stas Malyshev — 2013-08-08T01:03:35+00:00 URL: https://externals.io/message/68394#68411 · In reply to: #68408 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 --- ## #68415 — Yasuo Ohgaki — 2013-08-08T01:41:43+00:00 URL: https://externals.io/message/68394#68415 · In reply to: #68411 Hi Stas On Thu, Aug 8, 2013 at 10:03 AM, Stas Malyshev wrote: > > […] > > 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. > > […] > > 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 --- ## #68416 — Stas Malyshev — 2013-08-08T02:03:54+00:00 URL: https://externals.io/message/68394#68416 · In reply to: #68415 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 --- ## #68417 — Yasuo Ohgaki — 2013-08-08T02:11:33+00:00 URL: https://externals.io/message/68394#68417 · In reply to: #68416 Hi Stas, On Thu, Aug 8, 2013 at 11:03 AM, Stas Malyshev wrote: > > […] > > 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 --- ## #68419 — Yasuo Ohgaki — 2013-08-08T02:23:33+00:00 URL: https://externals.io/message/68394#68419 · In reply to: #68417 Hi Stas, > On Thu, Aug 8, 2013 at 11:03 AM, Stas Malyshev wrote: > > > […] > > > 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 --- ## #68428 — Lester Caine — 2013-08-08T08:26:10+00:00 URL: https://externals.io/message/68394#68428 · In reply to: #68419 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 --- ## #68431 — Yasuo Ohgaki — 2013-08-08T10:09:18+00:00 URL: https://externals.io/message/68394#68431 · In reply to: #68428 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 --- ## #68433 — Leigh — 2013-08-08T10:53:33+00:00 URL: https://externals.io/message/68394#68433 Sorry I failed at reply to all. > Hi Leigh, > > > > > […] > > > 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. --- ## #68434 — Yasuo Ohgaki — 2013-08-08T11:05:26+00:00 URL: https://externals.io/message/68394#68434 · In reply to: #68433 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 --- ## #68436 — Leigh — 2013-08-08T11:11:53+00:00 URL: https://externals.io/message/68394#68436 · In reply to: #68434 > 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. --- ## #68437 — Lester Caine — 2013-08-08T12:10:39+00:00 URL: https://externals.io/message/68394#68437 · In reply to: #68436 Leigh wrote: > > > > […] > 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 ----------------------------- 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 --- ## #68456 — Yasuo Ohgaki — 2013-08-08T22:31:03+00:00 URL: https://externals.io/message/68394#68456 · In reply to: #68437 Hi, > Leigh wrote: > > […] > > 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 --- ## #68461 — Leigh — 2013-08-09T08:28:04+00:00 URL: https://externals.io/message/68394#68461 · In reply to: #68456 > > 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. --- ## #68462 — Yasuo Ohgaki — 2013-08-09T09:26:56+00:00 URL: https://externals.io/message/68394#68462 · In reply to: #68461 Hi Leigh, > > > > […] > > 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 --- ## #68463 — Lester Caine — 2013-08-09T09:29:52+00:00 URL: https://externals.io/message/68394#68463 · In reply to: #68461 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 ----------------------------- 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 --- ## #68464 — Yasuo Ohgaki — 2013-08-09T09:54:59+00:00 URL: https://externals.io/message/68394#68464 · In reply to: #68463 Hi Lester, > Leigh wrote: > > > […] > > 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 --- ## #68465 — Lester Caine — 2013-08-09T10:08:59+00:00 URL: https://externals.io/message/68394#68465 · In reply to: #68464 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 ' 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. --- ## #68467 — Stas Malyshev — 2013-08-09T16:53:05+00:00 URL: https://externals.io/message/68394#68467 · In reply to: #68464 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 --- ## #68469 — Yasuo Ohgaki — 2013-08-09T21:14:26+00:00 URL: https://externals.io/message/68394#68469 · In reply to: #68467 Hi Stas, On Sat, Aug 10, 2013 at 1:53 AM, Stas Malyshev wrote: > > […] > 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 > > […] > > 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". > > […] > users' > > […] > > 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 --- ## #68470 — Yasuo Ohgaki — 2013-08-09T21:27:43+00:00 URL: https://externals.io/message/68394#68470 · In reply to: #68467 Hi Stas, On Sat, Aug 10, 2013 at 1:53 AM, Stas Malyshev wrote: > 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 --- ## #68482 — Stas Malyshev — 2013-08-10T22:54:50+00:00 URL: https://externals.io/message/68394#68482 · In reply to: #68469 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: 1. Adding php_serializer option for session is fine for 5.5, please make a pull req for it. 2. 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 --- Next page: https://externals.io/message/68394.md?page=2