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

Chore: drop $loop input parameters in favor of Loop::get() #176

Open
wants to merge 5 commits into
base: 3.x
Choose a base branch
from

Conversation

bpolaszek
Copy link

@bpolaszek bpolaszek commented Feb 24, 2024

Following roadmap reactphp/http#517

@SimonFrings
Copy link
Member

Hey @bpolaszek, thank you for your pull request, always happy about contributions 👍

I know the description says "not ready for review", but I'll add my initial thoughts anyway 😄

Changing the minimal supported PHP version to PHP 7.1 is something we want to see for ReactPHP v3, but @WyriHaximus already picked this up in #175. This PR is not far from being merged, so you may have to rebase in here once this goes live. In general, I also think we should use separate PRs for these topics to avoid bloating each pull request in complexity, which also makes them easier to review.

The rest of the changes are heading in a good direction. I think you can also take a look at clue/reactphp-redis#156 and use this as inspiration (for changes, PR description, commit history, etc.), as we also removed the optional $loop parameter in there. Thanks to our consistency across all our projects, we can apply similar changes in here.

@bpolaszek
Copy link
Author

Hi @SimonFrings,

Sorry for the delay !
No worries, I did that mostly for the CI to pass and to avoid the IDE to complain I'm using type hints with PHP < 7.
I will rebase once it's merged and erase those changes.
Usually I would have based my PR on the WyriHaximus-secret-labs:3.x-raise-minimum-php-version-to-7.1PLUS branch, except I didn't fork from him but from reactphp/stream so I cannot base my work on that branch.
Regarding the changes, I just dropped a few commits to be more consistent with your example - don't hesitate to tell me if I missed something.

Thanks!
Ben

@bpolaszek bpolaszek marked this pull request as ready for review March 5, 2024 09:33
@SimonFrings
Copy link
Member

I will rebase once it's merged and erase those changes.

@bpolaszek Sounds good, we're currently focusing on a v1.10.0 for HTTP which we're planning to release around next week, so @clue and I probably need a week or two before continuing with #175. Maybe we'll find some time in between, but we want to unblock the HTTP v3 development first which is currently depending on the upcoming v1.10.0 as described in reactphp/http#517.

Regarding the changes, I just dropped a few commits to be more consistent with your example - don't hesitate to tell me if I missed something

Nice, I will have a closer look at your changes later today or in the next days, I will keep you posted 👍

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.

2 participants