Edit report at https://bugs.php.net/bug.php?id=76833&edit=1
ID: 76833 Updated by: ka...@php.net Reported by: c...@php.net Summary: stream_socket_enable_crypto-win32.phpt fails Status: Closed Type: Bug Package: Testing related Operating System: Windows PHP Version: 7.2Git-2018-09-01 (Git) -Assigned To: +Assigned To: ab Block user comment: N Private report: N New Comment: Thank you Anatol! Previous Comments: ------------------------------------------------------------------------ [2018-09-04 08:58:21] a...@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:44:27] a...@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-01 17:29:57] ka...@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-01 16:12:17] c...@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 15:01:48] ka...@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? ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at https://bugs.php.net/bug.php?id=76833 -- Edit this bug report at https://bugs.php.net/bug.php?id=76833&edit=1
