[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
| @@ -180,6 +180,11 @@ public function getAttributes(): array | |||
| return $this->attributes; | |||
| } | |||
|
|
|||
| public function getAttribute(string $attribute): string|\Stringable|int|float|bool|array|null | |||
There was a problem hiding this comment.
Please update the documentation of the attributes property to be consistent with the type defined in withAttribute.
It would also be great to document the type of arrays being supported, and to add the precise return type in getAttributes
There was a problem hiding this comment.
maybe this should be a separate PR to merge it in 7.3 though, to provide the proper type sooner.
There was a problem hiding this comment.
I'll remove this new method because it's not part of the interface. Accessing array value with null coalescence if fine.
| } | ||
| } | ||
|
|
||
| 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.
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: