[HttpFoundation] Add type-hints #32271
Conversation
| @@ -48,11 +48,11 @@ public function __construct(array $items) | |||
| * | |||
| * @return self | |||
| */ | |||
| public static function fromString($headerValue) | |||
| public static function fromString(string $headerValue) | |||
There was a problem hiding this comment.
@param can be removed from multiple docblocks.
src/Symfony/Component/HttpFoundation/File/MimeType/ExtensionGuesserInterface.php
Outdated
Show resolved
Hide resolved
|
@julien57ok to finish this one? |
79fa925 to
36486a3
Compare
36486a3 to
e5c000b
Compare
|
I just push-forced on this PR, review welcome. |
e5c000b to
72eaa88
Compare
72eaa88 to
2181739
Compare
| } | ||
|
|
||
| $this->content = (string) $content; | ||
| $this->content = $content; |
There was a problem hiding this comment.
since $content is nullable, $this->content can also be null. this might break for people expecting string in getContent. so I guess we need to cast null to ''
There was a problem hiding this comment.
there is actually a failing test due to this: https://travis-ci.org/symfony/symfony/jobs/569486612
| @@ -209,11 +209,10 @@ public function __construct($content = '', int $status = 200, array $headers = [ | |||
| * | |||
| * @param mixed $content The response content, see setContent() | |||
| { | ||
| if (null !== $content && !\is_string($content) && !is_numeric($content) && !\is_callable([$content, '__toString'])) { | ||
| throw new \UnexpectedValueException(sprintf('The Response content must be a string or object implementing __toString(), "%s" given.', \gettype($content))); |
There was a problem hiding this comment.
fully agree as the casting should be done in user-land when they need strict types compatibility.
| @@ -1586,8 +1553,6 @@ public function getPreferredFormat(?string $default = 'html'): ?string | |||
| /** | |||
| * Returns the preferred language. | |||
| * | |||
| * @param array $locales An array of ordered available locales | |||
There was a problem hiding this comment.
I think we should document this as @param string[] $locales. otherwise the return type would also not work.
| * @return string|null The format (null if not found) | ||
| */ | ||
| public function getFormat($mimeType) | ||
| public function getFormat(?string $mimeType) |
There was a problem hiding this comment.
making this nullable seems strange. I don't see the use-case for this.
| * @return $this | ||
| * | ||
| * @throws \InvalidArgumentException | ||
| */ | ||
| public function setTargetUrl($url) | ||
| public function setTargetUrl(string $url) | ||
| { | ||
| if ('' === ($url ?? '')) { |
There was a problem hiding this comment.
the code suggests $url can be null, but it's not nullable at the moment.
| * @return \DateTime|null The parsed DateTime or the default value if the header does not exist | ||
| * | ||
| * @throws \RuntimeException When the HTTP header is not parseable | ||
| */ | ||
| public function getDate($key, \DateTime $default = null) | ||
| public function getDate(string $key, \DateTime $default = null) |
There was a problem hiding this comment.
we should probably change \DateTime to \DateTimeInterface and adjust the return phpdoc as well. but that could be a separate PR.
| * @return $this | ||
| */ | ||
| public function setAttribute($name, $value) | ||
| public function setAttribute(string $name, string $value) |
There was a problem hiding this comment.
there is still a (string) $value cast left in the implementation
This PR was merged into the 5.0-dev branch. Discussion ---------- [HttpFoundation] Add type-hints | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #32179 | License | MIT replace #32271 Commits ------- ead419b add type-hints
First commit to add type-hints to HttpFoundation