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

[8.x] Add missing force flag to queue:clear command #34795

Closed
wants to merge 1 commit into from

Conversation

bobbybouwmann
Copy link
Contributor

@bobbybouwmann bobbybouwmann commented Oct 11, 2020

Hi 👋 ,

I was recently deploying an application to production, where Laravel forge will run the php artisan queue:clear command. However, I was not able to call the command without confirming the confirmation.

If you run this command manually it will ask for confirmation when running this command on production.

$ php artisan queue:clear
**************************************
*     Application In Production!     *
**************************************

 Do you really wish to run this command? (yes/no) [no]:
 > 

There is a trait included that makes this possible called ConfirmableTrait. This makes it possible to make sure the user confirms the command before it's executed. You can work around confirmToProceed method by providing the --force option flag. However, the queue:clear command doesn't have a --force option at this point.

This PR adds the missing --force option. I was also so free to update the command to be in line with the other clear commands.

protected $signature = 'queue:clear
{connection? : The name of the queue connection to clear}
{--queue= : The name of the queue to clear}';
protected $signature = 'queue:clear';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just add a third line to the signature with {--force : Force the operation to run when in production}. There's really no need to override the argument/options methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paras-malhotra I know ;)

However, it's not in line with the other clear commands

I was also so free to update the command to be in line with the other clear commands.

If Taylor doesn't want this, he will probably tell me ;)

Copy link
Contributor

@paras-malhotra paras-malhotra Oct 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I added the ConfirmableTrait because it's quite unusual to clear queues in production (unlike clearing cache, cached routes, etc.). Sure, let's wait for Taylor's feedback 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, it doesn't make sense to add the ConfirmableTrait without being able to override it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree with that. I do think we need to add the force option. I'm just saying it's best for the trait to stay there.

@GrahamCampbell GrahamCampbell changed the title Add missing force flag to queue:clear command [8.x] Add missing force flag to queue:clear command Oct 12, 2020
@taylorotwell
Copy link
Member

@bobbybouwmann

I don't think the signature is updated properly. I don't think $signature is used at all when defining methods for arguments and options. You can see they are all missing now:

image

@taylorotwell
Copy link
Member

Feel free to re-open when fixed.

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.

3 participants