Tests: Test exception type in error tests

[NickSdot]

Test files sweep to make exception assertions more robust, and slightly unify style.

Specifically:

• adds thrown class to all exception assertions to protected against regressions
• further established the already recommended single exception formatting
• normalises files to end with a single empty line

Target format:

echo $e::class, ': ', $e->getMessage(), PHP_EOL; // + variations like line(), code(), etc.

Approach:
Rewrites are handled by this deterministic helper for conservative and safe, automated replacements. All exception assertion "flavours" from php-src were extracted, and the tests where they were found promoted to source fixtures. Rules were then ran against the fixtures, the results manually reviewed, and promoted to target fixtures. All replacement rules are tested against the source and target fixtures. The actual "fixing" works as follows:

1. Scan PHPT files for exception-message output inside catch blocks.
2. Run the original PHPT through run-tests.php; skipped or already failing tests are not rewritten.
3. Rewrite the PHPT to the normalised exception-output shape.
4. Run the rewritten PHPT through run-tests.php.
5. If output changed as expected, update the expected section from the actual run output.
6. Run the updated PHPT once more.
7. Keep only rewrites that pass verification.

Reviewing

This PR is large, since we probably don't want to review this line by line, some provenance was added. When I change the tool, a workflow in the tool repo can be triggered. The workflow pulls the latest master, then applies the fixers, then pushed to the PR repo. So the easiest review flow is:

1. Review the diff fixtures in tests/Fixtures/**/ran.diff
2. Review the workflow
3. Ensure the workflow Tool commit in the run summary matches the reviewed fixtures commit.
4. Ensure this PR's HEAD matches Generated head in the run summary.

However, for those that want to review line by line as I did a few times, I strongly recommend using https://diffshub.com/php/php-src/pull/22799 to avoid the unusable GitHub diffs.

Modules & Commits

Generated by Reproduce php-src PR.
Tool commit: bfc35a00c83e.

Target	Commit	Files
ext/bcmath	2fdf3d8	49
ext/bz2	74dcb2e	7
ext/calendar	6ce461f	15
ext/curl	9099c91	21
ext/date	cd9bb8b	24
ext/dba	1dfd901	6
ext/dom	6323744	194
ext/exif	560b4aa	4
ext/ffi	45828f0	30
ext/fileinfo	fd2d7ce	12
ext/filter	57616f6	13
ext/ftp	7a3dbbe	13
ext/gd	696f0d2	39
ext/gettext	460ae39	7
ext/gmp	4297442	65
ext/hash	1e0175d	19
ext/iconv	6168edd	3
ext/intl	ce8eefd	65
ext/json	589d7a1	8
ext/ldap	358dffd	9
ext/libxml	cd917f5	5
ext/mbstring	2d6772b	77
ext/mysqli	0116ef8	7
ext/odbc	ef3460a	1
ext/opcache	0827921	114
ext/openssl	334a02c	40
ext/pcntl	0c413df	14
ext/pcre	fc285b9	10
ext/pdo	315ba87	3
ext/pdo_sqlite	0176505	22
ext/pgsql	872fa27	2
ext/phar	3a77a1a	183
ext/posix	3d72cfd	9
ext/random	dd58f4a	21
ext/readline	875a5b5	2
ext/reflection	c9a1096	102
ext/session	8f020ce	18
ext/shmop	ff31198	3
ext/simplexml	84a96da	10
ext/snmp	ab53b74	2
ext/soap	098ae4d	17
ext/sockets	5af07e0	31
ext/sodium	326acbc	10
ext/spl	29e3028	171
ext/sqlite3	3513bb7	20
ext/standard	4da598a	511
ext/sysvmsg	3beb047	3
ext/sysvshm	66ba2a1	8
ext/tidy	130427e	10
ext/tokenizer	f861a9f	5
ext/uri	ab990d2	111
ext/xml	a52fd29	5
ext/xmlreader	65e3ee1	23
ext/xmlwriter	853e767	5
ext/zend_test	7361a10	13
ext/zip	2855305	17
ext/zlib	6ed3679	19
Zend	fe21aad	629
tests	4f9b011	55
sapi/cli	7bd1b03	1

Changelog:

• 2026-07-20: added ext/intl and ext/phar
• 2026-07-21: target format \PHP_EOL -> PHP_EOL

---

PoC, marked as draft for now, and in coordination with Tim for now only applied to ext/random and ext/uri.

[NickSdot]

@LamentXU123 I noticed your like and since you own ext/intl and ext/phar I applied the fixers to it and pushed two new commits. Would love your feedback!

Aside: since I didn't built with all extensions ext/intl serves as a good example for non-provable rewrites. In my run listformatter_with_parameters_error.phpt was skipped.

[thg2k]

Is the \PHP_EOF really necessary? I think it just is silly in the tests. Just PHP_EOF should be fine.

[NickSdot]

Is the \PHP_EOF really necessary? I think it just is silly in the tests. Just PHP_EOF should be fine.

Nah. Picked it out of habit. No strong opinion; can adjust to whatever we decide.

[LamentXU123]

I actually think this change is trivial. I don't have strong opinions on this (I am on the fence of this huge code churn) so might need to wait for other's opinion.

[LamentXU123]

nit: any reasons for targeting 8.5?

[NickSdot]

I actually think this change is trivial. I don't have strong opinions on this (I am on the fence of this huge code churn) so might need to wait for other's opinion.

Sometimes not avoidable to move things forward. :) Personally, I think it adds enough value to be justified. Flipping \PHP_EOL to PHP_EOL would decrease churn by a bit. Do you also think it's silly? If yes could pull the trigger on making it PHP_EOL.

nit: any reasons for targeting 8.5?

Nope. Could be anything. Which one you want?

[LamentXU123]

Nope. Could be anything. Which one you want?

Should be targeting master. (hint: convert this to draft when you are changing the base to avoid accidentally request a review from everyone)

[NickSdot]

Nope. Could be anything. Which one you want?

Should be targeting master. (hint: convert this to draft when you are changing the base to avoid accidentally request a review from everyone)

Given that you mentioned code churn, I am wondering if targeting master is the right call? Like, when someone diffs 8.6 against 8.5 this would all be extra in the diff. Why not 8.5 and then upmerge to master? Madee a full run over all tests, merging it up would have only 37 merge conflicts on a total of 2424 affected files. After the upmerge a separate master targeted run would only affect 83 new files. Moderate, no?

[NickSdot]

Changed to PHP_EOL; 37 files less affected; still targets PHP 8.5.

[TimWolla]

I am wondering if targeting master is the right call?

Yes. This is a refactoring, which is not acceptable in released branches.

---

To reduce churn, can the formatter also keep the type of quotes for the colon in cases where the general format is already as expected? The changes in ext/uri provide no value to improve the strength of the actual assertion. Phrased differently: If the change to the test code does not result in a change in the EXPECT section, then drop the change to the test code.

[NickSdot]

I am wondering if targeting master is the right call?

Yes. This is a refactoring, which is not acceptable in released branches.

Will target master! (surprised that this applies to tests)

To reduce churn, can the formatter also keep the type of quotes for the colon in cases where the general format is already as expected? The changes in ext/uri provide no value to improve the strength of the actual assertion. Phrased differently: If the change to the test code does not result in a change in the EXPECT section, then drop the change to the test code.

I evaluated this before. Not worth the extra complexity and inconsistency, IMO. Why: your modules are already so clean that ext/uri is in fact the clear outlier -- 73/104 files are in your ext/uri, only 31 more files that would be affected by what you propose are in all other tests (as in: all phpt, not only in the four modules committed here). Would not start to make things inconsistent or introduce extra rules for that, personally. However, if changing these 104 files (out of 2,369 total) would be an absolute dealbreaker I of course can make one manual commit to revert them. :)

Though, there are other cases like where output is fixed in try/catch that not actually throw. These are risk free to fix, I did so, and I think we should. Why: contributor comes to an existing test file to add test, copies existing test, style is wrong, you review and flag it. You wasted time, contributor wasted time. Or it's missed and merged. Both avoidable. Having one consistency sweep will decrease this "risk". Even code owners have recent PRs that do not follow the regression-safe pattern. When -- after this here would be merged -- everyone sees everywhere the exact same pattern it will naturally be be better adapted. The same would of course be possible if we would make this part of the CI; but you didn't seem very bullish on that?

Assumed we would do what you say, here some stats: 545 total files less; 243 are one lines changes, 334 out of it are <= 2 line diffs, 411 are <= 5 line diffs.

[NickSdot]

This is targeting master now. I wiped out some unnecessary edits with actual impact; e.g. end of lines are now unchanged. However, the total amount of files increased compared to my earlier comments -- that's because the CLI build is using more extensions now.

Additionally, I added a provenance workflow to the tool repo that automatically pushes to here to make reviews as easy as possible -- it's described in the PR desc.

I consider this done and marking as ready for review. 🫡

[kamil-tekiela]

I am ok with the concept, but I am not going to review the whole thing. So I only looked through mysqli and PDO, and they are fine. We should review the whole thing to avoid accidental typos or worse malware injection via tests.

[NickSdot]

I am ok with the concept, but I am not going to review the whole thing. So I only looked through mysqli and PDO, and they are fine. We should review the whole thing to avoid accidental typos or worse malware injection via tests.

Please check the "Review" section in the PR desc.

[LamentXU123]

the intl part

[LamentXU123]

Here, I doubt if this is a correct change. I think we can preserve the original , to separate error code and error message. At last, I don't see any benefits doing this and this causes quite a lot unnecessary churn in the intl code base.

Of course, this is just an example. There are lots of same changes in the diff.

[NickSdot]

As mentioned in the PR desc, it normalises the formatting to simplify things. There is zero change in functionality, hence this cannot be incorrect. Not worth to add more complexity.

[LamentXU123]

I see. If you are doing this by automatic scripts I won't object. And I actually encourage this because this makes things easier to review.

[NickSdot]

No manual interventions; it's one button click in the tool repo workflows. 🫡 Please see "Review" section in the PR desc.

[LamentXU123]

Okay the intl part is fine for me otherwise. I will take a look at zip and phar soon.

[DanielEScherzer]

Reviewed the ext/reflection tests, but most of the feedback applies to other tests too

Separator

it seems that most of the existing echos use . to separate the components - for the echos that are touched, this PR

• adds the class at the start, then : , both separated with ,
• replaces the existing concatenations with commas

While reasonable people can disagree on which is nicer to use, we shouldn't just mass-change the existing code to use commas instead of periods, and we should be consistent within the echo about using periods or commas

Thus, for existing code like (using ext/reflection/tests/002.phpt as an example since I'm the maintainer of that extension)

    echo $e->getMessage() . "\n";

should become

     echo $e::class . ': ' . $e->getMessage() . "\n";

rather than

    echo $e::class, ': ', $e->getMessage(), "\n";

if the echo already used commas, more commas are fine, but if it previously used periods, lets keep that

Quotes

there are also other changes being made to the existing strings that pollute the diff, e.g. ext/reflection/tests/ReflectionClass_CannotClone_basic.phpt switches

  echo $e::class, ": ", $e->getMessage(), PHP_EOL;

to

  echo $e::class, ': ', $e->getMessage(), PHP_EOL;

(replacing double quotes with single) - while double quotes aren't needed here, this change doesn't really improve anything

get_class

Some tests already report the type of the exception with get_class, those probably don't need to change. E.g. ext/reflection/tests/ReflectionConstant_inexistent.phpt replaced

    echo get_class($e) . ': ' . $e->getMessage() . "\n";

with

    echo $e::class, ': ', $e->getMessage(), "\n";

with no change in expectation - so why change anything at all? If the expectation section isn't changing, we can just skip that test

Dropping existing context

In ext/reflection/tests/ReflectionExtension_constructor_error.phpt the previous exception handling was

    echo "Ok - ".$re->getMessage().PHP_EOL;

to make it clear that the exceptions were expected; this was replaced with

    echo $re::class, ': ', $re->getMessage(), PHP_EOL;

which dropped the "Ok - " at the start

see also ext/reflection/tests/ReflectionProperty_getMangledName_dynamic.phpt

    echo "Expected exception for class-based reflection: " . $e->getMessage() . "\n";

became

    echo 'class-based reflection: ', $e::class, ': ', $e->getMessage(), "\n";

Unrelated changes

ext/reflection/tests/ReflectionProperty_constructor_variation1.phpt switched

echo "\n\n--> Reflect inherited private from declaring scope:\n";

to

echo "\n--> Reflect inherited private from declaring scope:\n";

why? Presumably because the change also added a newline to the previous echo that did not end with a newline, but can we just limit this PR to adding the exception class, and not mess with the other stuff? I.e. in that file

    echo $e->getMessage();

should become

    echo $e::class, ': ', $e->getMessage();

rather than

    echo $e::class, ': ', $e->getMessage(), "\n";

There is also ext/reflection/tests/ReflectionProperty_lazy_initialization_errors.phpt where there were newlines removed rather than just moved around, and here the newlines were actually useful to separate out the different cases of properties that cannot be subject to lazy initialization

Other

No objection to switching from var_dump($e->getMessage()) to using echo, that makes sense when you want to print the exception class on the same line

[NickSdot]

Thanks for the detailed review, Daniel!

All things you mentioned have no functional difference to now.

• agreed on the consistency argument; this is exactly what this PR achieves.
• "context" was removed only when it added no value. As in, targeted with a literal trash policy, for example "Throws" or "OK" is dropped (of course it's okay or throws when it is in "--EXPECT--"!), while an expressive string like " Using object argument: " is kept. That's why your "class-based reflection:" is how it is: bloat is dropped, value is kept. Please see fixtures, each exact case is in there.
• line endings were normalised, the result is functional identical. We have roughly 200 lines less, nice. Whether empty lines as in the lazy example are helpful is a matter of taste; personally I disagree and intentionally removed it.
• period or comma: as mentioned in earlier comments: to standardise is a good thing. In an earlier comment I did the math (now a bit off, because I did a wider run after the comment) and what you or Tim ask for will not significantly reduce edited lines scope. There is no justification for more complexity.

Side context:
my actual plan was to propose to make this eventually part of CI; to keep things a little bit organised (we could later add linters to catch missed cleanups, e.g. #22911, #22912 -- cause flakyness, and what not) and to make reviews easier by not allowing things to slip in the first place. That's why the tool does a bit more than the PR title (changed to what it is not by me!) indicates (but it's mentioned in the desc). However, this will not happen tomorrow or maybe never. So for now it is an one-time cleanup.

Thats said, and coming back to your consistency intro... It will be hard to match everyones personal preferences (as with every linter, fixer and coding standard) -- and there are surely a lot of things we could decide in one or another way. However, my PR takes this decisions, adds value in multiple ways, and makes things actually consistent. I have spent days to get this right, am very happy with the result, and can stand behind it. I kindly ask for your understanding that I don't plan to change things to be less consistent and less good than now.

I hope my train of thought is clear, and that the PR adds enough value to be merge-worthy as is. 🫡

[DanielEScherzer]

Thanks for the detailed review, Daniel!

All things you mentioned have no functional difference to now.

• agreed on the consistency argument; this is exactly what this PR achieves.
• "context" was removed only when it added no value. As in, targeted with a literal trash policy, for example "Throws" or "OK" is dropped (of course it's okay or throws when it is in "--EXPECT--"!), while an expressive string like " Using object argument: " is kept. That's why your "class-based reflection:" is how it is: bloat is dropped, value is kept. Please see fixtures, each exact case is in there.

I saw that trash policy, and I disagree with it

• line endings were normalised, the result is functional identical. We have roughly 200 lines less, nice. Whether empty lines as in the lazy example are helpful is a matter of taste; personally I disagree and intentionally removed it.

If the goal of this PR is to add exception type to error tests, that is one thing. If the goal is to do that and enforce your taste, that is another. It is indeed a matter of taste - the taste of the author who originally added it, and of the current extension maintainer (me) is that the empty lines are helpful

• period or comma: as mentioned in earlier comments: to standardise is a good thing. In an earlier comment I did the math (now a bit off, because I did a wider run after the comment) and what you or Tim ask for will not significantly reduce edited lines scope. There is no justification for more complexity.

The fact that you are using a tool to make a PR does not serve as justification for problems in the PR content - if you don't want to make the tool more complex, you can just not use a tool, or perform manual fixes afterwards

Side context: my actual plan was to propose to make this eventually part of CI; to keep things a little bit organised (we could later add linters to catch missed cleanups, e.g. #22911, #22912 -- cause flakyness, and what not) and to make reviews easier by not allowing things to slip in the first place. That's why the tool does a bit more than the PR title indicates (but it's mentioned in the desc). However, this will not happen tomorrow or maybe never. So for now it is an one-time cleanup.

Thats said, and coming back to your consistency intro... It will be hard to match everyones personal preferences (as with every linter, fixer and coding standard) -- and there are surely a lot of things we could decide in one or another way. However, my PR takes this decisions, adds value in multiple ways, and makes things actually consistent. I have spent days to get this right, am very happy with the result, and can stand behind it. I kindly ask for your understanding that I don't plan to change things to be less consistent and less good than now.

Do you have stats on if commas or periods are currently used more in the tests? You are making things "actually consistent" by enforcing the your preference - if the existing tests primarily use periods, and you want to have things be consistent, then the consistent result should match what is currently used most

I hope my train of thought is clear, and that the PR adds enough value to be merge-worthy as is. 🫡

[LamentXU123]

I agree with Daniel. Only sticking to one goal in one PR is preferable. Although I don't have strong objections in other changes (commas, empty lines, etc. mostly because the intl extension doesn't churn so hard in this PR)

A dedicated PR to only add exception classes tests will be easier to be accepted, reviewed and merged, in my very honest opinion.

[NickSdot]

No. It's not my preference. It's the style Tims PR review comments used (and were merged respectively) for well over a year. I really don't mean that rude, but I am not so excited to make this a personal-preference discussion. Not sure what else I can add, but as I mentioned above I am happy with the PR and I stand behind it as is (not yet as a maintainer, but as the author of the PR who did the work). If the PR is not welcome I can close it without hard feelings.

[DanielEScherzer]

No. It's not my preference. It's the style Tims PR review comments used (and were merged respectively) for well over a year.

Assuming TimWolla ?

I really don't mean that rude, but I am not so excited to make this a personal-preference discussion.

I didn't want it to be a personal-preference discussion either, but the alternative to picking one of comma or period (the personal-preference decision) is to be consistent with what was already there (which I had previously suggested and you shot down)

Not sure what else I can add, but as I mentioned above I am happy with the PR and I stand behind it as is (not yet as a maintainer, but as the author of the PR who did the work). If the PR is not welcome I can close it without hard feelings.

There is a difference between standing behind a PR and being unwilling to incorporate feedback - the PR is welcome, and if you decide you don't want to pursue it I'll probably fork your generating code when I have the time, but it would be a shame to lose the hard work you have put in

[NickSdot]

Assuming TimWolla ?

Yes.

I'll probably fork your generating code when I have the time

Please feel free.

unwilling to incorporate feedback

There is no strong reason to do what you ask for. I will not put my name on something that isn't to my standards or to what I do not agree to. And yes, that implicates that I cannot always incorporate feedback.

but it would be a shame to lose the hard work you have put in

Nah, that's actually fine. If you take it over it's not lost. :) My goal is to make PHP better, not to push my ego with a merged PR.

[NickSdot]

I agree with Daniel. Only sticking to one goal in one PR is preferable. Although I don't have strong objections in other changes (commas, empty lines, etc. mostly because the intl extension doesn't churn so hard in this PR)
A dedicated PR to only add exception classes tests will be easier to be accepted, reviewed and merged, in my very honest opinion.

There is no virtually no avoidable churn. I double checked the reflection extension. Only three (3!) lines out of 558 lines are style only changes. Every other line would have been edited either way. Hence, it makes sense to unify them in the same time. You yourself earlier said that it is a good thing and that you "encourage" it because it's easier to review.

[LamentXU123]

You yourself earlier said that it is a good thing and that you "encourage" it because it's easier to review.

I encourage you to use scripts in your changes so it is more transparent in what you've changed and easier for us to review, which doesn't conflicts to the point I think it is better to do them separately. But anyways, I am not interested in defending myself.

The zip and phar changes are fine, code wise. cc @iluuu1994 and @TimWolla as they might have opinions. I am still on the fence of this.

[NickSdot]

is to be consistent with what was already there

Turns out comma leads by a "huge" margin. So, comma it is.

CLion

Command

cd php-src && {
    printf 'comma: '
    rg -UP --count-matches --glob '*.phpt' '\becho\s+(?=[^;]*(?:->getMessage\s*\(\)|::class|get_class\s*\([^)]*\)))[^;]*,[^;]*;' . | awk -F: '{s += $2} END {print s + 0}'

    printf 'dot-concat: '
    rg -UP --count-matches --glob '*.phpt' '\becho\s+(?=[^;]*(?:->getMessage\s*\(\)|::class|get_class\s*\([^)]*\)))[^;]*\.[^;]*;' . | awk -F: '{s += $2} END {print s + 0}'

    printf 'mixed: '
    rg -UP --count-matches --glob '*.phpt' '\becho\s+(?=[^;]*,)(?=[^;]*\.)(?=[^;]*(?:->getMessage\s*\(\)|::class|get_class\s*\([^)]*\)))[^;]*;' . | awk -F: '{s += $2} END {print s + 0}'
  }

comma: 3735
dot-concat: 3110
mixed: 54

[alexdowad]

I looked at the mbstring stuff and it looks OK. Personally, I would loved to see this huge PR submitted piecemeal as one PR per extension, which the extension maintainer could review and merge.