Skip to content

Use a common function for "cannot be empty" ValueError #15489

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

Merged
merged 10 commits into from
Aug 21, 2024

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Aug 18, 2024

Commits should be reviewed individually.

Some extensions were not using the standard wording, so changed it by using the new helper.
There are also some drive-by refactoring that I noticed while changing the code to use the helper.

@Girgias
Copy link
Member Author

Girgias commented Aug 19, 2024

https://github.com/php/php-src/pull/15489/files#diff-040d741086e008767db208d24dc47cfad15873c8d8fb1d994e7fad82e870e302R5813 mb_str_pad 3rd parameter $pad_string seems can change to zend_argument_cannot_be_empty_error isn't it?

Ah indeed, I missed this
EDIT: Fixed and force pushed.

@Girgias Girgias force-pushed the helpers-common-value-error-empty branch from 4f11fa2 to 314410f Compare August 19, 2024 12:11
Copy link
Contributor

@youkidearitai youkidearitai left a comment

Choose a reason for hiding this comment

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

LGTM from mbstring. Thanks.

@@ -14,4 +14,4 @@ try {
}
?>
--EXPECT--
DOMDocument::loadHTML(): Argument #1 ($source) must not be empty
DOMDocument::loadHTML(): Argument #1 ($source) cannot be empty
Copy link
Member

Choose a reason for hiding this comment

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

Agreed with Tim

@Girgias Girgias force-pushed the helpers-common-value-error-empty branch from c47d283 to 34b57c9 Compare August 20, 2024 23:14
Zend/zend_API.c Outdated
@@ -439,6 +439,11 @@ ZEND_API ZEND_COLD void zend_argument_value_error(uint32_t arg_num, const char *
}
/* }}} */

ZEND_API ZEND_COLD void zend_argument_cannot_be_empty_error(uint32_t arg_num)
Copy link
Member

Choose a reason for hiding this comment

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

The function name now no longer matches the message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in-place in the last commit, which changes the wording from "cannot" to "must not"

@Girgias Girgias force-pushed the helpers-common-value-error-empty branch from 34b57c9 to c823ee4 Compare August 21, 2024 12:43
@Girgias Girgias merged commit 5853cdb into php:master Aug 21, 2024
10 checks passed
@Girgias Girgias deleted the helpers-common-value-error-empty branch August 21, 2024 20:12
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.

8 participants