8000 [Config] Early return for DirectoryResource by robfrawley · Pull Request #21458 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Config] Early return for DirectoryResource #21458

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

Merged
merged 2 commits into from
Feb 12, 2017
Merged

[Config] Early return for DirectoryResource #21458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
96107e2
return false early from directory resource
robfrawley Jan 29, 2017
d5746ec
fix directory resource considers same timestamp not fresh
robfrawley Jan 30, 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
12 changes: 9 additions & 3 deletions src/Symfony/Component/Config/Resource/DirectoryResource.php
View file
Open in desktop
8000
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,10 @@ public function isFresh($timestamp)
return false;
}

$newestMTime = filemtime($this->resource);
if ($timestamp < filemtime($this->resource)) {
return false;
}

foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($this->resource), \RecursiveIteratorIterator::SELF_FIRST) as $file) {
// if regex filtering is enabled only check matching files
if ($this->pattern && $file->isFile() && !preg_match($this->pattern, $file->getBasename())) {
Expand All @@ -81,10 +84,13 @@ public function isFresh($timestamp)
continue;
}

$newestMTime = max($file->getMTime(), $newestMTime);
// early return if a file's mtime exceeds the passed timestamp
if ($timestamp < $file->getMTime()) {
return false;
}
}

return $newestMTime < $timestamp;
return true;
}

public function serialize()
Expand Down
4 changes: 3 additions & 1 deletion src/Symfony/Component/Config/Tests/Resource/DirectoryResourceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,10 @@ public function testIsFreshNewFileWithDifferentPattern()
public function testIsFreshDeleteFile()
{
$resource = new DirectoryResource($this->directory);
$time = time();
sleep(1);
unlink($this->directory.'/tmp.xml');
$this->assertFalse($resource->isFresh(time()), '->isFresh() returns false if an existing file is removed');
$this->assertFalse($resource->isFresh($time), '->isFresh() returns false if an existing file is removed');
Copy link
Contributor Author
@robfrawley robfrawley Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@nicolas-grekas This fails with the "bug fix" to not return false when timestamp are the same (unlike the original implementation). This test change fixes failures originating from this behavioral change...

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we move this test into the time-sensitive group?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch; I didn't know about that convention. I only see that group at the class-level. Is it okay to use it at the test-level?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that should work the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding that group causes the test to fail. What does that group do and why would it result in it failing?

Copy link
Contributor Author
@robfrawley robfrawley Jan 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@xabbuh We can't use that because the internal implementation only relates to the PHP interpreter. This relies on the filesystem and is external of the interpreter.

An added bonus of using the ClockMock class is that time passes instantly. Using PHP's sleep(10) will make your test wait for 10 actual seconds (more or less). In contrast, the ClockMock class advances the internal clock the given number of seconds without actually waiting that time, so your test will execute 10 seconds faster.
http://symfony.com/blog/new-in-symfony-2-8-clock-mocking-and-time-sensitive-tests

}

public function testIsFreshDeleteDirectory()
Expand Down
0