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

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

Merged
merged 21 commits into from
Feb 12, 2024

Conversation

nunomaduro
Copy link
Member

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

@nunomaduro nunomaduro marked this pull request as ready for review February 12, 2024 17:42
@taylorotwell taylorotwell merged commit 0b1bf46 into 5.x Feb 12, 2024
12 checks passed
@taylorotwell taylorotwell deleted the feat/improve-console-output branch February 12, 2024 18:36
@sebastiaanluca
Copy link

sebastiaanluca commented Feb 15, 2024

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 - #1388

@nicolus
Copy link

nicolus commented Feb 15, 2024

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
Copy link

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

@driesvints
Copy link
Member

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.

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.

5 participants