Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pipe detection still broken #224

Open
joehoyle opened this issue Oct 8, 2020 · 3 comments
Open

Pipe detection still broken #224

joehoyle opened this issue Oct 8, 2020 · 3 comments
Labels
bug Existing functionality isn't behaving as expected developer advocacy Developer Advocacy related tasks should have Should be done, medium priority for now

Comments

@joehoyle
Copy link
Member

joehoyle commented Oct 8, 2020

Somehow even though I thought I had fixed pipe / tty detection about 3 times, it still seems to be broken. Most recently in #157.

Basically, I can not get the output TTY detection to work. In a plain PHP script:

// test.php
var_dump( posix_isatty( STDOUT ) );

Running php -f test.php will output bool(true), whereas php -f test.php | grep bool will output bool(false). This is expected, as in the first case, STDOUT is a TTY, and in the second case STDOUT is being piped to grep.

For some reason, when used via composer server cli, no matter what I check, posix_isatty( STDOUT ) is always false.

In Composer commands (Symfony Console), there is a way to detect the TTY support (well, color support), like in https://github.com/symfony/symfony/blob/2.6/src/Symfony/Component/Console/Output/StreamOutput.php#L101. In local-server if I even try to do posix_isatty( $output->getStream() ) I also get FALSE. So, the Console output stream is not a TTY, neither is STDOUT.

I'm not sure if this is just my setup, but either way, it's breaking the TTY detection, which means things like composer server cli post list will not show table columns / dividers etc.

We could disable the TTY detection for STDOUT, and only use the STDIN detection (which works fine). This would mean piping data in to CLI commands should be fine, and also tables and color formatting should work fine. However, it means doing something like composer server cli -- post list --fields=ID | xargs -n 1.... will not work as expected, as xargs will be receiving formatted output.

@joehoyle joehoyle added the bug Existing functionality isn't behaving as expected label Oct 8, 2020
@joehoyle joehoyle added this to the 5.0 milestone Oct 8, 2020
@roborourke
Copy link
Contributor

We're going to all this effort to support using the composer prefixed commands. We might be able to try out some alternative approaches to this problem using WP CLI remote execution.

WP CLI already has an SSH protocol handler for docker-compose eg in the vendor/altis/local-server/docker directory the following works:

COMPOSE_PROJECT_NAME=altis-dev wp --ssh=docker-compose:php post list --url=altis-dev.altis.dev --fields=ID --format=ids | xargs -n 1

If WP CLI (at least as much of it as is needed to run commands remotely) is included as a depedency of local server we might be able to use composer exec wp instead, by providing a generated default .wp-cli.yml.

@joehoyle
Copy link
Member Author

joehoyle commented Oct 9, 2020

I think that has too many weaknesses to pursue:

  • wp-cli needs to be installed on the host
  • --url and other automatable / known values need to be passed manually
  • We lose the ability to do any wp wrapping. If we had a composer command that would make use of your above command, we'd now just be at the same point (needing to forward things like if a TTY is present)

@roborourke
Copy link
Contributor

That's not strictly true if we generate a .wp-cli.yml, much of that can be configured. Agree though, needing to install WP CLI as a dep isn't ideal. Just a thought if it's a solved problem in WP CLI already.

@jennybeaumont jennybeaumont added the must have Must be done, high priority label Oct 15, 2020
@jennybeaumont jennybeaumont removed this from the 5.0 milestone Oct 15, 2020
@roborourke roborourke mentioned this issue Oct 28, 2020
13 tasks
@jennybeaumont jennybeaumont added this to the v7 sprint 1 milestone Feb 2, 2021
@rmccue rmccue added should have Should be done, medium priority for now and removed must have Must be done, high priority labels Apr 27, 2021
@yumito yumito removed the cross-team label Nov 23, 2021
@missjwo missjwo added the developer advocacy Developer Advocacy related tasks label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Existing functionality isn't behaving as expected developer advocacy Developer Advocacy related tasks should have Should be done, medium priority for now
Projects
None yet
Development

No branches or pull requests

6 participants