Skip to content

Fix minimum phpunit version and allow v5 - #115

Merged
clue merged 2 commits into
reactphp:masterfrom
andig:composer2
Feb 12, 2017
Merged

Fix minimum phpunit version and allow v5#115
clue merged 2 commits into
reactphp:masterfrom
andig:composer2

Conversation

@andig

@andigandig commented Feb 11, 2017

Copy link
Copy Markdown
Contributor

Follow-up of #111 and #114

phpunit ^5.0 is required since as of 4.8.20 php 7.0 is not supported (see https://github.com/sebastianbergmann/phpunit/blob/4.8.32/ChangeLog-4.8.md)

Welcome to version hell :/

@clue

clue commented Feb 11, 2017

Copy link
Copy Markdown
Member

phpunit ^5.0 is required since as of 4.8.20 php 7.0 is not supported (see https://github.com/sebastianbergmann/phpunit/blob/4.8.32/ChangeLog-4.8.md)

This change was actually reverted in 4.8.21. In other words: This actually works perfectly fine currently and installs 4.8.35 on PHP 7 (at the time of writing this).

That being said, I agree that it makes perfect sense to support PHPUnit 5 as well 👍

@clueclue modified the milestone: v0.4.4Feb 11, 2017
Comment threadcomposer.json Outdated
},
"require-dev": {
"phpunit/phpunit": "~4.8"
"phpunit/phpunit": "^4.8.10|^5.0"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should probably be ^4.8.10 || ^5.0?

Also, PHPUnit 5 emits several warnings on PHP 7, can you address these here? See also #93 and #107.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Arguably, fixing the lowest version and supporting PHPUnit 5 could be two separate PRs, but I'll leave this up to you 👍

@andig

Copy link
Copy Markdown
ContributorAuthor

@clue was a bit lazy and didn't split. Both phpunit 4 and 5 tests are passing now.

@clueclue left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM now :shipit: 👍

@andig

Copy link
Copy Markdown
ContributorAuthor

Once this is merged, #114 should pass, too.

@clue
clue merged commit 2f4b224 into reactphp:masterFeb 12, 2017
@andig
andig deleted the composer2 branch February 12, 2017 15:13
@clueclue mentioned this pull request Feb 13, 2017
Sign up for freeto 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

@andig@clue@jsor@WyriHaximus