Skip to content

Close-GH 15685: improve proc_open error reporting on Windows - #15687

Closed
cmb69 wants to merge 3 commits into
php:masterfrom
cmb69:cmb/gh15685
Closed

cmb69 wants to merge 3 commits into
php:masterfrom
cmb69:cmb/gh15685

Conversation

@cmb69

@cmb69 cmb69 commented Sep 1, 2024

Copy link
Copy Markdown
Member

While similar errors are already reported via strerror() on other platforms, this has apparently overlooked for Windows, where only the error code has been reported so far.

While similar errors are already reported via `strerror()` on other
platforms, this has apparently overlooked for Windows, where only the
error code has been reported so far.
@cmb69

cmb69 commented Sep 1, 2024

Copy link
Copy Markdown
Member Author

These error messages are localized on Windows, so there is the general question what to do about that in tests:

  1. use a placeholder (%s) for the error message
  2. assume en_US, and let the tests fail for other locales (e.g. ext/odbc/tests/bug73448.phpt)
  3. skip the test if the locale is not en_US (e.g. ext/com_dotnet/tests/variants_x64.phpt)

I prefer (1), but see valid arguments for (3); (2) is the best for controlled environments (like CI), but I consider it generally doubtful.

@cmb69 cmb69 linked an issue Sep 1, 2024 that may be closed by this pull request
We go with option (2), fow now.
@cmb69
cmb69 marked this pull request as ready for review September 1, 2024 11:34
@cmb69
cmb69 requested a review from bukka as a code owner September 1, 2024 11:34

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

This looks much better indeed.

@bukka

bukka commented Sep 2, 2024

Copy link
Copy Markdown
Member

In terms of tests, I would probably go for 3.

Unfortunately, there is no PHP userland function which allows us to
get the current system locale, so we work around.
@cmb69 cmb69 closed this in 3892529 Sep 3, 2024
@cmb69

cmb69 commented Sep 3, 2024

Copy link
Copy Markdown
Member Author

In terms of tests, I would probably go for 3.

Did this.

@cmb69
cmb69 deleted the cmb/gh15685 branch September 3, 2024 11:13
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.

improve proc_open error reporting on Windows

2 participants