8000 [Console] Require PHP 7 for ApplicationTest by siwinski · Pull Request #23934 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Console] Require PHP 7 for ApplicationTest #23934

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

Conversation

siwinski
Copy link
Contributor
Q A
Branch? 3.3
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Since ApplicationTest uses the Error class, this component should require-dev symfony/polyfill-php70

@nicolas-grekas
Copy link
Member

I suggest to use an @requires PHP 7 annotation on the test case instead.

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Aug 22, 2017
@siwinski siwinski force-pushed the pr-update-console-require-dev-with-polyfill branch from ace9db6 to c131a47 Compare August 23, 2017 20:53
@siwinski siwinski changed the title [Console] Add "symfony/polyfill-php70" to require-dev [Console] Require PHP 7 for ApplicationTest Aug 23, 2017
@siwinski
Copy link
Contributor Author

Changes made per request above. Branch rebased.

@@ -33,6 +33,9 @@
use Symfony\Component\Console\Exception\CommandNotFoundException;
use Symfony\Component\EventDispatcher\EventDispatcher;

/**
* @requires PHP 7
Copy link
Member

Choose a reason for hiding this comment

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

Only methods which use PHP7 features should be annotated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and rebased

@siwinski siwinski force-pushed the pr-update-console-require-dev-with-polyfill branch from c131a47 to 94d55ca Compare August 23, 2017 21:44
@chalasr
Copy link
Member
chalasr commented Aug 23, 2017

Good catch, thanks @siwinski.

@chalasr chalasr merged commit 94d55ca into symfony:3.3 Aug 23, 2017
chalasr pushed a commit that referenced this pull request Aug 23, 2017
This PR was merged into the 3.3 branch.

Discussion
----------

[Console] Require PHP 7 for ApplicationTest

| Q             | A
| ------------- | ---
| Branch?       | 3.3 <!-- see comment below -->
| Bug fix?      | no
| New feature?  | no <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | <!--highly recommended for new features-->

Since [ApplicationTest](https://github.com/symfony/symfony/blob/3.3/src/Symfony/Component/Console/Tests/ApplicationTest.php) uses the `Error` class, this component should require-dev `symfony/polyfill-php70`

Commits
-------

94d55ca [Console] Require PHP 7 for ApplicationTest
@siwinski siwinski deleted the pr-update-console-require-dev-with-polyfill branch October 21, 2017 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0