- Download Diff

Summary:

[Core] Session class

Review Request #22 — Created Dec. 6, 2012 and submitted Jan. 13, 2013, 6:35 p.m.

Information
Owner:	Felipe
Repository:	Lunr
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:	lunr
People:

Description

Class that deals with session handling. Unit tests in progress, please comment if you see something wrong.

Testing Done

Unit Tests

Issues

0
0
17
0
All Issues: 17

Description	From	Last Updated
I was trying to keep that list alphabetical...	pprkut	Dec. 7, 2012, 12:58 p.m.
not necessary	pprkut	Dec. 7, 2012, 12:58 p.m.
Code style	pprkut	Dec. 7, 2012, 12:58 p.m.
not necessary	pprkut	Dec. 7, 2012, 12:58 p.m.
Code style	pprkut	Dec. 7, 2012, 12:58 p.m.
not necessary	pprkut	Dec. 7, 2012, 12:58 p.m.
this should return NULL	pprkut	Jan. 8, 2013, 3:04 p.m.
early exit	pprkut	Jan. 8, 2013, 3:23 p.m.
5.4	pprkut	Jan. 10, 2013, 9:48 a.m.
not in between __construct() and __destruct()	pprkut	Jan. 10, 2013, 9:48 a.m.
This is not sufficient, since you don't test the passed parameters. Please add tests for expected and unexpected parameters.	pprkut	Jan. 10, 2013, 2:38 p.m.
'set' is already public	pprkut	Jan. 10, 2013, 9:51 a.m.
I'd add an assertArrayHasKey or whatever it's called before	pprkut	Jan. 10, 2013, 2:38 p.m.
not necessary, just declare it @runInSeparateProcess	pprkut	Jan. 10, 2013, 2:53 p.m.
ok, so just to be sure, you really only need this if the method is protected or private	pprkut	Jan. 10, 2013, 2:53 p.m.
wrong name	pprkut	Jan. 10, 2013, 2:53 p.m.
wrong name	pprkut	Jan. 10, 2013, 2:54 p.m.

Looks fine and simple, I would say ship it!

Ship it!

Ship It!

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

No no no no no no nooooooo!! no shipits without unittests!!

system/libraries/core/class.session.inc.php (Diff revision 1)
Show all issues
```
I was trying to keep that list alphabetical...
```
system/libraries/core/class.session.inc.php (Diff revision 1)
Show all issues
```
not necessary
```
system/libraries/core/class.session.inc.php (Diff revision 1)
Show all issues
```
Code style
```
system/libraries/core/class.session.inc.php (Diff revision 1)
Show all issues
```
not necessary
```
system/libraries/core/class.session.inc.php (Diff revision 1)
Show all issues
```
Code style
```
system/libraries/core/class.session.inc.php (Diff revision 1)
Show all issues
```
not necessary
```

Change Summary:

Fixes Fixes

Diff:

Revision 2 (+15 -16)

Show changes

system/libraries/core/class.session.inc.php

Change Summary:

PSR0 implemented
Unit Tests

Branch:

-	https://github.com/madsmash/lunr/tree/sessionHandler
+	https://github.com/madsmash/lunr/tree/newSessionHandler

Diff:

Revision 3 (+560 -16)

Show changes

	src/Lunr/Core/Session.php
	src/Lunr/Core/Tests/SessionBaseTest.php
	src/Lunr/Core/Tests/SessionTest.php
	tests/phpunit.xml

Branch:

-	https://github.com/madsmash/lunr/tree/newSessionHandler
+	master

```
Mostly fine
```
src/Lunr/Core/Session.php (Diff revision 3)
Show all issues
```
this should return NULL
```
src/Lunr/Core/Session.php (Diff revision 3)
Show all issues
```
early exit
```

src/Lunr/Core/Tests/SessionTest.php (Diff revision 3)

I have the feeling phpcs will complain here.

Felipe Jan. 8, 2013, 3:23 p.m.

yep, it complained because I didn't have a white line between this constant and the first function :P

Change Summary:

Code Style checks and comments from review

Testing Done:

Unit Tests

Diff:

Revision 4 (+593 -27)

Show changes

	src/Lunr/Core/Session.php
	src/Lunr/Core/Tests/SessionBaseTest.php
	src/Lunr/Core/Tests/SessionTest.php
	tests/phpunit.xml

Change Summary:

Small fix in unit tests

Diff:

Revision 5 (+592 -27)

Show changes

	src/Lunr/Core/Session.php
	src/Lunr/Core/Tests/SessionBaseTest.php
	src/Lunr/Core/Tests/SessionTest.php
	tests/phpunit.xml

src/Lunr/Core/Session.php (Diff revision 5)
Show all issues
```
5.4
```

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

Show all issues

not in between __construct() and __destruct()

src/Lunr/Core/Tests/SessionBaseTest.php (Diff revision 5)

Show all issues

This is not sufficient, since you don't test the passed parameters. Please add tests for expected and unexpected parameters.

src/Lunr/Core/Tests/SessionBaseTest.php (Diff revision 5)
Show all issues
```
'set' is already public
```
src/Lunr/Core/Tests/SessionBaseTest.php (Diff revision 5)
```
ditto
```
src/Lunr/Core/Tests/SessionBaseTest.php (Diff revision 5)
```
ditto
```
src/Lunr/Core/Tests/SessionBaseTest.php (Diff revision 5)
```
ditto
```
src/Lunr/Core/Tests/SessionBaseTest.php (Diff revision 5)
Show all issues
```
I'd add an assertArrayHasKey or whatever it's called before
```
src/Lunr/Core/Tests/SessionBaseTest.php (Diff revision 5)
Show all issues
```
not necessary, just declare it @runInSeparateProcess
```

src/Lunr/Core/Tests/SessionBaseTest.php (Diff revision 5)

Show all issues

ok, so just to be sure, you really only need this if the method is protected or private

src/Lunr/Core/Tests/SessionBaseTest.php (Diff revision 5)

Show all issues

wrong name

Felipe Jan. 9, 2013, 6:18 p.m.

could I get an example that your Majesty likes? :P 

http://devopsreactions.tumblr.com/post/37780353353/code-review-reviewee

pprkut Jan. 9, 2013, 6:22 p.m.
```
well, it returns NULL not FALSE :P
```

Felipe Jan. 10, 2013, 9:41 a.m.

I realized that just a few seconds after replying.... (facepalm)

src/Lunr/Core/Tests/SessionBaseTest.php (Diff revision 5)
Show all issues
```
wrong name
```

Change Summary:

Review comments fixed!

Diff:

Revision 6 (+623 -28)

Show changes

	src/Lunr/Core/Session.php
	src/Lunr/Core/Tests/SessionBaseTest.php
	src/Lunr/Core/Tests/SessionTest.php
	tests/phpunit.xml

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Merged into master

You have a pending review.

Review Board 3.0.24

[Core] Session class

Change Summary:

Diff:

Change Summary:

Branch:

Diff:

Branch:

Change Summary:

Testing Done:

Diff:

Change Summary:

Diff:

Change Summary:

Diff:

Status: Closed (submitted)

Change Summary: