php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #76833 stream_socket_enable_crypto-win32.phpt fails
Submitted: 2018-09-01 14:24 UTC Modified: 2018-09-04 09:13 UTC
Votes:1
Avg. Score:3.0 ± 0.0
Reproduced:0 of 1 (0.0%)
From: cmb@php.net Assigned: ab (profile)
Status: Closed Package: Testing related
PHP Version: 7.2Git-2018-09-01 (Git) OS: Windows
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: cmb@php.net
New email:
PHP Version: OS:

 

 [2018-09-01 14:24 UTC] cmb@php.net
Description:
------------
ext\standard\tests\streams\stream_socket_enable_crypto-win32.phpt
fails with openssl-1.1.0i-vc15-x64 on PHP-7.2, as can be seen on
Appveyor[1] (I can also reproduce this locally).  With older
openssl the test generally succeeds, but on a German system it
fails with the following diff:

012+ Warning: stream_socket_enable_crypto(): SSL: Der Vorgang wurde erfolgreich beendet.
012- Warning: stream_socket_enable_crypto(): SSL: The operation completed successfully.

[1] <https://ci.appveyor.com/project/php/php-src/build/PHP-7.2.build.8414/job/ay1uiffrme2rf5n8#L5512>


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2018-09-01 15:01 UTC] kalle@php.net
It seems like Caruso merged this Windows specific test with the Unix counterpart for master and you are right with the language specific error message, perhaps we should use %s in place there as the error error messages seems to come from the function itself.

As for the error itself, that I did not have enough time to dig into here, but does the master one pass?
 [2018-09-01 16:12 UTC] cmb@php.net
Yes, master and 7.3 are apparently okay.  Regarding the localized
message: it actually doesn't make sense to raise E_WARNING for
something that succeeded.  Anyhow, now that I had a closer look at
the test[1], I don't think it makes much sense at all.  Looks like
the test tries to increase code coverage, without testing the
actually relevant stuff (for instance, that after enabling crypto
the stream still “works”).

[1] <https://github.com/php/php-src/blob/php-7.3.0beta3/ext/standard/tests/streams/stream_socket_enable_crypto.phpt>
 [2018-09-01 17:29 UTC] kalle@php.net
Yeah I was a little curious on that when I was reading over the code, it does indeed seem like it was just for coverage. At least with our more recent point of view in regards to these, we should probably remove the test by itself.

As for testing actual relevant stuff, I guess what we could do for streams stuff, is to maybe make a forked version of already existing tests to enable crypto stuff where it makes sense unless someone steps up to specifically write such.
 [2018-09-04 08:44 UTC] ab@php.net
Yeah, this test has been a headache for some time already. And it's a bit controversial, as creating a socket server and then enabling crypto as a client seems quite odd. Such tests make sense for testing abnormal situation, but in this case it requires too much maintanance. I'll check to backport these changes from 7.3, otherwise this test should be removed.

Thanks.
 [2018-09-04 08:58 UTC] ab@php.net
Automatic comment on behalf of ab
Revision: http://git.php.net/?p=php-src.git;a=commit;h=2476fb76a75cf71b2d27070f1daeeb1ea4058096
Log: Fixed bug #76833, backport change to stream_socket_enable_crypto-win32.phpt from 7.3
 [2018-09-04 08:58 UTC] ab@php.net
-Status: Open +Status: Closed
 [2018-09-04 09:13 UTC] kalle@php.net
-Assigned To: +Assigned To: ab
 [2018-09-04 09:13 UTC] kalle@php.net
Thank you Anatol!
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Fri Mar 29 13:01:29 2024 UTC