laravel / horizon

Dashboard and code-driven configuration for Laravel queues.
https://laravel.com/docs/horizon
MIT License
3.83k stars 643 forks source link

[5.x] Improves console output and fixes Carbon v3 support #1387

Closed nunomaduro closed 6 months ago

nunomaduro commented 6 months ago

This pull request does three things:

  1. Drops Laravel 8 and its older PHP versions.
  2. Fixes support of Carbon v3, because we were trying to send a "60" as string to $date->addSeconds(...).
  3. Improves the console output of all commands.

Here is an example of the new console output:

Screenshot 2024-02-12 at 17 36 53

Here is another:

Screenshot 2024-02-12 at 17 37 31

And another:

1

sebastiaanluca commented 6 months ago

I think this broke the output of the commands, but I'm not sure.

We restart Horizon every so often via a scheduled job and after the update from yesterday we've been getting these in production. So it's interpreting the status message as output instead of the status code?

Can reproduce it.

In one terminal shell:

php artisan horizon

In another:

php artisan horizon:restart

The exceptions:

RuntimeException
Horizon returned an unknown status "   INFO  Horizon is running.  " and could not be restarted.

    protected $description = "Restart Horizon if it's currently running";
    public function handle(): int
    {
        if (! $this->isSupportedStatus($status = $this->getHorizonStatus())) {
            throw new RuntimeException(sprintf('Horizon returned an unknown status "%s" and could not be restarted.', $status));
        }
        if ($status !== 'Horizon is running.') {
            return static::SUCCESS;
        }
App\Exceptions\ScheduledCommandException
The scheduled command `'/usr/bin/php8.2' 'artisan' horizon:restart` failed to complete successfully. 
In RestartHorizon.php line 29:

  Horizon returned an unknown status "   INFO  Horizon is running.  " and cou  
  ld not be restarted.       

    {
        $output = file_exists($event->output)
            ? file_get_contents($event->output)
            : 'No output found.';
        return new self(sprintf(
            'The scheduled command `%s` failed to complete successfully. %s',
            $event->command ?? $event->description,
            $output,
        ));
    }

Edit: created a new issue for it - https://github.com/laravel/horizon/issues/1388

nicolus commented 6 months ago

I'm probably nitpicking but shouldn't this have been be Horizon 6.0 ?

It changes PHP and Laravel requirements, plus the fact that it changes the output of the CLI commands means that some third party scripts could break. For example if I have a deployment pipeline that uses "horizon:status" and parses the output to make sure it's running, it would suddenly start throwing errors when upgrading from 5.22 to 5.23.

sebastiaanluca commented 6 months ago

I agree, this happened in our case and our tests didn't catch it because we had to mock the horizon command.

driesvints commented 6 months ago

Hey al. First of all I'm sorry this has disrupted you. That definitely wasn't our intention.

About the version bumps: these are perfectly fine to make in minor releases. We do this all the time in our packages. Please see the rule here in the spec: https://semver.org/#spec-item-7 and here: https://semver.org/#what-should-i-do-if-i-update-my-own-dependencies-without-changing-the-public-api

I also want to note that we don't consider the output to be part of the public API and thus, in our eyes, there can be no breaking changes here. Again, we're sorry this disrupted you but we think we didn't do anything wrong here.