Add return typehints #95

Stilch · 2021-05-09T07:54:31Z

No description provided.

McLotos · 2021-06-20T19:22:28Z

@weierophinney @pmjones Have we an approximate date of merge? We can't wait several years for each such release =)

McLotos · 2021-06-20T19:36:49Z

@weierophinney @gabidj
I think this request is high time to merge with #83, then create a 2.0 version of http-message package.

ADmad · 2021-09-08T13:19:34Z

PHP 8.1 RC1 is out and here we are still waiting for a release for PHP 7.2+ with types 🙂.

ADmad · 2021-12-16T13:52:47Z

deluxetom · 2022-02-09T17:19:57Z

is this not maintained anymore? I would also love to see this PR merged

simensen · 2022-02-14T21:56:30Z

is this not maintained anymore? I would also love to see this PR merged

It isn't a question of maintained or not. I'm not in a position to do anything about this package anymore. Making changes to any published PSR or its associated packages is a lengthy process.

I know @weierophinney already touched on this via Twitter, so I'm including this here for context. :)

Not too long ago, the mailing list was the preferred way to bring up topics related to existing or future PSRs. Looks like Discord is on the list, now, too.

https://www.php-fig.org/get-involved/

simensen · 2022-02-14T21:59:16Z

Any of the methods that return an instance of the same interface (the "wither" methods) should ideally have a static return type since that is now available.

Stilch · 2022-02-16T18:29:42Z

I can add static return type, but I will have to set the minimum php version to 8.0

localheinz · 2022-02-16T18:31:33Z

composer.json

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


@Stilch

Looks like we are not only adding return type declarations here, but raising the PHP version requirement.

Should that be different pull requests?

@Stilch

Not sure who suggested to use this kind of version requirement, but for reference, there is a somewhat heated discussion why using >= is not a good idea, right here.

localheinz · 2022-02-16T18:31:54Z

src/MessageInterface.php

@@ -1,5 +1,7 @@
 <?php

+declare(strict_types=1);


@Stilch

Does not have any effect on interfaces.

localheinz · 2022-02-16T18:34:15Z

composer.json

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


Suggested change

"php": ">=7.2.0"

"php": "^7.2.0 || ^8.0.0"

…ct types

oanhnn · 2022-04-15T10:55:09Z

Will we continue to wait new version or will leave it (not implement interface)?
PHP 8.1 was released and PHP 7.4 Active support ended :(
https://www.php.net/supported-versions.php

ikkez · 2022-10-16T13:41:37Z

It's end of 2022, php 8.2 is around the corner and still no resolution to this issue. Please abandon this project... it would be at least an answer we can work with.

ADmad · 2022-10-18T06:40:03Z

Would be nice if PHP-FIG also provides some clarification if the PSR repos are maintained by the group as a whole or each one is handled by a BDFL.

cosminsandu · 2023-02-16T14:20:51Z

Anew news on this?

weierophinney · 2023-02-21T22:03:30Z

@ADmad —

clarification if the PSR repos are maintained by the group as a whole or each one is handled by a BDFL.

The bylaws make clear the chain of "power" for a given spec repo: editor, sponsor, then group.

I've been very slow on these, as I had quite some concerns about ecosystem impact. I've done testing on #94 at this point, and feel it's ready to go. I'm testing this one next, to determine impact for implementing libraries and existing consumers so I can write up a migration guide. When that's ready, we can start the evolution vote. I'm hoping to get that done in the next 1-4 weeks.

weierophinney

Questions:

Why no void return types?
Why are __toString() occurrences not updated with a string return type declaration?

Otherwise, this looks good.

I tested against the PSR-7 integration test suite and Diactoros. The changes:

Updating signatures. Surprisingly, we've added typehints previously in Diactoros, so there were not many changes needed. I ran into more when I tested an app that uses mezzio-swoole, as that library imlements StreamInterface, and needed both parameter and return hints.
Removing obsolete tests. A huge portion of both the integration tests and the Diactoros tests are around validating that invalid input will not be accepted. Since implementations previously needed to throw InvalidArgumentException, these now become caught by the language and become TypeError.
Fixing invalid tests. There were a few cases where now-invalid arguments were being passed to interface methods (e.g. when testing that IANA phrases were being used for known status code/reason phase combinations, Diactoros was not casting the status codes pulled from a text file to integer).

Once these changes were made, tests passed as expected.

I then tried the code in a few real-world apps, including one that uses mezzio-swoole, in order to see the effect on consumers. These worked as expected.

My main concern will be that in some cases, we allowed numeric strings for what are now typed as integer arguments, which could have some impact on consumers. That said, this change would require opting in to the new major version, and the changes can be messaged via a blog post or the release notes.

The other issue is going to be PSR-17 and its implementations, as it currently pins against v1, which prevents updating a library implementation that implements both PSR-7 and PSR-17 to a PSR-7 v2 release. We'll need to coordinate with @shadowhand when we're preparing this so that he can update the PSR-17 repo to allow usage with v2 as well.

I think if you can answer the questions listed at the start of this review (either with a reasonable comment response, or code changes), this will be ready. There's also the additional benefit that I've done work in one widely-used implementation, which means we should get adoption of the new version relatively quickly.

shadowhand · 2023-02-21T23:39:34Z

@weierophinney thank you for testing this out and the detailed report. I am happy to start working on updating PSR-17 (and PSR-15) for this change.

weierophinney · 2023-02-22T00:17:13Z

@shadowhand Awesome - I'll ping you if/when the vote passes!

weierophinney

Changes look good, thanks! I'll get the vote started for these this week!

Crell · 2023-02-27T22:05:25Z

composer.json

        }
    ],
    "require": {
-        "php": ">=5.3.0"
+        "php": "^7.2.0 || ^8.0.0"


As with the first PR, let's go straight to 8. There are no supported 7.x versions anymore.

I disagree.

Ubuntu 20.04, RHEL 7, and Debian 11 are all pinned to PHP 7.4 still, and these are all LTS editions. It makes sense to allow them to be able to update to the 1.1 release.

Add return typehints

54df5d4

Stilch changed the title ~~Add typehints~~ Add return typehints May 9, 2021

localheinz reviewed Feb 16, 2022

View reviewed changes

Changed the requirements for the PHP version and removed declare stri…

641e9f8

…ct types

weierophinney mentioned this pull request Feb 21, 2023

Update to PHP7 #83

Closed

weierophinney reviewed Feb 21, 2023

View reviewed changes

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

Added string as return types for __toString

520dd0e

Added void as return types

3def521

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

weierophinney mentioned this pull request Feb 27, 2023

Adds parameter typehints #94

Merged

ashnazg approved these changes Mar 9, 2023

View reviewed changes

Merge branch 'master' into add-param-and-return-type-hints

947607b

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add return typehints #95

Add return typehints #95

Stilch commented May 9, 2021

McLotos commented Jun 20, 2021

McLotos commented Jun 20, 2021

ADmad commented Sep 8, 2021 •

edited

ADmad commented Dec 16, 2021

deluxetom commented Feb 9, 2022

simensen commented Feb 14, 2022

simensen commented Feb 14, 2022

Stilch commented Feb 16, 2022

localheinz Feb 16, 2022

localheinz Feb 16, 2022

localheinz Feb 16, 2022

localheinz Feb 16, 2022

oanhnn commented Apr 15, 2022

ikkez commented Oct 16, 2022

ADmad commented Oct 18, 2022 •

edited

cosminsandu commented Feb 16, 2023

weierophinney commented Feb 21, 2023

weierophinney left a comment

shadowhand commented Feb 21, 2023

weierophinney commented Feb 22, 2023

weierophinney left a comment

Crell Feb 27, 2023

weierophinney Feb 27, 2023 •

edited

Add return typehints #95

Add return typehints #95

Conversation

Stilch commented May 9, 2021

McLotos commented Jun 20, 2021

McLotos commented Jun 20, 2021

ADmad commented Sep 8, 2021 • edited

ADmad commented Dec 16, 2021

deluxetom commented Feb 9, 2022

simensen commented Feb 14, 2022

simensen commented Feb 14, 2022

Stilch commented Feb 16, 2022

localheinz Feb 16, 2022

Choose a reason for hiding this comment

localheinz Feb 16, 2022

Choose a reason for hiding this comment

localheinz Feb 16, 2022

Choose a reason for hiding this comment

localheinz Feb 16, 2022

Choose a reason for hiding this comment

oanhnn commented Apr 15, 2022

ikkez commented Oct 16, 2022

ADmad commented Oct 18, 2022 • edited

cosminsandu commented Feb 16, 2023

weierophinney commented Feb 21, 2023

weierophinney left a comment

Choose a reason for hiding this comment

shadowhand commented Feb 21, 2023

weierophinney commented Feb 22, 2023

weierophinney left a comment

Choose a reason for hiding this comment

Crell Feb 27, 2023

Choose a reason for hiding this comment

weierophinney Feb 27, 2023 • edited

Choose a reason for hiding this comment

ADmad commented Sep 8, 2021 •

edited

ADmad commented Oct 18, 2022 •

edited

weierophinney Feb 27, 2023 •

edited