Skip to content
This repository has been archived by the owner on Jan 31, 2020. It is now read-only.

Use current time() instead of Request Time for the expiration TTL #85

Closed
wants to merge 4 commits into from
Closed

Use current time() instead of Request Time for the expiration TTL #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b516ad
Use current time() instead of Request Time for the expiration TTL
MatthiasKuehneEllerhold Aug 17, 2017
273e118
Unit-Tests
MatthiasKuehneEllerhold Aug 17, 2017
6d95a70
Add unit-test
MatthiasKuehneEllerhold Aug 17, 2017
be48bf9
Fix Unit-Test so that it correctly tests for the new behavior and fai…
MatthiasKuehneEllerhold Nov 29, 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
2 changes: 1 addition & 1 deletion src/AbstractContainer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,7 +509,7 @@ public function getIterator()
public function setExpirationSeconds($ttl, $vars = null)
{
$storage = $this->getStorage();
$ts = $_SERVER['REQUEST_TIME'] + $ttl;
$ts = time() + $ttl;
if (is_scalar($vars) && null !== $vars) {
$vars = (array) $vars;
}
Expand Down
39 changes: 33 additions & 6 deletions test/ContainerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,22 +193,24 @@ public function testContainerWritesToStorage()

public function testSettingExpirationSecondsUpdatesStorageMetadataForFullContainer()
{
$currentTimestamp = time();
$this->container->setExpirationSeconds(3600);
$storage = $this->manager->getStorage();
$metadata = $storage->getMetadata($this->container->getName());
$this->assertArrayHasKey('EXPIRE', $metadata);
$this->assertEquals($_SERVER['REQUEST_TIME'] + 3600, $metadata['EXPIRE']);
$this->assertEquals($currentTimestamp + 3600, $metadata['EXPIRE']);
}

public function testSettingExpirationSecondsForIndividualKeyUpdatesStorageMetadataForThatKey()
{
$this->container->foo = 'bar';
$currentTimestamp = time();
$this->container->setExpirationSeconds(3600, 'foo');
$storage = $this->manager->getStorage();
$metadata = $storage->getMetadata($this->container->getName());
$this->assertArrayHasKey('EXPIRE_KEYS', $metadata);
$this->assertArrayHasKey('foo', $metadata['EXPIRE_KEYS']);
$this->assertEquals($_SERVER['REQUEST_TIME'] + 3600, $metadata['EXPIRE_KEYS']['foo']);
$this->assertEquals($currentTimestamp + 3600, $metadata['EXPIRE_KEYS']['foo']);
}

public function testMultipleCallsToExpirationSecondsAggregates()
Expand All @@ -217,15 +219,40 @@ public function testMultipleCallsToExpirationSecondsAggregates()
$this->container->bar = 'baz';
$this->container->baz = 'bat';
$this->container->bat = 'bas';
$currentTimestamp = time();
$this->container->setExpirationSeconds(3600);
$this->container->setExpirationSeconds(1800, 'foo');
$this->container->setExpirationSeconds(900, ['baz', 'bat']);
$storage = $this->manager->getStorage();
$metadata = $storage->getMetadata($this->container->getName());
$this->assertEquals($_SERVER['REQUEST_TIME'] + 1800, $metadata['EXPIRE_KEYS']['foo']);
$this->assertEquals($_SERVER['REQUEST_TIME'] + 900, $metadata['EXPIRE_KEYS']['baz']);
$this->assertEquals($_SERVER['REQUEST_TIME'] + 900, $metadata['EXPIRE_KEYS']['bat']);
$this->assertEquals($_SERVER['REQUEST_TIME'] + 3600, $metadata['EXPIRE']);
$this->assertEquals($currentTimestamp + 1800, $metadata['EXPIRE_KEYS']['foo']);
$this->assertEquals($currentTimestamp + 900, $metadata['EXPIRE_KEYS']['baz']);
$this->assertEquals($currentTimestamp + 900, $metadata['EXPIRE_KEYS']['bat']);
$this->assertEquals($currentTimestamp + 3600, $metadata['EXPIRE']);
}

public function testSettingExpirationSecondsUsesCurrentTime()
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, this test doesn't prove the fix.

To test, I reverted the change you provided (s/time()/$_SERVER['REQUEST_TIME']/), and ran just this test; it passed. I tried resetting the sleep and expiration seconds values as well, but they always passed. I also tried putting the assignment prior to the setExpirationSeconds() method, and that did not work, either.

While I do not doubt that the fix is correct, we need an accurate way to test it that emulates what happens in a normal server-side web request so that we can ensure the fix is correct and will not break with future updates.

{
sleep(3);
$this->container->setExpirationSeconds(2);
$this->container->foo = 'bar';

// Simulate a second request, backup the request time and overwrite it with current time()
$requestTimeBackup = $_SERVER['REQUEST_TIME'];
$requestTimeFloatBackup = $_SERVER['REQUEST_TIME_FLOAT'] ?? null;
Copy link
Member

Choose a reason for hiding this comment

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

This library still supports PHP 5.6, so we'll need to do an isset() ? ... : null here. I can do that on merge, though.


$_SERVER['REQUEST_TIME'] = time();
$_SERVER['REQUEST_TIME_FLOAT'] = microtime(true);

try {
$this->assertEquals('bar', $this->container->foo);
} catch (\Throwable $e) {
// Restore the original values
$_SERVER['REQUEST_TIME'] = $requestTimeBackup;
$_SERVER['REQUEST_TIME_FLOAT'] = $requestTimeFloatBackup;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to do this; PHPUnit restores superglobals between test executions by default, IIRC.

I'll try removing it locally when I merge.


throw $e;
}
}

public function testPassingUnsetKeyToSetExpirationSecondsDoesNothing()
Expand Down