-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add TLS1.3 stream option #4
Add TLS1.3 stream option #4
Conversation
@ChristophWurst TLS 1.3 (and that constant) are only supported starting with PHP 7.4 from the looks of it so we can't use it bare like that. According to the php source (https://github.com/php/php-src/blob/PHP-7.4.0/main/streams/php_stream_transport.h#L176), it looks like the existing STREAM_CRYPTO_METHOD_TLS_CLIENT actually already includes all of the TLS versions in PHP 7.4 as well. So we can probably just use that constant and get of the OR's in the case of PHP 7.4. |
7.2 in fact: https://github.com/php/php-src/blob/PHP-7.2.0/main/streams/php_stream_transport.h#L176. I'll change it to that. |
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
a7cf4cc
to
74bfc54
Compare
@mrubinsk please let me know if the changes are OK or if you with to have it done differently ✌️ |
@ChristophWurst Yup, looks good. I'll merge this as soon as I have some time. |
This wouldn't work with PHP < 5.6 which we unfortunately still support. How about using: |
I still hope the PR will be merged. Is there any progress here? |
Apologies, this fell through the cracks. I had thought I committed this already. |
Discovered in nextcloud/mail#3368 (comment).
@mrubinsk hope this is ok. Do we need a check around this? I saw that
STREAM_CRYPTO_METHOD_TLSv1_2_CLIENT
doesn't have one either so I just added the constant. Let me know what you prefer.