Skip to content

[ExpressionLanguage][Node][BinaryNode] Process division by zero #34831

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

[ExpressionLanguage][Node][BinaryNode] Process division by zero #34831

wants to merge 1 commit into from

Conversation

tigr1991
Copy link
Contributor

@tigr1991 tigr1991 commented Dec 5, 2019

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

There is the same PR to master (as new feature)
Which way is more correct?

To be able to catch the error in expression like 1 / 0

Before PR:

try {
    1 / 0;
} catch (\Throwable $e) {
    // It won't be caught
    // PHP Warning:  Division by zero in...
}

try {
    1 % 0;
} catch (\Throwable $e) {
    // It will be caught
    // \DivisionByZeroError with message `Modulo by zero`
}

After PR:

try {
    1 / 0;
} catch (\Throwable $e) {
    // It will be caught
    // \DivisionByZeroError with message `Division by zero`
}

@tigr1991 tigr1991 force-pushed the language_expression/fix_exceptions branch from 2e14193 to 318b9eb Compare December 5, 2019 13:46
@xabbuh
Copy link
Member

xabbuh commented Dec 5, 2019

This looks good to me. Applies to the 3.4 branch too, right?

@tigr1991 tigr1991 mentioned this pull request Dec 5, 2019
Merged
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Dec 5, 2019
@tigr1991
Copy link
Contributor Author

tigr1991 commented Dec 6, 2019 •
edited
Loading

Is it 3.4 Milestone?
May be it is 4.4 Milestone?

What should I do with
#34842
#34814
?

@nicolas-grekas
Copy link
Member

Closing in favor of #34842: we merge lower branches into upper ones periodically so they'll get the fixes too (it's written in the PR template ;) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug ExpressionLanguage Status: Needs Review
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
4 participants
@tigr1991 @xabbuh @nicolas-grekas @carsonbot