[TYPO3-core] RFC #14367: Install Tool does not provide a logout possibility

[Lars Houmark]

Hi Ernesto,

Ernesto Baschny [cron IT] wrote:

> I think the session handling has nothing to do with "locking" the
> install tool. At least at the current state.

Well, tbh I really do not see the purpose of the extra wrapper function 
then. A part of the purpose of having your own destroy handler is 
exactly to be able to do extra stuff when doing it, but let the dev 
still call the PHP core function. So to use it the way it was done, we 
should have that extra code within the destroy() function of the session 
class. And since we now all agree that a part of destroying a session 
and make the whole thing safe, is also deleting the lock file, this is 
of course a part of the destroy of a session and should be implemented 
accordingly.

You implement your own session_destroy handler from PHP, and this should 
be done in order to be able to call that function directly, right?

Still you want another wrapper function (which makes it even more hard 
to understand the entire setup for new devs). But you do not centralize 
the code within that function. So what if we for some reason want to 
destroy the session because some kinda of attempt on MitM attack 
happened. Should we then re-create the lines for removing the lock file 
and the redirect? Or do you want yet another wrapper function within the 
tx_install class for that? ;)

I really think you misunderstood the purpose of using the 
session_set_save_handler, and you could as well have not used it at all 
and done all the session handling code you self, instead of having the 
extra overhead of inheriting the PHP core functions, when you want them 
wrapped by own functions anyways - without taking advantage of it! 
Reading your code, it's very limited how much code from the PHP session 
core functions you actually use.

Just my 2 cents - and nothing blocking the commit, but will for sure 
mean extra redundant code lines at some point - besides the ones already 
there.

-- 
Lars Houmark