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

Schedule gets registered with every artisan call #51354

Open
Plytas opened this issue May 9, 2024 · 5 comments
Open

Schedule gets registered with every artisan call #51354

Plytas opened this issue May 9, 2024 · 5 comments
Labels

Comments

@Plytas
Copy link
Contributor

Plytas commented May 9, 2024

Laravel Version

11.7.0

PHP Version

8.3.6

Database Driver & Version

No response

Description

When running any artisan command (just php artisan does the same), the code inside console.php route file and/or ->withSchedule() bootstrap method gets executed. Usually this wouldn't be a big deal, because the code should only register cron events. However, sometimes dynamic scheduling might involve database queries which can inflict unwanted performance penalty or in worst case scenario fail in CI setup.

We noticed this in our CI while upgrading from Laravel 10 to Laravel 11 (with slimmed down app structure). Our scheduler needs to be dynamic and it makes a DB query for that reason. When CI is setting up the application it runs composer install which then runs php artisan package:discover --ansi as a composer post-autoload-dump script. This is standard in every Laravel project. Since package:discover is an artisan command it boots up the scheduler as well. At this stage in CI database is not yet available thus we get a QueryException: SQLSTATE[HY000] [2002] Connection refused.

I have checked and this wasn't the case with Laravel 10. I'm not sure if this is now intended or not, but I would expect scheduler code to only execute when calling php artisan schedule:run and php artisan schedule:work.

Steps To Reproduce

  • Set up new Laravel 11 project.
  • Add dd(); inside console.php route file or inside ->withSchedule() app bootstrap method.
  • Run php artisan and observe the command to fail.
crynobone added a commit that referenced this issue May 10, 2024
`ApplicationBuilder::withScheduling()`

fixes #51354

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@driesvints driesvints added the bug label May 10, 2024
@Plytas
Copy link
Contributor Author

Plytas commented May 10, 2024

Thanks @crynobone for taking your time to look at this. Unfortunately your attempt unveiled another issue. I tried investigating this myself and here are my findings:

  1. Artisan command is invoked.
  2. Console Kernel is bootstrapped.
  3. Commands are discovered
  4. console.php route file is required.
    • If the file contains scheduled commands \Illuminate\Console\Scheduling\Schedule is resolved too.
  5. Console application is bootstrapped, executing bootstrappers registered with Artisan::starting()

Currently, ->withSchedule() looks like this:

Artisan::starting(fn () => $callback($this->app->make(Schedule::class)));

Meaning that the code inside ->withSchedule() gets executed with every artisan command.
@crynobone suggested to change this to

Artisan::starting(function () use ($callback) {
    $this->app->afterResolving(Schedule::class, fn ($schedule) => $callback($schedule));
});

Which unfortunately doesn't work if console.php route file contains scheduled commands, because \Illuminate\Console\Scheduling\Schedule gets resolved immediately the second time, skipping both $resolvingCallbacks and $afterResolvingCallbacks.


At this point, for this to work we would need to either detect scheduled commands inside console.php and defer registering them after console application is bootsrapped (step 5), or execute $afterResolvingCallbacks even if items, that have already been resolved before, are returned from container directly.
Perhaps I don't see the full picture yet and am missing some other hook that can be used?

Of course, another option is to treat this "bug" as "working as expected" and perhaps document it. After all, all code inside console.php is already being executed with every artisan call, so maybe code inside ->withSchedule() should be too.

@mohammadMghi
Copy link

Why you did not run 'schedule' inside a Artisan::command() function? like following:

Artisan::command('schedule:run', function () { //write schedulers here. });

So when run php artisan schedule:run only code there get executed.

@Plytas
Copy link
Contributor Author

Plytas commented May 19, 2024

Well... schedule:run is already a command. Overriding that would mean re-implementing the whole cron logic myself.

Unless I'm missing something?

@flexchar
Copy link

How about having a dedicated schedule.php file? During migration to L11 I found this to be very confusing and I am experiencing my commands not being run besides them appearing in the CLI list. It may be a completely another issue but it feels worth a second thought.

@Plytas
Copy link
Contributor Author

Plytas commented May 23, 2024

@flexchar You are able to do that using ->withSchedule() method on app bootstrap and then importing/requiring schedule.php file.

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