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

Fix for issues with closure-based scheduled commands in schedule:test #47862

Merged
merged 4 commits into from Jul 27, 2023

Conversation

mobidev86
Copy link
Contributor

This pull request addresses two issues related to the schedule:test command when dealing with multiple closure-based scheduled commands.

Issue 1: Incorrectly selecting scheduled closures by index number

  • The php artisan schedule:test command allowed users to select a task by its index number, but the task selection logic was based on the task name generated by the schedule:test command, causing all closure-based tasks to have the same name.
  • Solution: Modified the command to display options with their respective index numbers. Now, when the user selects an option, the correct task associated with that index is executed.

Issue 2: Incorrect message when using the --name option with multiple matching commands

  • Previously, when using the command php artisan schedule:test --name <name>, the returned message was "No matching scheduled command found," even if multiple commands with the same name existed. This was misleading.
  • Solution:
    1. Updated the logic to display the "No matching scheduled command found" message only when no command with the given name exists.
    2. Implemented new logic to prompt the user to choose the desired command when multiple commands match the given description.

These changes ensure that the schedule:test command works correctly with closure-based scheduled commands.

I have thoroughly tested these changes on the provided reproduction repository on the attached issue, and all scheduled closures are now correctly executed based on the user's input. Additionally, the --name option now works as intended, providing choices when multiple matching commands exist.

@taylorotwell taylorotwell merged commit 03564b1 into laravel:10.x Jul 27, 2023
20 checks passed
@mobidev86
Copy link
Contributor Author

Hello @taylorotwell

I would like to express my gratitude for the changes made and for merging this pull request; it is greatly appreciated.
However, I do have some inquiries that I would appreciate your input on:

  1. We have successfully resolved issues related to multiple closure commands with the same name by appending unique indexes to each one.

In addition to that, there is another issue as described in the initial message and also mentioned in issue #47783:

  1. Issue 2: Incorrect message when using the --name option with multiple matching commands.
    • Previously, executing the command php artisan schedule:test --name <name> would result in the message "No matching scheduled command found," even if multiple commands with the same name existed, which could be misleading.
    • Solution:
      1.1 The logic has been updated to display the "No matching scheduled command found" message only when no command with the given name exists.
      1.2 New logic has been implemented to prompt the user to choose the desired command when multiple commands match the given description.

If you believe it would be beneficial, I am more than willing to create another pull request with any necessary changes to address this issue. Your guidance on this matter would be highly appreciated.

Thank you for your time and consideration.

@driesvints
Copy link
Member

@mobidev86 Taylor doesn't sees replies on closed prs. Please try a new PR if you want us to look at something 👍

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.

None yet

3 participants