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

Allow Symfony4 #79

Merged
merged 3 commits into from Feb 10, 2018
Merged

Allow Symfony4 #79

merged 3 commits into from Feb 10, 2018

Conversation

VolCh
Copy link
Contributor

@VolCh VolCh commented Oct 21, 2017

This PR add support symfony-console-completion in projects that use console 4.0.0-BETA1 and(or) PHPUnit 5.7.

It seems safe to add "minimum-stability": "beta" in master, after release it should be removed. Or you can pull the PR in separate branch to use for example "stecman/symfony-console-completion": "dev-symfony4" in composer.json for testing projects with beta without add forking repo to composer.json

composer.json Outdated
@@ -8,12 +8,13 @@
"email": "stephen@stecman.co.nz"
}
],
"minimum-stability": "beta",
Copy link
Contributor

@aik099 aik099 Oct 22, 2017

Choose a reason for hiding this comment

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

This won't have any effect, because it's read only from master composer.json of your project and not the dependency library composer.json files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

composer.json Outdated
},
"require-dev": {
"phpunit/phpunit": "~4.4"
"phpunit/phpunit": "~4.4 || ~5.7"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add PHPUnit 6.x here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, but note that 4.x & 6.x are incompatible and 6.x not supports \PHPUnit_Framework_TestCase, you should use \PHPUnit\Framework\TestCase at least in your tests. 5.7 supports both FQCN cases

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, then please (if possible) send a separate PR where you replaces \PHPUnit_Framework_TestCase with TestCase + use PHPUnit\Framework\TestCase;.

@VolCh
Copy link
Contributor Author

VolCh commented Oct 22, 2017

Rewrited tests for use PHPUnit in 6.x style. Tested with 4.8, 5.7 and 6.4

@VolCh VolCh changed the title Support Symfony 4 and PHPUnit 5 Support Symfony 4 and PHPUnit 5 & 6 Oct 22, 2017
if ($status !== 0) {
$this->fail("Syntax check for $context failed:\n$output");
}
$this->assertEquals(0, $status, "Syntax check for $context failed:\n$output");
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not equivalent construct, because assertEquals doesn't do === internally. Use assertSame instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@aik099
Copy link
Contributor

aik099 commented Oct 22, 2017

Tests are failing PHP 5.5, because PHPUnit version on Travis CI isn't the one with support of TestCase polyfill class. Probably, as separate PR, we can install latest compatible PHPUnit version based on PHP version.

@VolCh VolCh force-pushed the symfony4 branch 2 times, most recently from 178b399 to e0fc0af Compare October 22, 2017 10:14
@VolCh
Copy link
Contributor Author

VolCh commented Oct 22, 2017

I set the lowest PHPUnit version to 4.8.36 that introduced forward compatible layer.

@VolCh VolCh changed the title Support Symfony 4 and PHPUnit 5 & 6 Allow Symfony4 Oct 22, 2017
@VolCh VolCh mentioned this pull request Oct 22, 2017
@VolCh
Copy link
Contributor Author

VolCh commented Oct 22, 2017

#81 - there used the phpunit from require-devs composer section.

@aik099 aik099 mentioned this pull request Oct 22, 2017
@aik099
Copy link
Contributor

aik099 commented Oct 22, 2017

@VolCh
Copy link
Contributor Author

VolCh commented Oct 28, 2017

Need some more steps on my part to release changes? Or are we waiting for a stable Symfony 4 version?

@VolCh
Copy link
Contributor Author

VolCh commented Nov 10, 2017

Rebases on #81 & #80 to test passing

@aik099
Copy link
Contributor

aik099 commented Nov 10, 2017

@stecman , any updates?

@DavertMik
Copy link

@stecman ping? ;)

@petrmalina
Copy link

@stecman pretty please?

Copy link
Owner

@stecman stecman left a comment

Choose a reason for hiding this comment

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

Looks good, thanks 👍 I'll give this a test tomorrow morning and get it merged

.travis.yml Outdated
@@ -2,13 +2,15 @@ language: php

matrix:
include:
- php: 5.3
Copy link
Contributor

@aik099 aik099 Nov 28, 2017

Choose a reason for hiding this comment

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

Instead add dist: precise so that PHP 5.3 build can work.

5.3 supported by Travis CI now in precise only
7.2 is RC
7.3 is nightly
HVVM is 5.6 based and I think that -prefer-lowest is enough nowadays
@VolCh
Copy link
Contributor Author

VolCh commented Nov 28, 2017

@aik099 thanks for advice: restored 5.3. Also added 7.3 (nightly) with allow failures

@aik099
Copy link
Contributor

aik099 commented Nov 28, 2017

Thanks @VolCh .

@danemacmillan
Copy link

Since Symfony 4 was released November 30th, are we going to see this merged in?

@timoschwarzer
Copy link

+1

@stecman stecman merged commit 210b1ca into stecman:master Feb 10, 2018
@route1rodent
Copy link

any chance to get this released as a semver tag? :) thanks

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

8 participants