Skip to content

[WIP][2.3][FrameworkBundle][Templating] Generate assets with absolute url #7722

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

Closed
wants to merge 5 commits into from
Closed

[WIP][2.3][FrameworkBundle][Templating] Generate assets with absolute url #7722

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c488d5c
Added boilerplate code for generating absolute url assets
inoryy Apr 19, 2013
da3513a
Added tests for AbsoluteUrlPackage
inoryy Apr 19, 2013
89c47e9
Implementation for AbsoluteUrlPackage::getUrl()
inoryy Apr 19, 2013
f76a244
Configure new AbsoluteUrlPackage & use it inside AssetsExtension
inoryy Apr 19, 2013
47f0f81
fixed CS
inoryy Apr 19, 2013
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
9 changes: 9 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/templating_php.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
<parameter key="templating.globals.class">Symfony\Bundle\FrameworkBundle\Templating\GlobalVariables</parameter>
<parameter key="templating.asset.path_package.class">Symfony\Bundle\FrameworkBundle\Templating\Asset\PathPackage</parameter>
<parameter key="templating.asset.url_package.class">Symfony\Component\Templating\Asset\UrlPackage</parameter>
<parameter key="templating.asset.absolute_url_package.class">Symfony\Bundle\FrameworkBundle\Templating\Asset\AbsoluteUrlPackage</parameter>
<parameter key="templating.asset.package_factory.class">Symfony\Bundle\FrameworkBundle\Templating\Asset\PackageFactory</parameter>
</parameters>

Expand Down Expand Up @@ -54,6 +55,14 @@
<argument /> <!-- version format -->
</service>

<service id="templating.asset.absolute_url_package" class="%templating.asset.absolute_url_package.class%" abstract="true">
<argument type="service" id="router.request_context" on-invalid="ignore" />
<argument /> <!-- version -->
<argument /> <!-- version format -->
</service>

<service id="absolute_url" alias="templating.asset.absolute_url_package" />
Copy link
Member

Choose a reason for hiding this comment

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

A service named absolute_url is a bad idea IMO. It does not descibe at all what the service is about.

thus, making the alias target the abstract service looks weird to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idea was to add a simple name that developers can reference when calling asset().
Do you think I should get rid of the alias completely or change to something else?

Copy link
Member

Choose a reason for hiding this comment

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

What you pass as argument when calling asset() is not a service id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, misunderstood the setup. Thanks!


<service id="templating.asset.request_aware_package" class="Symfony\Component\Templating\Asset\PackageInterface" factory-service="templating.asset.package_factory" factory-method="getPackage" abstract="true">
<argument type="service" id="request" strict="false" />
<argument /> <!-- http id -->
Expand Down
71 changes: 71 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Templating/Asset/AbsoluteUrlPackage.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\FrameworkBundle\Templating\Asset;

use Symfony\Component\Routing\RequestContext;
use Symfony\Component\Templating\Asset\Package;

/**
* This package attempts to return absolute URL for assets
*
* @author Roman Marintšenko <roman.marintsenko@knplabs.com>
*/
class AbsoluteUrlPackage extends Package
{
private $context;

/**
* @param RequestContext $context Request context
* @param string $version The version
Copy link
Contributor

Choose a reason for hiding this comment

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

string|null

* @param string $format The version format
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above?

*/
public function __construct(RequestContext $context, $version = null, $format = null)
{
$this->context = $context;

parent::__construct($version, $format);
}

public function getUrl($path)
{
if (false !== strpos($path, '://') || 0 === strpos($path, '//')) {
return $path;
}

$url = ltrim($path, '/');
if ($baseUrl = $this->context->getBaseUrl()) {
$url = trim($baseUrl, '/').'/'.$url;
Copy link
Contributor

Choose a reason for hiding this comment

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

so we are not leaving spaces between the contactenating .'s, thought i just saw @bschussek using them for his PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question!
I was under the same impression originally, but it's this way in other *Package classes and neither phpcsfixer nor scrutinizer seem to have caught that...

Copy link
Contributor

Choose a reason for hiding this comment

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

@cordoval I was pushing a few old PRs recently that I probably forgot to scan for this convention change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bschussek sorry, i'm still confused. Is it $string.$string2 or $string . $string2?

Copy link
Contributor

Choose a reason for hiding this comment

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

$string.$string :)

}

// get scheme, host and port from RequestContext
// based on \Symfony\Component\Routing\Generator\UrlGenerator::doGenerate()
$schemeAuthority = '';
if ($host = $this->context->getHost()) {
$scheme = $this->context->getScheme();

$port = '';
if ('http' === $scheme && 80 != $this->context->getHttpPort()) {
$port = ':'.$this->context->getHttpPort();
} elseif ('https' === $scheme && 443 != $this->context->getHttpsPort()) {
$port = ':'.$this->context->getHttpsPort();
}

$schemeAuthority .= $scheme.'://'.$host.$port;
}

$url = $schemeAuthority.'/'.$url;

$url = $this->applyVersion($url);

return $url;
}
}
55 changes: 55 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Tests/Templating/Asset/AbsoluteUrlPackageTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\FrameworkBundle\Tests\Templating\Asset;

use Symfony\Component\Routing\RequestContext;
use Symfony\Bundle\FrameworkBundle\Templating\Asset\AbsoluteUrlPackage;

/**
* This package attempts to return absolute URL for assets
*
* @author Roman Marintšenko <roman.marintsenko@knplabs.com>
*/
class AbsoluteUrlPackageTest extends \PHPUnit_Framework_TestCase
{
public function testGetUrl()
{
$package = new AbsoluteUrlPackage(new RequestContext());
$this->assertEquals('http://example.com/foo.js', $package->getUrl('http://example.com/foo.js'), '->getUrl() does nothing if an absolute URL is already given');

$package = new AbsoluteUrlPackage(new RequestContext('', 'GET', 'symfony.com'));
Copy link
Contributor

Choose a reason for hiding this comment

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

have you tried a dataprovider for this test? just wondering

$this->assertEquals('http://symfony.com/foo.js', $package->getUrl('foo.js'), '->getUrl() prepends host and scheme to a given path');
$this->assertEquals('http://symfony.com/foo.js', $package->getUrl('/foo.js'), '->getUrl() prepends host and scheme to a given absolute path');

$package = new AbsoluteUrlPackage(new RequestContext('foo', 'GET', 'symfony.com'));
$this->assertEquals('http://symfony.com/foo/foo.js', $package->getUrl('foo.js'), '->getUrl() prepends base url to relative path');
$this->assertEquals('http://symfony.com/foo/foo.js', $package->getUrl('/foo.js'), '->getUrl() prepends base url to absolute path');

$package = new AbsoluteUrlPackage(new RequestContext('/foo', 'GET', 'symfony.com'));
$this->assertEquals('http://symfony.com/foo/foo.js', $package->getUrl('foo.js'), '->getUrl() prepends base url with backslash at beginning to relative path');

$package = new AbsoluteUrlPackage(new RequestContext('foo/', 'GET', 'symfony.com'));
$this->assertEquals('http://symfony.com/foo/foo.js', $package->getUrl('foo.js'), '->getUrl() prepends base url with backslash at end to relative path');

$package = new AbsoluteUrlPackage(new RequestContext('', 'GET', 'symfony.com', 'http', 8080));
$this->assertEquals('http://symfony.com:8080/foo.js', $package->getUrl('foo.js'), '->getUrl() prepends port if it is different than 80');

$package = new AbsoluteUrlPackage(new RequestContext('', 'GET', 'symfony.com', 'https', 80, 443));
$this->assertEquals('https://symfony.com/foo.js', $package->getUrl('foo.js'), '->getUrl() does not prepend 443 port if scheme is https');

$package = new AbsoluteUrlPackage(new RequestContext('', 'GET', 'symfony.com', 'https', 80, 444));
$this->assertEquals('https://symfony.com:444/foo.js', $package->getUrl('foo.js'), '->getUrl() prepends port if scheme is https and it is different than 443');

$package = new AbsoluteUrlPackage(new RequestContext('', 'GET', 'symfony.com'), 'abcd');
$this->assertEquals('http://symfony.com/foo.js?abcd', $package->getUrl('foo.js'), '->getUrl() appends the version if defined');
}
}
19 changes: 19 additions & 0 deletions src/Symfony/Bundle/TwigBundle/Extension/AssetsExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public function getFunctions()
{
return array(
'asset' => new \Twig_Function_Method($this, 'getAssetUrl'),
'asset_url' => new \Twig_Function_Method($this, 'getAssetAbsoluteUrl'),
'assets_version' => new \Twig_Function_Method($this, 'getAssetsVersion'),
);
}
Expand All @@ -55,6 +56,24 @@ public function getAssetUrl($path, $packageName = null)
return $this->container->get('templating.helper.assets')->getUrl($path, $packageName);
}

/**
* Returns absolute URL for a given asset path
* Does not rely on assets_base_urls parameter, instead tries to match request scheme/host/port
*
* If called from CLI, then relies on router.request_context.* parameters:
* router.request_context.host
* router.request_context.scheme
* router.request_context.base_url
*
* @param string $path A public path
*
* @return string A public path which takes into account the base path and URL path
*/
public function getAssetAbsoluteUrl($path)
{
return $this->getAssetUrl($path, 'absolute_url');
Copy link
Member

Choose a reason for hiding this comment

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

This will be broken as soon as you use asset packages (you cannot get an absolute url based on the package) and your code does not play well with packages urls which are already absolute because of the base url.

Copy link
Member

Choose a reason for hiding this comment

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

This, nothing ensure that you have an asset package named absolute_url among the registered ones (I even tink you never have it currently)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stof I get that I can't rely on absolute_url package name, but can you help me understand how it would break with others?
The way I understood it only one package is used when generating the url - either default, which is either PathPackage or UrlPackage if assets_base_url is defined or the one user specified in the $packageName variable when calling asset(), so it can't really conflict with others?

Or you mean that asset_url() should fall back to UrlPackage if assets_base_urls is defined?

Copy link
Member

Choose a reason for hiding this comment

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

@Inori you cannot get the absolute url by using a different asset package. An asset package defines a set of base urls and a version. If we want to support absolute urls, I see 2 solutions:

  • add a method in the package returning an absolute url. the big drawback is that it is a BC break (changing the interafce and requiring more constructor arguments for PathPackage)
  • use the existing code to get the asset url and then prepend the scheme and host in case it is not already an absolute url

}

/**
* Returns the version of the assets in a package.
*
Expand Down