Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:81062
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.192.45 as permitted sender)
MIME-Version: 1.0
Sender: yohgaki@gmail.com
In-Reply-To: <54C2DE03.6090708@gmail.com>
References: <CAGa2bXZHP3-uHaXJW_QkEva8wECXwu9C3f=v+xGTVodtyCvQKw@mail.gmail.com>
 <54C1D562.1080402@gmail.com> <CAGa2bXZwdQeZHTE4G-6YmNPwT4xmjY9nmGTi_yDPse9vVAKm_w@mail.gmail.com>
 <54C1EE77.7040000@gmail.com> <CAGa2bXab+YhAw5P8u2YaHAm2+WbSmn5qs1dsMFNsVEwtns2qpg@mail.gmail.com>
 <54C2DE03.6090708@gmail.com>
Date: Sat, 24 Jan 2015 09:24:31 +0900
Message-ID: <CAGa2bXYOhmZtYqf6dujMq3oyE3Uk3crd9dBj-Qtj2BMrMBRgug@mail.gmail.com>
To: Stanislav Malyshev <smalyshev@gmail.com>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11c1257e315c44050d5af089
Subject: Re: [PHP-DEV] Removing base class from session handler
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

--001a11c1257e315c44050d5af089
Content-Type: text/plain; charset=UTF-8

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

--001a11c1257e315c44050d5af089--