Add PHP >= 8 support + updated tests and workflow. #1014
Conversation
Allow manual workflow run
Update comments
Remove unnecessary include from HttpServer
…d a better solution)
… maintaining PHP < 7.0 support.
Add http-foundation 6.0 back
Disable scheduled action run
Disable scheduled action run
|
Hey there, @cboden |
|
why php5.4 its useless... just drop php < 8 |
Agreed, however it's not my project so keeping/dropping support isn't up to me, I'm just getting it working again. |
|
@josh-gaby awesome work, hopefully it can get reviewed and merged soon! :) |
| @@ -80,61 +114,61 @@ public function run() { | |||
| * @param \React\Socket\ConnectionInterface $conn | |||
| */ | |||
| public function handleConnect($conn) { | |||
| $conn->decor = new IoConnection($conn); | |||
| $conn->decor->resourceId = (int)$conn->stream; | |||
| $io_conn = new IoConnection($conn); | |||
There was a problem hiding this comment.
I'm guessing $conn->decor should still be left to avoid a breaking change
There was a problem hiding this comment.
decor is not a property of \React\Socket\ConnectionInterface so can't be left in because php 8 deprecated dynamic property creation.
There was a problem hiding this comment.
Then maybe we also make a PR to reactphp/socket to add #[AllowDynamicProperties] to the interface https://github.com/reactphp/socket/blob/3.x/src/ConnectorInterface.php
There was a problem hiding this comment.
Or we simply use spl storage locally in IoServer to store the decor and retrieve it when needed
| */ | ||
| public function handleData($data, $conn) { | ||
| public function handleData($data, $io_conn) { |
There was a problem hiding this comment.
Changing the parameter there and in the other methods is a breaking change, because IoServer can be extended
There was a problem hiding this comment.
I agree, this a breaking change, however it was done because \React\Socket\ConnectionInterface doesn't actually have the decor property previously being used to pass the connection around and PHP 8 deprecated dymanic property creation.
|
|
||
| if (version_compare(\Composer\InstalledVersions::getVersion('symfony/http-foundation'), '6.0.0') === -1) { | ||
| // The version of http-foundation is < 6, include a class without return types | ||
| include 'VirtualProxy-HttpFoundation<6.php'; |
There was a problem hiding this comment.
This is unnecessary, the return types can be added without breaking the signature, it's removing or changing them that's an issue
There was a problem hiding this comment.
This is required if support for PHP < 7.0 is to be maintained, earlier versions didn't support return types.
There was a problem hiding this comment.
Why keep support for such old php? that's asking for maintenance hell, they have been EOL for more than 6 years, a PR like this can absolutely include such breaking changes
There was a problem hiding this comment.
I agree, but like I told the commenter above, it's not my project, so all I'm doing is getting it working again for php 8+
Personally, I would do it as a new major version that supported php >= 7.4 and leave < 7.4 using the current version but that's nothing to do with me.
Also, this pull request has been sitting here for a year now, I'm not sure how likely it is to even get merged at this point to be honest.
There was a problem hiding this comment.
There is ongoing work recently on ratchet from the maintainers to get the projects back alive
See
So it's quite likely
It's okay if it's not your project, you can do a PR with a breaking change and mark it as such so it will be released as a major, otherwise those kind of big PR's are a maintenance pain and it's actually more likely to be accepted if you don't add API breaking changes in your PR but drop support for older php versions because it's 100% sure that to bring it back to life it's a new major that's going to be released next, because they are releasing a major for reactphp/event-loop that also drops support for php7.1-
A compat layer is nice when the project doesn't need a major release, but this project hasn't had a release in 3 years so it will need one
So if you want a high likelihood that your PR is accepted, follow what they are doing with reactphp/event-loop and drop support for <=php7.0 in your PR only keep 7.1+, this will be a step up transition version, so no API breaking change but only php version
And then in the future we'll have another major that adds types to the Interfaces, this is so that users can upgrade painlessly to the new version at first but the next one will need some work from them
(Previously, the set test checked for existence, giving you no way of creating the property dynamically). Signed-off-by: Nigel Cunningham <nigel@nigelcunningham.com.au>
|
what is still needed to merge this PR ? @josh-gaby @clue ? |
Provide a way to set and unset a dynamic property.
|
@josh-gaby i don't get why you still havent received any reply yet from the maintainers. i see your still updating your fork. |
|
@dvdknaap I've been using it in a reasonably sized project for the last year, haven't had any issues at all so far but if you do find anything let me know and I'll try fix it |
Includes updated tests and workflow for running tests on PHP 5.4 -> 8.2
Also, due to symfony/http-foundation v6+ including return types and Ratchet supporting PHP < 7.0, there is a not very nice hack involving maintaining two versions of two classes (VirtualProxy and VirtualSessionStorage). This is currently working and passing all tests but definitly needs a better solution long-term.