Skip to content

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

Merged
Girgias merged 10 commits into
php:masterfrom
Girgias:helpers-common-value-error-empty
Aug 21, 2024
Merged

Use a common function for "cannot be empty" ValueError#15489
Girgias merged 10 commits into
php:masterfrom
Girgias:helpers-common-value-error-empty

Conversation

@Girgias

@GirgiasGirgias commented Aug 18, 2024

Copy link
Copy Markdown
Member

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

Girgias commented Aug 19, 2024

Copy link
Copy Markdown
MemberAuthor

https://github.com/php/php-src/pull/15489/files#diff-040d741086e008767db208d24dc47cfad15873c8d8fb1d994e7fad82e870e302R5813mb_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
Girgiasforce-pushed the helpers-common-value-error-empty branch from 4f11fa2 to 314410fCompareAugust 19, 2024 12:11

@youkidearitaiyoukidearitai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM from mbstring. Thanks.

Comment threadZend/zend_API.c Outdated

Copy link
Copy Markdown
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
Girgiasforce-pushed the helpers-common-value-error-empty branch from c47d283 to 34b57c9CompareAugust 20, 2024 23:14
Comment threadZend/zend_API.c Outdated

Copy link
Copy Markdown
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
Copy Markdown
MemberAuthor

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
Girgiasforce-pushed the helpers-common-value-error-empty branch from 34b57c9 to c823ee4CompareAugust 21, 2024 12:43
@Girgias
Girgias merged commit 5853cdb into php:masterAug 21, 2024
@Girgias
Girgias deleted the helpers-common-value-error-empty branch August 21, 2024 20:12
Sign up for freeto 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

@Girgias@alexdowad@TimWolla@youkidearitai@iluuu1994@devnexen@ndossche@zeriyoshi