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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds parameter typehints #94
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Psr\Http\Message; | ||
|
||
/** | ||
|
@@ -38,7 +40,7 @@ public function getProtocolVersion(); | |
* @param string $version HTTP protocol version | ||
* @return static | ||
*/ | ||
public function withProtocolVersion($version); | ||
public function withProtocolVersion(string $version); | ||
|
||
/** | ||
* Retrieves all message header values. | ||
|
@@ -75,7 +77,7 @@ public function getHeaders(); | |
* name using a case-insensitive string comparison. Returns false if | ||
* no matching header name is found in the message. | ||
*/ | ||
public function hasHeader($name); | ||
public function hasHeader(string $name); | ||
|
||
/** | ||
* Retrieves a message header value by the given case-insensitive name. | ||
|
@@ -91,7 +93,7 @@ public function hasHeader($name); | |
* header. If the header does not appear in the message, this method MUST | ||
* return an empty array. | ||
*/ | ||
public function getHeader($name); | ||
public function getHeader(string $name); | ||
|
||
/** | ||
* Retrieves a comma-separated string of the values for a single header. | ||
|
@@ -112,7 +114,7 @@ public function getHeader($name); | |
* concatenated together using a comma. If the header does not appear in | ||
* the message, this method MUST return an empty string. | ||
*/ | ||
public function getHeaderLine($name); | ||
public function getHeaderLine(string $name); | ||
|
||
/** | ||
* Return an instance with the provided value replacing the specified header. | ||
|
@@ -129,7 +131,7 @@ public function getHeaderLine($name); | |
* @return static | ||
* @throws \InvalidArgumentException for invalid header names or values. | ||
*/ | ||
public function withHeader($name, $value); | ||
public function withHeader(string $name, $value); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one is interesting; when we bump to PHP 8, we could make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So should we go ahead and do that before the vote? It seems wise to do so if that's what the spec already calls for. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No - we can do it for a later minor version that bumps the minimum version to 8. For this one, we should keep PHP 7 support (per my other comments). |
||
|
||
/** | ||
* Return an instance with the specified header appended with the given value. | ||
|
@@ -147,7 +149,7 @@ public function withHeader($name, $value); | |
* @return static | ||
* @throws \InvalidArgumentException for invalid header names or values. | ||
*/ | ||
public function withAddedHeader($name, $value); | ||
public function withAddedHeader(string $name, $value); | ||
|
||
/** | ||
* Return an instance without the specified header. | ||
|
@@ -161,7 +163,7 @@ public function withAddedHeader($name, $value); | |
* @param string $name Case-insensitive header field name to remove. | ||
* @return static | ||
*/ | ||
public function withoutHeader($name); | ||
public function withoutHeader(string $name); | ||
|
||
/** | ||
* Gets the body of the message. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Psr\Http\Message; | ||
|
||
/** | ||
|
@@ -55,10 +57,10 @@ public function getRequestTarget(); | |
* | ||
* @link http://tools.ietf.org/html/rfc7230#section-5.3 (for the various | ||
* request-target forms allowed in request messages) | ||
* @param mixed $requestTarget | ||
* @param string $requestTarget | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this an error before? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes - if you look at the linked RFC 7230 section, you'll see it can only be a string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. refs #78 |
||
* @return static | ||
*/ | ||
public function withRequestTarget($requestTarget); | ||
public function withRequestTarget(string $requestTarget); | ||
|
||
/** | ||
* Retrieves the HTTP method of the request. | ||
|
@@ -82,7 +84,7 @@ public function getMethod(); | |
* @return static | ||
* @throws \InvalidArgumentException for invalid HTTP methods. | ||
*/ | ||
public function withMethod($method); | ||
public function withMethod(string $method); | ||
|
||
/** | ||
* Retrieves the URI instance. | ||
|
@@ -125,5 +127,5 @@ public function getUri(); | |
* @param bool $preserveHost Preserve the original state of the Host header. | ||
* @return static | ||
*/ | ||
public function withUri(UriInterface $uri, $preserveHost = false); | ||
public function withUri(UriInterface $uri, bool $preserveHost = false); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Psr\Http\Message; | ||
|
||
/** | ||
|
@@ -224,7 +226,7 @@ public function getAttributes(); | |
* @param mixed $default Default value to return if the attribute does not exist. | ||
* @return mixed | ||
*/ | ||
public function getAttribute($name, $default = null); | ||
public function getAttribute(string $name, $default = null); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we move to PHP 8, we could add |
||
|
||
/** | ||
* Return an instance with the specified derived request attribute. | ||
|
@@ -241,7 +243,7 @@ public function getAttribute($name, $default = null); | |
* @param mixed $value The value of the attribute. | ||
* @return static | ||
*/ | ||
public function withAttribute($name, $value); | ||
public function withAttribute(string $name, $value); | ||
|
||
/** | ||
* Return an instance that removes the specified derived request attribute. | ||
|
@@ -257,5 +259,5 @@ public function withAttribute($name, $value); | |
* @param string $name The attribute name. | ||
* @return static | ||
*/ | ||
public function withoutAttribute($name); | ||
public function withoutAttribute(string $name); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote for going straight to 8.x and be done with it. Use the union types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted on #95, I think it makes sense to allow v7 here, due to the large install base of RHEL 7, Ubuntu 20.04, and Debian 11, all of which pin to PHP 7.4. We will have PHP 7 users for some years, even if it's not technically in support, and there's no reason to restrict its usage artificially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't there some places where you wanted to use
mixed
, though? Why punt on that unnecessarily?Most of the other PSR updates have targeted 8.x (they used
mixed
or union types), and in practice... how many people use the pinned old versions of PHP on distros rather than PPCs? I admittedly only know from one corporate environment (my own), but we're ignoring that. I don't get the sense that distro versions matters anywhere near as much as it used to.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do a 2.1 release that restricts to 8.0 and up and adds
mixed
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They absolutely do.
We (Zend) just released our annual PHP Landscape Report. It is very representative of the broader PHP community, has a representative mix of developers to decision makers, and covers a broad spectrum of company sizes.
This year, as in all years past that I can remember, 60% of respondents are currently using EOL versions of PHP.
There's no reason to artificially restrict what features or security fixes they can get in the PHP libraries they consume because they are on an EOL version of PHP. They may be getting security patches for the runtime already (e.g., via an operating system, or via a commercial LTS provider), so limiting what they can use makes it harder to keep the libraries they use updated.