From 532160026fb98f7ec7bd83c740d5500ba5994d19 Mon Sep 17 00:00:00 2001 From: Jason Desrosiers Date: Sun, 19 May 2013 11:41:10 -0700 Subject: [PATCH 1/5] Fixed two bugs in HttpCache 1. 304 responses always send "Content-Type: text/html; charset=UTF-8" header I discovered that the HttpCache::handle method calls Response::prepare after calling Response::isModified. Response::isModified removes the Content-Type header as it should, but Response::handle adds in the default Content-Type header when none is set. If the default Content-Type is not the correct Content-Type, then the Content-Type in the cache gets clobered. I solved this problem by moving the Response::isModified call after the Response::prepare call. I updated the testRespondsWith304WhenIfModifiedSinceMatchesLastModified and testRespondsWith304WhenIfNoneMatchMatchesETag tests to verify that the Content-Type header was not being sent for 304 responses. 2. Failure to invalidate cached entities referred to by the Location header I discovered that the Store::invalidate method was looking for Location and Content-Location headers to invalidate, but it was looking in the request headers instead of the response headers. Because the Store::invalidate method doesn't take a response, I decided it was better to move this logic to the HttpCache::invalidate method instead. I updated the testInvalidatesCachedResponsesOnPost test to verify that Location headers are getting invalidated correctly. --- .../Component/HttpKernel/HttpCache/HttpCache.php | 13 +++++++++++-- .../Component/HttpKernel/HttpCache/Store.php | 9 --------- .../HttpKernel/Tests/HttpCache/HttpCacheTest.php | 6 +++--- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php b/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php index 2592d3b171..e3d8c29fe6 100644 --- a/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php +++ b/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php @@ -193,8 +193,6 @@ class HttpCache implements HttpKernelInterface, TerminableInterface $response = $this->lookup($request, $catch); } - $response->isNotModified($request); - $this->restoreResponseBody($request, $response); $response->setDate(new \DateTime(null, new \DateTimeZone('UTC'))); @@ -213,6 +211,8 @@ class HttpCache implements HttpKernelInterface, TerminableInterface $response->prepare($request); + $response->isNotModified($request); + return $response; } @@ -262,6 +262,15 @@ class HttpCache implements HttpKernelInterface, TerminableInterface try { $this->store->invalidate($request, $catch); + // As per the RFC, invalidate Location and Content-Location URLs if present + foreach (array('Location', 'Content-Location') as $header) { + if ($uri = $response->headers->get($header)) { + $subRequest = $request::create($uri, 'get', array(), array(), array(), $request->server->all()); + + $this->store->invalidate($subRequest); + } + } + $this->record($request, 'invalidate'); } catch (\Exception $e) { $this->record($request, 'invalidate-failed'); diff --git a/src/Symfony/Component/HttpKernel/HttpCache/Store.php b/src/Symfony/Component/HttpKernel/HttpCache/Store.php index f7d4e2f1c3..b2caff64cc 100644 --- a/src/Symfony/Component/HttpKernel/HttpCache/Store.php +++ b/src/Symfony/Component/HttpKernel/HttpCache/Store.php @@ -231,15 +231,6 @@ class Store implements StoreInterface throw new \RuntimeException('Unable to store the metadata.'); } } - - // As per the RFC, invalidate Location and Content-Location URLs if present - foreach (array('Location', 'Content-Location') as $header) { - if ($uri = $request->headers->get($header)) { - $subRequest = $request::create($uri, 'get', array(), array(), array(), $request->server->all()); - - $this->invalidate($subRequest); - } - } } /** diff --git a/src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php b/src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php index 448ebc362b..a8064b832b 100644 --- a/src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php +++ b/src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php @@ -145,7 +145,7 @@ class HttpCacheTest extends HttpCacheTestCase $this->assertHttpKernelIsCalled(); $this->assertEquals(304, $this->response->getStatusCode()); - $this->assertEquals('text/html; charset=UTF-8', $this->response->headers->get('Content-Type')); + $this->assertEquals('', $this->response->headers->get('Content-Type')); $this->assertEmpty($this->response->getContent()); $this->assertTraceContains('miss'); $this->assertTraceContains('store'); @@ -158,7 +158,7 @@ class HttpCacheTest extends HttpCacheTestCase $this->assertHttpKernelIsCalled(); $this->assertEquals(304, $this->response->getStatusCode()); - $this->assertEquals('text/html; charset=UTF-8', $this->response->headers->get('Content-Type')); + $this->assertEquals('', $this->response->headers->get('Content-Type')); $this->assertTrue($this->response->headers->has('ETag')); $this->assertEmpty($this->response->getContent()); $this->assertTraceContains('miss'); @@ -845,7 +845,7 @@ class HttpCacheTest extends HttpCacheTestCase $this->assertTraceContains('fresh'); // now POST to same URL - $this->request('POST', '/'); + $this->request('POST', '/helloworld'); $this->assertHttpKernelIsCalled(); $this->assertEquals('/', $this->response->headers->get('Location')); $this->assertTraceContains('invalidate'); From 97b38edeff6c7c6451dec93afa4e2585630c3440 Mon Sep 17 00:00:00 2001 From: Rich Sage Date: Wed, 22 May 2013 11:07:07 +0100 Subject: [PATCH 2/5] Added type of return value in VoterInterface. --- .../Security/Core/Authorization/Voter/VoterInterface.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php b/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php index 1fc93e50cc..0840c1c622 100644 --- a/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php +++ b/src/Symfony/Component/Security/Core/Authorization/Voter/VoterInterface.php @@ -38,7 +38,7 @@ interface VoterInterface * * @param string $class A class name * - * @return true if this Voter can process the class + * @return Boolean true if this Voter can process the class */ public function supportsClass($class); From 45b68e02bd5838c0288db360ce117b714f9ceaca Mon Sep 17 00:00:00 2001 From: alquerci Date: Fri, 24 May 2013 10:59:59 +0200 Subject: [PATCH 3/5] [Finder] Fix unexpected duplicate sub path related AppendIterator issue --- .../Iterator/RecursiveDirectoryIterator.php | 8 ++++ .../Component/Finder/Tests/FinderTest.php | 26 +++++++++++++ .../Component/Finder/Tests/Fixtures/one/a | 0 .../Finder/Tests/Fixtures/one/b/c.neon | 0 .../Finder/Tests/Fixtures/one/b/d.neon | 0 .../Tests/Iterator/IteratorTestCase.php | 37 +++++++++++++++++++ 6 files changed, 71 insertions(+) create mode 100644 src/Symfony/Component/Finder/Tests/Fixtures/one/a create mode 100644 src/Symfony/Component/Finder/Tests/Fixtures/one/b/c.neon create mode 100644 src/Symfony/Component/Finder/Tests/Fixtures/one/b/d.neon diff --git a/src/Symfony/Component/Finder/Iterator/RecursiveDirectoryIterator.php b/src/Symfony/Component/Finder/Iterator/RecursiveDirectoryIterator.php index 508d11bf3c..5e54d6fedd 100644 --- a/src/Symfony/Component/Finder/Iterator/RecursiveDirectoryIterator.php +++ b/src/Symfony/Component/Finder/Iterator/RecursiveDirectoryIterator.php @@ -38,4 +38,12 @@ class RecursiveDirectoryIterator extends \RecursiveDirectoryIterator { return new SplFileInfo(parent::current()->getPathname(), $this->getSubPath(), $this->getSubPathname()); } + + public function rewind() + { + // @see https://bugs.php.net/bug.php?id=49104 + parent::next(); + + parent::rewind(); + } } diff --git a/src/Symfony/Component/Finder/Tests/FinderTest.php b/src/Symfony/Component/Finder/Tests/FinderTest.php index ec7db1ec06..4f419698e9 100644 --- a/src/Symfony/Component/Finder/Tests/FinderTest.php +++ b/src/Symfony/Component/Finder/Tests/FinderTest.php @@ -459,4 +459,30 @@ class FinderTest extends Iterator\RealIteratorTestCase $this->assertEquals(1, count($finder)); } + + /** + * Searching in multiple locations with sub directories involves + * AppendIterator which does an unnecessary rewind which leaves + * FilterIterator with inner FilesystemIterator in an ivalid state. + * + * @see https://bugs.php.net/bug.php?id=49104 + */ + public function testMultipleLocationsWithSubDirectories() + { + $locations = array( + __DIR__.'/Fixtures/one', + self::$files[8], + ); + + $finder = new Finder(); + $finder->in($locations)->depth('< 10')->name('*.neon'); + + $expected = array( + __DIR__.'/Fixtures/one'.DIRECTORY_SEPARATOR.'b'.DIRECTORY_SEPARATOR.'c.neon', + __DIR__.'/Fixtures/one'.DIRECTORY_SEPARATOR.'b'.DIRECTORY_SEPARATOR.'d.neon', + ); + + $this->assertIterator($expected, $finder); + $this->assertIteratorInForeach($expected, $finder); + } } diff --git a/src/Symfony/Component/Finder/Tests/Fixtures/one/a b/src/Symfony/Component/Finder/Tests/Fixtures/one/a new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/Symfony/Component/Finder/Tests/Fixtures/one/b/c.neon b/src/Symfony/Component/Finder/Tests/Fixtures/one/b/c.neon new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/Symfony/Component/Finder/Tests/Fixtures/one/b/d.neon b/src/Symfony/Component/Finder/Tests/Fixtures/one/b/d.neon new file mode 100644 index 0000000000..e69de29bb2 diff --git a/src/Symfony/Component/Finder/Tests/Iterator/IteratorTestCase.php b/src/Symfony/Component/Finder/Tests/Iterator/IteratorTestCase.php index 8810ce757d..845fc2c513 100644 --- a/src/Symfony/Component/Finder/Tests/Iterator/IteratorTestCase.php +++ b/src/Symfony/Component/Finder/Tests/Iterator/IteratorTestCase.php @@ -29,4 +29,41 @@ abstract class IteratorTestCase extends \PHPUnit_Framework_TestCase $this->assertEquals($expected, array_values($values)); } + + /** + * Same as IteratorTestCase::assertIterator with foreach usage + * + * @param array $expected + * @param \Traversable $iterator + */ + protected function assertIteratorInForeach($expected, \Traversable $iterator) + { + $values = array(); + foreach ($iterator as $file) { + $this->assertInstanceOf('Symfony\\Component\\Finder\\SplFileInfo', $file); + $values[] = $file->getPathname(); + } + + sort($values); + sort($expected); + + $this->assertEquals($expected, array_values($values)); + } + + /** + * Same as IteratorTestCase::assertOrderedIterator with foreach usage + * + * @param array $expected + * @param \Traversable $iterator + */ + protected function assertOrderedIteratorInForeach($expected, \Traversable $iterator) + { + $values = array(); + foreach ($iterator as $file) { + $this->assertInstanceOf('Symfony\\Component\\Finder\\SplFileInfo', $file); + $values[] = $file->getPathname(); + } + + $this->assertEquals($expected, array_values($values)); + } } From 169c0b93b5d90dc5d42c4316dd8016835d87934a Mon Sep 17 00:00:00 2001 From: alquerci Date: Fri, 24 May 2013 12:46:33 +0200 Subject: [PATCH 4/5] [Finder] Fix iteration fails with non-rewindable streams --- .../Finder/Iterator/FilterIterator.php | 9 +- .../Iterator/RecursiveDirectoryIterator.php | 40 +++++++++ .../Component/Finder/Tests/FinderTest.php | 17 ++++ .../Tests/Iterator/IteratorTestCase.php | 37 +++++++++ .../RecursiveDirectoryIteratorTest.php | 83 +++++++++++++++++++ 5 files changed, 185 insertions(+), 1 deletion(-) create mode 100644 src/Symfony/Component/Finder/Tests/Iterator/RecursiveDirectoryIteratorTest.php diff --git a/src/Symfony/Component/Finder/Iterator/FilterIterator.php b/src/Symfony/Component/Finder/Iterator/FilterIterator.php index 64da508ebd..f4da44c4cd 100644 --- a/src/Symfony/Component/Finder/Iterator/FilterIterator.php +++ b/src/Symfony/Component/Finder/Iterator/FilterIterator.php @@ -30,7 +30,14 @@ abstract class FilterIterator extends \FilterIterator { $iterator = $this; while ($iterator instanceof \OuterIterator) { - if ($iterator->getInnerIterator() instanceof \FilesystemIterator) { + $innerIterator = $iterator->getInnerIterator(); + + if ($innerIterator instanceof RecursiveDirectoryIterator) { + if ($innerIterator->isRewindable()) { + $innerIterator->next(); + $innerIterator->rewind(); + } + } elseif ($iterator->getInnerIterator() instanceof \FilesystemIterator) { $iterator->getInnerIterator()->next(); $iterator->getInnerIterator()->rewind(); } diff --git a/src/Symfony/Component/Finder/Iterator/RecursiveDirectoryIterator.php b/src/Symfony/Component/Finder/Iterator/RecursiveDirectoryIterator.php index 508d11bf3c..94f61b7a34 100644 --- a/src/Symfony/Component/Finder/Iterator/RecursiveDirectoryIterator.php +++ b/src/Symfony/Component/Finder/Iterator/RecursiveDirectoryIterator.php @@ -20,6 +20,11 @@ use Symfony\Component\Finder\SplFileInfo; */ class RecursiveDirectoryIterator extends \RecursiveDirectoryIterator { + /** + * @var Boolean + */ + private $rewindable; + public function __construct($path, $flags) { if ($flags & (self::CURRENT_AS_PATHNAME | self::CURRENT_AS_SELF)) { @@ -38,4 +43,39 @@ class RecursiveDirectoryIterator extends \RecursiveDirectoryIterator { return new SplFileInfo(parent::current()->getPathname(), $this->getSubPath(), $this->getSubPathname()); } + + /** + * Do nothing for non rewindable stream + */ + public function rewind() + { + if (false === $this->isRewindable()) { + return; + } + + parent::rewind(); + } + + /** + * Checks if the stream is rewindable. + * + * @return Boolean true when the stream is rewindable, false otherwise + */ + public function isRewindable() + { + if (null !== $this->rewindable) { + return $this->rewindable; + } + + if (false !== $stream = @opendir($this->getPath())) { + $infos = stream_get_meta_data($stream); + closedir($stream); + + if ($infos['seekable']) { + return $this->rewindable = true; + } + } + + return $this->rewindable = false; + } } diff --git a/src/Symfony/Component/Finder/Tests/FinderTest.php b/src/Symfony/Component/Finder/Tests/FinderTest.php index ec7db1ec06..40d18be0e8 100644 --- a/src/Symfony/Component/Finder/Tests/FinderTest.php +++ b/src/Symfony/Component/Finder/Tests/FinderTest.php @@ -459,4 +459,21 @@ class FinderTest extends Iterator\RealIteratorTestCase $this->assertEquals(1, count($finder)); } + + public function testNonSeekableStream() + { + try { + $i = Finder::create()->in('ftp://ftp.mozilla.org/')->depth(0)->getIterator(); + } catch (\UnexpectedValueException $e) { + $this->markTestSkipped(sprintf('Unsupported stream "%s".', 'ftp')); + } + + $contains = array( + 'ftp://ftp.mozilla.org'.DIRECTORY_SEPARATOR.'README', + 'ftp://ftp.mozilla.org'.DIRECTORY_SEPARATOR.'index.html', + 'ftp://ftp.mozilla.org'.DIRECTORY_SEPARATOR.'pub', + ); + + $this->assertIteratorInForeach($contains, $i); + } } diff --git a/src/Symfony/Component/Finder/Tests/Iterator/IteratorTestCase.php b/src/Symfony/Component/Finder/Tests/Iterator/IteratorTestCase.php index 8810ce757d..845fc2c513 100644 --- a/src/Symfony/Component/Finder/Tests/Iterator/IteratorTestCase.php +++ b/src/Symfony/Component/Finder/Tests/Iterator/IteratorTestCase.php @@ -29,4 +29,41 @@ abstract class IteratorTestCase extends \PHPUnit_Framework_TestCase $this->assertEquals($expected, array_values($values)); } + + /** + * Same as IteratorTestCase::assertIterator with foreach usage + * + * @param array $expected + * @param \Traversable $iterator + */ + protected function assertIteratorInForeach($expected, \Traversable $iterator) + { + $values = array(); + foreach ($iterator as $file) { + $this->assertInstanceOf('Symfony\\Component\\Finder\\SplFileInfo', $file); + $values[] = $file->getPathname(); + } + + sort($values); + sort($expected); + + $this->assertEquals($expected, array_values($values)); + } + + /** + * Same as IteratorTestCase::assertOrderedIterator with foreach usage + * + * @param array $expected + * @param \Traversable $iterator + */ + protected function assertOrderedIteratorInForeach($expected, \Traversable $iterator) + { + $values = array(); + foreach ($iterator as $file) { + $this->assertInstanceOf('Symfony\\Component\\Finder\\SplFileInfo', $file); + $values[] = $file->getPathname(); + } + + $this->assertEquals($expected, array_values($values)); + } } diff --git a/src/Symfony/Component/Finder/Tests/Iterator/RecursiveDirectoryIteratorTest.php b/src/Symfony/Component/Finder/Tests/Iterator/RecursiveDirectoryIteratorTest.php new file mode 100644 index 0000000000..f762514346 --- /dev/null +++ b/src/Symfony/Component/Finder/Tests/Iterator/RecursiveDirectoryIteratorTest.php @@ -0,0 +1,83 @@ + +* +* For the full copyright and license information, please view the LICENSE +* file that was distributed with this source code. +*/ + +namespace Symfony\Component\Finder\Tests\Iterator; + +use Symfony\Component\Finder\Iterator\RecursiveDirectoryIterator; + +class RecursiveDirectoryIteratorTest extends IteratorTestCase +{ + /** + * @dataProvider getPaths + * + * @param string $path + * @param Boolean $seekable + * @param Boolean $supports + * @param string $message + */ + public function testRewind($path, $seekable, $contains, $message = null) + { + try { + $i = new RecursiveDirectoryIterator($path, \RecursiveDirectoryIterator::SKIP_DOTS); + } catch (\UnexpectedValueException $e) { + $this->markTestSkipped(sprintf('Unsupported stream "%s".', $path)); + } + + $i->rewind(); + + $this->assertTrue(true, $message); + } + + /** + * @dataProvider getPaths + * + * @param string $path + * @param Boolean $seekable + * @param Boolean $supports + * @param string $message + */ + public function testSeek($path, $seekable, $contains, $message = null) + { + try { + $i = new RecursiveDirectoryIterator($path, \RecursiveDirectoryIterator::SKIP_DOTS); + } catch (\UnexpectedValueException $e) { + $this->markTestSkipped(sprintf('Unsupported stream "%s".', $path)); + } + + $actual = array(); + + $i->seek(0); + $actual[] = $i->getPathname(); + + $i->seek(1); + $actual[] = $i->getPathname(); + + $i->seek(2); + $actual[] = $i->getPathname(); + + $this->assertEquals($contains, $actual); + } + + public function getPaths() + { + $data = array(); + + // ftp + $contains = array( + 'ftp://ftp.mozilla.org'.DIRECTORY_SEPARATOR.'README', + 'ftp://ftp.mozilla.org'.DIRECTORY_SEPARATOR.'index.html', + 'ftp://ftp.mozilla.org'.DIRECTORY_SEPARATOR.'pub', + ); + $data[] = array('ftp://ftp.mozilla.org/', false, $contains); + + return $data; + } +} From 52fed7b1d20974378beee2d268c57fb1e7d47e70 Mon Sep 17 00:00:00 2001 From: John Bafford Date: Sun, 26 May 2013 14:42:07 -0400 Subject: [PATCH 5/5] Fix several instances of doubled words One in an exception; the rest in docblocks. --- .../Doctrine/DependencyInjection/AbstractDoctrineExtension.php | 2 +- src/Symfony/Component/Console/Formatter/OutputFormatter.php | 2 +- src/Symfony/Component/HttpFoundation/Response.php | 2 +- src/Symfony/Component/HttpKernel/HttpCache/Store.php | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Symfony/Bridge/Doctrine/DependencyInjection/AbstractDoctrineExtension.php b/src/Symfony/Bridge/Doctrine/DependencyInjection/AbstractDoctrineExtension.php index 00f6a40f2e..255f72c691 100644 --- a/src/Symfony/Bridge/Doctrine/DependencyInjection/AbstractDoctrineExtension.php +++ b/src/Symfony/Bridge/Doctrine/DependencyInjection/AbstractDoctrineExtension.php @@ -242,7 +242,7 @@ abstract class AbstractDoctrineExtension extends Extension if (!in_array($mappingConfig['type'], array('xml', 'yml', 'annotation', 'php', 'staticphp'))) { throw new \InvalidArgumentException(sprintf('Can only configure "xml", "yml", "annotation", "php" or '. '"staticphp" through the DoctrineBundle. Use your own bundle to configure other metadata drivers. '. - 'You can register them by adding a a new driver to the '. + 'You can register them by adding a new driver to the '. '"%s" service definition.', $this->getObjectManagerElementName($objectManagerName.'.metadata_driver') )); } diff --git a/src/Symfony/Component/Console/Formatter/OutputFormatter.php b/src/Symfony/Component/Console/Formatter/OutputFormatter.php index 2e7219b6df..72949fe9bd 100644 --- a/src/Symfony/Component/Console/Formatter/OutputFormatter.php +++ b/src/Symfony/Component/Console/Formatter/OutputFormatter.php @@ -239,7 +239,7 @@ class OutputFormatter implements OutputFormatterInterface * * @param string $text Input text * - * @return string string Styled text + * @return string Styled text */ private function applyCurrentStyle($text) { diff --git a/src/Symfony/Component/HttpFoundation/Response.php b/src/Symfony/Component/HttpFoundation/Response.php index 08ffd4e997..0105c6f942 100644 --- a/src/Symfony/Component/HttpFoundation/Response.php +++ b/src/Symfony/Component/HttpFoundation/Response.php @@ -690,7 +690,7 @@ class Response /** * Returns the number of seconds after the time specified in the response's Date - * header when the the response should no longer be considered fresh. + * header when the response should no longer be considered fresh. * * First, it checks for a s-maxage directive, then a max-age directive, and then it falls * back on an expires header. It returns null when no maximum age can be established. diff --git a/src/Symfony/Component/HttpKernel/HttpCache/Store.php b/src/Symfony/Component/HttpKernel/HttpCache/Store.php index b2caff64cc..57197f662f 100644 --- a/src/Symfony/Component/HttpKernel/HttpCache/Store.php +++ b/src/Symfony/Component/HttpKernel/HttpCache/Store.php @@ -241,7 +241,7 @@ class Store implements StoreInterface * @param array $env1 A Request HTTP header array * @param array $env2 A Request HTTP header array * - * @return Boolean true if the the two environments match, false otherwise + * @return Boolean true if the two environments match, false otherwise */ private function requestsMatch($vary, $env1, $env2) {