Skip to content

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

Merged
merged 2 commits into from
May 3, 2020
Merged

Update to php 7.3 and update dependencies #105

merged 2 commits into from
May 3, 2020

Conversation

aaajeetee
Copy link

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 uses phpunit/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.

Copy link
Collaborator

@kubawerlos kubawerlos left a 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.

@aaajeetee
Copy link
Author

aaajeetee commented Apr 15, 2020

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 guess I was a bit biased by some of the comments in other pull requests.

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 .travis.yml file now has twice the "COMPOSER_FLAGS" part (different php versions though).

Update: thanks for the feedback.

Copy link
Collaborator

@kubawerlos kubawerlos left a 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

@aaajeetee
Copy link
Author

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?

@kubawerlos
Copy link
Collaborator

Not really. I'd say let's change to GitHub Actions anyway if @povils is fine with it.

@aaajeetee
Copy link
Author

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?

@povils
Copy link
Owner

povils commented Apr 17, 2020

Hey everyone! I think a minor version bump here is good enough:)

@aaajeetee
Copy link
Author

@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.

@nullabe
Copy link

nullabe commented Apr 21, 2020

I agree with @aaajeetee , it would be nice to not have phpunit major version installed on parent project be locked by phpunit/php-timer dependency of this package. Anyway, thank you for your work !

@povils povils merged commit 51861e5 into povils:master May 3, 2020
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.

4 participants