Skip to content

[Asset] Add support for preloading with links and HTTP/2 push #21478

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.

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 16 commits into from
26 changes: 25 additions & 1 deletion src/Symfony/Bridge/Twig/Extension/AssetExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Bridge\Twig\Extension;

use Symfony\Component\Asset\Packages;
use Symfony\Component\Asset\Preload\PreloadManagerInterface;

/**
* Twig extension for the Symfony Asset component.
Expand All @@ -21,10 +22,12 @@
class AssetExtension extends \Twig_Extension
{
private $packages;
private $preloadManager;

public function __construct(Packages $packages)
public function __construct(Packages $packages, PreloadManagerInterface $preloadManager = null)
Copy link
Member

Choose a reason for hiding this comment

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

The default PreloadManagerInterface implementation is simple and does not have dependencies, so I suppose it does not make sense to have a proper runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I don't get what you mean.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we're just missing some doc about twig runtimes for extensions? :)

{
$this->packages = $packages;
$this->preloadManager = $preloadManager;
}

/**
Expand All @@ -35,6 +38,7 @@ public function getFunctions()
return array(
new \Twig_SimpleFunction('asset', array($this, 'getAssetUrl')),
new \Twig_SimpleFunction('asset_version', array($this, 'getAssetVersion')),
new \Twig_SimpleFunction('preload', array($this, 'preload')),
);
}

Expand Down Expand Up @@ -67,6 +71,26 @@ public function getAssetVersion($path, $packageName = null)
return $this->packages->getVersion($path, $packageName);
}

/**
* Preloads an asset.
*
* @param string $path A public path
* @param string $as A valid destination according to https://fetch.spec.whatwg.org/#concept-request-destination
* @param bool $nopush If this asset should not be pushed over HTTP/2
*
* @return string The path of the asset
*/
public function preload($path, $as = '', $nopush = false)
{
if (null === $this->preloadManager) {
throw new \RuntimeException('A preload manager must be configured to use the "preload" function.');
}

$this->preloadManager->addResource($path, $as, $nopush);

return $path;
}

/**
* Returns the name of the extension.
*
Expand Down
44 changes: 44 additions & 0 deletions src/Symfony/Bridge/Twig/Tests/Extension/AssetExtensionTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
<?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\Bridge\Twig\Tests\Extension;

use Symfony\Bridge\Twig\Extension\AssetExtension;
use Symfony\Component\Asset\Packages;
use Symfony\Component\Asset\Preload\PreloadManager;

/**
* @author Kévin Dunglas <dunglas@gmail.com>
*/
class AssetExtensionTest extends \PHPUnit_Framework_TestCase
{
public function testGetAndPreloadAssetUrl()
{
if (!class_exists(PreloadManager::class)) {
$this->markTestSkipped('Requires Asset 3.3+.');
}

$preloadManager = new PreloadManager();
$extension = new AssetExtension(new Packages(), $preloadManager);

$this->assertEquals('/foo.css', $extension->preload('/foo.css', 'style', true));
$this->assertEquals('</foo.css>; rel=preload; as=style; nopush', $preloadManager->buildLinkValue());
}

/**
* @expectedException \RuntimeException
*/
public function testNoConfiguredPreloadManager()
{
$extension = new AssetExtension(new Packages());
$extension->preload('/foo.css');
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,13 @@

<service id="assets.empty_version_strategy" class="Symfony\Component\Asset\VersionStrategy\EmptyVersionStrategy" public="false" />

<service id="assets.preload_manager" class="Symfony\Component\Asset\Preload\PreloadManager" public="false" />

<service id="asset.preload_listener" class="Symfony\Component\Asset\EventListener\PreloadListener">
<argument type="service" id="assets.preload_manager" />

<tag name="kernel.event_subscriber" />
</service>

</services>
</container>
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,12 @@ public function testAssetsDefaultVersionStrategyAsService()
$this->assertEquals('assets.custom_version_strategy', (string) $defaultPackage->getArgument(1));
}

public function testAssetHasPreloadListener()
{
$container = $this->createContainerFromFile('assets');
$this->assertTrue($container->hasDefinition('asset.preload_listener'));
}

public function testTranslator()
{
$container = $this->createContainerFromFile('full');
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Bundle/FrameworkBundle/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
"conflict": {
"phpdocumentor/reflection-docblock": "<3.0",
"phpdocumentor/type-resolver": "<0.2.0",
"symfony/asset": "<3.3",
"symfony/console": "<3.3"
},
"suggest": {
Expand Down
1 change: 1 addition & 0 deletions src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@

<service id="twig.extension.assets" class="Symfony\Bridge\Twig\Extension\AssetExtension" public="false">
<argument type="service" id="assets.packages" />
<argument type="service" id="assets.preload_manager" on-invalid="ignore" />
</service>

<service id="twig.extension.code" class="Symfony\Bridge\Twig\Extension\CodeExtension" public="false">
Expand Down
55 changes: 55 additions & 0 deletions src/Symfony/Component/Asset/EventListener/PreloadListener.php
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\Component\Asset\EventListener;

use Symfony\Component\Asset\Preload\PreloadManager;
use Symfony\Component\Asset\Preload\PreloadManagerInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;

/**
* Adds the preload Link HTTP header to the response.
*
* @author Kévin Dunglas <dunglas@gmail.com>
*/
class PreloadListener implements EventSubscriberInterface
{
private $preloadManager;

public function __construct(PreloadManagerInterface $preloadManager)
{
$this->preloadManager = $preloadManager;
}

public function onKernelResponse(FilterResponseEvent $event)
{
if (!$event->isMasterRequest()) {
return;
}

if ($value = $this->preloadManager->buildLinkValue()) {
$event->getResponse()->headers->set('Link', $value, false);

// Free memory
$this->preloadManager->clear();
}
}

/**
* {@inheritdoc}
*/
public static function getSubscribedEvents()
{
return array(KernelEvents::RESPONSE => 'onKernelResponse');
}
}
58 changes: 58 additions & 0 deletions src/Symfony/Component/Asset/Preload/PreloadManager.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
<?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\Component\Asset\Preload;

/**
* Manages preload HTTP headers.
*
* @author Kévin Dunglas <dunglas@gmail.com>
*/
class PreloadManager implements PreloadManagerInterface
Copy link
Member

Choose a reason for hiding this comment

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

could be final

Copy link
Member

Choose a reason for hiding this comment

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

would forbid decoration for no reason imho - see comments on the interface: building a profiler panel could require decorating this class

Copy link
Member

Choose a reason for hiding this comment

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

I meant decoration by inheritance of course.
My root issue is that final here would be only forbidding use cases while providing no benefit for us.
"final" should only used on method/classes that are not bound by any contract - like data objects.
When an interface covers the said methods, the contract is already enforced and inheritance should not be prevented.

Copy link
Contributor

Choose a reason for hiding this comment

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

My root issue is that final here would be only forbidding use cases while providing no benefit for us.

There are 2 benefits:

When an interface covers the said methods, the contract is already enforced and inheritance should not be prevented.

I don't see connection between contract enforcing and "inheritance should not be prevented". I'd say the opposite. See similar opinion, for example: http://ocramius.github.io/blog/when-to-declare-classes-final/ /cc @Ocramius

{
private $resources = array();

/**
* {@inheritdoc}
*/
public function addResource($uri, $as = '', $nopush = false)
{
$this->resources[$uri] = array('as' => $as, 'nopush' => $nopush);
}

/**
* {@inheritdoc}
*/
public function clear()
{
$this->resources = array();
}

/**
* {@inheritdoc}
*/
public function buildLinkValue()
{
if (!$this->resources) {
return;
}

$parts = array();
foreach ($this->resources as $uri => $options) {
$as = '' === $options['as'] ? '' : sprintf('; as=%s', $options['as']);
$nopush = $options['nopush'] ? '; nopush' : '';

$parts[] = sprintf('<%s>; rel=preload%s%s', $uri, $as, $nopush);
}

return implode(',', $parts);
}
}
43 changes: 43 additions & 0 deletions src/Symfony/Component/Asset/Preload/PreloadManagerInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?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\Component\Asset\Preload;

/**
* Manages resources to preload according to the W3C "Preload" specification.
*
* @see https://www.w3.org/TR/preload/
*
* @author Kévin Dunglas <dunglas@gmail.com>
*/
interface PreloadManagerInterface
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this interface for?

Copy link
Member Author

@dunglas dunglas Feb 7, 2017

Choose a reason for hiding this comment

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

This extension point allows to replace the manager by a smarter one (for instance if you want to create one giving access to the list of resources added or to change them programmatically).
IMO it doesn't hurt and allow to easily replace the global app manager to add custom behaviors.

Copy link
Member

Choose a reason for hiding this comment

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

it could also allow something to add a profiler panel reporting the preloaded resources for instance, by using a decorator implementation

{
/**
* Adds an element to the list of resources to preload.
*
* @param string $uri The resource URI
* @param string $as A valid destination according to https://fetch.spec.whatwg.org/#concept-request-destination
* @param bool $nopush If this asset should not be pushed over HTTP/2
*/
public function addResource($uri, $as = '', $nopush = false);

/**
* Clears the list of resources.
*/
public function clear();

/**
* Builds the value of the preload Link HTTP header.
*
* @return string|null
*/
public function buildLinkValue();
}
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\Component\Asset\Tests\EventListener;

use Symfony\Component\Asset\EventListener\PreloadListener;
use Symfony\Component\Asset\Preload\PreloadManager;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;

/**
* @author Kévin Dunglas <dunglas@gmail.com>
*/
class PreloadListenerTest extends \PHPUnit_Framework_TestCase
{
public function testOnKernelResponse()
{
$manager = new PreloadManager();
$manager->addResource('/foo');

$subscriber = new PreloadListener($manager);
$response = new Response('', 200, array('Link' => '<https://demo.api-platform.com/docs.jsonld>; rel="http://www.w3.org/ns/hydra/core#apiDocumentation"'));

$event = $this->getMockBuilder(FilterResponseEvent::class)->disableOriginalConstructor()->getMock();
$event->method('isMasterRequest')->willReturn(true);
$event->method('getResponse')->willReturn($response);

$subscriber->onKernelResponse($event);

$this->assertInstanceOf(EventSubscriberInterface::class, $subscriber);

$expected = array(
'<https://demo.api-platform.com/docs.jsonld>; rel="http://www.w3.org/ns/hydra/core#apiDocumentation"',
'</foo>; rel=preload',
);

$this->assertEquals($expected, $response->headers->get('Link', null, false));
$this->assertNull($manager->buildLinkValue());
}

public function testSubscribedEvents()
{
$this->assertEquals(array(KernelEvents::RESPONSE => 'onKernelResponse'), PreloadListener::getSubscribedEvents());
}
}
30 changes: 30 additions & 0 deletions src/Symfony/Component/Asset/Tests/Preload/PreloadManagerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<?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\Component\Asset\Preload;

/**
* @author Kévin Dunglas <dunglas@gmail.com>
*/
class PreloadManagerTest extends \PHPUnit_Framework_TestCase
{
public function testManageResources()
{
$manager = new PreloadManager();
$this->assertInstanceOf(PreloadManagerInterface::class, $manager);

$manager->addResource('/foo/bar.js', 'script', false);
$manager->addResource('/foo/baz.css');
$manager->addResource('/foo/bat.png', 'image', true);

$this->assertEquals('</foo/bar.js>; rel=preload; as=script,</foo/baz.css>; rel=preload,</foo/bat.png>; rel=preload; as=image; nopush', $manager->buildLinkValue());
}
}
Loading