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

[WebLink] Add class to parse Link headers from HTTP responses#60420

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
GromNaN merged 1 commit intosymfony:7.4fromGromNaN:weblink-parser
May 27, 2025

Conversation

GromNaN
Copy link
Member

QA
Branch?7.4
Bug fix?no
New feature?yes
Deprecations?no
Issues-
LicenseMIT

Some HTTP API expose a Link header for pagination (SeeGitHub API orSentry API), so it's necessary to parse this header to consume the API.

Since we already have a WebLink component, I think it's a good fit to add the logic for parsing the HTTP header into this component.

The existing packages use simplified pattern and does not support all the spec features:

@carsonbot

This comment was marked as outdated.

@carsonbotcarsonbot added this to the7.3 milestoneMay 14, 2025
@GromNaNGromNaNforce-pushed theweblink-parser branch 2 times, most recently from5a7a0d4 to4378fb8CompareMay 14, 2025 11:05
@GromNaN
Copy link
MemberAuthor

Thanks for the review@OskarStark, I addressed all your comments.

OskarStark reacted with heart emoji

@GromNaNGromNaNforce-pushed theweblink-parser branch 2 times, most recently from0783d20 tocb63475CompareMay 14, 2025 11:41
Copy link
Member

@dunglasdunglas left a comment

Choose a reason for hiding this comment

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

Nit

*
* @author Jérôme Tamarelle <jerome@tamarelle.net>
*/
final class HttpHeaderParser

Choose a reason for hiding this comment

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

final + no base type = not SOLID

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I blindly replicated the same thing asHttpHeaderSerializer. Let's remove thefinal.

Choose a reason for hiding this comment

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

We should remove final there also then.
BTW, did you consider a service definition to allow autowiring?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I removed thefinal from the other class and added framework integration.

Copy link
Member

@stofstofMay 15, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Should we add an interface to provide autowiring based on the interface rather than the concrete implementation ?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

In absolute terms, that's what we should do. We would do the same forHttpHeaderSerializer then.
But I don't see why anyone would write a different implementation.

$rels = preg_split('/\s+/', trim($params['rel']));
unset($params['rel']);

$link = new Link(array_shift($rels), $href);

Choose a reason for hiding this comment

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

I'd suggest starting with null instead of array_shift, it should be more performant

GromNaN reacted with thumbs up emoji
@GromNaNGromNaNforce-pushed theweblink-parser branch 2 times, most recently fromd3f3139 to2afdc15CompareMay 14, 2025 13:51
}
}

if (!isset($params['rel'])) {
Copy link
Member

Choose a reason for hiding this comment

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

what should be done ifrel is an array because it is specified multiple times ?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Good question. I'll take the first value according to the spec:

5.3. Relation Type
The relation type of a link is conveyed in the "rel" parameter's value. The "rel" parameter MUST NOT appear more than once in a given link-value; occurrences after the first MUST be ignored by parsers.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I wonder if we should still return links that doesn't have arel attribute. They will never be returned bygetLinksByRel, but give exhaustivity if the parser is used for tests.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Updated to return links without rel.

@GromNaNGromNaNforce-pushed theweblink-parser branch 3 times, most recently fromc74c0b3 to7df291fCompareMay 15, 2025 13:20
@fabpotfabpot modified the milestones:7.3,7.4May 26, 2025
@GromNaNGromNaN merged commit298e56a intosymfony:7.4May 27, 2025
7 of 11 checks passed
@GromNaNGromNaN deleted the weblink-parser branchMay 27, 2025 08:49
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@stofstofstof left review comments

@dunglasdunglasdunglas approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@OskarStarkOskarStarkOskarStark approved these changes

@alexandre-dauboisalexandre-dauboisalexandre-daubois approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.

8 participants
@GromNaN@carsonbot@dunglas@nicolas-grekas@stof@OskarStark@alexandre-daubois@fabpot
[8]ページ先頭
©2009-2025 Movatter.jp