Cphalcon: Standardize the session interface

[sergeyklay]

The implementation of destroy between Phalcon\Session\Adapter and Phalcon\Session\Adapter\* when destroying is totally different. For example:

Phalcon\Session\Adapter:

public function destroy ( boolean removeData = false ) -> boolean

Phalcon\Session\Adapter\Redis:

public function destroy ( string sessionId = null ) -> boolean

SessionHandlerInterface:

public function destroy ( string $session_id ) : boolean

I propose to refactor:

• [ ] Phalcon\Session\AdapterInterface
• [ ] Phalcon\Session\Adapter
• [ ] Phalcon\Session\Adapter\Files
• [ ] Phalcon\Session\Adapter\Libmemcached
• [ ] Phalcon\Session\Adapter\Memcache
• [ ] Phalcon\Session\Adapter\Redis

for full compliance with PHP's SessionHandlerInterface.

I believe that Phalcon\Session\AdapterInterface should inherit SessionHandlerInterface.

Obviously these are backward incompatible changes and they should be done in 4.x branch.

Refs:

• #2326
• #12835
• #11722
• #11341
• #10883
• #12981

Cc: @phalcon/core-team, @phalcon/framework-team

[rianorie]

+1ed - I believe consistency is key, any change that will promote that has my vote.

[linxlad]

I use Aerospike, worth checking if that needs a refactor :)

[niden]

Absolutely yes

We need to be consistent in all interfaces, all implementations of adapters. This has to be refactored and introduced in 4.x version.

public function destroy (string sessionId = null) -> bool

It will :

• if null it will destroy the current sessionId
• remove a particular sessionId

If successful returns true. If not false or if the sessionId was not found false again.

[linxlad]

Also no underscored variables in any interfaces or in general :(

[Jurigag]

You mean in methods body? @linxlad yea i agree. But underscored properties should remain - simply to give people option to add their own property for example like $name and we should use property _name imho.

[sergeyklay]

@linxlad This is different issue, not related to the session interface

[linxlad]

@sergeyklay I know, I just turned into an annoying PSR-2 warrior lol

[virgofx]

👍

[dschissler]

I'm not understanding why the new standard interface doesn't allow a default argument for the session id: public function destroy ( string $session_id ) : bool.

[dschissler]

It seems like that the way the signature is working it should be a static method, but its not. I'm not familiar with most of the adapter backends but it seems like the interface is so overly general that it will be annoying in the most simple cases.

[sergeyklay]

@dschissler What do you mean? Could you please explain a bit more?

[dschissler]

So in Phalcon\Session\Adapter\Files the object will already know what the session id, probably as a protected variable in the class. However then to tear down the session you need to specify the session id in the destroy method. Doesn't that seem weird?

Currently this is the sensible behavior:

<?php

var_dump(
    $session->destroy()
);

var_dump(
    $session->destroy(true)
);

With the new interface it seems that it would need to be like this:

$session->destroy($session->getId());

[dschissler]

Has anyone thought about this? As far as I can see SessionHandlerInterface seems like a faulty standard.

[sergeyklay]

@dschissler Could you elaborate on that. Why?

[dschissler]

This seems strange.

$session->destroy($session->getId());

[sergeyklay]

Yes, you may need remove not current session, that is owned by some user

[dschissler]

How about we add API sugar?:

public function destroyCurrent()
{
    $this->destroy($this->getId());
} 

[dschissler]

@sergeyklay How does Phalcon\Session\Adapter\Redis work with SessionHandlerInterface::open? I don't understand how string $save_path is relevant to service based sessions.

[sergeyklay]

This sugar is not compatible with the interface

[sergeyklay]

https://github.com/nrk/predis/blob/v1.1/src/Session/Handler.php

[stale[bot]]

Thank you for contributing to this issue. As it has been 90 days since the last activity, we are automatically closing the issue. This is often because the request was already solved in some way and it just wasn't updated or it's no longer applicable. If that's not the case, please feel free to either reopen this issue or open a new one. We will be more than happy to look at it again! You can read more here: https://blog.phalconphp.com/post/github-closing-old-issues

[niden]

Addressed in https://github.com/phalcon/cphalcon/pull/13673