8000 [Session] bad design / invalid calls / interface not respected · Issue #9233 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Session] bad design / invalid calls / interface not respected #9233

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.

Sign up for GitHub

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

Jump to bottom
Closed
Tobion opened this issue Oct 7, 2013 · 5 comments
Closed

[Session] bad design / invalid calls / interface not respected #9233

Tobion opened this issue Oct 7, 2013 · 5 comments
Labels
Enhancement HttpFoundation

Comments

@Tobion
Copy link
Contributor
Tobion commented Oct 7, 2013
  • For example Session:remove calls SessionStorageInterface->getBag()->remove(). This call is not valid because getBag returns a SessionBagInterface which does not have a remove() method. The remove method and some other methods are only defined for SessionAttributeBagInterface. There is a special getMetadataBag in session and storage to access specific methods on the metadatabag. But there is none for the AttributeBag. Design seems to have holes.
    So you could create fatal errors through symfony by
$wrongAttributeBag = new FlashBag();
$wrongAttributeBag->setName('attributes');
$session->registerBag($wrongAttributeBag);
$session->replace(array());
  • Also FlashBag and AttributeBag have a setName method which is not part of the interface. IMO it should be removed and the name should be part of the constructor instead. I don't see why the name is mutable and can make things out-of-synch with the Storage::registerBag functionality.
  • https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/Session/Storage/MetadataBag.php#L76 is not safe to access because it could not be set. And getLastUsed can return null, or it must be initialized with 0 (just as it's done for created etc.)
  • NativeSessionStorage::setSaveHandler can be called mid-session which has no effect. Either only allow it when session not started or no setter at all and constructor argument only.
  • SessionInterface::start return value makes no sense. It returns bool but also throws Exception when it does not work which conflicts with each other. Either return a bool and don't throw exception or don't have a return value at all because an exception already says whether it worked or not.
@Tobion
Copy link
Contributor Author
Tobion commented Apr 22, 2014

I also wonder about the session class design at whole.

  1. Why do we need a SessionStorageInterface and all the implementations?
    • Session storage is excatly what session save handlers are already
    • MockFileSessionStorage is the same as NativeFileSessionHandler with a different save path. Why is there a need to reimplement it in PHP with all possible flaws like no locking.
    • MockArraySessionStorage is the same as NullSessionHandler because it does not persist the data and initializing the Session data with NativeSessionStorage::loadSession(array &$session = null).
    • If there was really a case for a Session implementation without NativeSessionStorage, e.g. without save handlers, then it would simply be a new implementation of the SessionInterface. So we can simply get rid of SessionStorageInterface and all these proxies which is ridiulous: Session::setId calls SessionStorageInterface::setId which calls AbstractProxy::setId
  2. The save handler proxies like AbstractProxy seems not necessary to me. For example why do we need a AbstractProxy::setActive? Whether a session is active is already covered by logic in the Session (and the SessionStorage ...)

cc @Drak

@stof
Copy link
Member
stof commented Apr 22, 2014

@Tobion the issue is that the Session in 2.0 was not based on the SessionHandler interface of PHP 5.4+. At this time, the SessionStorage were used. But this implementation was creating a bunch of issues, and so the session system was reworked to fit better with the way PHP sessions work. Removing the SessionStorage was not possible for BC reasons

@Tobion
Copy link
Contributor Author
Tobion commented Apr 22, 2014

Yes but even before PHP 5.4 session save handlers existed (just without interface). The session subsystem is really a mess at the moment in symfony.
IMO we could deprecate much stuff and simply add a new NativeSession implements SessionInterface that merges Session + NativeSessionStorage. Et voila.

@Tobion Tobion changed the title [Session] invalid calls / interface not respected [Session] bad design / invalid calls / interface not respected Apr 22, 2014
@Tobion Tobion mentioned this issue Nov 6, 2014
Closed
@Koc
Copy link
Contributor
Koc commented Apr 22, 2015

What about return back to this issue before 3.0?

@Koc Koc mentioned this issue Apr 22, 2015
Closed
@IanAtOcucom
Copy link

For me the major challenge is that SessionBagInterface doesn't declare the get/set methods that all of its implementations contain, so it throws warnings in the IDE (and arguably breaks the spec)

Simply adding get/set methods would go a long way to fixing this.

@Tobion Tobion closed this as completed Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement HttpFoundation
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@javiereguiluz @Koc @stof @Tobion @IanAtOcucom
0