Add a new Link component #22273

dunglas · Apr 4, 2017

Q	A
Branch?	master
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	todo

This a proposal to extract HTTP preloading features introduced in #21478 in their own component.

There are some good reasons to do it:

HTTP preloading is not (only) about assets: this standalone component could be very useful to replace resources embedding in APIs by HTTP/2 pushes like described in this article by @evert. In such case, there is no reason to carry the whole asset component for an API.
There is no dependency nor relation at all between the code of the asset compnent and the one I've added for Preloading features
It makes the code cleaner (no more optional dependency in the Asset Twig extension)

This component would also better fit in HttpFoundation than in Asset. But there is no dependency between it and HttpFoundation and it can easily be used with PSR-7 too, so IMO it better belongs in a standalone component.

Btw, ~~~I plan to add support for prefetching to this component. Except a PR soon.~~~ Prefetching and prerendering support added in this PR.

ping @symfony/deciders

dunglas · Apr 4, 2017

A better name would probably be "Link Component": https://html.spec.whatwg.org/multipage/semantics.html#links

nicolas-grekas · Apr 4, 2017

Added to the 3.3 milestone because this is only about new features already planned for 3.3

stof · Apr 4, 2017

Not changing the service definition for assets.preload_manager looks suspicious to me (and btw, the name should be changed)

dunglas · Apr 4, 2017

I've renamed the component Link, added support for generic "Link" headers (preloading and resource hints) and introduced new Twig helpers for dns-prefetch, preconnect, prefetch and prerender.

This new component now support all types of prefetching and is smart enough to manage generic Link headers (for instance, when it will be available https://github.com/api-platform/core/blob/master/src/Hydra/EventListener/AddLinkHeaderListener.php in API Platform may be removed).

@stof comment also addressed.

tadcka · Apr 4, 2017

src/Symfony/Bundle/FrameworkBundle/composer.json

@@ -39,6 +39,7 @@
        "symfony/console": "~3.3",
        "symfony/css-selector": "~2.8|~3.0",
        "symfony/dom-crawler": "~2.8|~3.0",
+        "symfony/links": "~3.3",


symfony/link

andersonamuller · Apr 4, 2017

src/Symfony/Component/Link/composer.json

+        "symfony/http-kernel": "^2.8 || ^3.0"
+    },
+    "autoload": {
+        "psr-4": { "Symfony\\Component\\Preload\\": "" },


Symfony\Component\Link\

dunglas · Apr 5, 2017

Tests should be green after the merge.

xabbuh · Apr 5, 2017

src/Symfony/Bridge/Twig/Tests/Extension/LinkExtensionTest.php

+{
+    protected function setUp()
+    {
+        if (!class_exists(LinkManager::class)) {


I would add symfony/link to the require-dev section of the composer.json file instead.

xabbuh · Apr 5, 2017

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -208,6 +208,10 @@ public function load(array $configs, ContainerBuilder $container)
            $this->registerPropertyInfoConfiguration($config['property_info'], $container, $loader);
        }

+        if ($this->isConfigEnabled($container, $config['links'])) {


I think we should test for the Link component to be present first and throw a meaningful exception if not present.

Link should not be a mandatory component. You mean throwing if enabled is set to true but the component not installed?

yes, we did that recently for other optional dependencies too

xabbuh · Apr 5, 2017

src/Symfony/Bundle/FrameworkBundle/Resources/config/links.xml

+
+        <service id="links.link_manager" class="Symfony\Component\Link\LinkManager" public="false" />
+
+        <service id="links.link_listener" class="Symfony\Component\Link\EventListener\LinkListener">


Can be private?

IIRC, event listeners can't

since #20953 they can

xabbuh · Apr 5, 2017

src/Symfony/Bundle/FrameworkBundle/Resources/config/schema/symfony-1.0.xsd

@@ -62,6 +63,10 @@
        <xsd:attribute name="path" type="xsd:string" />
    </xsd:complexType>

+    <xsd:complexType name="links">
+        <xsd:attribute name="enabled" type="xsd:boolean" />


Does it make sense to set this to xsd:string to allow using a DI parameter in the config?

All others similar parameters use xsd:boolean. If we want to change this behavior, we should do it in another PR for all enabled parameters.

maybe something to "fix" by adding a new "boolean" type, borrowed from the XSD of the DI XmlFileLoader? (in another PR)

xabbuh · Apr 5, 2017

src/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/ConfigurationTest.php

@@ -238,6 +238,9 @@ protected static function getBundleDefaultConfig()
                'log' => true,
                'throw' => true,
            ),
+            'links' => array(
+                'enabled' => !class_exists(FullStack::class),


What about adding the component as a dev dependency to the composer.json file instead?

It is, it's not related.

sorry, I misread the code

xabbuh · Apr 5, 2017

src/Symfony/Component/Link/LinkManager.php

+ *
+ * @author Kévin Dunglas <dunglas@gmail.com>
+ */
+class LinkManager implements LinkManagerInterface


👍 on my side but I think it's against our CS (maybe @final). ping @nicolas-grekas

xabbuh · Apr 5, 2017

src/Symfony/Component/Link/README.md

+Link Component
+==============
+
+The Link component manages link between resources. It is particularly useful advise clients


[...] manages links between [...]

[...] useful to advise [...]

xabbuh · Apr 5, 2017

src/Symfony/Component/Link/composer.json

+        "symfony/http-kernel": ""
+    },
+    "require-dev": {
+        "symfony/http-kernel": "^2.8 || ^3.0"


The EventDispatcher and HttpFoundation components are used by the LinkListenerTest class too.

teohhanhui · Apr 5, 2017

Please, call this "HttpLink".

dunglas · Apr 5, 2017

@teohhanhui it's not really related to HTTP (only the listener is). It can also handle HTML links and in general links in any other protocol.

dunglas · Apr 5, 2017

@xabbuh's comments fixed

teohhanhui · Apr 5, 2017

I believe it's defined in RFC 5988 - Web Linking. Perhaps it can be named WebLink or ResourceLink.

dunglas · Apr 5, 2017

I like WebLink.

andersonamuller · Apr 5, 2017

WebLink is fine, but then the buildValues method on LinkManagerInterface should not mention HTTP headers

nicolas-grekas · Apr 10, 2017

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

 use Symfony\Component\Serializer\Serializer;
 use Symfony\Component\Translation\Translator;
 use Symfony\Component\Validator\Validation;
+use Symfony\Component\WebLink\WebLinkManagerInterface;


this interface does not exist anymore

nicolas-grekas

👍

fabpot · Apr 10, 2017

Some tests are broken on Travis.

dunglas · Apr 10, 2017

@fabpot IIUC, only because the component is not available on Packagist yet (DEPS=low)?

fabpot · Apr 10, 2017

Thank you @dunglas.

TomasVotruba · May 7, 2017

Great job!

I almost missed it... is there a post somewhere? I couldn't find any. Or is it reverted?

jdreesen · May 7, 2017

There is New in Symfony 3.3: WebLink component.

TomasVotruba · May 7, 2017

@jdreesen Ah, thank you. I probably looked for "Link*".

This PR was merged into the 3.4 branch. Discussion ---------- Add docs for the WebLink component The WebLink component is available since Symfony 3.3, but I never took the time to add the docs (however, a blog post explaining how to use it was available). This documentation is based on https://dunglas.fr/2017/10/symfony-4-http2-push-and-preloading/. If necessary, I can grant any copyright regarding this post to the Symfony project. symfony/symfony#21478 symfony/symfony#22273 Closes #7515. Commits ------- 91ee3bc Fix RST ea7b3da @nicolas-grekas' review 38fda88 fix build e12e776 RST 088690f Fix link e3d4036 RST 178821e refactor 9f4ae9b fix typo 6beb4eb Add docs for the WebLink component

carsonbot added the Status: Needs Review label Apr 4, 2017

dunglas force-pushed the extract_preload branch from f454bbf to 3d8a023 Compare April 4, 2017 12:08

nicolas-grekas added this to the 3.3 milestone Apr 4, 2017

dunglas force-pushed the extract_preload branch 2 times, most recently from c0d3a31 to e47b413 Compare April 4, 2017 17:13

tadcka reviewed Apr 4, 2017

View reviewed changes

andersonamuller reviewed Apr 4, 2017

View reviewed changes

dunglas changed the title ~~Add a new Preload component~~ Add a new Link component Apr 5, 2017

dunglas force-pushed the extract_preload branch 2 times, most recently from d0f40a0 to ff6b790 Compare April 5, 2017 06:48

xabbuh reviewed Apr 5, 2017

View reviewed changes

dunglas force-pushed the extract_preload branch from ff6b790 to 4687ea2 Compare April 5, 2017 10:49

dunglas added 10 commits April 10, 2017 12:20

Rename to WebLink

7be2a6c

Use PSR-13

66c4ed6

Fix naming and CS

aaa54c2

Remove the dependency to fig/link-util

c73ae6c

Inline the GenericLinkProvider service

949a207

Fix composer.json

fc4d63b

Use a request attribute instead of a stateful service

aba1dbb

Remove the getName method

4a82c3d

Fix @nicolasgrekas' comments

b136372

Fix @eko's comments

fd2dd50

dunglas force-pushed the extract_preload branch from 24e7b13 to fd2dd50 Compare April 10, 2017 10:20

eko approved these changes Apr 10, 2017

View reviewed changes

nicolas-grekas reviewed Apr 10, 2017

View reviewed changes

Remove references to WebLinkManagerInterface

f0ce807

dunglas force-pushed the extract_preload branch from d8fe813 to 73797d4 Compare April 10, 2017 14:26

Require fig/link-util in the WebLink component

209901b

dunglas force-pushed the extract_preload branch from 73797d4 to 209901b Compare April 10, 2017 14:30

nicolas-grekas approved these changes Apr 10, 2017

View reviewed changes

fabpot closed this in 5a76834 Apr 10, 2017

javiereguiluz mentioned this pull request Apr 21, 2017

Add docs for the new WebLink component symfony/symfony-docs#7515

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

dunglas mentioned this pull request Sep 10, 2018

Add docs for the WebLink component symfony/symfony-docs#10309

Merged


		<service id="links.link_manager" class="Symfony\Component\Link\LinkManager" public="false" />

		<service id="links.link_listener" class="Symfony\Component\Link\EventListener\LinkListener">

Search code, repositories, users, issues, pull requests...

Uh oh!

Add a new Link component #22273

Add a new Link component #22273

Uh oh!

Conversation

dunglas commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented Apr 4, 2017

Uh oh!

nicolas-grekas commented Apr 4, 2017

Uh oh!

stof commented Apr 4, 2017

Uh oh!

dunglas commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas commented Apr 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui commented Apr 5, 2017

Uh oh!

dunglas commented Apr 5, 2017

Uh oh!

dunglas commented Apr 5, 2017

Uh oh!

teohhanhui commented Apr 5, 2017

Uh oh!

dunglas commented Apr 5, 2017

Uh oh!

andersonamuller commented Apr 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

dunglas commented Apr 4, 2017 •

edited

Loading

dunglas Apr 5, 2017 •

edited

Loading

nicolas-grekas Apr 5, 2017 •

edited

Loading