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

Don't clear PHP buffer when an error occurs with debug=true #3059

Merged
merged 1 commit into from Jun 10, 2019

Conversation

Projects
None yet
3 participants
@lyrixx
Copy link
Contributor

commented Jun 6, 2019

fixes #3058

@fabpot

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

@lyrixx Can you add a test to check that we don't leak output on non-debug mode and that we have the output on debug mode? Also, can you add a note in the CHANGELOG?

@lyrixx

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@fabpot Done. That was not easy because PHPUnit buffer output + has an error handler. So the simplest thing to do was to use an external script

@lyrixx lyrixx force-pushed the lyrixx:blank-screen branch from 53aabcc to d6778f9 Jun 7, 2019

Show resolved Hide resolved test/Twig/Tests/ErrorTest.php Outdated

@lyrixx lyrixx force-pushed the lyrixx:blank-screen branch from d6778f9 to 827ba89 Jun 7, 2019

@stof

stof approved these changes Jun 7, 2019

@fabpot

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

@lyrixx Tests do not pass for 5.4 :)

@lyrixx lyrixx force-pushed the lyrixx:blank-screen branch from 827ba89 to 852d0ff Jun 7, 2019

@lyrixx

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Woho, PHP 5.4. Thanks for report. I fixed that in a very clean way 😂

@fabpot

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Yay for PHP 5.4... but some other PHP versions are now broken 😂

Show resolved Hide resolved CHANGELOG Outdated
Show resolved Hide resolved composer.json Outdated
Show resolved Hide resolved test/Twig/Tests/Fixtures/errors/leak-output.php Outdated
Show resolved Hide resolved test/Twig/Tests/Fixtures/errors/leak-output.php Outdated

@lyrixx lyrixx force-pushed the lyrixx:blank-screen branch from 852d0ff to 3fd0dde Jun 9, 2019

@lyrixx

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

I stop using symfony/process for using plain exec. Simpler to manage! And now it's all green 💚
I also addressed your comments.
Thanks.

@fabpot fabpot force-pushed the lyrixx:blank-screen branch from 3fd0dde to c910e71 Jun 10, 2019

@fabpot

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

Thank you @lyrixx.

@fabpot fabpot merged commit c910e71 into twigphp:1.x Jun 10, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

fabpot added a commit that referenced this pull request Jun 10, 2019

bug #3059 Don't clear PHP buffer when an error occurs with debug=true…
… (lyrixx)

This PR was squashed before being merged into the 1.x branch (closes #3059).

Discussion
----------

Don't clear PHP buffer when an error occurs with debug=true

fixes #3058

Commits
-------

c910e71 Don't clear PHP buffer when an error occurs with debug=true

@lyrixx lyrixx deleted the lyrixx:blank-screen branch Jun 10, 2019

public function testTwigLeakOutputInDebugMode()
{
$output = exec(sprintf('%s %s debug', \PHP_BINARY, __DIR__.'/Fixtures/errors/leak-output.php'));

This comment has been minimized.

Copy link
@stof

stof Jun 11, 2019

Contributor

you need to escape the argument for the path (__DIR__ might have spaces in it)

This comment has been minimized.

Copy link
@lyrixx

lyrixx Jun 11, 2019

Author Contributor

Good catch. #3062

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.