Skip to content

[PoC] [WIP] throw_legacy_failure declare statement #4549

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

Closed

Conversation

Girgias
Copy link
Member

@Girgias Girgias commented Aug 16, 2019

This is a proof of concept for letting internal/old functions be able to throw an Exception in case of failure instead of using the legacy behaviour of returning null or false.
This behaviour is controlled by the user on a per-file basis, as with strict_types, using the following declare statement: declare(throw_legacy_failure=1);

This declare statement is not meant to throw an Exception on valid false/null return values, e.g. strpos() returning false when the needle was not found in the string.

A couple of question still remain:

  • The name of the declare statement
  • Should there be general FailureException or more specialised exceptions per extension.
  • The functions which would benefit from this feature.

@deleugpn
Copy link

Perhaps following the JSON change on 7.3 and calling it throws_on_error=1 might be worth considering.

@@ -6419,7 +6419,7 @@ PHP_FUNCTION(array_combine)
num_values = zend_hash_num_elements(values);

if (num_keys != num_values) {
php_error_docref(NULL, E_WARNING, "Both parameters should have an equal number of elements");
php_exception_or_error_docref(NULL, E_WARNING, "Both parameters should have an equal number of elements");
Copy link
Member

Choose a reason for hiding this comment

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

I believe it's okay to convert this kind of error condition to an Error unconditionally (I've been doing this occasionally). Basically any error condition that indicates a programming error and no reasonable code should handle locally.

For this kind of directive, I think the area to focus on would be things like I/O errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, what is the best way to convert these? Using zend_throw_exception(zend_ce_error_exception, message, E_ERROR); directly instead or is there a predefined function/macro for throwing ErrorExceptions?

Copy link
Contributor

@strongholdmedia strongholdmedia Aug 17, 2019

Choose a reason for hiding this comment

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

I personally think these are not strictly programmer errors, and could save us from an awful lot of boilerplate pre-validation code if e.g. the arrays are populated from some file or even a HTTP request.
But surely, anything catchable is of help.

@krakjoe krakjoe added the RFC label Aug 19, 2019
@franzliedke
Copy link

I agree with @deleugpn that something along the lines of throws_on_error is far more self-explanatory than the throw_legacy_failure proposal.

👍 for the general idea, in any case. Thank you!

@Girgias
Copy link
Member Author

Girgias commented Sep 25, 2019

I agree with @deleugpn that something along the lines of throws_on_error is far more self-explanatory than the throw_legacy_failure proposal.

👍 for the general idea, in any case. Thank you!

I'll be changing it to that when I'll start working on that again.
I'm currently focused (when I got time again) on changing programming error Warnings into proper Errors (/ValueError/TypeError) to see what the scope of this should be. :)

@Girgias
Copy link
Member Author

Girgias commented Sep 3, 2020

Closes this: an improved version is located at Girgias#2

@Girgias Girgias closed this Sep 3, 2020
@Girgias Girgias deleted the throw-exception-instead-return-false branch September 3, 2020 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants