[WebLink] Add class to parse Link headers from HTTP responses #60420
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
5a7a0d4 to
4378fb8
Compare
|
Thanks for the review @OskarStark, I addressed all your comments. |
0783d20 to
cb63475
Compare
| * | ||
| * @author Jérôme Tamarelle <jerome@tamarelle.net> | ||
| */ | ||
| final class HttpHeaderParser |
There was a problem hiding this comment.
final + no base type = not SOLID
There was a problem hiding this comment.
I blindly replicated the same thing as HttpHeaderSerializer. Let's remove the final.
There was a problem hiding this comment.
We should remove final there also then.
BTW, did you consider a service definition to allow autowiring?
There was a problem hiding this comment.
I removed the final from the other class and added framework integration.
There was a problem hiding this comment.
Should we add an interface to provide autowiring based on the interface rather than the concrete implementation ?
There was a problem hiding this comment.
In absolute terms, that's what we should do. We would do the same for HttpHeaderSerializer 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); |
There was a problem hiding this comment.
I'd suggest starting with null instead of array_shift, it should be more performant
d3f3139 to
2afdc15
Compare
| } | ||
| } | ||
|
|
||
| if (!isset($params['rel'])) { |
There was a problem hiding this comment.
what should be done if rel is an array because it is specified multiple times ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I wonder if we should still return links that doesn't have a rel attribute. They will never be returned by getLinksByRel, but give exhaustivity if the parser is used for tests.
There was a problem hiding this comment.
Updated to return links without rel.
c74c0b3 to
7df291f
Compare
Some HTTP API expose a Link header for pagination (See GitHub API or Sentry 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: