Skip to content

Fix: PHP 7.4 cli logs in stderr output #10

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
merged 5 commits into from
Feb 25, 2020

Conversation

gabfr
Copy link

@gabfr gabfr commented Feb 24, 2020

I'm writing this PR to present a small patch to handle a new behavior of the PHP 7.4 CLI: it is attaching log messages of the built-in PHP server (php -S) to the stderr pipe.

Moreover, I opened an issue to discuss this problem with the guys from internations/http-mock (InterNations#53). At a first glance I thought it wasn't a problem of http mock itself but since this behavior was introduced only in PHP 7.4, they found feasible the idea of adding a patch to get the library working with the new version of PHP.

Since there isn't a builtin CLI option to change this behavior, I'm presenting the same solution here.

Let me know what you think of this approach to tackle this issue.

If you have a better solution in mind, I'll be glad to hear and discuss more about it! 😃

@gabfr gabfr requested a review from a team February 24, 2020 23:33
Copy link

@mtils mtils left a comment

Choose a reason for hiding this comment

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

Looks exactly too what I planned to change in my PR. (And therefor good for me)
I would just change the public static method like mentioned in the comments.

For that, I had to rewrite the test too
Copy link

@mtils mtils left a comment

Choose a reason for hiding this comment

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

For me its perfect now.

Copy link

@rgdevment rgdevment left a comment

Choose a reason for hiding this comment

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

What a great job!
It's great news that this is fixed!
It seems very good, I only have some suggestions that are not of great importance.

Congratulations

@gabfr gabfr requested a review from a team February 25, 2020 18:20
@gabfr gabfr requested a review from rgdevment February 25, 2020 18:27
@gabfr gabfr merged commit ed93802 into LinioPay:master Feb 25, 2020
@pr-triage pr-triage bot added the PR: merged label Feb 25, 2020
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