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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix php ini parsing from the CLI #63

Merged
merged 6 commits into from Apr 3, 2022
Merged

Fix php ini parsing from the CLI #63

merged 6 commits into from Apr 3, 2022

Conversation

dingo-d
Copy link
Sponsor Contributor

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

The rebasing didn't work as I thought it would on the previous PR so I just cherry-picked the commits against a correct branch.

Original message

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 馃檪

dingo-d and others added 4 commits April 3, 2022 08:24
Added the suggestion by Joe Watkins in sebastianbergmann/phpunit#4664 for parsing settings.
Added test for opcache and for the getCurrentSettings method.
src/Runtime.php Outdated Show resolved Hide resolved
src/Runtime.php Outdated Show resolved Hide resolved
Added strict checks for ini_get value and to check if the matched value is in the parsed config array (strict checking using !== instead of !=).
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