-
-
Notifications
You must be signed in to change notification settings - Fork 303
Add PHP 8.5 initial support #819
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
Conversation
Hahaha, I have been working on this, too. |
We should also put a watch on php/php-src#18660 because it might land in 8.5. |
# Conflicts: # src/globals/test-extensions.php
It was merged, opcache is always statically compiled into php 8.5. |
Looks like we have to wait alpha3 or test master branch directly. |
Looks like we don't need to patch opcache for PHP 8.5 at all. We only need to add version check and just ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change
I find a few related issues like here: https://stackoverflow.com/questions/14078182/openssl-file-get-contents-failed-to-enable-crypto Question is why that only happens when MUSL is defined |
Interesting. When I use So the conclusion for now:
Currently we could know that the ssl bug is not related to |
I will upstream a fix for that to php-src. There are better ways to detect musl without breaking cross-compilation. |
That's great! btw how do you plan to detect musl from cross toolchain? And please review all the changes again. I will merge this branch soon and then introduce some logger refactoring based on it. |
Musl doesn't define the _USE_GNU macro in features.h, so we can write a feature test #ifndef _GNU_SOURCE
#define _GNU_SOURCE
#endif
#include <sys/features.h>
#ifdef _USE_GNU
#error Not using musl
#endif and try to compile that. Won't break cross-compilation. |
Co-authored-by: Marc <[email protected]>
Co-authored-by: Marc <[email protected]>
Co-authored-by: Marc <[email protected]>
} else { | ||
$args .= ' --without-openssl-argon2'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not needed, I mean that we must disable the fake password-argon2 extension in case openssl-argon2 is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah got it. But --with-password-argon2
is different from --with-openssl-argon2
. Considering that Herd and some laravel projects use it, and it has not been deprecated yet, we should treat them separately. We cannot remove it until password-argon2 is deprecated in PHP standard extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with-openssl-argon2 implements the functionality of with-password-argon2. When both are defined, I'm not sure which backend will be used.
Essentially we need to have this check in both extensions and use the openssl one if possible, but fall back to the coding contest implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to config.m4 and function source, --with-password-argon2
is defined to use the standalone libargon2, while --with-openssl-argon2
is using openssl. And these are used from different function: password_hash
and openssl_password_hash
. They shouldn't conflict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
password_hash
exists either way. When no implementation is chosen, it falls back to libsodium (no threads option). Argon2 and openssl can change the bachend implementation, but I'm not sure which backend will be chosen in case both are enabled. Remi explicitly enables --with-openssl-argon2
and --without-password-argon2
.
We want it to use the openssl backend because it's the most well-maintained implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like if we enabled libargon2, it will use it first. Otherwise it will search from zend bucket.
Anyway, I don't think we have to adjust pwhash function and dependency here. But if we could confirm and validate that openssl is able to cover it, we could make password-argon2
to depends on openssl
instead of libargon2 later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, only from php 8.4 for NTS and 8.5 for ZTS sadly. My point is that we should let openssl be the backend for the password_hash function if possible. I'll look into it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could simplify it a little. |
closes #845 |
What does this PR do?
Support PHP 8.5 alpha.
TODO
Waiting upstream response PHP 8.5 windows support easysoft/phpmicro#23https://github.com/static-php/phpmicro/pull/8/filesTracked all unsupported things and future versions in #842 .
Checklist before merging
*.php
or*.json
, run them locally to ensure your changes are valid:PHP_CS_FIXER_IGNORE_ENV=1 composer cs-fix
composer analyse
composer test
bin/spc dev:sort-config
src/globals/test-extensions.php
.extension test
ortest extensions
to trigger full test suite.