Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[11.x] Defer registering schedule registered usingApplicationBuilder::withSchedule()#51364

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
crynobone wants to merge7 commits into11.xfromfixes-51354

Conversation

@crynobone
Copy link
Member

fixes#51354

@crynobonecrynobone marked this pull request as ready for reviewMay 10, 2024 01:29
@Plytas
Copy link
Contributor

Thanks for your input@crynobone, but this doesn't solve the full problem. The problem still exists if you use justconsole.php route file.

This change introduces a new issue where you cannot use both->withSchedule() on app bootstrap ANDconsole.php route file. You can observe this by having a task scheduled in both places and runningphp artisan schedule:list.

Note:inspire is scheduled insideconsole.php (comes by default in new install) andtest is scheduled inside bootstrap.

Before this change:
image

After this change:
image

@crynobone
Copy link
MemberAuthor

You should defer resolving code dedicated for Schedule ifconsole.php. The file itself needs to be loaded for all artisam requests.

@Plytas
Copy link
Contributor

You are correct, I forgot the fact thatconsole.php is used for more than just schedule.

So the only issue I see now is that both methods cannot be used simultaneously while it could have been before.

@crynobone
Copy link
MemberAuthor

So the only issue I see now is that both methods cannot be used simultaneously while it could have been before.

As in Laravel 10? It should only be possible to register Schedule withinApp\Console\Kernel::schedule(). So this should makewithScheduling() behave similar to the old method.

@Plytas
Copy link
Contributor

As in Laravel 11 before this change. I'll re-itterate my previous message.

This change introduces a new issue where you cannot schedule commands in both->withSchedule() on app bootstrap ANDconsole.php route file. You can observe this by having a task scheduled in both places and running php artisan schedule:list.

Note:inspire is scheduled inside console.php (comes by default in new install) andtest is scheduled inside bootstrap.

Before this change:
image

After this change:
image

With this change->withSchedule() is only resolved whenconsole.php route file is empty, otherwise it only lists (and processes) schedule fromconsole.php and ignores->withSchedule().

`ApplicationBuilder::withScheduling()`fixes#51354Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@taylorotwelltaylorotwell restored the fixes-51354 branchJune 6, 2024 14:53
@taylorotwell
Copy link
Member

I'm pretty concerned about this breaking other applications.

@taylorotwelltaylorotwell marked this pull request as draftJune 6, 2024 14:54
@Plytas
Copy link
Contributor

I've been thinking how to solve it and I like this approach if it didn't have that drawback. It feels like it would need a different hook to somehow call the callback only when actually calling schedule commands? Or somehow have scheduled commands insideconsole.php route file to not resolveSchedule class until they are actually needed.

@crynobone
Copy link
MemberAuthor

crynobone commentedJun 7, 2024
edited
Loading

The only possible breaking change is when someone useswithScheduling() to add and schedule command, but that's never how it was intended to be used:

return Application::configure(basePath:dirname(__DIR__))    ->withSchedule(function ($schedule) {        Artisan::command('go',fn () =>$this->comment('Done!'))->hourly();    })->create();

With this PR,go command is only loaded viaschedule:list andschedule:run and cannot be called directly viaartisan.

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@crynobone
Copy link
MemberAuthor

Add additional tests to cover the limitation when adding new artisan commands withinwithSchedule()

@crynobonecrynobone changed the title[11.x] Defer registering schedule registered usingApplicationBuilder::withScheduling()[11.x] Defer registering schedule registered usingApplicationBuilder::withSchedule()Jun 11, 2024
@crynobonecrynobone marked this pull request as ready for reviewJune 11, 2024 09:07
@taylorotwell
Copy link
Member

I dunno, I'm not comfortable merging this without much more community feedback and input.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Schedule gets registered with every artisan call

4 participants

@crynobone@Plytas@taylorotwell

[8]ページ先頭

©2009-2025 Movatter.jp