Removing base class from session handler

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