Hi all,
I would like to propose removal of SessionHandler object. PHP7 is perfect
opportunity.
Session module uses "previous save handler" as it's base class of
SessionHandler object.
e.g.
ini_set('session.save_handler','files);
$handler = new SessionHandler; // files save handler functions are used as
base class.
I guess original intention was to extend C written save handlers. However,
this feature is
unusable unless user script has access to PS(mod_data). PS(mod_data) is
initialized by following data structures.
files handler (mod_files.c)
typedef struct {
char *lastkey;
char *basedir;
size_t basedir_len;
size_t dirdepth;
size_t st_size;
int filemode;
int fd;
} ps_files;
ps_module ps_mod_files = {
PS_MOD_SID(files)
};
mm handler (mod_mm.c)
typedef struct ps_sd {
struct ps_sd next;
php_uint32 hv; / hash value of key /
time_t ctime; / time of last change */
void data;
size_t datalen; / amount of valid data /
size_t alloclen; / amount of allocated memory for data /
char key[1]; / inline key */
} ps_sd;
typedef struct {
MM *mm;
ps_sd **hash;
php_uint32 hash_max;
php_uint32 hash_cnt;
pid_t owner;
} ps_mm;
Note: PS(mod_data) is void pointer and save handler use it to store
whatever data required for the handler. All handlers have different
structures.
User script must have access the struct(PS(mod_data)) to extend base class.
However, user script does not have access to internal C struct, obviously.
PS(mod_data) differs handler by handler, so it is not feasible to provide
uniform access API.
In conclusion, SessionHandler object has no real use except as a toy and
makes session module complex unnecessary. Since it cannot have real use,
nobody is using it. Therefore, I would like to remove it.
Any comments?
Regards,
P.S. I would like to discuss Session Handler Interface cleanup also. I'll
create new thread for it later.
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
User script must have access the struct(PS(mod_data)) to extend base class.
Can't I extend the base class and then do something in overriding
methods and call parent, or override some methods but not others and
thus have the original methods still work just fine?
In conclusion, SessionHandler object has no real use except as a toy and
makes session module complex unnecessary. Since it cannot have real use,
nobody is using it. Therefore, I would like to remove it.
I'm not sure, what you actually are proposing to remove? I'm not sure
how you can "remove an object". Could you explain further?
--
Stas Malyshev
smalyshev@gmail.com
Hi Stas,
On Fri, Jan 23, 2015 at 2:00 PM, Stanislav Malyshev smalyshev@gmail.com
wrote:
User script must have access the struct(PS(mod_data)) to extend base
class.Can't I extend the base class and then do something in overriding
methods and call parent, or override some methods but not others and
thus have the original methods still work just fine?
User may extend SessionHandler class like
class MySession extends SessionHandler {}
but user cannot extend base class(SessionHandler) capability because user
script
cannot access to PS(mod_data).
Better way to define user defined session class is to use
SessionHandlerInterface*.
I would like to discuss interface cleanup, but in different thread.
In conclusion, SessionHandler object has no real use except as a toy and
makes session module complex unnecessary. Since it cannot have real use,
nobody is using it. Therefore, I would like to remove it.I'm not sure, what you actually are proposing to remove? I'm not sure
how you can "remove an object". Could you explain further?
I should have written "remove the SessionHandler class". As I wrote, user
may
extend SessionHandler class.
However, defining user session class as
class MySession extends SessionHandler {}
have no merit. Users must implement required methods to be useful.
Interface is there for this purpose.
I would like to remove SessionHandler class, but I don't object to have
empty
SessionHandler class for maximum compatibility.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
User may extend SessionHandler class like
class MySession extends SessionHandler {}
but user cannot extend base class(SessionHandler) capability because
user script
cannot access to PS(mod_data).
Not sure what you mean by that. Absence of access to PS(mod_data)
certainly doesn't mean the user can not extend it's function, e.g. the
manual demonstrates EncryptedSessionHandler - is there something wrong
with that? Of course, there can be many more examples of it.
I should have written "remove the SessionHandler class". As I wrote,
user may
extend SessionHandler class.
I don't think this is a good idea. There are valid uses of this, and
there's code using it. Removing it does not achieve any goal as far as I
can see.
However, defining user session class as
class MySession extends SessionHandler {}
have no merit. Users must implement required methods to be useful.
I'm not sure why you say that - if they don't implement any methods,
wouldn't that class work exactly as the native handler? Isn't the native
handler useful?
--
Stas Malyshev
smalyshev@gmail.com
Hi,
I agree that the low-level details of different session handlers makes
the SessionHandler class a bit weird. However, I disagree that it is
useless.
We've discussed this before and I want to re-iterate my suggestion to
simply provide a separate class for each underlying save_handler, like
FilesSessionHandler, MemcacheSessionHandler, etc.
Cheers,
Andrey.
Hi Stas,
On Fri, Jan 23, 2015 at 3:47 PM, Stanislav Malyshev smalyshev@gmail.com
wrote:
User may extend SessionHandler class like
class MySession extends SessionHandler {}
but user cannot extend base class(SessionHandler) capability because
user script
cannot access to PS(mod_data).Not sure what you mean by that. Absence of access to PS(mod_data)
certainly doesn't mean the user can not extend it's function, e.g. the
manual demonstrates EncryptedSessionHandler - is there something wrong
with that? Of course, there can be many more examples of it.
This is the only reasonable use I know. I would to write user
serializer(read/writer)
handler for it.
User serializer is much powerful. Users may implement JSON/BSON/YAML/XML/
whatever serialization methods, along with encrypt/decrypt,
compress/uncompress
session data.
NOTE: Session module always serialize session data with serializer, user
cannot
implement JSON/BSON/etc without native serializer.
My point is SessionHandler class is (almost) useless as a CLASS. It
requires
implementing ALL required functions by user. There is INTERFACE for class
that requires to implement all methods.
I should have written "remove the SessionHandler class". As I wrote,
user may
extend SessionHandler class.I don't think this is a good idea. There are valid uses of this, and
there's code using it. Removing it does not achieve any goal as far as I
can see.
Goal is removing abused class usage.(cleanup) e.g. Refer to
PHP_FUNCTION(session_set_save_handler).
Advocate OOP best practice. i.e. Use INTERFACE when user is required to
implement all required methods.
And provide good API (both internal/user). e.g. Missing functions like user
serializer, gc, create_id.
Some cleanups have done in my last PR, but there are more cleanups to do.
However, defining user session class as
class MySession extends SessionHandler {}
have no merit. Users must implement required methods to be useful.I'm not sure why you say that - if they don't implement any methods,
wouldn't that class work exactly as the native handler? Isn't the native
handler useful?
If user need native handler, they should use it directly. IMO.
Anyway, it seems it's better to propose user serializer first, then
SessionHandler
removal. (I would like to cleanup things first, then add new features one
by one though.
Session module can be simpler with a little compatibility sacrifice.)
I would like to discuss one by one. Shall we discuss user serializer? What
do you think?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
This is the only reasonable use I know. I would to write user
serializer(read/writer)
handler for it.
So we went from no reasonable use to one reasonable use, documented at
the manual. I think it is also reasonable to suppose there are more uses
for it.
My point is SessionHandler class is (almost) useless as a CLASS. It
I think we established it is not useless. It may not be useful for you,
but may be useful for other people. Creating a large BC break for a
reason that something is not useable for someone is not a good idea, I
think. I do not see any benefit such action would produce.
Goal is removing abused class usage.(cleanup) e.g. Refer to
The action should not be its own goal, and I don't see how it's
"abused". So far we have seen an example of use which you recognized as
reasonable, and which is endorsed by our own manual,, and no examples of
abuse.
PHP_FUNCTION(session_set_save_handler).
Advocate OOP best practice. i.e. Use INTERFACE when user is required to
implement all required methods.
There's no OOP best practice that says you can not extend classes and
override some of its methods in order to modify parent's behavior. On
the contrary, doing this is one of common OOP practices.
And provide good API (both internal/user). e.g. Missing functions like
user serializer, gc, create_id.
If you want to provide additional API, by all means please make an RFC.
But I do not see how removing this class is required for providing any API.
If user need native handler, they should use it directly. IMO.
I do not see any reason for that. You can clearly use native handler and
add modifications for it. In fact, that is exactly why that class exists.
I would like to discuss one by one. Shall we discuss user serializer?
What do you think?
Again, if you want to propose new feature, by all means please do. I
still don't see how it requires removing existing classes. This is a
very big BC break and requires very big gain to justify, and so far the
gain was not shown I think.
--
Stas Malyshev
smalyshev@gmail.com
Hi Stas,
On Sat, Jan 24, 2015 at 8:49 AM, Stanislav Malyshev smalyshev@gmail.com
wrote:
This is the only reasonable use I know. I would to write user
serializer(read/writer)
handler for it.So we went from no reasonable use to one reasonable use, documented at
the manual. I think it is also reasonable to suppose there are more uses
for it.
This is because session module lacks user defined serializer. Save handler
handles session data storage. Serialize handler handles how data is
converted/represented. IMHO.
Basic save handler design is good while it is bad not to provide user
serialize
handler.
My point is SessionHandler class is (almost) useless as a CLASS. It
I think we established it is not useless. It may not be useful for you,
but may be useful for other people. Creating a large BC break for a
reason that something is not useable for someone is not a good idea, I
think. I do not see any benefit such action would produce.Goal is removing abused class usage.(cleanup) e.g. Refer to
The action should not be its own goal, and I don't see how it's
"abused". So far we have seen an example of use which you recognized as
reasonable, and which is endorsed by our own manual,, and no examples of
abuse.
I've mentioned the code, PHP_FUNCTION(session_set_save_handler).
/* For compatibility reason, implemeted interface is not checked /
/ Find implemented methods - SessionHandlerInterface */
i = 0;
ZEND_HASH_FOREACH_STR_KEY(&php_session_iface_entry->function_table,
func_name) {
if ((current_mptr = zend_hash_find_ptr(&Z_OBJCE_P(obj)->function_table,
func_name))) {
if (!Z_ISUNDEF(PS(mod_user_names).names[i])) {
zval_ptr_dtor(&PS(mod_user_names).names[i]);
}
array_init_size(&PS(mod_user_names).names[i], 2);
Z_ADDREF_P(obj);
add_next_index_zval(&PS(mod_user_names).names[i], obj);
add_next_index_str(&PS(mod_user_names).names[i],
zend_string_copy(func_name));
} else {
php_error_docref(NULL, E_ERROR, "Session handler's function table is
corrupt");
RETURN_FALSE;
}
++i;
} ZEND_HASH_FOREACH_END();
/* Find implemented methods - SessionIdInterface (optional) */
ZEND_HASH_FOREACH_STR_KEY(&php_session_id_iface_entry->function_table,
func_name) {
if ((current_mptr = zend_hash_find_ptr(&Z_OBJCE_P(obj)->function_table,
func_name))) {
if (!Z_ISUNDEF(PS(mod_user_names).names[i])) {
zval_ptr_dtor(&PS(mod_user_names).names[i]);
}
array_init_size(&PS(mod_user_names).names[i], 2);
Z_ADDREF_P(obj);
add_next_index_zval(&PS(mod_user_names).names[i], obj);
add_next_index_str(&PS(mod_user_names).names[i],
zend_string_copy(func_name));
} else {
if (!Z_ISUNDEF(PS(mod_user_names).names[i])) {
zval_ptr_dtor(&PS(mod_user_names).names[i]);
ZVAL_UNDEF(&PS(mod_user_names).names[i]);
}
}
++i;
} ZEND_HASH_FOREACH_END();
/* Find implemented methods - SessionUpdateTimestampInterface (optional) */
ZEND_HASH_FOREACH_STR_KEY(&php_session_update_timestamp_iface_entry->function_table,
func_name) {
if ((current_mptr = zend_hash_find_ptr(&Z_OBJCE_P(obj)->function_table,
func_name))) {
if (!Z_ISUNDEF(PS(mod_user_names).names[i])) {
zval_ptr_dtor(&PS(mod_user_names).names[i]);
}
array_init_size(&PS(mod_user_names).names[i], 2);
Z_ADDREF_P(obj);
add_next_index_zval(&PS(mod_user_names).names[i], obj);
add_next_index_str(&PS(mod_user_names).names[i],
zend_string_copy(func_name));
} else {
if (!Z_ISUNDEF(PS(mod_user_names).names[i])) {
zval_ptr_dtor(&PS(mod_user_names).names[i]);
ZVAL_UNDEF(&PS(mod_user_names).names[i]);
}
}
++i;
} ZEND_HASH_FOREACH_END();
As you can see, it does not check if user class implements/extends
interface/class.
PHP_FUNCTION(session_set_save_handler).
Advocate OOP best practice. i.e. Use INTERFACE when user is required to
implement all required methods.There's no OOP best practice that says you can not extend classes and
override some of its methods in order to modify parent's behavior. On
the contrary, doing this is one of common OOP practices.
The PHP_FUNCTION(session_set_save_handler) code could be best practice
hardly.
And provide good API (both internal/user). e.g. Missing functions like
user serializer, gc, create_id.If you want to provide additional API, by all means please make an RFC.
But I do not see how removing this class is required for providing any API.
Fair enough. I'll implement user serialize handler. Removal of
SessionHandler
class may be discussed when use of user serialize handler became dominant.
I would like to make SessionHandler deprecated, encourage user serialize
handler for new PHP because it's more efficient and clean. Do you agree?
If user need native handler, they should use it directly. IMO.
I do not see any reason for that. You can clearly use native handler and
add modifications for it. In fact, that is exactly why that class exists.I would like to discuss one by one. Shall we discuss user serializer?
What do you think?Again, if you want to propose new feature, by all means please do. I
still don't see how it requires removing existing classes. This is a
very big BC break and requires very big gain to justify, and so far the
gain was not shown I think.
OK. I'm willing to remove SessionHandler class if nobody objects.
It appears there are two at least.
Let's keep SessionHandler class. However,
PHP_FUNCTION(session_set_save_handler)
should be cleaned up to verify implemented/extended interface/class. It's
BC.
Do you have opinion for this?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi!
Let's keep SessionHandler class. However,
PHP_FUNCTION(session_set_save_handler)
should be cleaned up to verify implemented/extended interface/class.
It's BC.
Do you have opinion for this?
I think it would be OK to require implementing the interface (and of
course the class should be implementing that as well). That sounds to me
like a kind of BC break that is acceptable in PHP 7. Probably requires
an RFC though as it is a BC break - and maybe there are reasons why it
wasn't done that I was missing, then they will be exposed when
discussing the RFC.
--
Stas Malyshev
smalyshev@gmail.com
Hi Stas,
On Sat, Jan 24, 2015 at 9:29 AM, Stanislav Malyshev smalyshev@gmail.com
wrote:
Let's keep SessionHandler class. However,
PHP_FUNCTION(session_set_save_handler)
should be cleaned up to verify implemented/extended interface/class.
It's BC.
Do you have opinion for this?I think it would be OK to require implementing the interface (and of
course the class should be implementing that as well). That sounds to me
like a kind of BC break that is acceptable in PHP 7. Probably requires
an RFC though as it is a BC break - and maybe there are reasons why it
wasn't done that I was missing, then they will be exposed when
discussing the RFC.
I don't remember well the reason why neither. I guess the reason why
session_set_save_handler()
does not check class/interface definition as usual is for PHP4
compatibility. It's legacy code anyway.
I'll write the RFC as soon as other proposal discussion finishes.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
Hi Stas,
On Sat, Jan 24, 2015 at 8:49 AM, Stanislav Malyshev smalyshev@gmail.com
wrote:This is the only reasonable use I know. I would to write user
serializer(read/writer)
handler for it.So we went from no reasonable use to one reasonable use, documented at
the manual. I think it is also reasonable to suppose there are more uses
for it.This is because session module lacks user defined serializer. Save handler
handles session data storage. Serialize handler handles how data is
converted/represented. IMHO.
That's not the only use case.
Some time ago I proposed a session.match_ip feature and argued that if
I wanted to implement it in userland code, I'd have to implement the
whole session handler from scratch. An example using the
SessionHandler class proved me wrong in that regard.
Surely, people are using it for other stuff as well and it's not about
the serializer.
I would like to make SessionHandler deprecated, encourage user serialize
handler for new PHP because it's more efficient and clean. Do you agree?
How can it possibly be more efficient and clean if you have to
re-implement everything with user-level code?
Let's keep SessionHandler class. However,
PHP_FUNCTION(session_set_save_handler)
should be cleaned up to verify implemented/extended interface/class. It's
BC.
Do you have opinion for this?
php > session_set_save_handler(new stdclass);
PHP Warning: session_set_save_handler() expects parameter 1 to be
SessionHandlerInterface, object given in php shell code on line 1
Cheers,
Andrey.
Hi Andrey,
Let's keep SessionHandler class. However,
PHP_FUNCTION(session_set_save_handler)
should be cleaned up to verify implemented/extended interface/class. It's
BC.
Do you have opinion for this?php > session_set_save_handler(new stdclass);
PHP Warning:session_set_save_handler()expects parameter 1 to be
SessionHandlerInterface, object given in php shell code on line 1
As I pasted code in this thread, current code does not check if supplied
object
has valid class/interface, but it checks methods implemented. SdtClass does
not
have them obviously, therefore it raises error.
It's a legacy code when PHP didn't have proper OOP support. This is the code
that I'm willing to remove/change.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi,
Hi Andrey,
Let's keep SessionHandler class. However,
PHP_FUNCTION(session_set_save_handler)
should be cleaned up to verify implemented/extended interface/class.
It's
BC.
Do you have opinion for this?php > session_set_save_handler(new stdclass);
PHP Warning:session_set_save_handler()expects parameter 1 to be
SessionHandlerInterface, object given in php shell code on line 1As I pasted code in this thread, current code does not check if supplied
object
has valid class/interface, but it checks methods implemented. SdtClass does
not
have them obviously, therefore it raises error.It's a legacy code when PHP didn't have proper OOP support. This is the code
that I'm willing to remove/change.
If I understand correctly, you're suggesting the removal of the
old-style session_set_save_handler() calls where each function is
passed as a separate argument?
That could be a quite significant BC break.
Cheers,
Andrey.
Hi Andrey,
If I understand correctly, you're suggesting the removal of the
old-stylesession_set_save_handler()calls where each function is
passed as a separate argument?That could be a quite significant BC break.
No. Class/Interface support is there.
I'm just going to remove legacy code and replace it to usual code.
i.e. Check object against class/interface name, not methods in there.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi Andrey,
This is because session module lacks user defined serializer. Save
handler
handles session data storage. Serialize handler handles how data is
converted/represented. IMHO.That's not the only use case.
Some time ago I proposed a session.match_ip feature and argued that if
I wanted to implement it in userland code, I'd have to implement the
whole session handler from scratch. An example using the
SessionHandler class proved me wrong in that regard.
For me, IP address matching check does not belong to save handler.
I would implement it in other place.
What's the reason why you need to implement IP address matching in
save handler?
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net
Hi again,
Hi Andrey,
This is because session module lacks user defined serializer. Save
handler
handles session data storage. Serialize handler handles how data is
converted/represented. IMHO.That's not the only use case.
Some time ago I proposed a session.match_ip feature and argued that if
I wanted to implement it in userland code, I'd have to implement the
whole session handler from scratch. An example using the
SessionHandler class proved me wrong in that regard.For me, IP address matching check does not belong to save handler.
I would implement it in other place.What's the reason why you need to implement IP address matching in
save handler?
To prevent session fixation?
Doesn't matter, I was just giving you an example.
Cheers,
Andrey.
Hi Andrey,
To prevent session fixation?
Doesn't matter, I was just giving you an example.
If app may assume that clients have constant IP, then IP may be used to
prevent
stolen sessions. Unfortunately, we live in mobile world, so this solution
may be
used under very limited environments. Using save handler for this purpose
may
trigger error from unknow file/line.
I would advise to write following code somewhere in usual locations.
if ($_SESSION['last_ip'] !== $_SERVER['REMOTE_ADDR']) {
log_security_breach();
session_regenerate_id();
session_unset();
die_or_trigger_error_if_it_is_needed();
}
Anyway, if anyone would like to implement something fancy in save handlers,
beware that it may result in consequences that you may not be willing to
have.
Regards,
--
Yasuo Ohgaki
yohgaki@ohgaki.net