Skip to content

Add ini_parse_quantity to convert ini directive "byte" shorthand to int #8454

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.

Sign up for GitHub

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 8 commits into from
Jul 10, 2022
Merged

Add ini_parse_quantity to convert ini directive "byte" shorthand to int #8454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ac03a9a
Add ini_bytes to translate ini directive byte shorthand to a count
dmsnell Apr 28, 2022
b7e3e5f
Rename parameter to "shorthand"
dmsnell May 4, 2022
55b76e2
Rename function to `ini_parse_quantity( $shorthand )`
dmsnell May 4, 2022
6ee5a87
Attempt to add basic tests for ini_parse_quantity()
dmsnell May 26, 2022
b840f14
Update tests to run successfully
dmsnell Jun 9, 2022
bf36ac1
Use zend_ini_parse_quantity
dmsnell Jun 21, 2022
ceda792
Add tests to test for warnings thrown
dmsnell Jun 21, 2022
618a8e0
Regenerate arginfo
dmsnell Jun 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions ext/standard/basic_functions.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "ext/session/php_session.h"
#include "zend_exceptions.h"
#include "zend_attributes.h"
#include "zend_ini.h"
#include "zend_operators.h"
#include "ext/standard/php_dns.h"
#include "ext/standard/php_uuencode.h"
Expand Down Expand Up @@ -1971,6 +1972,25 @@ PHP_FUNCTION(highlight_string)
}
/* }}} */

/* {{{ Get interpreted size from the ini shorthand syntax */
PHP_FUNCTION(ini_parse_quantity)
{
zend_string *shorthand;
zend_string *errstr;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_STR(shorthand)
ZEND_PARSE_PARAMETERS_END();

RETVAL_LONG(zend_ini_parse_quantity(shorthand, &errstr));

if (errstr) {
zend_error(E_WARNING, "%s", ZSTR_VAL(errstr));
zend_string_release(errstr);
}
}
/* }}} */

/* {{{ Get a configuration option */
PHP_FUNCTION(ini_get)
{
Expand Down
2 changes: 2 additions & 0 deletions ext/standard/basic_functions.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,8 @@ function ini_alter(string $option, string|int|float|bool|null $value): string|fa

function ini_restore(string $option): void {}

function ini_parse_quantity(string $shorthand): int {}

/** @refcount 1 */
function set_include_path(string $include_path): string|false {}

Expand Down
8 changes: 7 additions & 1 deletion ext/standard/basic_functions_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 36 additions & 0 deletions tests/basic/ini_parse_quantity_basic.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
--TEST--
ini_parse_quantity() function - basic test for ini_parse_quantity()
--INI--
error_reporting = E_ALL ^ E_WARNING
--FILE--
<?php
foreach (array(
'-1',
'-0x412',
'0',
'1',
'1b',
'1k',
'1m',
'1g',
'1gb',
'14.2mb',
'14.2bm',
'boat'
) as $input) {
echo ini_parse_quantity( $input ) . PHP_EOL;
}
?>
--EXPECT--
-1
-1042
0
1
1
1024
1048576
1073741824
1
14
14680064
0
Copy link
Member

Choose a reason for hiding this comment

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

This test looks good, but it'll fail. 😄

To run tests locally, use the run-tests.php script in the root of php-src. After you build PHP with your changes (using make), you can run the test like this:

php run-tests.php -p sapi/cli/php tests/basic/ini_parse_quantity_basic.phpt

Where -p sapi/cli/php is pointing to the new PHP executable that you just built that includes your changes.

There are many options you can pass to run-tests.php. You can check them out by entering php run-tests.php -h.

When you have a failing test, run-tests.php will create a bunch of additional files in the same directory alongside your test file, which you can use for debugging.

For example, when tests/basic/ini_parse_quantity_basic.php fails, you'll see the following new files created:

tests/basic/ini_parse_quantity_basic.diff
tests/basic/ini_parse_quantity_basic.expm
tests/basic/ini_parse_quantity_basic.log
tests/basic/ini_parse_quantity_basic.out
tests/basic/ini_parse_quantity_basic.php
tests/basic/ini_parse_quantity_basic.sh

Take a look at them to see what all they contain. *.out will tell you what the text actually printed out, while *.diff will show a diff of what was expected vs. what was actually printed.

For more information, check out the following:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is all great, and I'm quite happily surprised at how easy the testing setup is. Nothing wrong with test that fail before they succeed either 😄

These run locally for me, but I couldn't build them when I rebased against master (I think because of changes introduced in 8.1? Could not build PHP with the following error on make

make: *** No rule to make target `/Users/dmsnell/code/php-src/main/php_stdint.h', needed by `ext/opcache/ZendAccelerator.lo'.  Stop.

I'll try rebasing again in a work day or two as I presume this will be fixed upstream quickly (it's there for me in master anyway).

Maybe I'll hold off on a more exhaustive test suite until the interplay with #7951 is clearer. The zend_test_ini_parse_quantity functions there I think could be renamed as ini_parse_quantity and replace this entire patch 🙃

14 changes: 14 additions & 0 deletions tests/basic/ini_parse_quantity_warnings.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
--TEST--
ini_parse_quantity() function - warns when given inappropriate values
--INI--
error_reporting = E_ALL
--FILE--
<?php
ini_parse_quantity('-1');
ini_parse_quantity('1mb');
ini_parse_quantity('256 then skip a few then g')
?>
--EXPECTF--
Warning: Invalid quantity "1mb": unknown multipler "b", interpreting as "1" for backwards compatibility in %sini_parse_quantity_warnings.php on line 3

Warning: Invalid quantity "256 then skip a few then g", interpreting as "256 g" for backwards compatibility in %sini_parse_quantity_warnings.php on line 4