Add a new Link component #22273

Closed
wants to merge 14 commits into
from

Conversation

@dunglas
Member

dunglas commented 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

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 4, 2017

Member

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

Member

dunglas commented Apr 4, 2017

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

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

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Apr 4, 2017

Member

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

Member

nicolas-grekas commented Apr 4, 2017

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

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Apr 4, 2017

Member

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

Member

stof commented Apr 4, 2017

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

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 4, 2017

Member

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.

Member

dunglas commented 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.

@@ -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",

This comment has been minimized.

@tadcka

tadcka Apr 4, 2017

Contributor

symfony/link

@tadcka

tadcka Apr 4, 2017

Contributor

symfony/link

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

This comment has been minimized.

@andersonamuller

andersonamuller Apr 4, 2017

Symfony\Component\Link\

@andersonamuller

andersonamuller Apr 4, 2017

Symfony\Component\Link\

@dunglas dunglas changed the title from Add a new Preload component to Add a new Link component Apr 5, 2017

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 5, 2017

Member

Tests should be green after the merge.

Member

dunglas commented Apr 5, 2017

Tests should be green after the merge.

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

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2017

Member

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

@xabbuh

xabbuh Apr 5, 2017

Member

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

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

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2017

Member

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

@xabbuh

xabbuh Apr 5, 2017

Member

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

This comment has been minimized.

@dunglas

dunglas Apr 5, 2017

Member

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

@dunglas

dunglas Apr 5, 2017

Member

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

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2017

Member

yes, we did that recently for other optional dependencies too

@xabbuh

xabbuh Apr 5, 2017

Member

yes, we did that recently for other optional dependencies too

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

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2017

Member

Can be private?

@xabbuh

xabbuh Apr 5, 2017

Member

Can be private?

This comment has been minimized.

@dunglas

dunglas Apr 5, 2017

Member

IIRC, event listeners can't

@dunglas

dunglas Apr 5, 2017

Member

IIRC, event listeners can't

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2017

Member

since #20953 they can

@xabbuh

xabbuh Apr 5, 2017

Member

since #20953 they can

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

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2017

Member

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

@xabbuh

xabbuh Apr 5, 2017

Member

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

This comment has been minimized.

@dunglas

dunglas Apr 5, 2017

Member

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.

@dunglas

dunglas Apr 5, 2017

Member

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.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

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

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

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

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

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2017

Member

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

@xabbuh

xabbuh Apr 5, 2017

Member

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

This comment has been minimized.

@dunglas

dunglas Apr 5, 2017

Member

It is, it's not related.

@dunglas

dunglas Apr 5, 2017

Member

It is, it's not related.

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2017

Member

sorry, I misread the code

@xabbuh

xabbuh Apr 5, 2017

Member

sorry, I misread the code

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

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2017

Member

final?

@xabbuh

xabbuh Apr 5, 2017

Member

final?

This comment has been minimized.

@dunglas

dunglas Apr 5, 2017

Member

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

@dunglas

dunglas Apr 5, 2017

Member

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

src/Symfony/Component/Link/README.md
+Link Component
+==============
+
+The Link component manages link between resources. It is particularly useful advise clients

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2017

Member

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

@xabbuh

xabbuh Apr 5, 2017

Member

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

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2017

Member

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

@xabbuh

xabbuh Apr 5, 2017

Member

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

src/Symfony/Component/Link/composer.json
+ "symfony/http-kernel": ""
+ },
+ "require-dev": {
+ "symfony/http-kernel": "^2.8 || ^3.0"

This comment has been minimized.

@xabbuh

xabbuh Apr 5, 2017

Member

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

@xabbuh

xabbuh Apr 5, 2017

Member

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

@teohhanhui

This comment has been minimized.

Show comment
Hide comment
@teohhanhui

teohhanhui Apr 5, 2017

Contributor

Please, call this "HttpLink".

Contributor

teohhanhui commented Apr 5, 2017

Please, call this "HttpLink".

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 5, 2017

Member

@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.

Member

dunglas commented 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

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 5, 2017

Member

@xabbuh's comments fixed

Member

dunglas commented Apr 5, 2017

@xabbuh's comments fixed

@teohhanhui

This comment has been minimized.

Show comment
Hide comment
@teohhanhui

teohhanhui Apr 5, 2017

Contributor

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

Contributor

teohhanhui commented Apr 5, 2017

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

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 5, 2017

Member

I like WebLink.

Member

dunglas commented Apr 5, 2017

I like WebLink.

@andersonamuller

This comment has been minimized.

Show comment
Hide comment
@andersonamuller

andersonamuller Apr 5, 2017

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

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

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 5, 2017

Member

This method should be renamed buildHeaderValue() (obviously, it's not the same format for HTML links).

Member

dunglas commented Apr 5, 2017

This method should be renamed buildHeaderValue() (obviously, it's not the same format for HTML links).

@nicolas-grekas

Could "Link" be a too generic name? could we need a more scoped name? HtmlLink?
Just wondering - keep "Link" might be the best :)

+use Symfony\Component\Link\LinkManagerInterface;
+
+/**
+ * Twig extension for the Symfony Preload component.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

Symfony Link

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

Symfony Link

This comment has been minimized.

@Simperfit

Simperfit Apr 5, 2017

Contributor

WebLink is a good name ;)

@Simperfit

Simperfit Apr 5, 2017

Contributor

WebLink is a good name ;)

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

this file is missing autowiring aliases

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

this file is missing autowiring aliases

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

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

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

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

+ * Adds an element to the list of resources to preload.
+ *
+ * @param string $uri The relation URI
+ * @param string $rel The relation type (e.g. "preload", "prefetch", "prerender" or "dns-prefetch")

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

worth adding constants for the existing types?

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

worth adding constants for the existing types?

This comment has been minimized.

@evert

evert Apr 5, 2017

There are hundreds, and it's extendable with uris

@evert

evert Apr 5, 2017

There are hundreds, and it's extendable with uris

This comment has been minimized.

@dunglas

dunglas Apr 5, 2017

Member

Is it worth it? Those names make sense in the Twig extension because they are useful for assets. In the component, as pointed out by @evert they there are a lot values and why choosing some and not all?

@dunglas

dunglas Apr 5, 2017

Member

Is it worth it? Those names make sense in the Twig extension because they are useful for assets. In the component, as pointed out by @evert they there are a lot values and why choosing some and not all?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

there are hundreds of them, but only a very few are standardized for technical use (eg only 4 in https://www.w3.org/TR/resource-hints/)
So to me these may deserve a const to help discoverability.

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

there are hundreds of them, but only a very few are standardized for technical use (eg only 4 in https://www.w3.org/TR/resource-hints/)
So to me these may deserve a const to help discoverability.

src/Symfony/Component/Link/README.md
+==============
+
+The Link component manages links between resources. It is particularly useful to advise clients
+to preload and prefetch documents through HTTP and HTTP/2 pushes.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

if the component follows some sort of standard, it could be worth mentioning it here

@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

if the component follows some sort of standard, it could be worth mentioning it here

+use Symfony\Component\Link\LinkManagerInterface;
+
+/**
+ * Twig extension for the Symfony Preload component.

This comment has been minimized.

@Simperfit

Simperfit Apr 5, 2017

Contributor

WebLink is a good name ;)

@Simperfit

Simperfit Apr 5, 2017

Contributor

WebLink is a good name ;)

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Apr 5, 2017

Contributor

Is there any relation with PSR-13 about links? Would it make sense to implement it?

For reference:

I am not familiar at all with the standard so I may be completely off.

Contributor

mnapoli commented Apr 5, 2017

Is there any relation with PSR-13 about links? Would it make sense to implement it?

For reference:

I am not familiar at all with the standard so I may be completely off.

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

This comment has been minimized.

@fabpot

fabpot Apr 5, 2017

Member

web-link or weblink

@fabpot

fabpot Apr 5, 2017

Member

web-link or weblink

@@ -33,6 +33,7 @@
"symfony/routing": "~2.8|~3.0",
"symfony/templating": "~2.8|~3.0",
"symfony/yaml": "~2.8|~3.0",
+ "symfony/link": "~3.3",

This comment has been minimized.

@fabpot

fabpot Apr 5, 2017

Member

weblink or web-link

@fabpot

fabpot Apr 5, 2017

Member

weblink or web-link

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 5, 2017

Member

@mnapoli I had totally forgotten this PSR, but it's related. We may use https://github.com/php-fig/link-util instead of the primitive types. Are you aware of other PSR-13 implementations?

Member

dunglas commented Apr 5, 2017

@mnapoli I had totally forgotten this PSR, but it's related. We may use https://github.com/php-fig/link-util instead of the primitive types. Are you aware of other PSR-13 implementations?

@mnapoli

This comment has been minimized.

Show comment
Hide comment
@mnapoli

mnapoli Apr 5, 2017

Contributor

@dunglas not really, pinging @Crell who may have some insight. On Packagist there are not a lot of dependent packages: https://packagist.org/packages/psr/link/dependents

Contributor

mnapoli commented Apr 5, 2017

@dunglas not really, pinging @Crell who may have some insight. On Packagist there are not a lot of dependent packages: https://packagist.org/packages/psr/link/dependents

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 5, 2017

Member

So, about PSR-13, the manager can be transformed in a serializer (according to the PSR terminology) and it can use https://github.com/php-fig/link-util/blob/master/src/GenericLinkProvider.php instead of the current simple array.

It's a minor change and will improve interoperability with other libraries using this PSR (Drupal?). WDYT @symfony/deciders?

Member

dunglas commented Apr 5, 2017

So, about PSR-13, the manager can be transformed in a serializer (according to the PSR terminology) and it can use https://github.com/php-fig/link-util/blob/master/src/GenericLinkProvider.php instead of the current simple array.

It's a minor change and will improve interoperability with other libraries using this PSR (Drupal?). WDYT @symfony/deciders?

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Apr 5, 2017

Member

Let's use PSR-13 then!

Member

nicolas-grekas commented Apr 5, 2017

Let's use PSR-13 then!

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 6, 2017

Member

PSR-13 support added (it deserves a review because of how I deal with immutability).

Member

dunglas commented Apr 6, 2017

PSR-13 support added (it deserves a review because of how I deal with immutability).

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 6, 2017

Member

Failures not related or will be fixed by the merge.

Member

dunglas commented Apr 6, 2017

Failures not related or will be fixed by the merge.

composer.json
@@ -23,6 +23,7 @@
"psr/container": "^1.0",
"psr/log": "~1.0",
"psr/simple-cache": "^1.0",
+ "fig/link-util": "^1.0",

This comment has been minimized.

@teohhanhui

teohhanhui Apr 6, 2017

Contributor

I think we should require psr/link directly too? Because fig/link-util is just an implementation, even though it does add a transitive dependency on psr/link.

@teohhanhui

teohhanhui Apr 6, 2017

Contributor

I think we should require psr/link directly too? Because fig/link-util is just an implementation, even though it does add a transitive dependency on psr/link.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

any psr/link-implementation to add also in "provide" section?

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

any psr/link-implementation to add also in "provide" section?

This comment has been minimized.

@teohhanhui

teohhanhui Apr 6, 2017

Contributor

How easy it is to define a virtual package (basically just list it in provide - that's why it's called virtual):
https://devedge.wordpress.com/2014/09/27/composer-and-virtual-packages/

And some counter points:
https://php-and-symfony.matthiasnoback.nl/2014/10/composer-provide-and-dependency-inversion/

@teohhanhui

teohhanhui Apr 6, 2017

Contributor

How easy it is to define a virtual package (basically just list it in provide - that's why it's called virtual):
https://devedge.wordpress.com/2014/09/27/composer-and-virtual-packages/

And some counter points:
https://php-and-symfony.matthiasnoback.nl/2014/10/composer-provide-and-dependency-inversion/

This comment has been minimized.

@dunglas

dunglas Apr 6, 2017

Member

We use the implementation here: https://github.com/symfony/symfony/pull/22273/files#diff-78128eb201bbbb556131b3ae4bac192bR29.
We may require only a psr/link-implementation instead of fig/link-util but it will hurt the DX (the constructor argument is optional). I'm to let it as is because you can already use any implementation but it also works out of the box.

@dunglas

dunglas Apr 6, 2017

Member

We use the implementation here: https://github.com/symfony/symfony/pull/22273/files#diff-78128eb201bbbb556131b3ae4bac192bR29.
We may require only a psr/link-implementation instead of fig/link-util but it will hurt the DX (the constructor argument is optional). I'm to let it as is because you can already use any implementation but it also works out of the box.

@@ -0,0 +1,42 @@
+{
+ "name": "symfony/weblink",

This comment has been minimized.

@teohhanhui

teohhanhui Apr 6, 2017

Contributor

This is just very confusing and inconsistent. We either rename the component Weblink (bad idea), or we should stick to symfony/web-link.

@teohhanhui

teohhanhui Apr 6, 2017

Contributor

This is just very confusing and inconsistent. We either rename the component Weblink (bad idea), or we should stick to symfony/web-link.

This comment has been minimized.

@dunglas

dunglas Apr 6, 2017

Member

We indeed use a caret for other components (event-dispatcher, http-foundation), but as @fabpot wants.

@dunglas

dunglas Apr 6, 2017

Member

We indeed use a caret for other components (event-dispatcher, http-foundation), but as @fabpot wants.

This comment has been minimized.

@teohhanhui

teohhanhui Apr 6, 2017

Contributor

@fabpot seemed ok with symfony/web-link: #22273 (review)

@teohhanhui

teohhanhui Apr 6, 2017

Contributor

@fabpot seemed ok with symfony/web-link: #22273 (review)

This comment has been minimized.

@dunglas

dunglas Apr 6, 2017

Member

@teohhanhui it was called web-link and he asked for a change :) #22273 (comment)
I've no strong opinion about that even if I prefer with the caret.

@dunglas

dunglas Apr 6, 2017

Member

@teohhanhui it was called web-link and he asked for a change :) #22273 (comment)
I've no strong opinion about that even if I prefer with the caret.

This comment has been minimized.

@jvasseur

jvasseur Apr 6, 2017

Contributor

Maybe we could rename the namespace to Weblink instead to keep consistency between package name and namespace.

@jvasseur

jvasseur Apr 6, 2017

Contributor

Maybe we could rename the namespace to Weblink instead to keep consistency between package name and namespace.

This comment has been minimized.

@fabpot

fabpot Apr 6, 2017

Member

What about going back to "Link":

  • The PSR is named Link, not Weblink;
  • Short names are better;
  • No dash (that helps :));
  • Do we think that we will have a non-web link component at some point? (or do we think that Link can be confusing?)

WDYT? If everybody think that web-link is better, that's of course an option as well.

@fabpot

fabpot Apr 6, 2017

Member

What about going back to "Link":

  • The PSR is named Link, not Weblink;
  • Short names are better;
  • No dash (that helps :));
  • Do we think that we will have a non-web link component at some point? (or do we think that Link can be confusing?)

WDYT? If everybody think that web-link is better, that's of course an option as well.

This comment has been minimized.

@dunglas

dunglas Apr 6, 2017

Member

The (new) problem with Link is that it's not an implementation of the PSR-13, it just uses it. It may be confusing :)

symfony/link and symfony/web-link are fine to me. It's time to start a Twitter poll?

@dunglas

dunglas Apr 6, 2017

Member

The (new) problem with Link is that it's not an implementation of the PSR-13, it just uses it. It may be confusing :)

symfony/link and symfony/web-link are fine to me. It's time to start a Twitter poll?

This comment has been minimized.

@teohhanhui

teohhanhui Apr 6, 2017

Contributor

I think Link is too generic. This component deals with Web Linking. It's a very specific concept. When you say link, people usually think of URI, which is not what this is about.

@teohhanhui

teohhanhui Apr 6, 2017

Contributor

I think Link is too generic. This component deals with Web Linking. It's a very specific concept. When you say link, people usually think of URI, which is not what this is about.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

the confusion is non existent: that's not the same vendor, people do not know about all PSRs
the generic name is also not an issue: the fig already use that name, so it wasn't too generic for them, and again we have the "symfony" prefix, which really suggests "web" to many.

I'm 👍 for going back to just "Link"

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

the confusion is non existent: that's not the same vendor, people do not know about all PSRs
the generic name is also not an issue: the fig already use that name, so it wasn't too generic for them, and again we have the "symfony" prefix, which really suggests "web" to many.

I'm 👍 for going back to just "Link"

@@ -18,7 +18,7 @@
use Symfony\Component\Config\Definition\Builder\TreeBuilder;
use Symfony\Component\Config\Definition\ConfigurationInterface;
use Symfony\Component\Form\Form;
-use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\WebLink\WebLinkManagerInterface;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

alpha order

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

alpha order

@@ -29,6 +29,7 @@
use Symfony\Component\Finder\Finder;
use Symfony\Component\HttpKernel\DependencyInjection\Extension;
use Symfony\Component\Config\FileLocator;
+use Symfony\Component\WebLink\WebLinkManagerInterface;

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

alpha order

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

alpha order

+ *
+ * @author Kévin Dunglas <dunglas@gmail.com>
+ */
+interface WebLinkManagerInterface

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

would it be possible to have only the PSR interfaces and not add any new in the component? that'd play well with the goal of the psr

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

would it be possible to have only the PSR interfaces and not add any new in the component? that'd play well with the goal of the psr

This comment has been minimized.

@dunglas

dunglas Apr 6, 2017

Member

Yes, but we still need the manager because we need a "mutable" service to use with event listeners.

@dunglas

dunglas Apr 6, 2017

Member

Yes, but we still need the manager because we need a "mutable" service to use with event listeners.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

OK, let's keep it, that allows swapping the implem of the service

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

OK, let's keep it, that allows swapping the implem of the service

src/Symfony/Component/WebLink/README.md
+WebLink Component
+=================
+
+The Link component manages links between resources. It is particularly useful to advise clients

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

The WebLink

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

The WebLink

+ $this->assertSame(array($link1, $link2), array_values($this->manager->getLinkProvider()->getLinks()));
+ }
+
+ public function testClear() {

This comment has been minimized.

@sstok

sstok Apr 6, 2017

Contributor

{ should be on new line 😛 is fabbot on a break? Same goes for other files.

@sstok

sstok Apr 6, 2017

Contributor

{ should be on new line 😛 is fabbot on a break? Same goes for other files.

@@ -79,7 +81,8 @@
"symfony/validator": "For using validation",
"symfony/yaml": "For using the debug:config and lint:yaml commands",
"symfony/property-info": "For using the property_info service",
- "symfony/process": "For using the server:run, server:start, server:stop, and server:status commands"
+ "symfony/process": "For using the server:run, server:start, server:stop, and server:status commands",
+ "symfony/web-links": "For using web links features such as preloading, prefetching or prerendering"

This comment has been minimized.

@fabpot

fabpot Apr 6, 2017

Member

web-link

@fabpot

fabpot Apr 6, 2017

Member

web-link

+ public function getName()
+ {
+ return 'web_link';
+ }

This comment has been minimized.

@fabpot

fabpot Apr 6, 2017

Member

We don't need this method.

@fabpot

fabpot Apr 6, 2017

Member

We don't need this method.

+{
+ private $manager;
+
+ public function __construct(WebLinkManagerInterface $manager)

This comment has been minimized.

@fabpot

fabpot Apr 6, 2017

Member

Instead of injecting this dep here, it would be better to create a Twig Runtime.

@fabpot

fabpot Apr 6, 2017

Member

Instead of injecting this dep here, it would be better to create a Twig Runtime.

+ $link = $link->withAttribute($key, $value);
+ }
+
+ $this->manager->add($link);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

as discussed with @dunglas on Slack, this means the manager is mutable, which conflicts with the fact that it is registered as a service
using an attribute on the request might be the solution

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

as discussed with @dunglas on Slack, this means the manager is mutable, which conflicts with the fact that it is registered as a service
using an attribute on the request might be the solution

+ $link = $link->withAttribute($key, $value);
+ }
+
+ $linkProvider = $request->attributes->get('_links', new GenericLinkProvider());

This comment has been minimized.

@dunglas

dunglas Apr 6, 2017

Member

Is this ok or should we always populate this key? The current implementation is more memory efficient.

@dunglas

dunglas Apr 6, 2017

Member

Is this ok or should we always populate this key? The current implementation is more memory efficient.

This comment has been minimized.

@teohhanhui

teohhanhui Apr 7, 2017

Contributor

Actually, I really think the ideal case would be for Symfony\Component\HttpFoundation\Request to implement Psr\Link\EvolvableLinkProviderInterface. That's what I understand after reading PSR-13.

But I understand that's not probably not possible considering the mutable nature of our Request. That said, I think using attributes for this is wrong.

@teohhanhui

teohhanhui Apr 7, 2017

Contributor

Actually, I really think the ideal case would be for Symfony\Component\HttpFoundation\Request to implement Psr\Link\EvolvableLinkProviderInterface. That's what I understand after reading PSR-13.

But I understand that's not probably not possible considering the mutable nature of our Request. That said, I think using attributes for this is wrong.

This comment has been minimized.

@dunglas

dunglas Apr 7, 2017

Member

Actually, I really think the ideal case would be for Symfony\Component\HttpFoundation\Request to implement Psr\Link\EvolvableLinkProviderInterface.

I agree but as you pointed out, it's not possible.

That said, I think using attributes for this is wrong.

Why?

@dunglas

dunglas Apr 7, 2017

Member

Actually, I really think the ideal case would be for Symfony\Component\HttpFoundation\Request to implement Psr\Link\EvolvableLinkProviderInterface.

I agree but as you pointed out, it's not possible.

That said, I think using attributes for this is wrong.

Why?

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 6, 2017

Member

Implementation revamped to get rid of the stateful service and use a request attribute instead.

Member

dunglas commented Apr 6, 2017

Implementation revamped to get rid of the stateful service and use a request attribute instead.

+ "fig/link-util": "^1.0",
+ "symfony/event-dispatcher": "^2.8 || ^3.0",
+ "symfony/http-foundation": "^2.8 || ^3.0",
+ "symfony/http-kernel": "^2.8 || ^3.0"

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

the convention is ^2.8|^3.0

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

the convention is ^2.8|^3.0

- // Free memory
- $this->preloadManager->clear();
+ $linkProvider = $event->getRequest()->attributes->get('_links');
+ if (!$linkProvider instanceof LinkProviderInterface || !($links = $linkProvider->getLinks())) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

no need for the brackets after the ||

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

no need for the brackets after the ||

@@ -18,9 +18,11 @@
"require": {
"php": ">=5.5.9",
"doctrine/common": "~2.4",
+ "fig/link-util": "^1.0",

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

this dep is in require-dev in all components, but in require here, looks suspicious

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

this dep is in require-dev in all components, but in require here, looks suspicious

This comment has been minimized.

@dunglas

dunglas Apr 7, 2017

Member

We need this package in the Twig extension and the FrameworkBundle (even at runtime). But the those packages have only a soft dependency to WebLink. In the context of the full stack, I think it makes sense to add it to have something working out of the box.

@dunglas

dunglas Apr 7, 2017

Member

We need this package in the Twig extension and the FrameworkBundle (even at runtime). But the those packages have only a soft dependency to WebLink. In the context of the full stack, I think it makes sense to add it to have something working out of the box.

This comment has been minimized.

@teohhanhui

teohhanhui Apr 7, 2017

Contributor

It's not required by any of the packages in replace, so it should indeed remain in require-dev.

@teohhanhui

teohhanhui Apr 7, 2017

Contributor

It's not required by any of the packages in replace, so it should indeed remain in require-dev.

This comment has been minimized.

@dunglas

dunglas Apr 7, 2017

Member

It is required if you use WebLink && (Twig Bridge || Framework Bundle). We cannot express such conditions with Composer, but the full stack framework install WebLink + Twig Bridge + Framework Bundle so it needs to have this package as dependency.

@dunglas

dunglas Apr 7, 2017

Member

It is required if you use WebLink && (Twig Bridge || Framework Bundle). We cannot express such conditions with Composer, but the full stack framework install WebLink + Twig Bridge + Framework Bundle so it needs to have this package as dependency.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 10, 2017

Member

Still looks inconsistent to me. symfony/symfony should not behave differently than using standalone components...

@nicolas-grekas

nicolas-grekas Apr 10, 2017

Member

Still looks inconsistent to me. symfony/symfony should not behave differently than using standalone components...

This comment has been minimized.

@dunglas

dunglas Apr 10, 2017

Member

fig/link-util added back as a dependency of the WebLink component. It fixes the inconsistency, makes the component easier to use for end users and... it's the only one implementation of PSR-13 for now.

@dunglas

dunglas Apr 10, 2017

Member

fig/link-util added back as a dependency of the WebLink component. It fixes the inconsistency, makes the component easier to use for end users and... it's the only one implementation of PSR-13 for now.

{
- $this->preloadManager = $preloadManager;
+ $this->serializer = new HttpHeaderSerializer();

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

having this class be instantiated directly here means it's an internal implementation detail to the listener
imho, we should either make it a service, or merge the implement in this class, don't you think?

@nicolas-grekas

nicolas-grekas Apr 6, 2017

Member

having this class be instantiated directly here means it's an internal implementation detail to the listener
imho, we should either make it a service, or merge the implement in this class, don't you think?

This comment has been minimized.

@dunglas

dunglas Apr 7, 2017

Member

It's indeed an internal detail of the listener.

But the HttpHeaderSerializer class can be used alone through the component. I don't think it makes sense (for now) to expose it as a service but it may be useful for library creators to serialize a list of Link instances to a valid HTTP header value (it's almost all this component do).

I would keep this as is (btw the serializer is final but not internal for the same reason).

@dunglas

dunglas Apr 7, 2017

Member

It's indeed an internal detail of the listener.

But the HttpHeaderSerializer class can be used alone through the component. I don't think it makes sense (for now) to expose it as a service but it may be useful for library creators to serialize a list of Link instances to a valid HTTP header value (it's almost all this component do).

I would keep this as is (btw the serializer is final but not internal for the same reason).

+
+ <services>
+
+ <service id="web_link.add_link_header_listener" class="Symfony\Component\Link\WebLink\AddLinkHeaderListener" public="false">

This comment has been minimized.

@eko

eko Apr 10, 2017

Contributor

It looks like there is a typo in class name here, should be: Symfony\Component\WebLink\AddLinkHeaderListener

@eko

eko Apr 10, 2017

Contributor

It looks like there is a typo in class name here, should be: Symfony\Component\WebLink\AddLinkHeaderListener

+ "name": "symfony/web-link",
+ "type": "library",
+ "description": "Symfony WebLink Component",
+ "keywords": ["link", "psr13", "http", "HTTP/2", "preload", "prefetch", "pre-render", "dns-prefetch", "push", "performance"],

This comment has been minimized.

@eko

eko Apr 10, 2017

Contributor

I think pre-render here should be fixed to prerender?

@eko

eko Apr 10, 2017

Contributor

I think pre-render here should be fixed to prerender?

dunglas added some commits Apr 4, 2017

@eko

eko approved these changes Apr 10, 2017

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 10, 2017

Member

this interface does not exist anymore

@nicolas-grekas

nicolas-grekas Apr 10, 2017

Member

this interface does not exist anymore

dunglas added some commits Apr 10, 2017

@nicolas-grekas

👍

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Apr 10, 2017

Member

Some tests are broken on Travis.

Member

fabpot commented Apr 10, 2017

Some tests are broken on Travis.

@dunglas

This comment has been minimized.

Show comment
Hide comment
@dunglas

dunglas Apr 10, 2017

Member

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

Member

dunglas commented Apr 10, 2017

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

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Apr 10, 2017

Member

Thank you @dunglas.

Member

fabpot commented Apr 10, 2017

Thank you @dunglas.

@fabpot fabpot closed this in 5a76834 Apr 10, 2017

@javiereguiluz javiereguiluz referenced this pull request in symfony/symfony-docs Apr 21, 2017

Open

Add docs for the new WebLink component #7515

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

@TomasVotruba

This comment has been minimized.

Show comment
Hide comment
@TomasVotruba

TomasVotruba May 7, 2017

Contributor

Great job!

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

Contributor

TomasVotruba commented May 7, 2017

Great job!

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

@jdreesen

This comment has been minimized.

Show comment
Hide comment
Contributor

jdreesen commented May 7, 2017

@TomasVotruba

This comment has been minimized.

Show comment
Hide comment
@TomasVotruba

TomasVotruba May 7, 2017

Contributor

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

Contributor

TomasVotruba commented May 7, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment