Skip to content

Conversation

krakjoe
Copy link
Member

@krakjoe krakjoe commented Sep 1, 2025

alternative #18583

fwrite(ZSTR_VAL(buf.s), 1, ZSTR_LEN(buf.s), stdout);
fwrite("\r\n", 1, sizeof("\r\n"), stdout);
} else {
fprintf(stderr,
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not certain this can actually happen.

@krakjoe
Copy link
Member Author

krakjoe commented Sep 1, 2025

This is bit tricky to write tests for because it relies on build configuration, writing a test that passes everywhere would not be a very valuable test ... everything used is extremely stable API ...

If a test is proposed, I'll add it.

@DanielEScherzer
Copy link
Member

This is bit tricky to write tests for because it relies on build configuration, writing a test that passes everywhere would not be a very valuable test ... everything used is extremely stable API ...

If you would like this in PHP 8.5, I'm not comfortable with approving this without any kind of testing. You can try adding a test to run-extra-tests.php that confirms that the output

@krakjoe
Copy link
Member Author

krakjoe commented Sep 1, 2025

It's possible we could add a test that executes the thing, but doesn't actually depend on the output, this makes sure that it doesn't fault ? Beyond that we are just testing that json works anyway ?

@DanielEScherzer
Copy link
Member

DanielEScherzer commented Sep 1, 2025

You can't test part of the JSON output? Making sure it includes some specific values?

@krakjoe
Copy link
Member Author

krakjoe commented Sep 1, 2025

You can't test part of the JSON output? Making sure it includes some specific values?

After a bit of digging, yes I can ... test added ... good job you pushed, because I had made a mistake ;)

@krakjoe krakjoe force-pushed the krakjoe/ini-json branch 2 times, most recently from cf352e0 to 7e0fb97 Compare September 1, 2025 12:38
@krakjoe
Copy link
Member Author

krakjoe commented Sep 2, 2025

@php/release-managers-85 test passing everywhere, ready for review ...

@krakjoe krakjoe requested a review from a team September 2, 2025 04:23
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