Skip to content

[PHP 8.2] Add ini_parse_quantity function #411

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

Closed
wants to merge 1 commit into from
Closed

[PHP 8.2] Add ini_parse_quantity function #411

wants to merge 1 commit into from

Conversation

Ayesh
Copy link
Contributor

@Ayesh Ayesh commented Jul 12, 2022

Polyfill taken from PHP.Watch - PHP 8.2: New ini_parse_quantity function

Tries to mimic the native warnings with E_USER_WARNING errors, and overflow/underflow conditions by inspecting the int to float type change.

Polyfill taken from [PHP.Watch - PHP 8.2: New `ini_parse_quantity` function](https://php.watch/versions/8.2/ini_parse_quantity#polyfill)

Tries to mimic the native warnings with `E_USER_WARNING` errors, and overflow/underflow conditions by inspecting the int to float type change.
@Ayesh
Copy link
Contributor Author

Ayesh commented Jul 12, 2022

Tried adding the following tests, but they don't seem to work for some reason:

    /**
     * @covers \Symfony\Polyfill\Php82\Php82::ini_parse_quantity
     */
    public function testIniParseQuantityWarningsInvalidCharactersIgnored()
    {
        $this->expectWarning();
        $this->expectWarningMessageMatches('/Invalid quantity "5em", interpreting as "5m" for backwards compatibility/');
        ini_parse_quantity('5em');
    }

    /**
     * @covers \Symfony\Polyfill\Php82\Php82::ini_parse_quantity
     */
    public function testIniParseQuantityWarningsNoDigits()
    {
        $this->expectWarning();
        $this->expectWarningMessageMatches('/Invalid quantity "NOVALUE": no valid leading digits, interpreting as "0" for backwards compatibility/');
        ini_parse_quantity('NOVALUE');
    }

    /**
     * @covers \Symfony\Polyfill\Php82\Php82::ini_parse_quantity
     */
    public function testIniParseQuantityUnknownMultiplier()
    {
        $this->expectWarning();
        $this->expectWarningMessageMatches('/Invalid quantity "123R": unknown multiplier "R", interpreting as "123" for backwards compatibility/');
        ini_parse_quantity('123R');
    }

@Ayesh Ayesh marked this pull request as ready for review July 12, 2022 23:58
// Removing whitespace characters.
$shorthand = preg_replace('/\s/', '', $shorthand);

$suffix = strtoupper(substr($shorthand, -1));
Copy link
Contributor

Choose a reason for hiding this comment

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

case 'K':case 'k': better than strtoupper()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hesitant about it because it would require adding duplicate case lines for all three current suffixes.

}

// If there is no suffix, return the integer value with the sign.
if (preg_match('/^\d+$/', $shorthand, $matches)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

$value = (float) $shorthand;
if ($value == $shorthand) {
   return $value;
}
$suffix = substr($shorthand, -1);

@WinterSilence
Copy link
Contributor

Tried adding the following tests, but they don't seem to work for some reason

you can try add anchors /^...$/i or use expectWarningMessage()

case 'M':
$multiplier *= 1024 * 1024;
break;
case 'G':
Copy link
Contributor

@IonBazan IonBazan Nov 5, 2022

Choose a reason for hiding this comment

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

This function is missing support for setting the base (using o, x or b characters). See: https://github.com/php/php-src/blob/master/Zend/zend_ini.c#L550

Please add them also in the test cases. Also, what about edge cases, like combining all multipliers at once, like 123KMG? You can look for inspiration at https://github.com/php/php-src/tree/master/Zend/tests/zend_ini

Copy link
Contributor

@IonBazan IonBazan Nov 24, 2022

Choose a reason for hiding this comment

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

@Ayesh I've tried to implement above on my fork - please have a look and maybe you can get some inspiration from there: https://github.com/IonBazan/polyfill/tree/feature/parse_ini_quantity

It's almost working - only the error message seems to be slightly different than on PHP 8.2.

@fisharebest
Copy link
Contributor

only the error message seems to be slightly different than on PHP 8.2.

If it wasn't for the error-messages, this would be a trivial function to polyfill :-)

Here is a simple, brute-force test script I wrote while trying to create my own implementation.

<?php
require 'src/Php82/Php82.php';
use Symfony\Polyfill\Php82\Php82;

function do_test($test) {
    error_clear_last();
    $x = @ini_parse_quantity($test);
    $err_x = error_get_last()['message'] ?? '';

    error_clear_last();
    $y = @Php82::ini_parse_quantity($test);
    $err_y = error_get_last()['message'] ?? '';

    if ($x !== $y || $err_x !== $err_y) {
        echo 'FAIL: "', $test, '"', PHP_EOL, $x, PHP_EOL, $y, PHP_EOL, $err_x, PHP_EOL, $err_y, PHP_EOL;
        exit;
    }
}

$chars = [' ', '-', '+', '0', '1', '7', '9', 'a', 'b', 'o', 'f', 'g', 'k', 'm', 'x', 'z'];

do_test('');
foreach ($chars as $char1) {
    do_test($char1);
    foreach ($chars as $char2) {
        do_test($char1 . $char2);
        foreach ($chars as $char3) {
            do_test($char1 . $char2 . $char3);
            foreach ($chars as $char4) {
                do_test($char1 . $char2 . $char3 . $char4);
                foreach ($chars as $char5) {
                    do_test($char1 . $char2 . $char3 . $char4 . $char5);
                    foreach ($chars as $char6) {
                        do_test($char1 . $char2 . $char3 . $char4 . $char5 . $char6);
                    }
                }
            }
        }
    }
}

@King2500
Copy link

What's the status on this?

@nicolas-grekas
Copy link
Member

The C implementation is here:
https://github.com/php/php-src/blob/master/Zend/zend_ini.c#L590

Would anyone be interested in porting this to PHP?
I suppose @Ayesh is not available to finish this PR.

@nicolas-grekas
Copy link
Member

I'm closing to signal that someone else should take over. PR welcome.

@fisharebest
Copy link
Contributor

Would anyone be interested in porting this to PHP?

I have done this and have a working solution. (I need it for one of my own projects.)

Using the brute-force test technique above, it gives the same output and errors as the core function for all strings up to 7 characters.

  • The tests only run on PHP82, as they require the core function as a reference.
  • The code follows the C source - including gotos and pointer-based string access.

Would you want me to create a PR now (with this rather hideous code) or wait until I have time to clean it up (which might not be soon)?

@fisharebest
Copy link
Contributor

I've submitted PR #439

nicolas-grekas added a commit that referenced this pull request Aug 25, 2023
…in PHP (fisharebest)

This PR was squashed before being merged into the 1.x branch.

Discussion
----------

Add polyfill for ini_parse_quantity based on the C code in PHP

See #411 for a previous attempt to polyfill this function.

The `ini_parse_quantity()` is difficult to polyfill as the error handling is non-trivial, and several invalid inputs are treated as valid.

This polyfill is a simple/direct implementation of the C code from PHP.
As such it uses `goto`s and pointer-based string access.
I am fully aware that this would fail most code reviews :-)

However it works and I hope it will be useful until a cleaner implementation can be provided.

The unit tests provide 100% code coverage.  I have also included (but commented out) a brute-force test which takes many hours to run. I found it was very helpful in finding edge cases.  It may be useful for anyone who tries to refactor the code.

The two overflow tests will probably fail on a 32bit build of PHP, however the code itself should work.
I just haven't been able to work out the expected values.

Commits
-------

d07580f Add polyfill for ini_parse_quantity based on the C code in PHP
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.

6 participants