Skip to content

Check whether less pager exists before trying to pipe content through it #6020

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

benlk
Copy link

@benlk benlk commented Nov 24, 2024

Problem

in the corner case where:

  1. wp-cli is running on a non-Windows system
  2. the PAGER environment variable is not set
  3. the command less is not installed
  4. the user triggers output of the wp help command, such as by running that command explicitly or by running an invalid wp-cli command

wp-cli crashes, as documented in #5333

An example of an environment with out less is many "minimalist" Docker server containers.

Changes

  • Fixes Command less not available on the current machine #5333 by removing the assumption that less exists in non-Windows environments when the environment variable $PAGER is not set. Instead, in all environments, invocations of wp help will first check to see if less exists, and use that, before falling back to more. The previous behavior was to pipe output through less -R on non-Windows systems without first checking if less exists.
  • Adds a docblock to Help_Command::pass_through_pager()

Alternatives considered

Replacing less -R with cat

This would replace a pager with a non-pager, which seems like a feature regression

Replacing less -R with more in all cases

Ruled out by @mrsdizzie at #6020 (comment), and removed in b88c438

Accurately checking whether less exists

I started a branch with this intent, at benlk@832bb1e , but this approach proved nonviable.

To accurately test whether less exists:

  • On Windows, we would need to add a test to wp-cli to determine whether wp-cli is running in cmd.exe or Powershell, as cmd.exe can use where to determine whether a command exists, but Powershell needs to use a different command
  • On POSIX systems, where command -v accurately determines whether a command exists, we would also need to check for the existence of an alias, which is a separate can of worms
  • In all cases, we would need to perform extensive checks on the return of the check, because proc_close() isn't guaranteed to return the command's exit code

This seemed like way too much scope creep, introducing too much code, requiring too much testing, for a fallback scenario around displaying a help message.

Testing

The existing tests in https://github.com/wp-cli/wp-cli/blob/main/features/help.feature do not care about whether more or less is used.

If additional tests are needed for this change, I'm open to suggestions on what should be tested.

For local testing, on all OS:

  1. Set up this branch according to https://make.wordpress.org/cli/handbook/contributions/pull-requests/#working-on-the-project-as-a-whole
  2. Run echo $PAGER to check what your local shell's PAGER variable is. If it is set to any value, including an emptystring, run unset PAGER to unset it.
  3. Run less to determine whether less exists on your system. If it does, run command -v less (POSIX) or where less (Windows) to determine whether it exists as a binary or as an alias.
  4. Run wp without any other commands or options
  5. Expect that the output of wp help is displayed. If less exists on your system, it should be paged in less. If more exists in your system, it should be paged in more.

@benlk benlk requested a review from a team as a code owner November 24, 2024 21:45
@benlk
Copy link
Author

benlk commented Nov 24, 2024

Noting that, due to Thanksgiving, I will respond to comments on this PR in December.

@mrsdizzie
Copy link
Member

Thank you for the pull request.

I don't think it works to change the default pager since it would be a downgrade and create new issues for the majority of users:

  • more isn't guaranteed to have search (and when it does it isn't as good)
  • It doesn't process escape sequences which docs use heavily and we rely on less -R handling

So replacing less -R as a default would be a breaking change and is a no-go imo.

The standard PAGER env var already allows people to override this which seems like a fine tradeoff here given the small percent of overall users that are on linux and don't have less available. I agree it would be nice if it told users that instead of saying less: not found but I wouldn't consider that a crash or particularly cryptic error. less is just an intentional requirement unless you override it.

I think if you wanted to attempt to detect less it is OK if it isn't 100% reliable in every corner case, since if it works in 95% of an already small error case then it is that much better than it was without downgrading behavior for most existing users. We already have similar stuff elsewhere.

@benlk benlk changed the title Remove erroneous assumption that less exists on non-Windows systems Check whether less pager exists before trying to pipe content through it Dec 6, 2024
@benlk
Copy link
Author

benlk commented Dec 6, 2024

@mrsdizzie I've updated this to use the same Process-based mechanism you mentioned to detect whether less exists, with a conditional to use where on Windows.

Unfortunately, I don't have a Windows machine handy to test this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command less not available on the current machine
3 participants