Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromdunglas:addLink
Oct 24, 2018

Conversation

@dunglas
Copy link
Member

@dunglasdunglas commentedOct 15, 2018•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRtodo

This provides a convenient method to addLink headers to the currentResponse directly from theRequest object.
It improves the developer experience and the discoverability ofthe WebLink component.

Usage:

namespaceApp\Controller;useFig\Link\Link;useSymfony\Component\HttpFoundation\Request;useSymfony\Component\HttpFoundation\Response;useSymfony\Bundle\FrameworkBundle\Controller\AbstractController;class MyActionextends AbstractController{publicfunction__invoke(Request$request):Response    {$this->addLink($request,newLink('mercure','https://demo.mercure.rocks'));return$this->json(['foo' =>'bar']);    }}

ro0NL and danielkay reacted with thumbs up emoji
@ro0NL
Copy link
Contributor

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

sstok and zmitic reacted with thumbs up emoji

@dunglas
Copy link
MemberAuthor

@ro0NL very good idea. Updated accordingly.

@dunglasdunglasforce-pushed theaddLink branch 2 times, most recently from43394b6 to4924de1CompareOctober 15, 2018 11:55
@dunglasdunglas changed the title[FWBundle] Add a new method AbstractController::addLink()[HttpFoundation] Add a new method Request::addLink()Oct 15, 2018
*/
publicfunctionaddLink(Link$link)
{
if (!class_exists(HttpHeaderSerializer::class)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

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

@fabpot
Copy link
Member

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

@dunglas
Copy link
MemberAuthor

@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? LikeaddLinkToResponse orstoreLink?

@ro0NL
Copy link
Contributor

ro0NL commentedOct 16, 2018•
edited
Loading

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

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

sstok reacted with laugh emoji

@javiereguiluz
Copy link
Member

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

ro0NL reacted with confused emoji

@dunglas
Copy link
MemberAuthor

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

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

@ro0NL
Copy link
Contributor

ro0NL commentedOct 16, 2018•
edited
Loading

Actually I prefer having it in Request

i see why we might favorAbstracController::addLink overRequest::addLink in terms of semantics. But fact is, links arealready stored in/tied toRequest (as attributes), to me that legitimatesRequest::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.

@nicolas-grekasnicolas-grekas added this to thenext milestoneOct 17, 2018
@dunglasdunglas changed the title[HttpFoundation] Add a new method Request::addLink()[FWBundle] Add a new method AbstractController::addLink()Oct 21, 2018
@dunglas
Copy link
MemberAuthor

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.

*
* @final
*/
protectedfunctionaddLink(Request$request,Link$link)

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

To support all features ofLink it should beaddLink(Request $request, array|string $rels, string $link, array $attributes = []) then.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it bePsr\Link\LinkInterface?

4.2.0
-----

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

Choose a reason for hiding this comment

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

AbstractController, same-package code excludes the NS usually

@fabpot
Copy link
Member

Thank you@dunglas.

@fabpotfabpot merged commit4d20c39 intosymfony:masterOct 24, 2018
fabpot added a commit that referenced this pull requestOct 24, 2018
…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 <!-- don't forget to update src/**/CHANGELOG.md files -->| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | n/a   <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | todoThis 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:```phpnamespace 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()
This was referencedNov 3, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+2 more reviewers

@thewilkybarkidthewilkybarkidthewilkybarkid left review comments

@ro0NLro0NLro0NL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

7 participants

@dunglas@ro0NL@fabpot@javiereguiluz@nicolas-grekas@thewilkybarkid@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp