[RFC] Io\Terminal

• 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.

Fair point. The RFC was assuming too much terminal background there.

In canonical mode the terminal normally buffers input until a line is
complete, may echo it, and handles some control characters itself. Raw
mode turns off that line-oriented processing so the application can
react to individual key presses and terminal sequences directly.

One thing that also wasn't clear is that callers don't need to call
enableRawMode() before readKey() or readSecret(). Both handle the
temporary mode change internally.

enableRawMode() is there for longer-running interactive code that
wants to keep the terminal raw across multiple reads/redraws.

I've clarified that in the RFC.

Thanks, that does make it clearer! So the reason to use raw mode yourself would be, for instance, for a game where you're capturing the arrow keys and WASD, or something like that? (Concrete examples would help.).

This also makes me think that a readLine() method makes sense in this base tool, not at a higher level. (I haven't done much console-GUI works so I am not familiar with the typical patterns, but it seems like the natural complement to readKey().)

• That said, raw mode looks like a textbook case for a context manager. :-)

Conceptually, yes. That's what I was trying to model with ModeToken:
it represents the active lease, and restoreMode() gives an explicit
way to release it in a try/finally.

PHP doesn't have a general language-level context-manager construct,
and I didn't want to add a terminal-specific callback abstraction just
for this, so I kept the primitive explicit.

Yes, that's more of an aside for the audience. Arnaud and I have an RFC out (currently on hold) for context managers, and this would be another very good use case for them.

• 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 agree with the testing concern. I've added TerminalInterface while
keeping the native Terminal final, so application and library code can
type against something that can be replaced by a userland fake.

I also added ModeTokenInterface for fake implementations. The native
Terminal still only accepts a native token belonging to the same
logical terminal; foreign, stale, or unrelated tokens are rejected
with ValueError.

Conventions for Internals say to not use a *Interface suffix. It's unnecessary. I would suggest either

interface Terminal {
public function readKey();
// ...
}

class SystemTerminal implements Terminal {
public static function fromStdIo(): self {}
public static function fromStreams(): self {}
}

or possibly:

class StdIoTerminal implements Terminal {
// .. No factories.
}

class StreamsTerminal implements Terminal {
public function __construct($in, $out = null) {}
}

For ModeToken, I'm not sure if it makes sense to have separate classes for each core terminal. It's just an opaque value object, really, so I don't know what pattern we'd want here.

--Larry Garfield

Hi

• 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.)

I very strongly disagree with the interface suggestions for the exact
“implementation detail” reason you mentioned and the introduction of the
interface has made the API much worse:

The (System)Terminal class is an implementation detail of something
else and it is only exercised as part of the glue code within an
integration test. It is not the IO boundary, the “TTY stream” you
provide to the Terminal is. You need the TTY stream separately anyway,
because the (System)Terminal does not provide any mechanism to write to
the output (and the underlying stream is not exposed either). In your
tests you would then attach an appropriate stream to the terminal (e.g.
using proc_open() with pty descriptors, which is what the RFC’s own
implementation already uses for testing).

This is very much like the final Random\Randomizer where you would
have unit tests for the “number consumer”, passing hardcoded values. And
then you have an integration test where you provide a class implementing
the Random\Engine interface as the IO boundary. How the glue code
turns a “Random\Engine” into a “random number” that is passed to the
unit-tested “number consumer” is an implementation detail that is not
worth testing.

There are additional indicators that the SystemTerminal class is more
akin to a “helper” class:

1. Large parts of the Terminal’s API surface don’t need to be instance
   methods, because they don’t really care about the object state (beyond
   the stream). They could also be bare functions and they were in the
   initial pre-RFC implementation. Having them as instance methods makes
   the API a little more convenient to use and enables the RAII reset.

2. The interface’s contract is “What SystemTerminal does”. As you say
   yourself, you are not sure if there can be multiple non-testing
   implementations, and I can't think of any either: The Terminal
   interface does not define an “abstract concept” with multiple equally
   legal implementations.

This is contrary to, say, Time\Clock where the Clock is the IO
boundary and you can have SystemClock, GpsClock (when a GPS receiver is
attached to your computer), HttpClock (fetching an HTTP service to
obtain the current time, not useful, but theoretically meaningful), ….

3. The interface is humongous and not well-defined: Terminals expose a
   broad API surface and this is reflected by the SystemTerminal class
   and the RFC even cut out some of the methods that are there in the
   pre-RFC implementation. It is likely that the API surface will grow in
   future PHP versions with additional helper functionality. This also
   means that the interface needs to grow, which is an obvious breaking
   change. Or new non-well-defined interfaces need to be added, which is
   bad API design. In fact v0.3 of the RFC added a readLine() method
   which (at least on POSIX) is effectively redundant with fgets(), but
   allows consistent access to the “input side” of the terminal. It is not
   unlikely that a future PHP version might want to add readUntilEof() or
   similar.

4. The interface being a direct mapping of the Terminal API surface also
   resulted in the ModeToken interface being added, which is an interface
   for something that is an opaque “token” (value) object. And then due to
   interface constraints / the lack of generics, the Terminal interface
   requires any Terminal to accept any ModeToken in the signature of
   restoreMode() just to validate that it is the ModeToken
   implementation that belongs to the Terminal. The RFC specifies a
   ValueError here, but it really is a TypeError that cannot be
   expressed by the type system.

5. This has also resulted in the naming weirdness: The obvious name is
   “Terminal”, the “System” part in “SystemTerminal” carries no additional
   information.

Best regards
Tim Düsterhus

My main concern is being able to mock the terminal in order to effectively test code that uses the terminal, without putting a proprietary thin wrapper around it (which largely defeats the purpose of having a good API in core). Interfaces are the standard way of doing that. If you have a suggestion for a better way, I'm happy to see it.

--Larry Garfield

Hi

• 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.)

I very strongly disagree with the interface suggestions for the exact
“implementation detail” reason you mentioned and the introduction of
the interface has made the API much worse:

The (System)Terminal class is an implementation detail of something
else and it is only exercised as part of the glue code within an
integration test. It is not the IO boundary, the “TTY stream” you
provide to the Terminal is. You need the TTY stream separately anyway,
because the (System)Terminal does not provide any mechanism to write
to the output (and the underlying stream is not exposed either). In
your tests you would then attach an appropriate stream to the terminal
(e.g. using proc_open() with pty descriptors, which is what the
RFC’s own implementation already uses for testing).

This is very much like the final Random\Randomizer where you would
have unit tests for the “number consumer”, passing hardcoded values.
And then you have an integration test where you provide a class
implementing the Random\Engine interface as the IO boundary. How the
glue code turns a “Random\Engine” into a “random number” that is
passed to the unit-tested “number consumer” is an implementation
detail that is not worth testing.

There are additional indicators that the SystemTerminal class is
more akin to a “helper” class:

1. Large parts of the Terminal’s API surface don’t need to be
   instance methods, because they don’t really care about the object
   state (beyond the stream). They could also be bare functions and they
   were in the initial pre-RFC implementation. Having them as instance
   methods makes the API a little more convenient to use and enables the
   RAII reset.

2. The interface’s contract is “What SystemTerminal does”. As you
   say yourself, you are not sure if there can be multiple non-testing
   implementations, and I can't think of any either: The Terminal
   interface does not define an “abstract concept” with multiple equally
   legal implementations.

This is contrary to, say, Time\Clock where the Clock is the IO
boundary and you can have SystemClock, GpsClock (when a GPS receiver
is attached to your computer), HttpClock (fetching an HTTP service to
obtain the current time, not useful, but theoretically meaningful), ….

3. The interface is humongous and not well-defined: Terminals expose a
   broad API surface and this is reflected by the SystemTerminal class
   and the RFC even cut out some of the methods that are there in the
   pre-RFC implementation. It is likely that the API surface will grow in
   future PHP versions with additional helper functionality. This also
   means that the interface needs to grow, which is an obvious breaking
   change. Or new non-well-defined interfaces need to be added, which is
   bad API design. In fact v0.3 of the RFC added a readLine() method
   which (at least on POSIX) is effectively redundant with fgets(), but
   allows consistent access to the “input side” of the terminal. It is
   not unlikely that a future PHP version might want to add
   readUntilEof() or similar.

4. The interface being a direct mapping of the Terminal API surface
   also resulted in the ModeToken interface being added, which is an
   interface for something that is an opaque “token” (value) object. And
   then due to interface constraints / the lack of generics, the
   Terminal interface requires any Terminal to accept any ModeToken
   in the signature of restoreMode() just to validate that it is the
   ModeToken implementation that belongs to the Terminal. The RFC
   specifies a ValueError here, but it really is a TypeError that
   cannot be expressed by the type system.

5. This has also resulted in the naming weirdness: The obvious name is
   “Terminal”, the “System” part in “SystemTerminal” carries no
   additional information.

Best regards
Tim Düsterhus

Well laid out Tim,

the I/O boundary comes from the streams passed to fromStreams().

For any mocking purposes / different terminals, it's a matter of
composing the stream. We just need to make sure that it's actually
possible to construct such a tty in tests for example, which can
intercept the events / set state for size etc..

Terminal (yes, SystemTerminal shouldn't be a thing) is just a class
which takes stuff from the stream and outputs it. There's no IO
operations in itself (from userland perspective), only on the resource,
which is the actual underlying entity forwarding IO operations to the
kernel.

Thanks,

Bob