Refactor session management utilities #1005
Conversation
Relates to shaarli#324 Added: - `SessionManager` class to group session-related features - unit tests Changed: - `getToken()` -> `SessionManager->generateToken()` - `tokenOk()` -> `SessionManager->checkToken()` - inject a `$token` parameter to `PageBuilder`'s constructor Signed-off-by: VirtualTam <virtualtam@flibidi.net>
Relates to shaarli#324 Changed: - `is_session_id_valid()` -> `SessionManager::checkId()` - update tests Signed-off-by: VirtualTam <virtualtam@flibidi.net>
| @@ -165,7 +167,7 @@ | |||
| } | |||
|
|
|||
| // Display the installation form if no existing config is found | |||
| install($conf); | |||
| install($conf, $sessionManager); | |||
There was a problem hiding this comment.
What does this do? I assume it will run __construct() from SessionManager.php?
There was a problem hiding this comment.
$sessionManager is instantiated near the beginning of the script: https://github.com/shaarli/Shaarli/pull/1005/files#diff-828e0013b8f3bc1bb22b4f57172b019dR125
This instance is then passed and reused throughout the whole script :)
There was a problem hiding this comment.
Thanks, I've re-read the patch and now I understand
| /** | ||
| * Generate and check a session token | ||
| */ | ||
| public function testGenerateAndCheckToken() |
There was a problem hiding this comment.
Why 2 separate tests testGenerateToken() and testGenerateAndCheckToken()? Would it make sense to only have testGenerateAndCheckToken since it tests the whole process?
There was a problem hiding this comment.
There are two methods under test:
generateToken()checkToken()
The test for checkToken() could actually be rewritten not to rely upon calling generateToken(), which would result in:
- a unit test for
generateToken() - a unit test for
checkToken() - a functional test covering the overall sequence
There was a problem hiding this comment.
So your approach is shorter and still allows testing the 2 methods independently. 👍 Thanks
There was a problem hiding this comment.
Yup :) PR updated to improve tests according to the comment above
Signed-off-by: VirtualTam <virtualtam@flibidi.net>
| /** | ||
| * Fake ConfigManager | ||
| */ | ||
| class FakeConfigManager |
There was a problem hiding this comment.
I'd recommend to use an anonymous class (or a separate file) in this case to keep 1 file per class.
There was a problem hiding this comment.
Anonymous classes have been introduced with PHP 7: https://secure.php.net/manual/en/language.oop5.anonymous.php
The fake class could indeed be moved to a separate file, I think tests for Login & Session management might be moved to a dedicated folder in the future.
| * @param array $session The $_SESSION array (reference) | ||
| * @param ConfigManager $conf ConfigManager instance (reference) | ||
| */ | ||
| public function __construct(& $session, & $conf) |
There was a problem hiding this comment.
I don't see any reason to pass the ConfigManager instance as a reference.
Relates to shaarli#1005 Changed: - pass a copy of the ConfigManager instance instead of a reference - move FakeConfigManager to a dedicated file - update tests Signed-off-by: VirtualTam <virtualtam@flibidi.net>
Relates to #324
See commit messages for details