Skip to content

Fix minimum phpunit version and allow v5 #115

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

Merged
merged 2 commits into from
Feb 12, 2017
Merged

Conversation

andig
Copy link
Contributor

@andig andig commented Feb 11, 2017

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
Copy link
Member

clue commented Feb 11, 2017

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 👍

@clue clue modified the milestone: v0.4.4 Feb 11, 2017
composer.json Outdated
@@ -16,6 +16,6 @@
}
},
"require-dev": {
"phpunit/phpunit": "~4.8"
"phpunit/phpunit": "^4.8.10|^5.0"
Copy link
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
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
Contributor Author

andig commented Feb 12, 2017

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

Copy link
Member

@clue clue left a comment

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
Contributor Author

andig commented Feb 12, 2017

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

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