- Download Diff

Summary:

[Core] Session DAO

Review Request #37 — Created Jan. 8, 2013 and submitted Jan. 24, 2013, 8:31 a.m.

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

Description

Session DatabaseAccessObject implementation 

Depends on http://reviews.lunr.nl/r/21/

Testing Done

Unit Tests

Issues

0
0
16
1
All Issues: 17

Description	From	Last Updated
It's not a Model	pprkut	Jan. 10, 2013, 4:03 p.m.
I would visually separate the query building, just to make it easier readable	pprkut	Jan. 10, 2013, 4:04 p.m.
superfluous	pprkut	Jan. 10, 2013, 4:06 p.m.
put at the end	pprkut	Jan. 10, 2013, 4:09 p.m.
superfluous	pprkut	Jan. 10, 2013, 4:10 p.m.
reverse the if. Makes it more readable	pprkut	Jan. 10, 2013, 4:14 p.m.
needs to be escaped	pprkut	Jan. 10, 2013, 4:16 p.m.
early exit	pprkut	Jan. 10, 2013, 4:21 p.m.
time() is an int, not a string	pprkut	Jan. 10, 2013, 4:23 p.m.
what in here requires 5.4?	pprkut	Jan. 11, 2013, 10:53 a.m.
move below setUp	pprkut	Jan. 10, 2013, 4:27 p.m.
hmm?	pprkut	Jan. 22, 2013, 11:05 a.m.
No longer exists	pprkut	Jan. 22, 2013, 11:08 a.m.
I vaguely remember mentioning something about valueMap. Was that about this or another review?	pprkut	Jan. 22, 2013, 11:09 a.m.
you can drop the pool here. The DatabaseAccessObject needs to support db connection pooling for DAOs that want to/need to ...	pprkut	Jan. 22, 2013, 5:27 p.m.
wrong type	pprkut	Jan. 22, 2013, 5:29 p.m.
Hmm, while this is technically correct I miss a bit of uniformity here. The DAO is not meant to be ...	pprkut	Jan. 22, 2013, 5:29 p.m.

Change Summary:

Codestyle fixes and Session handler test fix

Diff:

Revision 2 (+731 -162)

Show changes

	src/Lunr/Core/SessionDAO.php
	src/Lunr/Core/SessionModel.php
	src/Lunr/Core/Tests/DataBaseSessionHandlerTest.php
	src/Lunr/Core/Tests/SessionDAOTest.php
	tests/phpunit.xml

src/Lunr/Core/SessionDAO.php (Diff revision 2)
Show all issues
```
It's not a Model
```

src/Lunr/Core/SessionDAO.php (Diff revision 2)

Show all issues

I would visually separate the query building, just to make it easier readable

src/Lunr/Core/SessionDAO.php (Diff revision 2)
Show all issues
```
superfluous
```
src/Lunr/Core/SessionDAO.php (Diff revision 2)
Show all issues
```
put at the end
```
src/Lunr/Core/SessionDAO.php (Diff revision 2)
Show all issues
```
superfluous
```
src/Lunr/Core/SessionDAO.php (Diff revision 2)
Show all issues
```
reverse the if. Makes it more readable
```

src/Lunr/Core/SessionDAO.php (Diff revision 2)

you may want to store it in a different variable

Felipe Jan. 10, 2013, 4:27 p.m.

Not really, I have finished using the previous builder. What's the benefit of putting it on a new variable?

pprkut Jan. 11, 2013, 8:23 a.m.

If you need something from the previous builder for debugging, it's no longer reachable. Note that since this is also the only DAO we have in Lunr right now, it's an example case.
People will pick the code from here and do it similarly in other implementations and even though in this case it might be obvious you don't need the previous builder, it might not be in others.

src/Lunr/Core/SessionDAO.php (Diff revision 2)
Show all issues
```
needs to be escaped
```

src/Lunr/Core/SessionDAO.php (Diff revision 2)

Isn't that done explicitely by end_transaction?

Felipe Jan. 10, 2013, 4:27 p.m.

End transaction sets autocommit to true. According to the comments in php doc site, setting autocommit to true will commit all pending transactions but I couldn't find any official document where it states so.

src/Lunr/Core/SessionDAO.php (Diff revision 2)
Show all issues
```
early exit
```
1. Felipe Jan. 10, 2013, 4:27 p.m.
  reverted if to make it more readable as stated before.
src/Lunr/Core/SessionDAO.php (Diff revision 2)
Show all issues
```
time() is an int, not a string
```

src/Lunr/Core/Tests/SessionDAOTest.php (Diff revision 2)

Show all issues

what in here requires 5.4?

Felipe Jan. 10, 2013, 4:27 p.m.

Lunr 0.2?

I don't know by heart the differences between 5.3 and 5.4, nor I have time to check it for every file I write.. Hence why I use 5.4 on all files. If you want to have 5.3 on files that don't need 5.4 then you will have to spot it on all reviews and nag, or fix it yourself when you do a general style fix or something similar.

pprkut Jan. 11, 2013, 8:23 a.m.

As a PHP developer you should know what features are new in the current release. Thus, if you note it needs 5.4, I will ask what does require it. Happy researching!

Felipe Jan. 11, 2013, 10:53 a.m.

I was about to change all array definitions... but it's easier to put down the php version :P

src/Lunr/Core/Tests/SessionDAOTest.php (Diff revision 2)
Show all issues
```
move below setUp
```

Change Summary:

Fixed review comments

Diff:

Revision 3 (+729 -162)

Show changes

	src/Lunr/Core/SessionDAO.php
	src/Lunr/Core/SessionModel.php
	src/Lunr/Core/Tests/DatabaseSessionHandlerTest.php
	src/Lunr/Core/Tests/SessionDAOTest.php
	tests/phpunit.xml

Change Summary:

Fixed PHP version
Builders not reused

Diff:

Revision 4 (+730 -163)

Show changes

	src/Lunr/Core/SessionDAO.php
	src/Lunr/Core/SessionModel.php
	src/Lunr/Core/Tests/DatabaseSessionHandlerTest.php
	src/Lunr/Core/Tests/SessionBaseTest.php
	src/Lunr/Core/Tests/SessionDAOTest.php
	tests/phpunit.xml

Description:

		Session DatabaseAccessObject implementation
	+
	+	Depends on http://reviews.lunr.nl/r/21/

src/Lunr/Core/Tests/SessionBaseTest.php (Diff revision 4)
Show all issues
```
hmm?
```
1. Felipe Jan. 22, 2013, 11:40 a.m.
  Doesn't belong to this review, dunno how it got here

src/Lunr/Core/Tests/SessionDAOTest.php (Diff revision 4)

Show all issues

No longer exists

Felipe Jan. 22, 2013, 11:40 a.m.
```
Replaced with FileLogger
```

pprkut Jan. 22, 2013, 11:55 a.m.

Still wrong. You need to use the Interface, since at runtime I could also use the NullLogger or the PHPLogger

Felipe Jan. 22, 2013, 12:11 p.m.
```
Changed!
```

src/Lunr/Core/Tests/SessionDAOTest.php (Diff revision 4)

Show all issues

I vaguely remember mentioning something about valueMap. Was that about this or another review?

Felipe Jan. 22, 2013, 11:40 a.m.

yep, it's in this one, it's in a function below, fixed here too.

Change Summary:

Fixed comments

Diff:

Revision 5 (+727 -1)

Show changes

	src/Lunr/Core/SessionDAO.php
	src/Lunr/Core/Tests/DatabaseSessionHandlerTest.php
	src/Lunr/Core/Tests/SessionDAOTest.php
	tests/phpunit.xml

Change Summary:

Changed logger mock to interface

Diff:

Revision 6 (+726 -1)

Show changes

	src/Lunr/Core/SessionDAO.php
	src/Lunr/Core/Tests/DatabaseSessionHandlerTest.php
	src/Lunr/Core/Tests/SessionDAOTest.php
	tests/phpunit.xml

src/Lunr/Core/SessionDAO.php (Diff revision 6)

Show all issues

you can drop the pool here.
The DatabaseAccessObject needs to support db connection pooling for DAOs that want to/need to access a database over more than one connection or multiple databases at once.
Neither is the case for the SessionDAO, it will always be only one connection, accessing the same database.

src/Lunr/Core/Tests/SessionDAOTest.php (Diff revision 6)
Show all issues
```
wrong type
```

src/Lunr/Core/Tests/SessionDAOTest.php (Diff revision 6)

Show all issues

Hmm, while this is technically correct I miss a bit of uniformity here. The DAO is not meant to be used with anything but MySQL, that comes with the dml query builder interface. However, then you can just as well Mock the mysql connection/query result instead of the generic db one.

Change Summary:

Changes from review

Diff:

Revision 7 (+732 -1)

Show changes

	src/Lunr/Core/SessionDAO.php
	src/Lunr/Core/Tests/DatabaseSessionHandlerTest.php
	src/Lunr/Core/Tests/SessionDAOTest.php
	tests/phpunit.xml

Status: Closed (submitted)

Change Summary:

Merged into master

You have a pending review.

Review Board 3.0.24

[Core] Session DAO

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Description:

Change Summary:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)

Change Summary: