Skip to content
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

[RoadRunner] Add support for RoadRunner 2023 and 2024 #172

Merged
merged 6 commits into from
Nov 17, 2024

Conversation

msmakouz
Copy link
Contributor

This pull request adds support for RoadRunner 2023 and 2024. The required PHP packages have been added to the dependencies.

With the old version of RoadRunner (2.x), need to explicitly install it (composer req spiral/roadrunner:^2.0 -W), which will install older versions of spiral/roadrunner-http (v2.2.0) and spiral/roadrunner-worker(v2.3.0).

composer.json Outdated Show resolved Hide resolved
@@ -12,14 +12,16 @@
"require": {
"php": ">=8.0.5",
"nyholm/psr7": "^1.4",
"spiral/roadrunner": "^2.0",
"spiral/roadrunner-cli": "^2.6",
"spiral/roadrunner-http": "^2.2 || ^3.5.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have really a min requirement to 2.2 and 3.5.1? Or can we go with "^2 || ^3". Keep the versions wide as possible makes upgrading for others mostly easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexander-schranz
3.x - Starting from version 3.5.0, support for RoadRunner 2024.1 has been added. Versions below this will only support RoadRunner 2023.3.12 and earlier. I wanted to ensure support for 2024. If ensuring support for 2024 is not necessary, the version can be downgraded to 3.0.
2.x - Version 2.x is required to support RoadRunner 2.x. I added 2.2 because it was the latest 2.x version, released on November 30, 2022. I assumed that users have been using it for a long time. However, we can downgrade to 2.0 if necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From library perspective we should keep it as open as possible if we don't require a newer version we should keep it in our range aslong as possible. This make it easier for projects adopting lib updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense, changed to ^2.0 || ^3.0.

@@ -88,7 +88,7 @@ jobs:
composer-options: "--ignore-platform-reqs" # required for PHP 8.2 as not yet official supported

- name: Psalm
run: psalm --no-progress --output-format=github
run: psalm --php-version=8.2 --no-progress --output-format=github
Copy link
Member

@alexander-schranz alexander-schranz Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unfamiliar with pslam @Nyholm can you have a look at this change if you are fine with it or not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is inferred from the PHP version we are running on.

I dont mind adding it, but it would be nice to hear what value it brings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msmakouz can you answer that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexander-schranz @Nyholm In the Psalm action, PHP 8.2 is used, and the latest dependencies are used. However, without this option, Psalm checks with the PHP version specified as the minimum in composer.json, and it displays two errors:

Error: src/roadrunner-nyholm/src/Runner.php:23:19: UndefinedClass: Class, interface or enum named Spiral\RoadRunner\Worker does not exist (see https://psalm.dev/019)
Error: src/roadrunner-symfony-nyholm/src/Runner.php:35:19: UndefinedClass: Class, interface or enum named Spiral\RoadRunner\Worker does not exist (see https://psalm.dev/019)

https://github.com/php-runtime/runtime/actions/runs/9156994654/job/25172484593

I assume this is because the package spiral/roadrunner-worker (v3.6.0) requires a minimum PHP version of 8.1. If this option is added, these errors are not present. Need to specify this option or lower the PHP version in the action to 8.0 so that the dependencies requiring a minimum PHP version of 8.0 are installed.

Copy link
Member

@alexander-schranz alexander-schranz Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds for me like we are missing the requirement to spiral/roadrunner-worker package in our own composer.json also. So think we need to add:

"spiral/roadrunner-worker": "^2 || ^3"

also.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with Psalm is not related to this. This package was installed anyway because it is required by roadrunner-php/http. However, spiral/roadrunner-worker is used in this package, so it needs to be added, you are right. I have added it.

@gsdevme
Copy link
Contributor

gsdevme commented Aug 27, 2024

Hey, sorry checking in here from the original #138

Is there something more to do here?

@racastellanosm
Copy link

I'm very glad to know that this issue is ready to be merged.

There's anyone who knows when this should happen? (i'm waiting for this to be able to upgrade my symfony application to use the latest RR version)

@@ -12,14 +12,17 @@
"require": {
"php": ">=8.0.5",
"nyholm/psr7": "^1.4",
"spiral/roadrunner": "^2.0",
Copy link

@racastellanosm racastellanosm Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about kept this dependency and add the 2023 and 2024 versions in it?

"spiral/roadrunner": "^2 || ^2023 || ^2024",

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See discussions before that is just incorrect and need handled differently.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants