-
Notifications
You must be signed in to change notification settings - Fork 22
Feature/native cron runner 291 #408
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
Feature/native cron runner 291 #408
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #408 +/- ##
============================================
- Coverage 80.72% 80.65% -0.07%
- Complexity 2465 2641 +176
============================================
Files 229 237 +8
Lines 6541 7020 +479
============================================
+ Hits 5280 5662 +382
- Misses 1261 1358 +97 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
armanist
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this is a solid and well-thought-out feature 👍
The cron runner architecture, task loading model, and execution flow look clean and consistent with Quantum’s design principles. Supporting multiple task formats (array, object, helper, fluent schedule()) is especially nice, and the implementation stays readable without introducing unnecessary complexity.
I’ve left several inline comments that need to be addressed before merge — mostly around clarity, edge cases, and a few behavioral details — so I’m marking this review as Changes required for now.
As a nice-to-have (not a blocker): it would be great in the future to support a command() API on the fluent schedule() builder (similar to Laravel), which would compile into a callback that spawns a console command in a separate process. This would make running Quantum console commands via cron more ergonomic, while keeping the current execution model intact.
Once the requested changes are addressed, this should be in good shape to move forward.
armanist
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the issues are solved, only few issues was pointed by scrutinizer here that will need to be addressed (mostly related to suppressing errors/warnings via @.):
https://scrutinizer-ci.com/g/softberg/quantum-php-core/inspections/cfd49738-baf5-4a91-8b8f-a7acfb5c5cec/issues/files/src/Libraries/Cron/CronLock.php?status=new&orderField=path&order=asc&honorSelectedPaths=0
Closes #291