Adds parameter typehints #94

Stilch · 2021-05-09T07:37:50Z

No description provided.

localheinz · 2022-02-16T18:35:42Z

Quick note, they are called type and return type declarations, see https://www.php.net/manual/en/language.types.declarations.php.

weierophinney

I only have one change to make (PHP constraint), and can make that myself and push to your branch if you like.

I've tested this against the PSR-7 integration test suite, and also tried it in a few applications to see the impact; all worked as expected, indicating it's backwards compatible with the previous versions. As such, I think this one is ready, pending the evolution approval.

Will turn to reviewing #95 next. I expect that's going to be the harder one.

weierophinney · 2023-02-21T21:01:46Z

composer.json

@@ -11,7 +11,7 @@
        }
    ],
    "require": {
-        "php": ">=5.3.0"
+        "php": ">=7.2.0"


Please do not use >= (or, worse, *) for constraints; there's no way of knowing if a later major will result in BC breaks.

Suggested change

"php": ">=7.2.0"

"php": "^7.2 || ^8.0"

Is there consensus on this topic (no intention to start a discussion about the pros and cons)? It looks like most other PSRs and also the existing v1 use >=, so we may want to keep using this here as well for consistency reasons, no?

We don't have any bylaws for it, and many of these pre-dated some of our current understanding of what is reasonable to expect for backwards compatibility guarantees from PHP.

Laminas, for instance, uses ~X.Y.0 constraints, because we've found that there are subtle BC breaks often between minor versions.

Considering we do not know what keywords will be reserved for a new major, nor what subtle changes to the object model might cause breakage, I feel we should be cautious in what we say we support. We can always create a new minor to add support for a new PHP major when the time comes, but removing support for the new major if we discover breakage would technically be a BC break.

I think we both agree this would not be the right place to have this discussion and I think we're both very well aware of the pros and cons of both approaches. Neither approach is perfect, it's a tradeoff. Hence my question if there is (or should be) a bylaw that goes into more detail.

In my opinion, it really comes down to expectations and how we (can) look into our crystal ball. I would argue I have no reason to believe a set of interfaces like PSR-7 would be incompatible with PHP 9 and beyond – or at least I have equal certainty it will be compatible with PHP 8.4 or beyond. Both assumptions are guess work at this point imho, so I could even understand the move to pin to the minor version like done for Laminas and others.

I guess I could rephrase my thoughts: Do we have more reason to believe PHP 9 would break PSR-7 v2 than we have reason to believe that a future PSR-7 v2.x would be ready in time PHP 9 is foreseeable? I would hate to hinder adoption of new PHP versions in the future.

weierophinney · 2023-02-21T21:10:05Z

src/MessageInterface.php

@@ -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);


This one is interesting; when we bump to PHP 8, we could make $value into string|array, though an array type would still require validation.

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.

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).

weierophinney · 2023-02-21T21:10:53Z

src/ServerRequestInterface.php

@@ -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);


When we move to PHP 8, we could add mixed as the type for $default.

weierophinney

RequestInterface::withRequestTarget() should add a string typehint for the $requestTarget argument, as those are all that are allowed based on RFC-7230.

weierophinney

Looks good! I'll open the vote for these updates this week!

Crell · 2023-02-27T22:03:13Z

src/RequestInterface.php

@@ -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


Was this an error before?

Yes - if you look at the linked RFC 7230 section, you'll see it can only be a string.

Crell · 2023-02-27T22:04:15Z

composer.json

@@ -11,7 +11,7 @@
        }
    ],
    "require": {
-        "php": ">=5.3.0"
+        "php": "^7.2 || ^8.0"


I'd vote for going straight to 8.x and be done with it. Use the union types.

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.

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.

Aren't there some places where you wanted to use mixed, though? Why punt on that unnecessarily?

We can do a 2.1 release that restricts to 8.0 and up and adds mixed.

I don't get the sense that distro versions matters anywhere near as much as it used to.

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.

Adds parameter typehints

5cd5ad7

weierophinney reviewed Feb 21, 2023

View reviewed changes

This was referenced Feb 21, 2023

Update to PHP7 #83

Closed

Add return typehints #95

Merged

weierophinney requested changes Feb 21, 2023

View reviewed changes

Aleksandr Golouz added 2 commits February 25, 2023 11:58

Changed php version

f84c2d7

Added a string typehint for the $requestTarget argument

6ae06cd

weierophinney approved these changes Feb 27, 2023

View reviewed changes

weierophinney mentioned this pull request Feb 27, 2023

Add errata for addition of parameter and return type declarations to PSR-7 php-fig/fig-standards#1296

Merged

Crell reviewed Feb 27, 2023

View reviewed changes

ashnazg approved these changes Mar 9, 2023

View reviewed changes

vdelau merged commit cb6ce48 into php-fig:master Apr 4, 2023

Art4 mentioned this pull request Aug 28, 2023

withRequestTarget() accepts string only #78

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds parameter typehints #94

Adds parameter typehints #94

Stilch commented May 9, 2021

localheinz commented Feb 16, 2022

weierophinney left a comment

weierophinney Feb 21, 2023

clue Feb 22, 2023

weierophinney Feb 22, 2023

clue Feb 24, 2023

weierophinney Feb 21, 2023

Crell Feb 27, 2023

weierophinney Mar 3, 2023

weierophinney Feb 21, 2023

weierophinney left a comment

weierophinney left a comment

Crell Feb 27, 2023

weierophinney Feb 27, 2023

Art4 Aug 28, 2023

Crell Feb 27, 2023

weierophinney Feb 27, 2023

Crell Mar 1, 2023

weierophinney Mar 1, 2023

weierophinney Mar 1, 2023

Adds parameter typehints #94

Adds parameter typehints #94

Conversation

Stilch commented May 9, 2021

localheinz commented Feb 16, 2022

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney left a comment

Choose a reason for hiding this comment

weierophinney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment