Skip to content

Applied fixers to improve test robustness - #23018

Merged
Girgias merged 23 commits into
php:masterfrom
NickSdot:test/hardening/no-codeowner-extensions
Aug 4, 2026
Merged

Applied fixers to improve test robustness#23018
Girgias merged 23 commits into
php:masterfrom
NickSdot:test/hardening/no-codeowner-extensions

Conversation

@NickSdot

@NickSdot NickSdot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Extensions with no code owners.

@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.

Mostly good, but I spotted some bad error messages, if you could fix those before?

Comment thread ext/readline/tests/readline_callback_handler_install_001.phpt
Comment on lines 1 to 6
--TEST--
readline_completion_function(): Basic test
--EXTENSIONS--
readline
--FILE--
<?php

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 whole test seems to be extremely useless.

Comment thread ext/filter/tests/017.phpt
string(4) "data"
bool(false)
bool(false)
filter_var(): "regexp" option is missing

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 feels like we should be able to tell what argument is the problem.

Comment thread ext/filter/tests/029.phpt
Comment on lines -80 to -82
filter_var(): Option must be a valid callback
filter_var(): Option must be a valid callback
filter_var(): Option must be a valid callback

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 arg num would be helpful with the option name

Comment thread ext/filter/tests/039.phpt
filter_var_array(): Argument #2 ($options) cannot contain empty keys
ValueError: filter_var_array(): Argument #2 ($options) cannot contain empty keys

Warning: filter_var_array(): Unknown filter with ID 0 in %s on line %d

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.

Surprised this is a warning, probably should be elevated.

Warning: preg_replace_callback_array(): No ending delimiter '/' found in %spreg_replace_callback_array2.php on line %d
NULL
string(1) "1"
Exception: 1

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.

What, this error message is clearly broken in some way.

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.

The output seems correct. But also not quite sure what's actually the goal. Was introduced here 784c60a#diff-3e9a96d15eda69304d23fedb498daa371541c29aa9bff6dd4d170c30241a295dR22 and has a security audit label from much later. Not sure.

Comment on lines +46 to +51
SodiumException: a PHP string is required
string(3) "bbc"
string(3) "abc"
string(3) "bbc"
string(3) "abc"
PHP strings are required
SodiumException: PHP strings are required

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.

These should probably be converted to normal TypeErrors

Comment on lines 38 to 42
Exception: test exception

Exception: test exception

Exception: test exception

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.

Why are there empty new lines all of a sudden?

Comment thread ext/zlib/tests/deflate_add_error.phpt
Comment thread ext/zlib/tests/inflate_add_error.phpt
@NickSdot
NickSdot marked this pull request as ready for review August 4, 2026 12:09
@Girgias
Girgias merged commit 1da3a63 into php:master Aug 4, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment