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

Ensure final input of CommandTester works with default #29754

Merged
merged 1 commit into from Jan 4, 2019

Conversation

Projects
None yet
4 participants
@Firehed
Copy link
Contributor

Firehed commented Jan 3, 2019

Q A
Branch? 4.x
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

If the final element of CommandTester::setInputs() is an empty string (to send the default value), the internal stream the tester uses hits EOF and triggers the Aborted exception. This appends an additional EOL to the stream after the implode to simulate one final return key, allowing the final input to use the default value when used in the tester's documented style.

A test has been added to cover the new behavior, which failed before this change.

@Firehed Firehed force-pushed the Firehed:patch-1 branch from 42f3e9f to 9f5c8d3 Jan 3, 2019

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Jan 3, 2019

@@ -162,7 +162,7 @@ private static function createStream(array $inputs)
{
$stream = fopen('php://memory', 'r+', false);
fwrite($stream, implode(PHP_EOL, $inputs));
fwrite($stream, implode(PHP_EOL, $inputs).PHP_EOL);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jan 3, 2019

Member

When an empty array is passed, this writes a PHP_EOL. Legit?
Should the code be rewritten as such?

foreach ($inputs as $line) {
    fwrite($stream, $line.PHP_EOL);
}
@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

ping @Simperfit could you review and tell if it's legit to you?
Note that this should target 4.1

@chalasr

chalasr approved these changes Jan 3, 2019

Copy link
Member

chalasr left a comment

Looks good to me (code owner here). 👍 once @nicolas-grekas's comment addressed

@Firehed

This comment has been minimized.

Copy link
Contributor

Firehed commented Jan 3, 2019

I prefer that approach too. Changed :)

Ensure final input of CommandTester works with default
If the final element of `CommandTester::setInputs()` is an empty string (to send the default value), the internal stream the tester uses hits EOF and triggers the `Aborted` exception. This appends an additional EOL to the stream after the `implode` to simulate one final return key, allowing the final input to use the default value when used in the tester's documented style.

@chalasr chalasr changed the base branch from master to 3.4 Jan 4, 2019

@chalasr chalasr modified the milestones: 4.1, 3.4 Jan 4, 2019

@chalasr chalasr force-pushed the Firehed:patch-1 branch from b26afb4 to 6b87b67 Jan 4, 2019

@chalasr chalasr added the Console label Jan 4, 2019

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Jan 4, 2019

Rebased on 3.4. Note to mergers: patch needs to be moved to TesterTrait starting from 4.1.

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Jan 4, 2019

Good catch, thanks @Firehed.

@chalasr chalasr merged commit 6b87b67 into symfony:3.4 Jan 4, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

chalasr added a commit that referenced this pull request Jan 4, 2019

bug #29754 Ensure final input of CommandTester works with default (Fi…
…rehed)

This PR was merged into the 3.4 branch.

Discussion
----------

Ensure final input of CommandTester works with default

| Q             | A
| ------------- | ---
| Branch?       | 4.x
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

If the final element of `CommandTester::setInputs()` is an empty string (to send the default value), the internal stream the tester uses hits EOF and triggers the `Aborted` exception. This appends an additional EOL to the stream after the `implode` to simulate one final return key, allowing the final input to use the default value when used in the tester's documented style.

A test has been added to cover the new behavior, which failed before this change.

Commits
-------

6b87b67 Ensure final input of CommandTester works with default
@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Jan 4, 2019

And thank you for contributing to Symfony!

This was referenced Jan 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment