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

Allow customizing the cancel behaviour #100

Merged
merged 5 commits into from
Dec 29, 2023

Conversation

weitzman
Copy link
Contributor

@weitzman weitzman commented Dec 3, 2023

I'm coming from the Drush project, the CLI for Drupal. We are switching our prompts to use this package.

One minor gotcha is calling exit() when a user cancels a prompt. Prompts renders a nice Cancelled message and then calls exit(1) which triggers PHP shutdown handling. It would be more convenient if an Exception is thrown here, so implementing projects can react appropriately. A shutdown handler is too restricted in what it can do.

This PR is just a start so I can get feedback on the idea. Maybe we make the this behavior configurable?

Thanks for releasing Prompts as an independant package!

@taylorotwell taylorotwell marked this pull request as draft December 4, 2023 19:27
@taylorotwell
Copy link
Member

Will let @jessarcher take a look. 👍

@jessarcher
Copy link
Member

Hey @weitzman, thanks for the PR! Very cool that you're adding Prompts to Drush!

This PR is just a start so I can get feedback on the idea. Maybe we make the this behavior configurable?

I think making it configurable might be the go, otherwise, all existing projects using Prompts won't exit cleanly on Ctrl+c until they add a handler:

image

We could add a handler in laravel/framework that will cover most projects, but there are others (such as laravel/installer and various third-party non-Laravel projects) that would also be impacted.

I'm thinking a static method on the Prompt class that accepts a callback. E.g:

Prompt::cancelUsing(fn () => throw new Exception);

If no callback has been registered, the default exit(1) behaviour can be retained.

What do you think?

@jessarcher jessarcher self-requested a review December 27, 2023 05:11
@weitzman
Copy link
Contributor Author

Sounds good to me. See latest push. I've confirmed this is working with the Drush PR. I looked at the tests and don't see a way to test this. I'm happy to add a test if anyone can think of one.

@weitzman weitzman marked this pull request as ready for review December 28, 2023 12:52
@taylorotwell taylorotwell marked this pull request as draft December 28, 2023 16:45
@taylorotwell
Copy link
Member

Drafting until @jessarcher takes a look again.

@jessarcher
Copy link
Member

Thanks @weitzman! I've added a quick test.

@jessarcher jessarcher marked this pull request as ready for review December 28, 2023 23:53
@jessarcher jessarcher changed the title Throw a new UserAbortException instead of exit() Allow customizing the cancel behaviour Dec 28, 2023
@taylorotwell taylorotwell merged commit 6f9c9ac into laravel:main Dec 29, 2023
4 checks passed
@weitzman weitzman deleted the user-abort branch January 4, 2024 05:30
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