Promote some warnings to ValueError in OCI8

[Girgias]

So no test updates currently.
@cjbj could you maybe handle that and confirm this change is okay, this attempts to align part of it with the rest of PHP extensions in PHP 8.0.

[cjbj]

Hi @Girgias

• can you rebase on master?
• it wasn't clear to me why other E_WARNINGS in oci8_interface.c weren't touched.
• how do you want to handle the various test updates? Possibly easier if I merge them after this PR? Note I need to update them so they don't die on the first fatal error before I can verify the PR.

[kocsismate]

it wasn't clear to me why other E_WARNINGS in oci8_interface.c weren't touched.

@cjbj Only those warnings can be converted which are programmatic errors according to the https://wiki.php.net/rfc/engine_warnings RFC.

[Girgias]

Hi @Girgias

* can you rebase on master?

Will do ASAP

* it wasn't clear to me why other E_WARNINGS in oci8_interface.c weren't touched.

As @kocsismate already said this change is more about promoting programming error Warnings to Errors instead of a blanket update, however as the maintainer of the extension I think you can promote other runtime warnings to error but that would be a separate discussion.

* how do you want to handle the various test updates? Possibly easier if I merge them after this PR?  Note I need to update them so they don't die on the first fatal error before I can verify the PR.

Seems like the best way, updating tests is usually the most cumbersome part of this process, otherwise you could always send a PR against this branch on my fork to streamline it into one commit, but either way is fine with me. :)

[cjbj]

As @kocsismate already said this change is more about promoting programming error Warnings to Errors instead of a blanket update, however as the maintainer of the extension I think you can promote other runtime warnings to error but that would be a separate discussion.

Should all the php_error_docref(NULL, E_WARNING, "Unable to find descriptor property"); calls become ValueErrors? I'm happy to follow whatever the 'standard' is.

Also what is your advice for a couple of warnings in https://github.com/php/php-src/blob/master/ext/oci8/oci8.c#L917 and later the warning
https://github.com/php/php-src/blob/master/ext/oci8/oci8.c#L1014. And https://github.com/php/php-src/blob/master/ext/oci8/oci8_lob.c#L627 and a few at https://github.com/php/php-src/blob/master/ext/oci8/oci8_lob.c#L809 ? There are a few others places I could query, too.

(A side comment: RFCs would be better historical records if they documented in writing the outcome(s) of the vote(s), instead of leaving the 1/2 or 2/3 majority calculation to the reader. Funny how I hadn't thought of this step before; it could have been added to https://blogs.oracle.com/opal/the-mysterious-php-rfc-process-and-how-you-can-change-the-web !)

[Girgias]

As @kocsismate already said this change is more about promoting programming error Warnings to Errors instead of a blanket update, however as the maintainer of the extension I think you can promote other runtime warnings to error but that would be a separate discussion.

Should all the php_error_docref(NULL, E_WARNING, "Unable to find descriptor property"); calls become ValueErrors? I'm happy to follow whatever the 'standard' is.

Also what is your advice for a couple of warnings in https://github.com/php/php-src/blob/master/ext/oci8/oci8.c#L917 and later the warning
https://github.com/php/php-src/blob/master/ext/oci8/oci8.c#L1014. And https://github.com/php/php-src/blob/master/ext/oci8/oci8_lob.c#L627 and a few at https://github.com/php/php-src/blob/master/ext/oci8/oci8_lob.c#L809 ? There are a few others places I could query, too.

I would promote all of them other than the charset one as I imagine there is no way to pre-check its value.

The general guideline is that flag type arguments, constant minimal/maximal values and non empty string/arrays are promotable to ValueError as they do not depend on a runtime condition (I/O, user input, etc.) or are condition which are easy to check if they can improve the return type. Hope this clarifies it, but feel free to ask more questions.

(A side comment: RFCs would be better historical records if they documented in writing the outcome(s) of the vote(s), instead of leaving the 1/2 or 2/3 majority calculation to the reader. Funny how I hadn't thought of this step before; it could have been added to https://blogs.oracle.com/opal/the-mysterious-php-rfc-process-and-how-you-can-change-the-web !)

That's an interesting thought :)

[kocsismate]

@cjbj Very nice blog post! It's actually gap filling, and most of the newcomers should read it (especially the 3rd and the 4th list items... :D ). And I pretty much agree with the percentage calculation. In case you don't know about it, you can use https://php-rfc-watch.beberlei.de/ for doing so (+ getting real time updates without page refreshes ^^)

[Girgias]

I forgot to talk about the "Unable to find descriptor property" Warnings, do these correspond to a resource which "disapeared"/uninitialized? Because those are Error using zend_throw_error(NULL, "Message") for unconstructed objects or objects in an invalid state in other extensions.

[cjbj]

@kocsismate:

In case you don't know about it, you can use https://php-rfc-watch.beberlei.de/ for doing so (+ getting real time updates without page refreshes ^^)

Excellent - thanks.

I wish point 18 was followed - life would be easier if all emails contained the relevant RFC URL.

@Girgias :
Thanks for the comments. Let me check on the tests before getting back to you.

[cjbj]

The '2' is off-by-one for $lob->erase() but is valid for the oci_lob_erase(). For $lob->erase(-10) the resulting message from the PR refers to $length where it should be $offset, which is the first arg.

Since oci_lob_erase() is not documented, the simple solution is to subtract one from the parameter number and ignore the wrong message if anyone is actually using oci_lob_erase() incorrectly . Or is there a way to distinguish between the method and function usage?

The same problem occurs for the next zend_argument_value_error() call, and others in this file.