[Core] Database session handler

Review Request #21 — Created Dec. 6, 2012 and submitted

Felipe
Lunr
https://github.com/madsmash/lunr/tree/newSessionClass
lunr
Database Session Handler implementation
Unit Tests
  • 0
  • 0
  • 15
  • 3
  • 18
Description From Last Updated
There are no open issues
olivier
  1. 
      
  2. The issue has been resolved. Show all issues
    Maybe I dont picture it good but I would prefer a static method or even a simple getter to retrieve the path instead of a global var.
    1. This is legacy code from 0.1, I'm not sure where else this is used, lets see what heinz has to say.
    2. As the comment states, it's completely arbitrary. IIRC I took that code off an example. Maybe we can throw a log warning there or something if it is used, otherwise we might just as well reduce it to return TRUE.
  3. 
      
pprkut
  1. 
      
  2. The issue has been dropped. Show all issues
    not necessary
    1. This has nothing to do with Model vs. DAO. If you'd do s/Model/DAO/ it would still be superfluous
  3. The issue has been dropped. Show all issues
    misnamed. We already know this is going to be a DAO so no need to keep calling it Model
    1. these changes you mention belong to the SessionDAO review IMHO
    2. That would just create more changes than necessary IMHO. Semantically the SessionModel as it is right now is a DAO.
      The naming is already introduced so it's not something foreign nobody heard about. The API won't change either, so why force a meaningless change later if you can do it properly now?
  4. 
      
Felipe
Felipe
Felipe
  1. 
      
  2. The issue has been resolved. Show all issues
    I don't know what to put here... What's going to be the namespace for the session DAO??
  3. 
      
Felipe
pprkut
  1. Mostly fine
  2. The issue has been resolved. Show all issues
    SessionHandlerInterface needs php 5.4
  3. The issue has been resolved. Show all issues
    it's an instance, not a reference
  4. src/Lunr/Core/DataBaseSessionHandler.php (Diff revision 4)
     
     
     
    I think we can drop this
  5. The issue has been resolved. Show all issues
    missing with()
  6. 
      
Felipe
Felipe
olivier
  1. 
      
  2. The issue has been dropped. Show all issues
    Maybe it could be a good idea to have a setter with fluid interface here?
    What do you think?
    1. No, that'd be confusing. Session lifetime should always be as defined in the php.ini. If you want to override it, use ini_set()
    2. Ok then, was just an idea.
  3. 
      
pprkut
  1. 
      
  2. The issue has been resolved. Show all issues
    weird
  3. The issue has been resolved. Show all issues
    wrong caps. Should be DatabaseSessionHandler
  4. src/Lunr/Core/DataBaseSessionHandler.php (Diff revision 5)
     
     
     
     
    Not really necessary to separate those by an empty line IMHO
  5. The issue has been resolved. Show all issues
    5.4
  6. The issue has been resolved. Show all issues
    wrong namespace
  7. The issue has been resolved. Show all issues
    coding style
  8. The issue has been resolved. Show all issues
    Please align the object operator
  9. The issue has been resolved. Show all issues
    5.4
  10. The issue has been resolved. Show all issues
    wong namespace
  11. The issue has been resolved. Show all issues
    pls align the object operator
  12. tests/phpunit.xml (Diff revision 5)
     
     
    The issue has been resolved. Show all issues
    don't list abstract classes
  13. 
      
Felipe
Felipe
Review request changed

Status: Closed (submitted)

Change Summary:

Merged into master
Loading...