Skip to content

ext/standard: applied fixers to improve test robustness - #23136

Merged
Girgias merged 4 commits into
php:masterfrom
NickSdot:test/hardening/ext-standard
Sep 20, 2026
Merged

Girgias merged 4 commits into
php:masterfrom
NickSdot:test/hardening/ext-standard

Conversation

@NickSdot

@NickSdot NickSdot commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Asserts error class; extracted from: #22799

GitHub is lagging this, easier to review with: https://diffshub.com/php/php-src/pull/23136

@NickSdot
NickSdot marked this pull request as ready for review August 8, 2026 08:19
@NickSdot
NickSdot requested a review from bukka as a code owner August 8, 2026 08:19
@NickSdot

Copy link
Copy Markdown
Contributor Author

@bukka following your comment yesterday I reviewed this one once again, and wanted to double check.

  1. Out of 4,210 total lines, 13 (in 4 files) are affected by the same style-only thematic. Are you here okay with unifying those 13 lines with the rest or do you want me to make a manual follow up?
  2. And while we are on it... Currently line endings are untouched; some said output always should be \n, others don't wanted them to be touched. Would you prefer to convert to \n or nah?

I would make a manual follow up to get these addressed.

Files affected by the style-only thematic:
ext/standard/tests/array/gh22678.phpt
ext/standard/tests/filters/gh20370.phpt
ext/standard/tests/strings/gh20906_1.phpt
ext/standard/tests/strings/setlocale_null_byte.phpt

@NickSdot
NickSdot force-pushed the test/hardening/ext-standard branch 4 times, most recently from ebced5a to f9206bf Compare August 21, 2026 18:25

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are loads of ZPP tests that should be removed, would make the diff smaller and reduce the test suite aswell.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a ZPP test and should be removed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto ZPP test

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Diito

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto

@NickSdot

Copy link
Copy Markdown
Contributor Author

There are loads of ZPP tests that should be removed, would make the diff smaller and reduce the test suite aswell.

@Girgias that's done in #23413 -- when merged I will rebase and resolve here.

@NickSdot
NickSdot force-pushed the test/hardening/ext-standard branch from f9206bf to 713ec9a Compare September 16, 2026 13:40
@NickSdot

Copy link
Copy Markdown
Contributor Author

Rebased.

@NickSdot
NickSdot requested a review from Girgias September 16, 2026 13:47

@Girgias Girgias left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So there are still a few ZPP tests, but everything looks good to me. (I did ignore most of the printf variation tests as those are kinda broken already)

[2]=>
string(6) "Line 3"
}
file(): Argument #2 ($flags) must be a valid flag value

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Note for future, it might be a good idea to improve the error message.

Comment on lines -64 to -72
flock(): Argument #2 ($operation) must be of type int, array given
TypeError: flock(): Argument #2 ($operation) must be of type int, array given
--- Iteration 4 ---
flock(): Argument #2 ($operation) must be of type int, array given
TypeError: flock(): Argument #2 ($operation) must be of type int, array given
--- Iteration 5 ---
flock(): Argument #2 ($operation) must be of type int, string given
TypeError: flock(): Argument #2 ($operation) must be of type int, string given
--- Iteration 6 ---
flock(): Argument #2 ($operation) must be of type int, string given
TypeError: flock(): Argument #2 ($operation) must be of type int, string given
--- Iteration 7 ---
flock(): Argument #2 ($operation) must be of type int, string given

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems like we have some ZPP tests here (keep the stream resource one as that's not fully ZPP)

tempnam(): Argument #2 ($prefix) must not contain any null bytes
ValueError: tempnam(): Argument #2 ($prefix) must not contain any null bytes
-- Iteration 6 --
tempnam(): Argument #2 ($prefix) must be of type string, array given

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ZPP test

tempnam(): Argument #1 ($directory) must not contain any null bytes
ValueError: tempnam(): Argument #1 ($directory) must not contain any null bytes
-- Iteration 6 --
tempnam(): Argument #1 ($directory) must be of type string, array given

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ZPP test

Comment on lines 75 to -83
Warning: Undefined variable $obj in %s on line %d
register_shutdown_function(): Argument #1 ($callback) must be a valid callback, first array member is not a valid class name or object
TypeError: register_shutdown_function(): Argument #1 ($callback) must be a valid callback, first array member is not a valid class name or object

Warning: Undefined variable $obj in %s on line %d
register_shutdown_function(): Argument #1 ($callback) must be a valid callback, first array member is not a valid class name or object
register_shutdown_function(): Argument #1 ($callback) must be a valid callback, first array member is not a valid class name or object
register_shutdown_function(): Argument #1 ($callback) must be a valid callback, class bar does not have a method "foo"
register_shutdown_function(): Argument #1 ($callback) must be a valid callback, first array member is not a valid class name or object
register_shutdown_function(): Argument #1 ($callback) must be a valid callback, function "bar" not found or invalid function name

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A lot of these are ZPP tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we really remove bug tests?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmmmm not usually.

@@ -125,5 +125,5 @@ string(10) "helloworld"
-- Iteration 20 --
string(10) "helloworld"
-- Iteration 21 --
strip_tags(): Argument #2 ($allowed_tags) must be of type array|string|null, resource given

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ZPP test and seems the try catch block is only for this case?

Comment on lines -85 to -89
stristr(): Argument #2 ($needle) must be of type string, array given
TypeError: stristr(): Argument #2 ($needle) must be of type string, array given
-- Iteration 9 --
stristr(): Argument #2 ($needle) must be of type string, array given
TypeError: stristr(): Argument #2 ($needle) must be of type string, array given
-- Iteration 10 --
stristr(): Argument #2 ($needle) must be of type string, array given

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ZPP tetst

Position of 'Hello' with offset '0' is => int(0)
Position of 'Hello' with offset '1' is => int(74)
Position of 'Hello' with offset '' is => int(0)
Position of 'Hello' with offset 'string12' is =>
strpos(): Argument #3 ($offset) must be of type int, string given

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ZPP tests

@@ -89,11 +89,11 @@ string(6) "m1tatm"
-- Iteration 6 --
string(6) "tm0atm"
-- Iteration 7 --
strtr(): Argument #2 ($from) must be of type string, array given

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ZPP tests

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Loads of ZPP tests

@Girgias
Girgias merged commit f978c5a into php:master Sep 20, 2026
18 checks passed
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.

2 participants