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

Fix php ini parsing from the CLI #62

Closed
wants to merge 49 commits into from
Closed

Fix php ini parsing from the CLI #62

wants to merge 49 commits into from

Conversation

dingo-d
Copy link
Sponsor Contributor

@dingo-d dingo-d commented Apr 2, 2022

This is a (possible) fix for the PHPUnit issue sebastianbergmann/phpunit#4664

I've added the suggestion by @krakjoe (link) and some tests to cover this part of the code.

I've run the tests on the PHPUnit locally on the latest branch to check if anything broke, and the existing tests passed. I guess this should be tested a bit thoroughly.

When I ran the tests on the PHPUnit repo and put the breakpoint to inspect the result of merging the settings with getCurrentSettings method in the AbstractPhpProcess.php file inside the getCommand method I could see my xdebug settings correctly parsed, so I think there should be no breaks.

I had to add the updated code manually inside the vendor file to test this because I got a composer error when trying to use my branch instead of the v6 for the php-code-coverage package.

 Problem 1
    - phpunit/php-code-coverage dev-master requires sebastian/environment ^6.0 -> found sebastian/environment[dev-master, 6.0.x-dev (alias of dev-master)] but it conflicts with your root composer.json require (dev-fix-php-ini-parsing).
    - phpunit/php-code-coverage 10.0.x-dev is an alias of phpunit/php-code-coverage dev-master and thus requires it to be installed too.
    - Root composer.json requires phpunit/php-code-coverage ^10.0 -> satisfiable by phpunit/php-code-coverage[10.0.x-dev (alias of dev-master)].

This is the image of the breakpoint and part of the $settings array:

image

I'd love to have somebody test it a bit more just to be sure.

Oh and I'd love to add a co-authored commit for @krakjoe but I'm not sure what the no-reply email is 🙈 I can update the commit message to add it once I find out the correct email 🙂

@sebastianbergmann
Copy link
Owner

Can you please send this against the 5.1 branch? Thanks!

@dingo-d dingo-d changed the base branch from master to 5.1 April 3, 2022 05:54
@dingo-d
Copy link
Sponsor Contributor Author

dingo-d commented Apr 3, 2022

I'll rebase and fix the conflicts when I get to the computer

@dingo-d
Copy link
Sponsor Contributor Author

dingo-d commented Apr 3, 2022

I messed something up with rebasing (probably because I've never rebased against a tag 😬). I'll open up a new PR

@dingo-d dingo-d closed this Apr 3, 2022
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

2 participants