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

[Templating] Handle uri query strings passed to templating extension and helper #933

Closed
wants to merge 1 commit into from
Closed

[Templating] Handle uri query strings passed to templating extension and helper #933

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 11 additions & 2 deletions Templating/Helper/ImagineHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ public function __construct(CacheManager $cacheManager)
}

/**
* Gets the browser path for the image and filter to apply.
* Gets the browser path for the image and filter to apply. If your path inadvertently contains a query string
* - which might happen if you use asset versioning - the query string will be stripped from the path, the
* URL will be resolved using the path without query string, and the stripped query string will be appended to
* the resulting URL.
*
* @param string $path
* @param string $filter
Expand All @@ -40,7 +43,13 @@ public function __construct(CacheManager $cacheManager)
*/
public function filter($path, $filter, array $runtimeConfig = array())
{
return $this->cacheManager->getBrowserPath($path, $filter, $runtimeConfig);
$pathParts = explode('?', $path, 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd love to hear a better name than $pathParts 🙄

I tried list($path, $queryString), which looks a lot better, but doesn't work when the original path doesn't contain a question mark (undefined index 1)

Copy link

Choose a reason for hiding this comment

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

you can try this, to keep list function use.

list($path, $queryString) = array_pad(explode('?', $path, 2), 2, null);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works, but is quite hard to read IMO.

Copy link

Choose a reason for hiding this comment

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

I agree, $pathParts sounds betters.

Waiting this merge.

$url = $this->cacheManager->getBrowserPath($pathParts[0], $filter, $runtimeConfig);
if (empty($pathParts[1])) {
return $url;
}

return $url.(strpos($url, '?') ? '&' : '?').$pathParts[1];
}

/**
Expand Down
15 changes: 12 additions & 3 deletions Templating/ImagineExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,27 @@ public function getFilters()
}

/**
* Gets the browser path for the image and filter to apply.
* Gets the browser path for the image and filter to apply. If your path inadvertently contains a query string
* - which might happen if you use asset versioning - the query string will be stripped from the path, the
* URL will be resolved using the path without query string, and the stripped query string will be appended to
* the resulting URL.
*
* @param string $path
* @param string $filter
* @param array $runtimeConfig
* @param string $resolver
*
* @return \Twig_Markup
* @return string
*/
public function filter($path, $filter, array $runtimeConfig = array(), $resolver = null)
{
return $this->cacheManager->getBrowserPath($path, $filter, $runtimeConfig, $resolver);
$pathParts = explode('?', $path, 2);
$url = $this->cacheManager->getBrowserPath($pathParts[0], $filter, $runtimeConfig, $resolver);
if (empty($pathParts[1])) {
return $url;
}

return $url.(strpos($url, '?') ? '&' : '?').$pathParts[1];
}

/**
Expand Down
26 changes: 26 additions & 0 deletions Tests/Templating/Helper/ImagineHelperTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,30 @@ public function testProxyCallToCacheManagerOnFilter()

$this->assertEquals($expectedCachePath, $helper->filter($expectedPath, $expectedFilter));
}

public function testStripsQueryStringFromPathAndAppendToFinalUrl()
{
$cacheManager = $this->createCacheManagerMock();

$cacheManager
->method('getBrowserPath')
->will($this->returnValue('/resolved/abc.png'));

$extension = new ImagineHelper($cacheManager);

$this->assertEquals('/resolved/abc.png?v=123', $extension->filter('abc.png?v=123', 'foo'));
}

public function testAppendsQueryStringToExistingQueryStringInFinalUrl()
{
$cacheManager = $this->createCacheManagerMock();

$cacheManager
->method('getBrowserPath')
->will($this->returnValue('/resolved/abc.png?foo=bar'));

$extension = new ImagineHelper($cacheManager);

$this->assertEquals('/resolved/abc.png?foo=bar&v=123', $extension->filter('abc.png?v=123', 'foo'));
}
}
26 changes: 26 additions & 0 deletions Tests/Templating/ImagineExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,30 @@ public function testAddsFilterMethodToFiltersList()
$this->assertInternalType('array', $filters);
$this->assertCount(1, $filters);
}

public function testStripsQueryStringFromPathAndAppendToFinalUrl()
{
$cacheManager = $this->createCacheManagerMock();

$cacheManager
->method('getBrowserPath')
->will($this->returnValue('/resolved/abc.png'));

$extension = new ImagineExtension($cacheManager);

$this->assertEquals('/resolved/abc.png?v=123', $extension->filter('abc.png?v=123', 'foo'));
}

public function testAppendsQueryStringToExistingQueryStringInFinalUrl()
{
$cacheManager = $this->createCacheManagerMock();

$cacheManager
->method('getBrowserPath')
->will($this->returnValue('/resolved/abc.png?foo=bar'));

$extension = new ImagineExtension($cacheManager);

$this->assertEquals('/resolved/abc.png?foo=bar&v=123', $extension->filter('abc.png?v=123', 'foo'));
}
}