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

PHP 8.4 | Exit as function: fix incorrect parameter name #15433

Merged

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Aug 15, 2024

Follow up on #13483

As previously reported in #13483 (comment):

The parameter names seem to be incorrect.

It should be $status, not $code.

The RFC explicitly uses that parameter name in the proposal: https://wiki.php.net/rfc/exit-as-function#proposal

It is also the name already used in the manual.

Lastly, the parameter name $status better covers what can be passed: either a status message or a status code.
While $code would read pretty weird when passing a message:

exit(code: 'message');

This commit attempts to fix this.

Includes adding a test for exit/die using a named argument.


P.S.: I hope I got it right, but can't test locally, so I'm opening this in draft to see the CI results and keeping my 🤞.

@jrfnl jrfnl force-pushed the feature/fix-up-13483-exit-as-function-param-name branch 2 times, most recently from a3e58fd to 1c95377 Compare August 15, 2024 20:26
@jrfnl
Copy link
Contributor Author

jrfnl commented Aug 15, 2024

Build passes, moving to ready for review.

@jrfnl jrfnl marked this pull request as ready for review August 15, 2024 21:12
@jrfnl jrfnl requested a review from kocsismate as a code owner August 15, 2024 21:12
@TimWolla TimWolla requested a review from Girgias August 16, 2024 03:58
Follow up on 13483

As previously reported in php#13483 (comment):

> The parameter names seem to be incorrect.
>
> It should be `$status`, not `$code`.
>
> The RFC explicitly uses that parameter name in the proposal: https://wiki.php.net/rfc/exit-as-function#proposal
>
> It is also the name already used in the [manual](https://www.php.net/exit).
>
> Lastly, the parameter name `$status` better covers what can be passed: either a status _message_ or a status _code_.
> While `$code` would read pretty weird when passing a message:
> ```php
> exit(code: 'message');
> ```

This commit attempts to fix this.

Includes adding a test for exit/die using a named argument.
@jrfnl jrfnl force-pushed the feature/fix-up-13483-exit-as-function-param-name branch from 1c95377 to 88ed90e Compare August 16, 2024 04:19
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Change the variable names in the C implementation was not strictly required, but better to be consistent.

Thank you for catching this, I somehow forgot to update the name.

@Girgias Girgias merged commit 4c5767f into php:master Aug 16, 2024
10 checks passed
@jrfnl
Copy link
Contributor Author

jrfnl commented Aug 16, 2024

Change the variable names in the C implementation was not strictly required, but better to be consistent.

Good to know. Keep in mind: I know next to nothing about C, so this really was me guessing what was needed 😂

Thank you for catching this, I somehow forgot to update the name.

Happens to the best of us. I'm just happy I noticed relatively quickly. Would have been a lot more awkward to fix in a few months time.

@jrfnl jrfnl deleted the feature/fix-up-13483-exit-as-function-param-name branch August 16, 2024 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants