Add logic to new stream functions on Windows #105
Conversation
| */ | ||
| public function testSapiWindowsVt100Support() | ||
| { | ||
| if ('\\' !== DIRECTORY_SEPARATOR) { |
There was a problem hiding this comment.
The current code matches the convention we're following, LGTM as is.
There was a problem hiding this comment.
what's behind that convention? looks pretty weird to me to not use solution provided by test runner but do it manually
There was a problem hiding this comment.
Dunno exactly, maybe just discoverability is better, so more commonly used.
There was a problem hiding this comment.
I'll do it whatever way you chaps want.
There was a problem hiding this comment.
let's keep it this way, that's consistent with the code base
There was a problem hiding this comment.
I think the reason is support for older PHPUnit versions (IIRC, OSFAMILY was added later)
src/Php72/Php72.php
Outdated
| { | ||
| // We cannot actually enable vt100 support | ||
| if (true === $enable || !self::stream_isatty($stream)) { | ||
| return false; |
There was a problem hiding this comment.
I'm just wondering if I have got the logic right here, in automatically returning false if the enable param is true. It makes sense in the strictest way, in that you cannot actually enable the setting without using the native function. But in terms of how the polyfill will used, it may be better to remove this bit (because an application would probably always set the enable param to true). Thoughts please.
Also, I haven't covered the case where enable = false.
There was a problem hiding this comment.
Perhaps if (false === $enable ... would be better, in that one would simply not apply the vt100 formatting sequences, regardless of what the native function would return.
|
I've changed the logic in The only way to enable support for vt100 sequences in modern Windows consoles is to use the So in userland, an app that outputs vt100 sequences will either enable it early on, or on-demand in an output handler. It would unnecessary to disable it, because an app would simply omit vt100 sequences if they were inappropriate. |
|
I would trust you on this one, looks 👍 to me. |
|
@nicolas-grekas Thanks. I reviewed the PR that added this to php-src and contributed some mods and stuff (like the |
|
@nicolas-grekas Apologies, but I found some Git Bash weirdness happening when testing composer/xdebug-handler In addition, if you invoke |
| */ | ||
| public function testSapiWindowsVt100Support() | ||
| { | ||
| if ('\\' !== DIRECTORY_SEPARATOR) { |
There was a problem hiding this comment.
let's keep it this way, that's consistent with the code base
tests/Php72/Php72Test.php
Outdated
| $this->markTestSkipped('Windows only test'); | ||
| } | ||
|
|
||
| $this->assertFalse(p::sapi_windows_vt100_support(STDOUT, false)); |
There was a problem hiding this comment.
we should make the test pass without the p:: prefix: the testing framework will ensure the test is run twice: with and without the polyfill when possible, so that behavior parity is "proven" on 7.2
There was a problem hiding this comment.
we do know it, but as you see it's not obvious, maybe it worth to put that knowledge somewhere? (contributing file perhaps?)
There was a problem hiding this comment.
@keradus Why not :) The readme has this sentence on the topic:
Polyfills are unit-tested alongside their native implementation so that feature and behavior parity can be proven and enforced in the long run.
Feel free to submit a PR to amend the README.md, or another file that could be a better fit to you.
tests/Php72/Php72Test.php
Outdated
| public function testStreamIsatty() | ||
| { | ||
| $fp = fopen('php://temp', 'r+'); | ||
| $this->assertFalse(p::stream_isatty($fp)); |
|
@nicolas-grekas To confirm: The tests pass on PHP-7.2 on Windows |
|
Thank you @johnstevenson. |
The tests are pretty useless, but it is impossible to guarantee what the descriptors are pointing to in all test environments.