[RFC][VOTE] User defined session serializer

8 years ago by Marco Pivetta — view source

unread

Hi Yasuo,

I voted "no" because I don't see any advantage over using a custom session
save handler, besides adding more API that partially covers custom session
save handlers.
This is just confusing, in my opinion, and the API can be replicated in
userland with a custom session save handler decorator instead.

Greets,

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

Hi all,

This RFC exposes session serializer interface to user space. It works
like user defined session save handler.

Users are able to encrypt/validate session data transparently. e.g.
You can save encrypted session data to database, decrypt encrypted
data from database transparently.

https://wiki.php.net/rfc/user_defined_session_serializer
Vote starts: 2016-12-05 Vote ends: 2016-12-19 UTC 23:59:59

Although we don't have consensus about number of votes to pass, I set
this RFC to require 2/3 votes.

Questions are welcomed if you have.
Thank you for voting.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi Marco,

Thank you for explaining the reason why!

I voted "no" because I don't see any advantage over using a custom session
save handler, besides adding more API that partially covers custom session
save handlers.

You mean current OO custom save handler, I suppose.

Firstly, current OO custom save handler design (use of previously used
internal save handler as its base class) is not good. Overriding
open()/close()/etc are useless, moreover harmful. Number of bugs
proved it is not good.

Secondly, the only use case, that current OO save handler design is
useful, is read()/write() override to encrypt/validate session data.

Thirdly, I fixed many of current OO API bugs, but fixing them all is
impossible by design.

Current OO save handler API is wrong in many ways and should be
deprecated and replaced by clean OO API as mentioned in the RFC.

Is there any good reasons to keep problematic API for good?

This is just confusing, in my opinion, and the API can be replicated in
userland with a custom session save handler decorator instead.

I may be too familiar to session module. I don't understand what's
confusing. Session module has 2 distinct submodules for saving and
serializing data. These have different tasks.

The RFC allows to save session data in whatever format. e.g.
XML/JSON/BSON/etc. This is impossible by save handler submodule.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

The RFC allows to save session data in whatever format. e.g.
XML/JSON/BSON/etc. This is impossible by save handler submodule.

I've never tried, but it would be possible technically.
The code wouldn't be clean code, though.

With this RFC, JSON session serializer could be

session_set_serializer(
function ($input_array) {
return json_encode($input_array);
},
function ($input_string) {
return json_decode($input_string);
}
);
// User may use "interface" alternatively.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Marco Pivetta — view source

unread

Hi Marco,

Thank you for explaining the reason why!

I voted "no" because I don't see any advantage over using a custom
session
save handler, besides adding more API that partially covers custom
session
save handlers.

You mean current OO custom save handler, I suppose.

Yes

Firstly, current OO custom save handler design (use of previously used
internal save handler as its base class) is not good. Overriding
open()/close()/etc are useless, moreover harmful. Number of bugs
proved it is not good.

Number of bugs? In which adapters? There are well tested session
savehandlers out there, and I'd trade a userland bug for an internals bug
anytime.

Secondly, the only use case, that current OO save handler design is
useful, is read()/write() override to encrypt/validate session data.

What does this RFC do that a custom session savehandler CANNOT?

Thirdly, I fixed many of current OO API bugs, but fixing them all is
impossible by design.

It would need a deprecation of the current session module, and a
replacement with a newer, cleaner API.

Current OO save handler API is wrong in many ways and should be
deprecated and replaced by clean OO API as mentioned in the RFC.

Is there any good reasons to keep problematic API for good?

There is a problem with adding a half-feature to fix half the problem, and
then having to deprecate this half-feature when the new full solution is
actually replacing it in a correct way.

This is just confusing, in my opinion, and the API can be replicated in
userland with a custom session save handler decorator instead.

I may be too familiar to session module. I don't understand what's
confusing. Session module has 2 distinct submodules for saving and
serializing data. These have different tasks.

It is confusing that we now have two ways of passing down serializers for
sessions.
The Redis session savehandlers already use igbinary where applicable, for
example, so why do I need to have a second equivalent API? Providing a
userland base session savehandler with pluggable serializers (written in
PHP, not in C, please!) is a much better idea.

The RFC allows to save session data in whatever format. e.g.

XML/JSON/BSON/etc. This is impossible by save handler submodule

Savehandlers even save to flat DB tables, so I'm not understanding what is
being explained here...

Even in the docs, the first example shows how to add encryption to a custom
savehandler: https://secure.php.net/manual/en/class.sessionhandler.php

Adding a custom serialization is as simple as an unserialize() + custom
serializer call (Yes, I can already hear all those microseconds screaming!
The horror!)

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

8 years ago by Yasuo Ohgaki — view source

unread

Hi Marco,

Firstly, current OO custom save handler design (use of previously used
internal save handler as its base class) is not good. Overriding
open()/close()/etc are useless, moreover harmful. Number of bugs
proved it is not good.

Number of bugs? In which adapters? There are well tested session
savehandlers out there, and I'd trade a userland bug for an internals bug
anytime.

If you search bug db by "session crash"

https://bugs.php.net/search.php?search_for=session+crash&boolean=0&limit=30&order_by=&direction=DESC&cmd=display&status=Closed&bug_type=All&project=All&php_os=&phpver=&cve_id=&assign=&author_email=&bug_age=0&bug_updated=0

You'll find many save handler bug reports.

Secondly, the only use case, that current OO save handler design is
useful, is read()/write() override to encrypt/validate session data.

What does this RFC do that a custom session savehandler CANNOT?

Currently, users cannot use arbitrary serialization.
Dirty hack (which is inefficient also) is required to do this.

Thirdly, I fixed many of current OO API bugs, but fixing them all is
impossible by design.

It would need a deprecation of the current session module, and a replacement
with a newer, cleaner API.

Although some APIs need improvements, but basic module design is good.
If you have a design improvement idea, I appreciate it.

Current OO save handler API is wrong in many ways and should be
deprecated and replaced by clean OO API as mentioned in the RFC.

Is there any good reasons to keep problematic API for good?

There is a problem with adding a half-feature to fix half the problem, and
then having to deprecate this half-feature when the new full solution is
actually replacing it in a correct way.

Correct way would be

deprecate problematic current OO API which uses previously defined
save handler as base object.
implement new OO API which simply uses user defined handler.

BTW, this is not a topic of this RFC, but it's future scope.
Please don't mixed up.

This RFC provides mandatory feature that customize session data
representation. i.e. serialization format, encrypt/decrypt,
validation, etc.

This is just confusing, in my opinion, and the API can be replicated in
userland with a custom session save handler decorator instead.

I may be too familiar to session module. I don't understand what's
confusing. Session module has 2 distinct submodules for saving and
serializing data. These have different tasks.

It is confusing that we now have two ways of passing down serializers for
sessions.
The Redis session savehandlers already use igbinary where applicable, for
example, so why do I need to have a second equivalent API? Providing a
userland base session savehandler with pluggable serializers (written in
PHP, not in C, please!) is a much better idea.

C module could do anything, but user script cannot.
Please don't mixed up C code and user script.

This RFC generalizes "serialization" and provide interface to user
script which C module cannot.

(Since C submodule can do anything, so it's possible that save
handlers to provide their own serializer interface/API. However,
what's good about this? Session module defines serializer as submodule
already. We don't need module A provide A serializer interface, module
B provides B serializer interface. It's just a mess and confusing.)

The RFC allows to save session data in whatever format. e.g.
XML/JSON/BSON/etc. This is impossible by save handler submodule

Savehandlers even save to flat DB tables, so I'm not understanding what is
being explained here...

Even in the docs, the first example shows how to add encryption to a custom
savehandler: https://secure.php.net/manual/en/class.sessionhandler.php

Adding a custom serialization is as simple as an unserialize() + custom
serializer call (Yes, I can already hear all those microseconds screaming!
The horror!)

It seems you misunderstood this RFC.
I have to say, you are wrong and your statement is FUD.

Try to write your own serializer with current API by yourself.
If you could write clean one like this,

session_set_serializer(
function ($input_array) {
return json_encode($input_array);
},
function ($input_string) {
return json_decode($input_string);
}
);
// User may use "interface" alternatively.
// Optionally, encryption/validation can be with these functions.

please show us.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Tony Marston — view source

unread

"Yasuo Ohgaki" wrote in message
news:CAGa2bXYaX05JbJAvyxfSJyy6xiA+4u14NPFGYwScL4aoOFQGhw@mail.gmail.com...

Hi Marco,

Thank you for explaining the reason why!

I voted "no" because I don't see any advantage over using a custom
session
save handler, besides adding more API that partially covers custom
session
save handlers.

You mean current OO custom save handler, I suppose.

Firstly, current OO custom save handler design (use of previously used
internal save handler as its base class) is not good. Overriding
open()/close()/etc are useless, moreover harmful. Number of bugs
proved it is not good.

I have been using session_set_save_handler() since 2002 to store all session
data in a database, and I have never encountered any problems. Why is it
"not good"? What bugs are there?

I do not see the point in this RFC.

--
Tony Marston

8 years ago by Yasuo Ohgaki — view source

unread

Hi Tony,

"Yasuo Ohgaki" wrote in message
news:CAGa2bXYaX05JbJAvyxfSJyy6xiA+4u14NPFGYwScL4aoOFQGhw@mail.gmail.com...

Hi Marco,

Thank you for explaining the reason why!

I voted "no" because I don't see any advantage over using a custom
session
save handler, besides adding more API that partially covers custom
session
save handlers.

You mean current OO custom save handler, I suppose.

Firstly, current OO custom save handler design (use of previously used
internal save handler as its base class) is not good. Overriding
open()/close()/etc are useless, moreover harmful. Number of bugs
proved it is not good.

I have been using session_set_save_handler() since 2002 to store all session
data in a database, and I have never encountered any problems. Why is it
"not good"? What bugs are there?

It works if it is used correctly. However, we've got number of bug
reports for abuse/misuse cases. Refer to bugs.php.net for details.

I do not see the point in this RFC.

Did you read the RFC?
It enables to write serializer by PHP script, clean and simple. It
cannot be done by save handler.

Besides, the reason why we don't have user defined serializer is
"register_globals", the reason why we have problematic base class for
OO API save handler is "register_globals". We should get rid of
obsolete features at some point also.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

I do not see the point in this RFC.

Did you read the RFC?
It enables to write serializer by PHP script, clean and simple. It
cannot be done by save handler.

Besides, the reason why we don't have user defined serializer is
"register_globals", the reason why we have problematic base class for
OO API save handler is "register_globals". We should get rid of
obsolete features at some point also.

I thought I made it clear what the point of this RFC.

"Expose serializer interface to userspace"

but it seems I'm failed to do so.

Those who don't understand point of this RFC, please try to write
JSON/XML serializer with current APIs. You should be able to
understand what's the issue.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi voters,

Following people are vote against this RFC for now.

bwoebi (bwoebi)
danack (danack)
hywan (hywan)
leigh (leigh)
levim (levim)
nikic (nikic)
ocramius (ocramius)
peehaa (peehaa)
ryat (ryat)

I suppose bwoebi and levim vote against due to error and exception
usage. Thank you for feedback, Levi and Bob.

How many of you vote "no" because of error and exception issue?

IMHO. Exception in session module is out of scope of RFC, mixing
error and exception in a module is confusing and inconsistent. Session
module is not language engine, so I'm not 100% sure if we should use
TypeError exception for normal module's invalid return type. However,
I don't mind much to use exception if many of us insist.
(BTW, "interface" parameter issue is out of scope this RFC also. It
should be addressed by OO API cleanup RFC consistently. I'm trying to
resolve issues one by one.)

I wonder how many of you understand this RFC is really a
"register_globals" legacy cleanup? Do you really think we should keep
"register_globals" legacy because of irrelevant issue?

Thank you for feedback!
Unless there are feedbacks, nobody cannot figure out what's the reason
behind for "no" votes.

Regards,

--
Yasuo Ohgaki

8 years ago by Bob Weinand — view source

unread

Am 07.12.2016 um 21:49 schrieb Yasuo Ohgaki yohgaki@ohgaki.net:

Hi voters,

Following people are vote against this RFC for now.

bwoebi (bwoebi)
danack (danack)
hywan (hywan)
leigh (leigh)
levim (levim)
nikic (nikic)
ocramius (ocramius)
peehaa (peehaa)
ryat (ryat)

I suppose bwoebi and levim vote against due to error and exception
usage. Thank you for feedback, Levi and Bob.

How many of you vote "no" because of error and exception issue?

IMHO. Exception in session module is out of scope of RFC, mixing
error and exception in a module is confusing and inconsistent. Session
module is not language engine, so I'm not 100% sure if we should use
TypeError exception for normal module's invalid return type. However,
I don't mind much to use exception if many of us insist.
(BTW, "interface" parameter issue is out of scope this RFC also. It
should be addressed by OO API cleanup RFC consistently. I'm trying to
resolve issues one by one.)

I wonder how many of you understand this RFC is really a
"register_globals" legacy cleanup? Do you really think we should keep
"register_globals" legacy because of irrelevant issue?

Thank you for feedback!
Unless there are feedbacks, nobody cannot figure out what's the reason
behind for "no" votes.

Regards,

--
Yasuo Ohgaki

Exceptions is one thing.

That's why I initially voted no.

Though thinking more about it, I still do not see the need.

session_set_save_handler() ultimately can just do the job completely.
Encryption? save_handler can do that.
Saving to somewhere else? It can do that.

Note that your RFC is not a replacement. Especially the last thing, saving to somewhere else, which isn't a file, cannot be prevented without it.

If there were:
save(string $uuid, array $data)
read(string $uuid): array $data

That'd make much more sense. Having to do flock() + file_get_contents() / file_put_contents() yourself to sys_get_temp_dir()."/$uuid" is not a problem in case you want to save it yourself.
Perhaps we should provide session_default_write(string $data)/session_default_read(string $data). I do not care too much.

And how is that related to register_globals?!

If this RFC were a proper replacement with also a proposed deprecation plan (e.g. in 7.4 (or 8.0) whatever comes first), that'd be a plus too. But currently it looks like we're heading to just have yet another separate system for sessions.

Bob

8 years ago by Bob Weinand — view source

unread

Am 07.12.2016 um 21:07 schrieb Yasuo Ohgaki yohgaki@ohgaki.net:

Hi Tony,

"Yasuo Ohgaki" wrote in message
news:CAGa2bXYaX05JbJAvyxfSJyy6xiA+4u14NPFGYwScL4aoOFQGhw@mail.gmail.com...

Hi Marco,

Thank you for explaining the reason why!

I voted "no" because I don't see any advantage over using a custom
session
save handler, besides adding more API that partially covers custom
session
save handlers.

You mean current OO custom save handler, I suppose.

Firstly, current OO custom save handler design (use of previously used
internal save handler as its base class) is not good. Overriding
open()/close()/etc are useless, moreover harmful. Number of bugs
proved it is not good.

I have been using session_set_save_handler() since 2002 to store all session
data in a database, and I have never encountered any problems. Why is it
"not good"? What bugs are there?

It works if it is used correctly. However, we've got number of bug
reports for abuse/misuse cases. Refer to bugs.php.net http://bugs.php.net/ for details.

I do not see the point in this RFC.

Did you read the RFC?
It enables to write serializer by PHP script, clean and simple. It
cannot be done by save handler.

We can just directly read from $_SESSION (in write) and use session_encode() (for returning on read()) as a workaround.
It isn't the cleanest way, but it works.

I appreciate a cleaner way, but it really should (see the last mail) cover everything, including the need to store to different locations.

Besides, the reason why we don't have user defined serializer is
"register_globals", the reason why we have problematic base class for
OO API save handler is "register_globals". We should get rid of
obsolete features at some point also.

Could you please that explain further?
Also cleaning up obsolete features would be nice, rather sooner than later.

Bob

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net mailto:yohgaki@ohgaki.net

--

<http://www.php.net/unsub.php

8 years ago by Yasuo Ohgaki — view source

unread

Hi Bob,

Thank you for feedback!

We can just directly read from $_SESSION (in write) and use session_encode()
(for returning on read()) as a workaround.
It isn't the cleanest way, but it works.

I appreciate a cleaner way, but it really should (see the last mail) cover
everything, including the need to store to different locations.

Besides, the reason why we don't have user defined serializer is
"register_globals", the reason why we have problematic base class for
OO API save handler is "register_globals". We should get rid of
obsolete features at some point also.

Could you please that explain further?
Also cleaning up obsolete features would be nice, rather sooner than later.

It's been years ago, so I barely remember the current OO save handler
API implementation discussion. If anyone remember the discussion,
please correct me if I'm wrong.

IIRC, when current OO API was introduced, user defined serializer was
considered also. However, serializers had to consider register_globals
compliance that registers session variable as a global vars via
session_register(). Unlike $_SESSION/$HTTP_SESSION_VARS,
session_register()ed global vars are marked as session variable even
when they are undefined. Serializers (php and php_binary) saves these
undefined global session vars with PS_UNDEF_MAKER/PS_BIN_UNDEF
and restores them. i.e. Session variables are designed to work like
other GPC "register_global"ed vars at first.

Note: I recently noticed that there are remained codes for global
session var support, so I created PR to remove them.
https://github.com/php/php-src/pull/2233

Because of PS_UNDEF_MARKER(undefined global session vars), it was too
easy for users to implement broken serializers at that time. Instead
of exposing serializer submodule to users, OO API was made to override
session read()/write() for encryption/etc. (I completely forgot about
this, but I remembered this when I discussed about this with Stas
about a year(?) ago) There were discussion about overriding
open()/close(). Broken open()/close() override would cause problems,
but overriding was allowed for consistency sake, open()/close()
override is allowed because it might be useful for some cases.

IMO, this design was reasonable choice at that time. Thankfully, we
removed register_globals support altogether. The choice became
obsolete. i.e. We can use normal serialization like "php_serialize"
serializer as well as JSON/XML/etc serialization.

We can comply original session module design now, serializer submodule
to convert/restore session data for saving/from retrieved data, save
handlers submodule to save/retrieve session data from db.

We may keep current OO API as long as users are using it, but I'll
propose new/clean/complete OO API before 7.2. i.e. No base object and
full method interface by a single interface definition - there are 3
interfaces now.

BTW, the main RFC objective is to allow "User defined serializer" as
RFC title states. Session data "encryption/validation/modification" is
subproduct.

Please note that users cannot write clean/efficient user defined
serializer without this RFC.

I hope I explained the background well.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Please note that users cannot write clean/efficient user defined
serializer without this RFC.

It may be better to explain real working code.

Since I added "php_serialize" to recent PHP, user defined
serialization with current API became a lot simpler, but you'll see
this hack is inefficient. i.e. There are needless serializations as
well as needless handler calls.

https://gist.github.com/yohgaki/432579e535ae97856a1227e4d47d0e2e

Efficient and clean code by this RFC is written as comment below.
In addition, it works both OO and procedural API.

Note: Since 7.0, updateTimeStamp feature does not work with OO API.
This will result in a lot slower session performance for session
database over network. i.e. Memcached/Redis/session_pgsql/etc. This is
a bug though. I'll fix this by new OO API.
http://lxr.php.net/xref/PHP-7.1/ext/session/tests/bug71162.phpt

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Markus Fischer — view source

unread

Hi,

This RFC exposes session serializer interface to user space. It works
like user defined session save handler.

Users are able to encrypt/validate session data transparently. e.g.
You can save encrypted session data to database, decrypt encrypted
data from database transparently.

https://wiki.php.net/rfc/user_defined_session_serializer
Vote starts: 2016-12-05 Vote ends: 2016-12-19 UTC 23:59:59

Although we don't have consensus about number of votes to pass, I set
this RFC to require 2/3 votes.

thanks for the RFC. I voted "yes", because I see the value in having the
format of the serialization being separated from how sessions are
loaded/stored.

thanks,

Markus

8 years ago by Levi Morrison — view source

unread

Hi all,

This RFC exposes session serializer interface to user space. It works
like user defined session save handler.

Users are able to encrypt/validate session data transparently. e.g.
You can save encrypted session data to database, decrypt encrypted
data from database transparently.

https://wiki.php.net/rfc/user_defined_session_serializer
Vote starts: 2016-12-05 Vote ends: 2016-12-19 UTC 23:59:59

Although we don't have consensus about number of votes to pass, I set
this RFC to require 2/3 votes.

Questions are welcomed if you have.
Thank you for voting.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

--

I apologize for the late review of your RFC. This RFC has a few issues
which I think need to be fixed (yes, this means I am asking you to
pull it out of voting phase).

It does not specify how to indicate an encoding or decoding error. As
these are unexpected and userland code may throw exceptions anyway I
am fine with an exception being used here (as opposed to returning
null). However in its current form it is incomplete.

The RFC says this about the return value of the encoding function:

Returning anything other than string raises `E_RECOVERABLE_ERROR`

And similarly this for the decoding function:

Returning anything other than array raises E_RECOVERABLE_ERROR

These are not consistent with return type checking. This must reuse
the error checking that is already in-place and not provide something
new to accomplish the same thing.

8 years ago by Yasuo Ohgaki — view source

unread

Hi Levi,

I apologize for the late review of your RFC. This RFC has a few issues
which I think need to be fixed (yes, this means I am asking you to
pull it out of voting phase).

It does not specify how to indicate an encoding or decoding error. As
these are unexpected and userland code may throw exceptions anyway I
am fine with an exception being used here (as opposed to returning
null). However in its current form it is incomplete.

The RFC says this about the return value of the encoding function:
Returning anything other than string raises `E_RECOVERABLE_ERROR` 
And similarly this for the decoding function:

Returning anything other than array raises E_RECOVERABLE_ERROR

These are not consistent with return type checking. This must reuse
the error checking that is already in-place and not provide something
new to accomplish the same thing.

What do you mean by inconsistent?

Encode function must return "string" for successful operation.
Decode function must return "array" for successful operation.

Anything other than these return types are invalid and should fail
always. Making "false" error condition and treat other invalid types
differently? This does not make sense.

What do you think missing here?
What is your proposal?

Thank you.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Levi Morrison — view source

unread

These are not consistent with return type checking. This must reuse
the error checking that is already in-place and not provide something
new to accomplish the same thing.

What do you mean by inconsistent?

We do not use E_RECOVERABLE_ERROR for return type mismatches; we use
TypeError. That is the inconsistency.

8 years ago by Yasuo Ohgaki — view source

unread

Hi Levi,

These are not consistent with return type checking. This must reuse
the error checking that is already in-place and not provide something
new to accomplish the same thing.

What do you mean by inconsistent?

We do not use E_RECOVERABLE_ERROR for return type mismatches; we use
TypeError. That is the inconsistency.

It depends. Session module uses errors rather than exceptions. Mixing
them is inconsistent for a module.

I wrote I prefer exceptions and I'll handle error handling issue
(error -> exception) with other RFC during discussion. So don't worry
about it for now.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Levi Morrison — view source

unread

Hi Levi,

These are not consistent with return type checking. This must reuse
the error checking that is already in-place and not provide something
new to accomplish the same thing.

What do you mean by inconsistent?

We do not use E_RECOVERABLE_ERROR for return type mismatches; we use
TypeError. That is the inconsistency.

It depends. Session module uses errors rather than exceptions. Mixing
them is inconsistent for a module.

I wrote I prefer exceptions and I'll handle error handling issue
(error -> exception) with other RFC during discussion. So don't worry
about it for now.

Currently all return types use TypeError when they do not match. Do
not introduce anything different; we do not need any more special
cases than we already have.

8 years ago by Yasuo Ohgaki — view source

unread

Hi Levi,

Hi Levi,

These are not consistent with return type checking. This must reuse
the error checking that is already in-place and not provide something
new to accomplish the same thing.

What do you mean by inconsistent?

We do not use E_RECOVERABLE_ERROR for return type mismatches; we use
TypeError. That is the inconsistency.

It depends. Session module uses errors rather than exceptions. Mixing
them is inconsistent for a module.

I wrote I prefer exceptions and I'll handle error handling issue
(error -> exception) with other RFC during discussion. So don't worry
about it for now.

Currently all return types use TypeError when they do not match. Do
not introduce anything different; we do not need any more special
cases than we already have.

Error handling is out of scope of this RFC. It will be handled by other RFC.

Others requested "minimal" and "single scope" RFC to me. I don't mind
much to replace error to exception, since I'm going to do this anyway.

Currently all return types use TypeError when they do not match.

It's irrelevant for this RFC, but it seems not many features uses
TypeError now. It seems only Zend is using TypeError.

$ find . -name '*.c' | xargs grep -n zend_ce_type_error
./Zend/zend_closures.c:330:
zend_throw_exception_ex(zend_ce_type_error, 0, "Failed to create
closure from callable: %s", error);
./Zend/zend_closures.c:333:
zend_throw_exception_ex(zend_ce_type_error, 0, "Failed to create
closure from callable");
./Zend/zend_API.c:256: zend_throw_error(zend_ce_type_error, "%s%s%s()
expects parameter %d to be a valid callback, %s",
./Zend/zend_exceptions.c:38:ZEND_API zend_class_entry *zend_ce_type_error;
./Zend/zend_exceptions.c:715: if ((Z_OBJCE_P(exception) ==
zend_ce_type_error || Z_OBJCE_P(exception) ==
zend_ce_argument_count_error) && strstr(ZSTR_VAL(message), ", called
in ")) {
./Zend/zend_exceptions.c:877: zend_ce_type_error =
zend_register_internal_class_ex(&ce, zend_ce_error);
./Zend/zend_exceptions.c:878: zend_ce_type_error->create_object =
zend_default_exception_new;
./Zend/zend_exceptions.c:881: zend_ce_argument_count_error =
zend_register_internal_class_ex(&ce, zend_ce_type_error);
./Zend/zend.c:1363: zend_throw_exception(zend_ce_type_error, message, 0);
./Zend/zend.c:1376: zend_throw_exception(zend_ce_type_error, message, 0);

I'm willing to adopt exceptions. Not only Zend, but also SPL defines
exceptions. I think we are better to have general guideline how we
should use exceptions to avoid needless exception usage inconsistency.
This is out of scope of this RFC clearly.

e.g. Invalid encode()/decode() return type could be
spl_ce_RuntimeException instead of zend_ce_type_error.
zend_ce_type_error could be reserved only for Zend, for example.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

This RFC exposes session serializer interface to user space. It works
like user defined session save handler.

Users are able to encrypt/validate session data transparently. e.g.
You can save encrypted session data to database, decrypt encrypted
data from database transparently.

https://wiki.php.net/rfc/user_defined_session_serializer
Vote starts: 2016-12-05 Vote ends: 2016-12-19 UTC 23:59:59

Although we don't have consensus about number of votes to pass, I set
this RFC to require 2/3 votes.

Questions are welcomed if you have.
Thank you for voting.

Since there is misunderstanding of this RFC, I'll explain some
backgrounds and related topics.

What is session serializer?

Session serializer serializes $_SESSION into string format, vice
versa. Session serializer is implemented submodule and can have any
kind of serialization method as long as it can convert array to string
(encode), string to array(decode).

Currently, we can only choose internally defined (C written)
serializers, php_serialize (recommended), php (default) or php_binary,
via php.ini setting.

What this RFC patch does?

This patch opens serializer implementation to userspace. Users can
write serialization method as PHP script. i.e. Users can use any
serialization method, JSON/XML/etc.

Since serializer handles array to string, string to array conversion,
user may perform additional tasks such as validation, encryption,
add/remove transparent data for session management, etc by PHP script.

Why serializer submodule was not exposed to user space until now?

This is because of "register_globals". "register_globals" feature
required "special serialization format" like php/php_binary
serializers. We don't have "register_globals" now, so serializer can
be exposed to users.

Save handler (OO API) can encrypt/decrypt. Why this is needed?

First of all, the main objective of this RFC is to "open serializer
implementation to user script". Without this patch, it requires
dirty/inefficient hack to implement user defined serializer.

Save handler should perform saving data, not converting session data
into other format. This is the intended design.
Conversion(serialization) is serializer's task. Session module
provides procedural API also. This is mandatory for procedural API.

Why current OO API save handler should be deprecated?

Current OO save handler API uses previously defined save handler, i.e.
internal C written save handler sub module, as it's base object. Since
C written save handlers are not expecting to be called by users
abusive/wrong manner, we had many bug reports for this. Overriding
open()/close()/etc is useless, moreover it's harmful.

I suppose this design is chosen because serializer interface cannot be
exposed to userspace due to "register_globals". Since we don't have
"register_globals" now, we are better to deprecate obsolete and
problematic API and replace it with new clean API.

What would be new OO API save handler?

New OO API save handler will be pure PHP script save handler.
Therefore, there will be no issues that old API had. i.e. No C written
save handler can be called from user script.

With new OO API, encrypting session data cannot be possible?

This is the one of the reason why this RFC is proposed. Encryption can
be done with this RFC, both OO and procedural API. Session data
conversion is serializer task in the first place. Encryption is better
to be performed by serializer.

Why it does not use exception for error?

Session module uses "errors", not "exceptions" basically. Session save
handler uses "errors" also. We should avoid inconsistency in a module.
Exception adoption will be addressed and exceptions will be used
consistent manner by other RFC.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Bob Weinand — view source

unread

Hey,

Am 06.12.2016 um 08:06 schrieb Yasuo Ohgaki yohgaki@ohgaki.net:

Hi all,

snip

Why it does not use exception for error?

Session module uses "errors", not "exceptions" basically. Session save
handler uses "errors" also. We should avoid inconsistency in a module.
Exception adoption will be addressed and exceptions will be used
consistent manner by other RFC.

This is not session module specific, your interface is specifying a return type. Thus this should be enforced on the interface implementation level. (I.e. it will be impossible for an user to define a function not having a return type of string/array)

Also, if a wrong argument is passed to a function, it should throw a type error. Perhaps old modules from pre-PHP 5 do emit warnings or recoverables, but we then shall transition from that to TypeError, and definitely use TypeError in new functionality.

Bob

8 years ago by Yasuo Ohgaki — view source

unread

Hi Bob,

Am 06.12.2016 um 08:06 schrieb Yasuo Ohgaki yohgaki@ohgaki.net:

Hi all,

snip

Why it does not use exception for error?

Session module uses "errors", not "exceptions" basically. Session save
handler uses "errors" also. We should avoid inconsistency in a module.
Exception adoption will be addressed and exceptions will be used
consistent manner by other RFC.

This is not session module specific, your interface is specifying a return
type. Thus this should be enforced on the interface implementation level.
(I.e. it will be impossible for an user to define a function not having a
return type of string/array)

Also, if a wrong argument is passed to a function, it should throw a type
error. Perhaps old modules from pre-PHP 5 do emit warnings or recoverables,
but we then shall transition from that to TypeError, and definitely use
TypeError in new functionality.

If we use TypeError for this, session will be the first module that
uses TypeError in module. It may be better to use TypeError with
caution.

Let's have a RFC for exception usage guideline. There isn't clear
consensus how we should use exceptions now. When we have consensus, we
follow the guideline. For the time being, let exception usage is out
of this RFC scope

Since this RFC is targeting master (7.2), we have enough time for
exception adoption.

P.S. I don't like mixing errors and exceptions for submodules,
serialize and save handler. Please note that save handlers raise
"error" for invalid return types currently. If a module should adopt
exception, all applicable errors should be changed to exception at
once, and patch work should be avoided. IMO.

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

This RFC exposes session serializer interface to user space. It works
like user defined session save handler.

Users are able to encrypt/validate session data transparently. e.g.
You can save encrypted session data to database, decrypt encrypted
data from database transparently.

https://wiki.php.net/rfc/user_defined_session_serializer
Vote starts: 2016-12-05 Vote ends: 2016-12-19 UTC 23:59:59

Although we don't have consensus about number of votes to pass, I set
this RFC to require 2/3 votes.

Questions are welcomed if you have.
Thank you for voting.

This patch is simple register_globals legacy fix for me, i.e. If there
wasn't register_globals, there would be user defined serializer already[1]
Users cannot implement their own serialization clean and efficient
way without this patch.[2]

Vote is 9 in favor and 9 against currently.
https://wiki.php.net/rfc/user_defined_session_serializer#proposed_voting_choices
It is surprise for me that this many people against the RFC.

bwoebi (bwoebi)
danack (danack)
hywan (hywan)
leigh (leigh)
levim (levim)
nikic (nikic)
ocramius (ocramius)
peehaa (peehaa)
ryat (ryat)

Denial of this RFC will result in keeping obsolete/unclean design.

Macro thinks we are ok with obsolete[3]/problematic[4] design,
i.e. current OO API, because it works/enough for him. IMHO,
this isn't a reasonable reason for voting "no", but it's great to hear
the reason. I can point it out "it does not seem reasonable reason
for me" at least.

Bob and Levi seems strongly insisting use of TypeError rather than
error. It's irrelevant for this RFC.[5] IMHO.
We don't even have guideline nor RFC for exception usage currently.
Do you really think we should keep obsolete/unclean API for this
reason?

Other "no" voters do not state reason why.
What's the reason for against this RFC?

I'm not a perfectionist, so I don't mind much to keep obsolete,
unclean and incomplete[6] design, but I do think people who vote
against RFC must expose the reason. It's technical discussion
after all.

Thank you for your feedback!

Regards,

[1] The reason why user defined serializer is not exposed is
"register_globals compliance required special serialization format".
[2] Requires needless serializations, save handler calls. It's dirty hack
rather than API.
[3] OO API was added to address "user defined serializer is not
exposed" issue. Internal save handler as base object is obsolete
design now.
[4] We have/had number of crash bugs for hacky OO API design.
[5] Session module uses errors now. Exception adoption should be
done at once for a module. This requires other RFC.
[6] Procedural API cannot encrypt/validate/serialize session data.

P.S.
I should have realize sooner that the new function name should be
session_set_serialize_handler() rather than session_set_serilizer().
session_set_serialize_handler() is consistent with INI setting name
and save handler feature.
Does this change require vote restart?

--
Yasuo Ohgaki
yohgaki@ohgaki.net

8 years ago by Yasuo Ohgaki — view source

unread

Hi all,

This RFC exposes session serializer interface to user space. It works
like user defined session save handler.

Users are able to encrypt/validate session data transparently. e.g.
You can save encrypted session data to database, decrypt encrypted
data from database transparently.

https://wiki.php.net/rfc/user_defined_session_serializer
Vote starts: 2016-12-05 Vote ends: 2016-12-19 UTC 23:59:59

Although we don't have consensus about number of votes to pass, I set
this RFC to require 2/3 votes.

Questions are welcomed if you have.
Thank you for voting.

This RFC is declined by 9 in favor vs 10 against.
I'm surprised by this result.

This result could be adding new PHP sadness entry. Refer to this
gist for the reason.
https://gist.github.com/yohgaki/432579e535ae97856a1227e4d47d0e2e

We must require people against RFC to explain/expose
the reason why, mustn't we? Why we must force users to
write inefficient and imprecise code?

Anyway, vote is vote.
I may address this issue again.

Thank you for voting folks!

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net