- Download Diff

Summary:

[Core] Database session handler

Review Request #21 — Created Dec. 6, 2012 and submitted Jan. 20, 2013, 10:13 p.m.

Information
Owner:	Felipe
Repository:	Lunr
Branch:	https://github.com/madsmash/lunr/tree/newSessionClass
Bugs:
Depends On:
Reviewers
Groups:	lunr
People:

Description

Database Session Handler implementation

Testing Done

Unit Tests

Issues

0
0
15
3
All Issues: 18

Description	From	Last Updated
not necessary	pprkut	Dec. 7, 2012, 1 p.m.
misnamed. We already know this is going to be a DAO so no need to keep calling it Model	pprkut	Dec. 7, 2012, 1 p.m.
Maybe I dont picture it good but I would prefer a static method or even a simple getter to retrieve ...	olivier	Jan. 8, 2013, 5:10 p.m.
I don't know what to put here... What's going to be the namespace for the session DAO??	Felipe	Dec. 11, 2012, 2:51 p.m.
SessionHandlerInterface needs php 5.4	pprkut	Jan. 8, 2013, 3:53 p.m.
it's an instance, not a reference	pprkut	Jan. 8, 2013, 3:53 p.m.
missing with()	pprkut	Jan. 8, 2013, 3:57 p.m.
weird	pprkut	Jan. 10, 2013, 3:28 p.m.
wrong caps. Should be DatabaseSessionHandler	pprkut	Jan. 10, 2013, 3:38 p.m.
Maybe it could be a good idea to have a setter with fluid interface here? What do you think?	olivier	Jan. 10, 2013, 3:10 p.m.
5.4	pprkut	Jan. 10, 2013, 3:39 p.m.
wrong namespace	pprkut	Jan. 10, 2013, 3:39 p.m.
coding style	pprkut	Jan. 10, 2013, 3:51 p.m.
Please align the object operator	pprkut	Jan. 10, 2013, 3:51 p.m.
5.4	pprkut	Jan. 10, 2013, 3:39 p.m.
wong namespace	pprkut	Jan. 10, 2013, 3:39 p.m.
pls align the object operator	pprkut	Jan. 10, 2013, 3:51 p.m.
don't list abstract classes	pprkut	Jan. 10, 2013, 3:46 p.m.

system/libraries/core/class.databasesessionhandler.inc.php (Diff revision 1)

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.

Felipe Dec. 6, 2012, 6:08 p.m.

This is legacy code from 0.1, I'm not sure where else this is used, lets see what heinz has to say.

pprkut Dec. 6, 2012, 6:21 p.m.

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.

system/libraries/core/class.databasesessionhandler.inc.php (Diff revision 1)
Show all issues
```
not necessary
```
1. pprkut Dec. 8, 2012, 3:28 p.m.
  This has nothing to do with Model vs. DAO. If you'd do s/Model/DAO/ it would still be superfluous

system/libraries/core/class.databasesessionhandler.inc.php (Diff revision 1)

Show all issues

misnamed. We already know this is going to be a DAO so no need to keep calling it Model

Felipe Dec. 7, 2012, 1 p.m.

these changes you mention belong to the SessionDAO review IMHO

pprkut Dec. 8, 2012, 3:28 p.m.

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?

Change Summary:

Code style fixes

Diff:

Revision 2 (+153 -156)

Show changes

	system/libraries/core/class.databasesessionhandler.inc.php
	system/libraries/core/class.sessionmanager.inc.php

Change Summary:

Renamed sessionModel to sessionDAO

Diff:

Revision 3 (+153 -156)

Show changes

	system/libraries/core/class.databasesessionhandler.inc.php
	system/libraries/core/class.sessionmanager.inc.php

system/libraries/core/class.databasesessionhandler.inc.php (Diff revision 3)

Show all issues

I don't know what to put here... What's going to be the namespace for the session DAO??

Change Summary:

PSR0 implemented
Unit Tests

Diff:

Revision 4 (+358 -157)

Show changes

	src/Lunr/Core/DataBaseSessionHandler.php
	src/Lunr/Core/SessionManager.php
	src/Lunr/Core/SessionModel.php
	src/Lunr/Core/Tests/DataBaseSessionHandlerBaseTest.php
	src/Lunr/Core/Tests/DataBaseSessionHandlerTest.php
	tests/phpunit.xml

```
Mostly fine
```
src/Lunr/Core/DataBaseSessionHandler.php (Diff revision 4)
Show all issues
```
SessionHandlerInterface needs php 5.4
```
src/Lunr/Core/DataBaseSessionHandler.php (Diff revision 4)
Show all issues
```
it's an instance, not a reference
```
src/Lunr/Core/DataBaseSessionHandler.php (Diff revision 4)
```
I think we can drop this
```
src/Lunr/Core/Tests/DataBaseSessionHandlerBaseTest.php (Diff revision 4)
Show all issues
```
missing with()
```

Change Summary:

Code style fixes + review comments

Diff:

Revision 5 (+356 -157)

Show changes

	src/Lunr/Core/DataBaseSessionHandler.php
	src/Lunr/Core/SessionManager.php
	src/Lunr/Core/SessionModel.php
	src/Lunr/Core/Tests/DataBaseSessionHandlerBaseTest.php
	src/Lunr/Core/Tests/DataBaseSessionHandlerTest.php
	tests/phpunit.xml

Description:

~		Database Session Handler implementation. Unit tests in progress, please comment if you see something wrong.
	~	Database Session Handler implementation

Testing Done:

Unit Tests

src/Lunr/Core/DataBaseSessionHandler.php (Diff revision 5)

Show all issues

Maybe it could be a good idea to have a setter with fluid interface here?
What do you think?

pprkut Jan. 9, 2013, 4:24 p.m.

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()

olivier Jan. 9, 2013, 4:43 p.m.
```
Ok then, was just an idea.
```

src/Lunr/Core/DataBaseSessionHandler.php (Diff revision 5)
Show all issues
```
weird
```
src/Lunr/Core/DataBaseSessionHandler.php (Diff revision 5)
Show all issues
```
wrong caps. Should be DatabaseSessionHandler
```

src/Lunr/Core/DataBaseSessionHandler.php (Diff revision 5)

Not really necessary to separate those by an empty line IMHO

src/Lunr/Core/Tests/DataBaseSessionHandlerBaseTest.php (Diff revision 5)
Show all issues
```
5.4
```
src/Lunr/Core/Tests/DataBaseSessionHandlerBaseTest.php (Diff revision 5)
Show all issues
```
wrong namespace
```
src/Lunr/Core/Tests/DataBaseSessionHandlerBaseTest.php (Diff revision 5)
Show all issues
```
coding style
```
src/Lunr/Core/Tests/DataBaseSessionHandlerBaseTest.php (Diff revision 5)
Show all issues
```
Please align the object operator
```
src/Lunr/Core/Tests/DataBaseSessionHandlerTest.php (Diff revision 5)
Show all issues
```
5.4
```
src/Lunr/Core/Tests/DataBaseSessionHandlerTest.php (Diff revision 5)
Show all issues
```
wong namespace
```
src/Lunr/Core/Tests/DataBaseSessionHandlerTest.php (Diff revision 5)
Show all issues
```
pls align the object operator
```
tests/phpunit.xml (Diff revision 5)
Show all issues
```
don't list abstract classes
```

Change Summary:

Changes from review!

Diff:

Revision 6 (+355 -157)

Show changes

	src/Lunr/Core/DatabaseSessionHandler.php
	src/Lunr/Core/SessionManager.php
	src/Lunr/Core/SessionModel.php
	src/Lunr/Core/Tests/DatabaseSessionHandlerBaseTest.php
	src/Lunr/Core/Tests/DatabaseSessionHandlerTest.php
	tests/phpunit.xml

Status: Closed (submitted)

Change Summary:

Merged into master

You have a pending review.

Review Board 3.0.24

[Core] Database session handler

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Description:

Testing Done:

Change Summary:

Diff:

Status: Closed (submitted)

Change Summary: