-
Notifications
You must be signed in to change notification settings - Fork 7.8k
Fix minimal Windows version #15975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix minimal Windows version #15975
Conversation
As of PHP 8.3.0, Windows 8/Server 2012 are the minimum requirement. However, PR php#9104 only updated `_WIN32_WINNT`, but not `WINVER`[1], `NTDDI_VERSION`[2] nor the manifest[3]. [1] <https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers> [2] <https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers> [3] <https://learn.microsoft.com/en-us/windows/win32/sbscs/application-manifests>
Only now noticed the "ABI break" label, but I think there is no ABI break for practical purposes. Shall I revert the commit from PHP-8.3 nonetheless? |
I don't know, it doesn't run on Windows 7 anyway, but who knows who uses |
I wonder what happens exactly (without this patch). Does it try to start up, and then reports a missing function? Or would it crash? The change to the manifest should prevent starting up in the first place.
I think |
@bukka, would you consider this change to be a problem for PHP-8.3? |
It does not complete startup, once you launch the EXE you get a "missing procedure" error from Windows. |
@cmb69 I will trust your judgement on this. I don't mind to leave it in 8.3 if you think it's safe. |
I'm lacking the imagination for what these constants could be explicitly used. I mean, clearly so far somebody could have build an extension which works on Windows 7, but when trying to start PHP to use it, it would fail to start anyway. Of course, someone might use I'd say this change is not guilty, until proven otherwise. |
As of PHP 8.3.0, Windows 8/Server 2012 are the minimum requirement. However, PR #9104 only updated
_WIN32_WINNT
, but notWINVER
[1],NTDDI_VERSION
[2] nor the manifest[3].[1] https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers
[2] https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers
[3] https://learn.microsoft.com/en-us/windows/win32/sbscs/application-manifests