Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/is expired#58 #59

Merged
merged 8 commits into from
Oct 18, 2017
Merged

Feature/is expired#58 #59

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
718f79b
Added isExpired() method #58.
OGKevin Oct 12, 2017
24387ab
Merge branch 'develop' into feature/isExpired#58
OGKevin Oct 12, 2017
18d62ca
Some cleaning :fire: #58
OGKevin Oct 12, 2017
e2da44c
Removed double null check :clap: #58.
OGKevin Oct 13, 2017
4b30d69
Renamed to isSessionActive() :smirk: #58.
OGKevin Oct 13, 2017
80a3a70
Fixed the issue where client will cause infinite loop :clap: #58.
OGKevin Oct 14, 2017
91f95fb
Isset :fire:
OGKevin Oct 18, 2017
552ef46
Tiny styling cleanup
dnl-blkv Oct 18, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/Context/ApiContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,15 +273,19 @@ private function dropSessionContext()
*/
public function ensureSessionActive()
{
if (is_null($this->sessionContext)) {
return;
if (!is_null($this->sessionContext) && $this->isSessionExpired()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check for is_null($this->sessionContext) can go from here, because you do it in isSessionExpired already :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you follow my suggestion and change isSessionExpired to isSessionActive or hasActiveSession, Please do not forget to add an exclamation mark here (only reset if NOT has an active session :))

Copy link
Contributor Author

@OGKevin OGKevin Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mhmm AFAIK if we do this then if session is null it will run in an infinite loop resetting the session. At least that was my expiernce in Java 🤔 will double check to make sure.

Copy link
Contributor Author

@OGKevin OGKevin Oct 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yea! The infinite loop:

Maximum function nesting level of '256' reached, aborting!

And it also fails because:

 $timeExpiry = $this->sessionContext->getExpiryTime()->getTimestamp();

This will cause an error as of sessionContext is null! So therefore we need !is_null($this->sessionContext) in ensureSessionActive still 🤔. The null sessionContext will get handled higher up the code before a request is made.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah but then based on your suggestion below this can be fixed 👍

$this->resetSession();
}
}

/**
* @return bool
*/
public function isSessionExpired(): bool
{
$timeExpiry = $this->sessionContext->getExpiryTime()->getTimestamp();

if ($timeExpiry - time() < self::TIME_TO_SESSION_EXPIRY_MINIMUM_SECONDS) {
$this->resetSession();
}
return is_null($this->sessionContext) || $timeExpiry - time() < self::TIME_TO_SESSION_EXPIRY_MINIMUM_SECONDS;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you do is_null($this->sessionContext) check here, the method can be be opposite to what we've had and be called isSessionActive or hasActiveSession. This also fits well into our ensureSessionActive :)

Also, I'd go for an explanatory variable here. I missed it when initially built ensureSessionActive %).

So something like this:

if (is_null($this->sessionContext)) {
    return false;
} else {
    $timeExpiry = $this->sessionContext->getExpiryTime()->getTimestamp();
    $timeToExpiry = $timeExpiry - time();

    return self::TIME_TO_SESSION_EXPIRY_MINIMUM_SECONDS < $timeToExpiry;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bueno

}

/**
Expand Down