[FWBundle] Add a new method AbstractController::addLink() #28875

dunglas · 2018-10-15T11:39:52Z

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

This provides a convenient method to add Link headers to the current Response directly from the Request object.
It improves the developer experience and the discoverability of the WebLink component.

Usage:

namespace App\Controller;

use Fig\Link\Link;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;

class MyAction extends AbstractController
{
    public function __invoke(Request $request): Response
    {
        $this->addLink($request, new Link('mercure', 'https://demo.mercure.rocks'));

        return $this->json(['foo' => 'bar']);
    }
}

ro0NL · 2018-10-15T11:42:39Z

what about Request::addWebLink()? does it make sense.. currently the feat. is still tied to AbstractController, that's a bummer :(

dunglas · 2018-10-15T11:53:26Z

@ro0NL very good idea. Updated accordingly.

ro0NL · 2018-10-15T18:02:54Z

src/Symfony/Component/HttpFoundation/Request.php

+     */
+    public function addLink(Link $link)
+    {
+        if (!class_exists(HttpHeaderSerializer::class)) {


actually just having fig/link-util is enough for this method to work, so we can test for GenericLinkProvider.. but i like the suggestion for symfony/web-link in the message :))

src/Symfony/Component/HttpFoundation/Request.php

ro0NL

does it need a Request::getLinks() as well, for completeness (at least keeps the _links attribute an implementation detail) and enables a return type :)

fabpot · 2018-10-16T00:39:04Z

Having the method on Request looks really weird to me as it is really about the Response.

dunglas · 2018-10-16T06:12:11Z

@fabpot I thought the same thing, but the current, temporary, list of values must be stored in request attributes. Then the header is added to the response by the listener.
Maybe can we just find a better name? Like addLinkToResponse or storeLink?

ro0NL · 2018-10-16T14:43:43Z

maybe it should be dealt by a new service, provided by WebLink component.

Something like ResponseLinkManagerInterface, though we should try to find some better name then "manager", but that's what it is :)

javiereguiluz · 2018-10-16T14:57:35Z

My preference would be the original proposal from Kévin: AbstractController::addLink() I know that there are some developers that prefer not to use AbstractController but in my experience they are a minority.

dunglas · 2018-10-16T15:37:56Z

And they can always use _links manually. I preserved the initial commit. I can revert the last one.

(Actually I prefer having it in Request, but I also don't feel confortable with the DX problem rose by Fabien, so the initial commit can be a good compromise)

ro0NL · 2018-10-16T15:50:37Z

Actually I prefer having it in Request

i see why we might favor AbstracController::addLink over Request::addLink in terms of semantics. But fact is, links are already stored in/tied to Request (as attributes), to me that legitimates Request::addLink on the technical level at least.

Speaking about DX, right now standalone it's

$linkProvider = $request->attributes->get('_links', new GenericLinkProvider());
$request->attributes->set('_links', $linkProvider->withLink($link));

over $request->addLink($link).. just saying ;)

👍 for AbstractController, that means working with attribtues remains the "true" API. We can live with that i guess.

dunglas · 2018-10-21T10:45:35Z

Last commit reverted and typo fixed.
Even if this feature partially work without the listener, I think we should check that the listener is present and that the header will be generated, especially now that it's in FrameworkBundle.

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php

nicolas-grekas · 2018-10-23T09:23:02Z

src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php

+     *
+     * @final
+     */
+    protected function addLink(Request $request, Link $link)


would it make sense to change the signature to Request $request, string $rel, string $href?

To support all features of Link it should be addLink(Request $request, array|string $rels, string $link, array $attributes = []) then.

I'm not sure it is worth it. WDYT?

OK, I missed EvolvableLinkTrait, let's keep as is.

Shouldn't it be Psr\Link\LinkInterface?

ro0NL · 2018-10-23T09:53:23Z

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

 4.2.0
 -----

+ * Added a `Symfony\Bundle\FrameworkBundle\Controller\AbstractController::addLink()` method to add Link headers to the current response


AbstractController, same-package code excludes the NS usually

fabpot · 2018-10-24T03:29:01Z

Thank you @dunglas.

…k() (dunglas) This PR was squashed before being merged into the 4.2-dev branch (closes #28875). Discussion ---------- [FWBundle] Add a new method AbstractController::addLink() | Q | A | ------------- | --- | Branch? | master | Bug fix? |no | New feature? | yes  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | n/a  | License | MIT | Doc PR | todo This provides a convenient method to add `Link` headers to the current `Response` directly from the `Request` object. It improves the developer experience and the discoverability of [the WebLink component](symfony/symfony-docs#10309). Usage: ```php namespace App\Controller; use Fig\Link\Link; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; class MyAction extends AbstractController { public function __invoke(Request $request): Response { $this->addLink($request, new Link('mercure', 'https://demo.mercure.rocks')); return $this->json(['foo' => 'bar']); } } ``` Commits ------- 4d20c39 [FWBundle] Add a new method AbstractController::addLink()

carsonbot added Status: Needs Review Feature labels Oct 15, 2018

dunglas force-pushed the addLink branch from b865f5d to 45f62a6 Compare October 15, 2018 11:40

dunglas force-pushed the addLink branch 2 times, most recently from 43394b6 to 4924de1 Compare October 15, 2018 11:55

dunglas changed the title ~~[FWBundle] Add a new method AbstractController::addLink()~~ [HttpFoundation] Add a new method Request::addLink() Oct 15, 2018

dunglas added the HttpFoundation label Oct 15, 2018

dunglas force-pushed the addLink branch from 4924de1 to e5be0ec Compare October 15, 2018 12:09

dunglas mentioned this pull request Oct 15, 2018

[Mercure] Add the component #28877

Closed

ro0NL reviewed Oct 15, 2018

View reviewed changes

src/Symfony/Component/HttpFoundation/Request.php Outdated Show resolved Hide resolved

ro0NL reviewed Oct 15, 2018

View reviewed changes

nicolas-grekas added this to the next milestone Oct 17, 2018

dunglas force-pushed the addLink branch from e5be0ec to c376a36 Compare October 21, 2018 10:40

dunglas changed the title ~~[HttpFoundation] Add a new method Request::addLink()~~ [FWBundle] Add a new method AbstractController::addLink() Oct 21, 2018

nicolas-grekas reviewed Oct 23, 2018

View reviewed changes

nicolas-grekas approved these changes Oct 23, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 23, 2018

ro0NL reviewed Oct 23, 2018

View reviewed changes

ro0NL approved these changes Oct 23, 2018

View reviewed changes

nicolas-grekas modified the milestones: next, 4.2 Oct 23, 2018

dunglas added FrameworkBundle and removed HttpFoundation labels Oct 23, 2018

fabpot approved these changes Oct 24, 2018

View reviewed changes

[FWBundle] Add a new method AbstractController::addLink()

4d20c39

fabpot force-pushed the addLink branch from 3292c9f to 4d20c39 Compare October 24, 2018 03:28

fabpot merged commit 4d20c39 into symfony:master Oct 24, 2018

This was referenced Oct 25, 2018

Added a new AbstractController::addLink() shortcut symfony/symfony-docs#10564

Closed

[FWBundle] Add a new method AbstractController::addLink() symfony/symfony-docs#10612

Closed

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

This was referenced Dec 12, 2018

[RFC] Add HTTP Responder pattern #29574

Closed

[FrameworkBundle] Added ControllerTrait::isFormValid #24576

Merged

Uh oh!

[FWBundle] Add a new method AbstractController::addLink() #28875

[FWBundle] Add a new method AbstractController::addLink() #28875

Uh oh!

Conversation

dunglas commented Oct 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ro0NL commented Oct 15, 2018

Uh oh!

dunglas commented Oct 15, 2018

Uh oh!

ro0NL Oct 15, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ro0NL left a comment

Choose a reason for hiding this comment

Uh oh!

fabpot commented Oct 16, 2018

Uh oh!

dunglas commented Oct 16, 2018

Uh oh!

ro0NL commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

javiereguiluz commented Oct 16, 2018

Uh oh!

dunglas commented Oct 16, 2018

Uh oh!

ro0NL commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dunglas commented Oct 21, 2018

Uh oh!

Uh oh!

nicolas-grekas Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

dunglas Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

thewilkybarkid Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

ro0NL Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

fabpot commented Oct 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dunglas commented Oct 15, 2018 •

edited

Loading

ro0NL commented Oct 16, 2018 •

edited

Loading

ro0NL commented Oct 16, 2018 •

edited

Loading