bug #21458 [Config] Early return for DirectoryResource (robfrawley)

fabpot · fabpot · commit 2c302cee1993 · 2017-02-12T20:12:54.000+01:00
This PR was squashed before being merged into the 2.7 branch (closes #21458). Discussion ---------- [Config] Early return for DirectoryResource | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | sure? | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | | Related PRs | #21440 | License | MIT | Doc PR | n/a Alternate PR that implements an early return for `DirectoryResource` to increase the speed on large file sets. We can never return early with `true` without checking all assets within the resource, as the aforementioned referenced PR did; hence this PR takes the counter approach and returns `false` early where appropriate. _Conversation about possible bug at #21458 (comment) Commits ------- d5746ec fix directory resource considers same timestamp not fresh 96107e2 return false early from directory resource
diff --git a/src/Symfony/Component/Config/Resource/DirectoryResource.php b/src/Symfony/Component/Config/Resource/DirectoryResource.php
@@ -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())) {
@@ -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()
diff --git a/src/Symfony/Component/Config/Tests/Resource/DirectoryResourceTest.php b/src/Symfony/Component/Config/Tests/Resource/DirectoryResourceTest.php
@@ -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');
     }
 
     public function testIsFreshDeleteDirectory()

Original file line number	Diff line number	Diff line change
`@@ -68,7 +68,10 @@ public function isFresh($timestamp)`
`68`	`68`	`return false;`
`69`	`69`	`}`
`70`	`70`
`71`		`- $newestMTime = filemtime($this->resource);`
	`71`	`+ if ($timestamp < filemtime($this->resource)) {`
	`72`	`+ return false;`
	`73`	`+ }`
	`74`	`+`
`72`	`75`	`foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($this->resource), \RecursiveIteratorIterator::SELF_FIRST) as $file) {`
`73`	`76`	`// if regex filtering is enabled only check matching files`
`74`	`77`	`if ($this->pattern && $file->isFile() && !preg_match($this->pattern, $file->getBasename())) {`
`@@ -81,10 +84,13 @@ public function isFresh($timestamp)`
`81`	`84`	`continue;`
`82`	`85`	`}`
`83`	`86`
`84`		`- $newestMTime = max($file->getMTime(), $newestMTime);`
	`87`	`+ // early return if a file's mtime exceeds the passed timestamp`
	`88`	`+ if ($timestamp < $file->getMTime()) {`
	`89`	`+ return false;`
	`90`	`+ }`
`85`	`91`	`}`
`86`	`92`
`87`		`- return $newestMTime < $timestamp;`
	`93`	`+ return true;`
`88`	`94`	`}`
`89`	`95`
`90`	`96`	`public function serialize()`
Original file line number	Diff line number	Diff line change
`@@ -96,8 +96,10 @@ public function testIsFreshNewFileWithDifferentPattern()`
`96`	`96`	`public function testIsFreshDeleteFile()`
`97`	`97`	`{`
`98`	`98`	`$resource = new DirectoryResource($this->directory);`
	`99`	`+ $time = time();`
	`100`	`+ sleep(1);`
`99`	`101`	`unlink($this->directory.'/tmp.xml');`
`100`		`- $this->assertFalse($resource->isFresh(time()), '->isFresh() returns false if an existing file is removed');`
	`102`	`+ $this->assertFalse($resource->isFresh($time), '->isFresh() returns false if an existing file is removed');`
`101`	`103`	`}`
`102`	`104`
`103`	`105`	`public function testIsFreshDeleteDirectory()`