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

Bump PHP7 version and PHPUnit #773

Merged
merged 6 commits into from
May 28, 2020
Merged

Bump PHP7 version and PHPUnit #773

merged 6 commits into from
May 28, 2020

Conversation

simPod
Copy link
Contributor

@simPod simPod commented Jan 12, 2020

Let's bump PHP to secure supported version. So we can make further improvements to the lib without pain. Eg. see #774. There is v0.4 released with PHP5 support so should be no deal to consumers.

I tried to keep changes at minimum but eg. it's impossible to decently test code with ini_get/ini_set so I had to do small modifications (see Changelog).

@simPod
Copy link
Contributor Author

simPod commented Mar 4, 2020

@cboden answering #774 (comment) here as it's related to this PR.

It was impossible to write tests for it properly when code relied on ini*()_ calls.

Tests throw ini_set(): Headers already sent. You cannot change the session module's ini settings at this time so I figured we should not use these in tests.

@cboden
Copy link
Member

cboden commented Mar 4, 2020

I see. In that case I think it would be best to alter the SessionProvider constructor signature to make add OptionsHandler to the end and make it an optional parameter (see how $serializer is done). This will prevent a BC break.

@simPod
Copy link
Contributor Author

simPod commented Mar 4, 2020

What about now

@cboden
Copy link
Member

cboden commented Mar 4, 2020

Tests are failing 😬

@simPod
Copy link
Contributor Author

simPod commented Mar 4, 2020

Run them locally and passed. Dunno why travis installs symfony/routing v5 when constrain is ^2.6|^3.0|^4.0

Rebasing on current master and fixing.

@patrickbussmann
Copy link

Its very useful. Thanks!

@simPod
Copy link
Contributor Author

simPod commented Apr 4, 2020

@cboden hi, do you have any further comments?

@cboden cboden self-requested a review May 21, 2020 14:47
@cboden cboden added this to the 0.5 milestone May 21, 2020
@cboden
Copy link
Member

cboden commented May 21, 2020

Hey @simPod. Thank you for your work on this and my apologies for my delayed response.

We think we're going to use your PR as a base for v0.5.

One small change request: could you move the OptionsHandler code to our Session namespace please? Since it's not used outside Sessions and it's a requirement for Sessions I don't think we need it more widely scoped.

@simPod
Copy link
Contributor Author

simPod commented May 21, 2020

done

@mbonneau mbonneau changed the base branch from master to v0.5 May 21, 2020 20:24
Copy link
Member

@mbonneau mbonneau left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for PR @simPod .

Copy link
Member

@cboden cboden left a comment

Choose a reason for hiding this comment

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

Looks good. A couple tiny comments. Thank you for all your hard work @simPod

src/Ratchet/Session/PhpOptionsHandler.php Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@cboden cboden merged commit 0469b63 into ratchetphp:v0.5 May 28, 2020
@simPod simPod deleted the php7 branch May 28, 2020 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants