laravel / tinker

Powerful REPL for the Laravel framework.
https://laravel.com/docs/artisan#tinker
MIT License
7.32k stars 130 forks source link

[2.x] Forward input options to psysh #98

Closed GrahamCampbell closed 4 years ago

GrahamCampbell commented 4 years ago

This PR forwards things like --no-ansi and -vv to psysh in the proper way, instead of ignoring them. Once this PR is merged, we can tag tinker v2.4.0.

// cc @bobthecow

GrahamCampbell commented 4 years ago

Heh, our tests shouldn't have passed. I guess we are not actually testing the tinker command. ;)

GrahamCampbell commented 4 years ago

Hmm, asking for verbose seems to just hang forever?

image
bobthecow commented 4 years ago

It's not hanging forever. It's just running as --quiet.

$input->getOption('verbose') is returning true, which PHP matches to anything truthy in this switch because PHP is awesome:

https://github.com/bobthecow/psysh/blob/5f97c26be75a2334a08530e7f99ead9cf0137a21/src/Configuration.php#L268

Apparently your verbose isn't compatible with my verbose. This seems at odds with what it spits out for --help on the command line:

Screen Shot 2020-04-06 at 12 51 54 PM

Adding explicit handling for === true fixes it, and that seems like the reasonable answer, so I'll do that. But you should track down what's going on with your --verbose :)

bobthecow commented 4 years ago

Confirmed that the psysh change fixed running with -v.

GrahamCampbell commented 4 years ago

Oh, lol. Our verbose is just whatever symfony does as the default for commands.

GrahamCampbell commented 4 years ago

Is there a way for psysh to match up with those, or is it intentionally not defined like that?

bobthecow commented 4 years ago

PsySH does match up (with how Console\Application handles it, maybe something else does it differently?)

https://github.com/symfony/console/blob/619054da7627df1c2169c7aabe0df78f9320847c/Application.php#L869-L883

bobthecow commented 4 years ago

Oh cool. Symfony used the wrong definition for how they actually implemented the handling:

https://github.com/symfony/console/blob/619054da7627df1c2169c7aabe0df78f9320847c/Application.php#L975

bobthecow commented 4 years ago

Here we go. When the levels for --verbose flags were first added, it was defined as VALUE_OPTIONAL, as it needs to be for it to work with the values they're actually parsing:

https://github.com/symfony/console/commit/039ba6f73dfb9bfd5e70fa7ba32df4553d5f225a

But the same day, it was changed to VALUE_NONE with the commit message "Fix tests" 😬

https://github.com/symfony/console/commit/841817ba90a85639ec7cccceb7966213f463228e

… so the input definition has been broken the entire time, and the only reason it mostly works is Symfony doesn't actually use the input definition to parse the input for --verbose.

Given this, I think the answer is to stop trying to use getOption at all for verbosity, because using the bound input option will give the wrong answer.

bobthecow commented 4 years ago

All better! https://github.com/bobthecow/psysh/commit/4884edff6ffc9a9640af3aa21f8147524f91543b

GrahamCampbell commented 4 years ago

@bobthecow Thanks for this. The no-interaction flag doesn't seem to work, however:

image
GrahamCampbell commented 4 years ago

It looks like it does get set to disabled, but the configuration later doesn't get respected?

driesvints commented 4 years ago

Why would you use --no-interaction with Tinker?

GrahamCampbell commented 4 years ago

Why would you use --no-interaction with Tinker?

Well, all laravel's commands should respect the no interaction flag. In the case of the tinker command, instead of reading from the input "live", we'd just one-time read whatever was piped into from stdin, and execute it.

GrahamCampbell commented 4 years ago

I'd say this PR is ready for merge now. Handling of no-interaction is to be handled outside of the tinker package. We already forward the config properly.

bobthecow commented 4 years ago

It (probably) is doing "no interaction", you're just putting it in a weird place by not giving it anything to execute.

Screen Shot 2020-04-07 at 7 01 25 AM
GrahamCampbell commented 4 years ago

It (probably) is doing "no interaction", you're just putting it in a weird place by not giving it anything to execute.

Surely psysh should either exit straight away with code 0 in that case, or it should throw an exception saying nothing to do?

bobthecow commented 4 years ago

It's a pretty straightforward change. I'm not entirely convinced it's what people will expect though :)

Screen Shot 2020-04-07 at 8 03 08 AM
GrahamCampbell commented 4 years ago

Isn't the whole point of no-interaction is that the process will never wait for input?