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

Update to symfony/console 6.0 and newer version of phpunit #61

Merged
merged 5 commits into from
Feb 20, 2022

Conversation

cjobeili
Copy link
Contributor

@cjobeili cjobeili commented Dec 16, 2021

This PR aims to update symfony/console version in order to make it work for brefphp/bref in ref of #60

I have updated to a newer version of phpunit in a separated commit too

composer.json Outdated
"php-di/invoker": "~2.0",
"psr/container": "^1.0|^2.0"
},
"require-dev": {
"phpunit/phpunit": "~6.4",
"phpunit/phpunit": ">=6.4",
Copy link
Owner

Choose a reason for hiding this comment

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

This runs the risk of breaking with new PHPUnit versions. Maybe try something like ^6.4 || 7 || 8?

@mnapoli
Copy link
Owner

mnapoli commented Dec 21, 2021

🤦 thank you Travis-CI… we have no CI anymore.

@osteel
Copy link

osteel commented Feb 8, 2022

Hi!

What's preventing this from moving forward? Since Valet uses Silly as a dependency, it seems that every local environment featuring Valet is currently prevented from upgrading to symfony/console 6.

Cheers

@mnapoli
Copy link
Owner

mnapoli commented Feb 8, 2022

Honestly, it depends on our degree of confidence here since we have no CI.

The ideal scenario is someone sends a PR to add GitHub Actions CI, but I know that this takes time (as I won't have much time in the near future to tackle that). If this PR is really blocking, and you both are confident this PR is good, I'm fine with merging. I don't want to be the SPOF here.

Also there's this minor comment that needs to be adjusted before merging: #61 (comment)

Any help is welcome!

@osteel
Copy link

osteel commented Feb 8, 2022

Ok cool. Can't promise anything but I may try and look into adding GitHub CI instead (in a separate PR)

@cjobeili
Copy link
Contributor Author

cjobeili commented Feb 8, 2022

Also there's this minor comment that needs to be adjusted before merging: #61 (comment)

I've amend the last commit with the changes suggested ! Thanks and sorry for the delay !

@osteel
Copy link

osteel commented Feb 8, 2022

Sweet, thanks @cjobeili. I think a good approach now would be to fix CI in a different PR, get that other PR merged in, then merge master back into this PR and make sure CI passes. Does that sound sensible?

@cjobeili
Copy link
Contributor Author

cjobeili commented Feb 8, 2022

Sweet, thanks @cjobeili. I think a good approach now would be to fix CI in a different PR, get that other PR merged in, then merge master back into this PR and make sure CI passes. Does that sound sensible?

Not at all, i've just worked on github actions so https://github.com/cjobeili/silly/actions/runs/1813643403 .
Here you can see tests for 7.0 to 8.1 in a matrix (with 8+ failing due to composer file). I will make a PR for this.

@mnapoli
Copy link
Owner

mnapoli commented Feb 8, 2022

I've merged #64

@cjobeili
Copy link
Contributor Author

cjobeili commented Feb 8, 2022

Updated with master and kaboom ! I'm looking at it

@cjobeili
Copy link
Contributor Author

cjobeili commented Feb 8, 2022

So here my "investigations":

7.0, 7.1 and 7.2 fails because of the new composer condition added >=7.4
8.1 and 8.0 fails because of the version of phpunit , i need to add || 9 i think
and i've missed 7.4 on the github workflow.

Are we ok to drop all version before 7.4 ?

Thanks!

@mnapoli
Copy link
Owner

mnapoli commented Feb 8, 2022

Yes!

@cjobeili
Copy link
Contributor Author

cjobeili commented Feb 9, 2022

The last fail is due to --prefer-lowest composer arg.
For 8.1 it tried to install phpunit in version 8.5.12 and fails because of :

Run vendor/bin/phpunit PHP Fatal error: Cannot acquire reference to $GLOBALS in /home/runner/work/silly/silly/vendor/phpunit/phpunit/src/Util/Configuration.php on line 40[7](https://github.com/cjobeili/silly/runs/5122961894?check_suite_focus=true#step:7:7) Error: Process completed with exit code 255.

this bug has been fixed since 8.5.14 commit but composer seems to like this version.

Any idea how to handle this issue ?

Thanks!

@osteel
Copy link

osteel commented Feb 9, 2022

@cjobeili this may be a naive answer but couldn't you just change the dependency requirement for this?

"phpunit/phpunit": "^6.4|^7|^8.5.14|^9",

Since PHPUnit 8 is introduced for the first time anyway.

Thanks for doing this by the way!

@PieterCappelle
Copy link

Please update & merge this.

@mnapoli
Copy link
Owner

mnapoli commented Feb 18, 2022

@PieterCappelle feel free to send a PR.

@mnapoli
Copy link
Owner

mnapoli commented Feb 20, 2022

@cjobeili I'm fine to merge this, is this ready to merge for you?

@cjobeili
Copy link
Contributor Author

@mnapoli I'm fine with this, i've drop the low-deps matrix because it was a pain to make it work "quickly". If it's ok for you it's ok for me ! All tests passes with standard deps.

@mnapoli mnapoli merged commit c35cd1d into mnapoli:master Feb 20, 2022
@mnapoli
Copy link
Owner

mnapoli commented Feb 20, 2022

Thank you!

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.

None yet

4 participants