New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize the session interface #12833

Closed
sergeyklay opened this Issue May 2, 2017 · 22 comments

Comments

Projects
7 participants
@sergeyklay
Copy link
Member

sergeyklay commented May 2, 2017

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:

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

@rianorie

This comment has been minimized.

Copy link
Contributor

rianorie commented May 2, 2017

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

@linxlad

This comment has been minimized.

Copy link
Member

linxlad commented May 2, 2017

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

@niden

This comment has been minimized.

Copy link
Member

niden commented May 2, 2017

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

This comment has been minimized.

Copy link
Member

linxlad commented May 2, 2017

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

@Jurigag

This comment has been minimized.

Copy link
Member

Jurigag commented May 2, 2017

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

This comment has been minimized.

Copy link
Member Author

sergeyklay commented May 2, 2017

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

@linxlad

This comment has been minimized.

Copy link
Member

linxlad commented May 2, 2017

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

@virgofx

This comment has been minimized.

Copy link
Member

virgofx commented May 2, 2017

👍

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented May 6, 2017

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

This comment has been minimized.

Copy link
Contributor

dschissler commented May 6, 2017

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

This comment has been minimized.

Copy link
Member Author

sergeyklay commented May 6, 2017

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

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented May 6, 2017

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

This comment has been minimized.

Copy link
Contributor

dschissler commented Nov 8, 2017

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

@sergeyklay

This comment has been minimized.

Copy link
Member Author

sergeyklay commented Nov 9, 2017

@dschissler Could you elaborate on that. Why?

@sergeyklay sergeyklay added the Breaks BC label Nov 9, 2017

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Nov 9, 2017

This seems strange.

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

@sergeyklay

This comment has been minimized.

Copy link
Member Author

sergeyklay commented Nov 9, 2017

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

@dschissler

This comment has been minimized.

Copy link
Contributor

dschissler commented Nov 10, 2017

How about we add API sugar?:

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

This comment has been minimized.

Copy link
Contributor

dschissler commented Nov 10, 2017

@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 comment has been minimized.

Copy link
Member Author

sergeyklay commented Nov 10, 2017

This sugar is not compatible with the interface

@sergeyklay

This comment has been minimized.

@stale

This comment has been minimized.

Copy link

stale bot commented Apr 16, 2018

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

@stale stale bot added the stale label Apr 16, 2018

@sergeyklay sergeyklay closed this Apr 16, 2018

@sergeyklay sergeyklay reopened this May 2, 2018

niden added a commit to niden/cphalcon that referenced this issue Dec 21, 2018

@niden niden referenced this issue Dec 21, 2018

Merged

T12833 session interface #13673

3 of 3 tasks complete

niden added a commit to niden/cphalcon that referenced this issue Dec 21, 2018

niden added a commit that referenced this issue Dec 21, 2018

niden added a commit that referenced this issue Dec 21, 2018

niden added a commit that referenced this issue Dec 21, 2018

niden added a commit that referenced this issue Dec 21, 2018

niden added a commit that referenced this issue Dec 21, 2018

niden added a commit that referenced this issue Dec 21, 2018

niden added a commit that referenced this issue Dec 21, 2018

niden added a commit that referenced this issue Dec 21, 2018

niden added a commit that referenced this issue Dec 21, 2018

@niden

This comment has been minimized.

Copy link
Member

niden commented Dec 21, 2018

Addressed in #13673

@niden niden closed this Dec 21, 2018

4.0 Release automation moved this from In progress to Done Dec 21, 2018

niden added a commit to niden/cphalcon that referenced this issue Dec 22, 2018

[phalcon#12676] - Merge branch '4.0.x' into T12676-add-methods-to-int…
…erfaces

* 4.0.x: (40 commits)
  [4.0.x] - Another correction to the test
  Corrected test
  Fixed tests
  Removed obsolete file
  Fixed the method signature
  PHPCS fix
  Fixed tabs
  Corrected tests
  [phalcon#12833] - Updated the changelog
  [phalcon#12833] - Deleted obsolete tests
  [phalcon#12833] - PHPCS fixes
  [phalcon#12833] - Corrections to the manager and test (cleanup superglobal on destroy)
  [phalcon#12833] - Fixes and corrections to the tests
  [phalcon#12833] - Corrections to the tests and files adapter
  [phalcon#12833] - Fixed tests; Added exception in session for non valid handler
  [phalcon#12833] - Full tests for Session\Adapter; Adjustments to the environment; Test stubs
  [phalcon#12833] - Corrections and adding files adapter tests
  [phalcon#12833] - Correction to the redis session adapter; Work on the test traits
  [phalcon#12833] - Setup default session_save path to /tmp
  [phalcon#12833] - Cleanup for tests
  ...

niden added a commit to niden/cphalcon that referenced this issue Dec 22, 2018

[phalcon#10406] - Merge branch '4.0.x' into T10406-model-collection-i…
…nterfaces

* 4.0.x: (40 commits)
  [4.0.x] - Another correction to the test
  Corrected test
  Fixed tests
  Removed obsolete file
  Fixed the method signature
  PHPCS fix
  Fixed tabs
  Corrected tests
  [phalcon#12833] - Updated the changelog
  [phalcon#12833] - Deleted obsolete tests
  [phalcon#12833] - PHPCS fixes
  [phalcon#12833] - Corrections to the manager and test (cleanup superglobal on destroy)
  [phalcon#12833] - Fixes and corrections to the tests
  [phalcon#12833] - Corrections to the tests and files adapter
  [phalcon#12833] - Fixed tests; Added exception in session for non valid handler
  [phalcon#12833] - Full tests for Session\Adapter; Adjustments to the environment; Test stubs
  [phalcon#12833] - Corrections and adding files adapter tests
  [phalcon#12833] - Correction to the redis session adapter; Work on the test traits
  [phalcon#12833] - Setup default session_save path to /tmp
  [phalcon#12833] - Cleanup for tests
  ...

niden added a commit to niden/cphalcon that referenced this issue Dec 22, 2018

[phalcon#12295] - Merge branch '4.0.x' into T12295-PSR-11
* 4.0.x: (33 commits)
  [4.0.x] - Another correction to the test
  Corrected test
  Fixed tests
  Removed obsolete file
  Fixed the method signature
  PHPCS fix
  Fixed tabs
  Corrected tests
  [phalcon#12833] - Updated the changelog
  [phalcon#12833] - Deleted obsolete tests
  [phalcon#12833] - PHPCS fixes
  [phalcon#12833] - Corrections to the manager and test (cleanup superglobal on destroy)
  [phalcon#12833] - Fixes and corrections to the tests
  [phalcon#12833] - Corrections to the tests and files adapter
  [phalcon#12833] - Fixed tests; Added exception in session for non valid handler
  [phalcon#12833] - Full tests for Session\Adapter; Adjustments to the environment; Test stubs
  [phalcon#12833] - Corrections and adding files adapter tests
  [phalcon#12833] - Correction to the redis session adapter; Work on the test traits
  [phalcon#12833] - Setup default session_save path to /tmp
  [phalcon#12833] - Cleanup for tests
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment