laravel / prompts

Beautiful and user-friendly forms for your command-line PHP applications.
https://laravel.com/docs/prompts
MIT License
533 stars 94 forks source link

Spinner runs `$callback()` twice if statically rendered #65

Closed brianclogan closed 1 year ago

brianclogan commented 1 year ago

Laravel Prompts Version

0.1.6

Laravel Version

10.1.2

PHP Version

8.1.4

Operating System & Version

Ubuntu 20.04 via WSL

Terminal Application

Windows Terminal

Description

First time bug submitter for OSS, if I need to add any more detail let me know!

When using Spinner (not documented yet, found when viewing source) if rendered statically, it will run $callback() twice.

It appears in the Spinner.php class, in the spin() method it calls it here:

if (! function_exists('pcntl_fork')) {
    $this->renderStatically($callback);
}
protected function renderStatically(Closure $callback): mixed
    {
        $this->static = true;
        $this->render();
        return $callback();
    }

And then regardless if it ran via the renderStatically() method, it will run it again in the try block.

Steps To Reproduce

Create a spinner, and have it call statically. Add a echo to the function called, and it will write it to terminal twice if the spinner is rendered statically, only once if rendered normally.

brianclogan commented 1 year ago

My temp fix locally was just to set $result = null at the start of the spin() method, and set it if rendered statically, and do a check. Don't know if that is the best fix, but it's a working one for me locally.

jessarcher commented 1 year ago

It looks like a return got dropped somewhere along the way.

I couldn't replicate your exact scenario, though. For me, it has a fatal error when it erroneously tries to call pcntl_fork. I tested by replacing all occurrences of pcntl_fork with Xpcntl_fork to simulate the function not existing.

brianclogan commented 1 year ago

I guess the reason it would do that though is due to the return not happening sooner, which would force it to call twice if pcntl is installed. I am forcing the static renderer to run if it is on CI, to prevent the logs from filling with a ton of updates on the spinner using a custom Spinner class. Realistically, the only difference is adding another check to the renderStatically() if statement, but if it's not returning then, that would cause the issue I was experiencing.

Now time to modify my class on my local version. Thanks so much for being so fast to respond! Love the work you have done here, and already tinkering with the themes and everything. :)

jessarcher commented 1 year ago

Oh, understood! If you're manually triggering the static mode, then I guess #66 won't help - depending on how you're doing it.

I'm currently working on a "non-interactive" mode for Prompts - mostly to prevent prompting for user input when it's not an interactive terminal session, without needing to fall back to Symfony. We could look at changing the spinner behaviour when non-interactive as well.