On Mon, Sep 28, 2026, at 12:17 PM, Pratik Bhujel wrote: > Hi Tim, Nicolas, > > One quick follow-up after my earlier mail: I've now finished the > corresponding cleanup in the php-src PR as well. > > Tim, I went through the implementation/test review comments and > addressed the remaining points there. > > Nicolas, the shared raw-mode record and terminal identity changes are > now in place too, including restoration through the record's own > descriptor and keeping unrelated PTYs separate. > > The RFC text and the implementation should now describe the same > lifetime/error semantics: > > https://wiki.php.net/rfc/io_terminal > https://github.com/php/php-src/pull/23941 > > I also fixed the Linux PTY EIO test expectation on the current head. > > I don't want to keep adding noise to the list, so I'll leave it here > unless I've missed something. Thanks again for the detailed review. > > Best, > Pratik
Please remember to bottom-post. :-) I am in favor of this RFC in general. It addresses the sort of problem that does belong in stdlib. - getSize()'s error return is null. Not false. False-on-error is an anti-pattern we should be exterminating with extreme prejudice. Or possibly an exception, but not false. - As I'm not familiar with the underlying OS tools... what is raw mode? That seems to be just glossed over. It looks like the only useful API method (readKey() ) requires going into raw mode, so I wonder what its purpose is. - That said, raw mode looks like a textbook case for a context manager. :-) - Again, readKey() should return null, not false, for all the same reasons. - Why does readSecret() not need the same duration/timeout controls as readKey()? - I understand all of the usual arguments for making the Terminal class final. However, it also has no interface. That means it's basically impossible to mock for testing purposes. That strikes me as a problem, because any IO boundary should be mockable. I don't know that multiple non-testing implementations makes sense (maybe alternatives to the static constructors?), but we do need some straightforward mechanism to mock a Terminal object. (I assume someone is going to respond with "it's an implementation detail of something else," which is only partially true; I don't want to have to create a pass-through wrapper for something just for testing purposes, and even then, it would be the same API in name only, since it cannot share a type. That hurts interoperability.) --Larry Garfield
