Skip to content

Zend/zend_fibers: change return value to zend_result #10622

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Zend/zend_fibers: change return value to zend_result #10622

wants to merge 1 commit into from

Conversation

MaxKellermann
Copy link
Contributor

According to @nikic:

The current guideline for use of bool and zend_result in php-src is
that bool is an appropriate return value for "is" or "has" style
functions, which return a yes/no answer. zend_result is an
appropriate return value for functions that perform some operation
that may succeed or fail.

devnexen
devnexen approved these changes Feb 19, 2023
View reviewed changes
Copy link
Member

@devnexen devnexen left a comment

Choose a reason for hiding this comment

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

makes sense to me

@devnexen devnexen requested a review from trowski February 19, 2023 08:39
MaxKellermann added a commit to CM4all/php-src that referenced this pull request Feb 22, 2023
@MaxKellermann
sapi/fpm: change lots of return types to zend_result
f31be66 
More of php#10622
@MaxKellermann MaxKellermann mentioned this pull request Feb 22, 2023
Closed
@devnexen
Copy link
Member

Since those are apis, might deserve an entry into UPGRADING.INTERNALS

@MaxKellermann
Zend/zend_fibers: change return value to zend_result
5856602 
According to @nikic:

> The current guideline for use of bool and zend_result in php-src is
> that bool is an appropriate return value for "is" or "has" style
> functions, which return a yes/no answer. zend_result is an
> appropriate return value for functions that perform some operation
> that may succeed or fail.
@MaxKellermann
Copy link
Contributor Author

Since those are apis, might deserve an entry into UPGRADING.INTERNALS

Added. I've made this a list (with just one item currently), because I believe there will be more soon.

@devnexen devnexen closed this in 371ae12 Feb 26, 2023
@MaxKellermann MaxKellermann deleted the fibers_zend_result branch February 26, 2023 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devnexen devnexen devnexen approved these changes

@trowski trowski Awaiting requested review from trowski

Assignees
No one assigned
Labels
Category: Engine Category: Fibers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MaxKellermann @devnexen