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

[12.x] add type declarations for Console Events #53947

Merged
merged 3 commits into from
Dec 18, 2024

Conversation

browner12
Copy link
Contributor

the added types match the existing constructor docblocks.

not sure what the current sentiment on stricter typing is from the core team, but thought I'd throw this out there. these events are a good place to start as their construction should basically only be internal.

I checked all the calling code, and this may have already uncovered a bug. the CommandStarting claims to accept a string $command parameter, but the calling code possibly sends null. two ways to fix this would be to accept string|null, or to have the calling code null coalesce and empty string (?? '').

if accepted, I'll update all the other events as well.

the added types match the existing constructor docblocks
@shaedrich
Copy link
Contributor

I predict, this will be rejected due to backwards compatibility

@taylorotwell
Copy link
Member

@browner12 under what situation does CommandStarting currently receive null?

@browner12
Copy link
Contributor Author

@taylorotwell I don't know if it actually happens, but in

https://github.com/laravel/framework/blob/11.x/src/Illuminate/Foundation/Console/Kernel.php#L166

the getCommand() can return null, and the getName() can also return null. my guess is it never actually happens, but based on their declared return types they are nullable.

@taylorotwell
Copy link
Member

taylorotwell commented Dec 18, 2024

Hmm, ok, I guess we should coalesce in the callers then so the event itself only receives strings.

both the `CommandStarting` and `CommandFinished` require a string to be passed as the first argument, so we need to account for nullable scenarios.
@browner12
Copy link
Contributor Author

@taylorotwell code has been updated. I realized the CommandFinished also required a string.

Very weird on symfonys part, how could you have a command event without a command?

@taylorotwell taylorotwell merged commit 8383bea into laravel:master Dec 18, 2024
38 checks passed
@browner12 browner12 deleted the AB-console-event-types branch December 18, 2024 19:26
@crynobone
Copy link
Member

Very weird on symfonys part, how could you have a command event without a command?

php artisan --version

This could be one example.

@browner12
Copy link
Contributor Author

Very weird on symfonys part, how could you have a command event without a command?

php artisan --version

This could be one example.

if that's not a command, then why is it firing off a command event?

I'm sure they had good reason, just seems odd from the outside looking in.

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.

4 participants