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

Bumped Symfony version #130

Closed
wants to merge 1 commit into from

Conversation

DonCallisto
Copy link

No description provided.

@@ -12,14 +12,12 @@ env:
- SYMFONY_PHPUNIT_DIR=$HOME/.phpunit-bridge
- SYMFONY_PHPUNIT_REMOVE='' # don't remove prophecy

php: [7.2, 7.1, 7.0, 5.6, 5.5, 5.4]
php: [7.2, 7.1]
Copy link
Member

Choose a reason for hiding this comment

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

Dropping support for older PHP version in a PR described as bumping Symfony version ? That's not good description of what the PR does.

Copy link
Author

Choose a reason for hiding this comment

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

You're right but without those drops the CI won't go green.
After this #129 is merged, this change won't be there anymore.

@stof
Copy link
Member

stof commented Dec 10, 2019

I'm closing this PR, because it is not at all about supporting new Symfony versions, but about dropping some PHP versions, which is not necessary for now, and is not what this PR claims to do.

@stof stof closed this Dec 10, 2019
@DonCallisto
Copy link
Author

@stof have you even read my comment?

@stof
Copy link
Member

stof commented Dec 10, 2019

#132 has added support for Symfony 5, without dropping anything of the current support.

@DonCallisto
Copy link
Author

DonCallisto commented Dec 10, 2019

@stof So? what's the point here? Is it a comparison?
I've explicitly stated the motivations behind this choice.
If you don't want to drop the older PHP versions let's state clearly it the related PR (#129) not here.
If the related PR was closed (and it's was not) and the red CI is acceptable from your POV I would have go with your indications.
Behaving like you have done here is a no-go from my standpoint. Yes you can act as you prefer and I can only imagine how much OSS work you have (thanks, by the way!) but this kind of miscommunication is what keep some people out of OSS contribution.

@DonCallisto
Copy link
Author

Don’t get me wrong, I’m not complaining about having this PR closed (what you stayed in #129 is perfectly understandable), just sayin’ it would have been better to explain what supplementare work could have been done in order to make this PR acceptable.
Closing with te motivation of php version bump (that’s not the main focus of the PR) is totally unrelated as the bump itself.

@xabbuh
Copy link

xabbuh commented Dec 14, 2019

@DonCallisto Can you explain what you think is missing from this PR after #132 was merged?

@DonCallisto
Copy link
Author

I was not arguing against having close this PR.
However just bumping the minors still supported but as stof said is something you don’t want to do, and rightfully so after his explaination.

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

3 participants