-
Notifications
You must be signed in to change notification settings - Fork 47
Update to php 7.3 and update dependencies #105
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for introducing the BC? Simply adding the higher versions of dependencies in composer.json
should be enough.
No particular reason to be honest, more in a way that 'old' versions can't be supported forever. However, it's not up to me to decide if a major version should be released. I have changed the code. Also, I am new to travis and appveyor, so I have copied the php7.2 lines, but I am not sure if that is correct. Mainly the Update: thanks for the feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more in a way that 'old' versions can't be supported forever.
if we have good reason to drop some version support the fine, in this case, we don't change a line of code, so no reason. After all Composer is still supporting PHP 5.3 and will be in next major.
Travis config looks good now, you can also add PHP 7.4
For the AppVeyor I'm not an expert, I always look at this config to find solution: https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.16/.appveyor.yml
Thanks. I have added php7.4 support. Regarding appveyor: I see other PR's also fail on appveyor. Even the last commit on master branch fails. Is it related to this PR to fix appveyor then? |
Not really. I'd say let's change to GitHub Actions anyway if @povils is fine with it. |
So what happens next? I suppose (one of) the maintainer(s) should merge this PR? I could use this fix, same as other packages/ projects that require phpmnd. Any ideas on when this will be merged? |
Hey everyone! I think a minor version bump here is good enough:) |
@povils thanks for your reply. In fact, after feedback from @kubawerlos I have changed to code to increase the minor version, not the major version. |
I agree with @aaajeetee , it would be nice to not have phpunit major version installed on parent project be locked by |
I'm a great fan of this tool. However, when trying to install alongside other tools such as phpunit version 9, phpmnd cannot be installed because it needs
phpunit/php-timer:2.0
. Phpunit nowadays usesphpunit/php-timer:3.0
.I upgraded to php7.3 as well because new version of used tools have this as a requirement (phpunit v9 and php-timer v3 for example).
Since this is a backwards incompatible change, a new major version should be released for this.
Tests and codestyle are still good.
Please provide feedback if I have missed anything or did something wrong.
I did not see any issues (specifically) for this, so I decided to work on this myself. Please let me know if I overlooked.