Created stubs for the OCI8 extension. #5701

jensdenies · 2020-06-11T21:47:02Z

Hi,

I noticed the OCI8 extension didn't have stubs yet, that's why i decided to try to add them. I went over the implementation of the functions and methods to determine the types, but it would be nice if someone could double check them.

I've ran the tests and about 78% passed, the ones that failed didn't seem to be related to my changes. I could be wrong, however, so it would be nice if someone could check whether the functions and classes still work as expected.

If i recall correctly, someone mentioned the addition of the "false" return type. Did the "false" return type make it into PHP8? If so, should i add return types for the functions which return false and something else?

kocsismate

Very nice (and courageous) work! :) I only have a cursory look yet, so I'll continue later.

ext/oci8/oci8.stub.php

ext/oci8/oci8.c

ext/oci8/oci8.stub.php

kocsismate · 2020-06-11T23:24:15Z

@jensdenies Yes, false is a valid union type (together with at least one more type). For more information, have a look at https://wiki.php.net/rfc/union_types_v2

I believe not many people can run these tests so it would be appreciated if you could share some of the failing ones. These are not even run in CI.

ext/oci8/oci8.stub.php

jensdenies · 2020-06-12T18:38:46Z

@kocsismate Thanks for the kind words and your review. I've processed some of your feedback and replied to some of your comments.

Some failing tests include:

ext/oci8/tests/old_oci_close1.phpt (fatal TypeError instead of expected warning).
ext/oci8/tests/define1.phpt (fatal TypeError instead of expected warning).
ext/oci8/tests/bug42496_1.phpt ( ** ERROR: process timed out **)
ext/oci8/tests/array_bind_float.phpt (expected: 2.5658, actual: 2.5658000000000003)

These are just a few.

kocsismate

Another batch of comments :)

ext/oci8/oci8.stub.php

jensdenies · 2020-06-16T22:33:22Z

@kocsismate I've fixed the things you requested. I also updated the oci8_statement.c file to use Z_PARAM_STR_OR_LONG, could you please have a look to check whether it is okay?

~~I have converted this to a draft PR because i did not run the tests with the newest changes yet.~~

kocsismate

This is the penultimate review batch :)

ext/oci8/oci8.stub.php

ext/oci8/oci8_statement.c

jensdenies · 2020-06-18T20:04:04Z

Thanks @kocsismate, i committed the fixes.

kocsismate

My part is done with the review. If the current issues get fixed, we can hand it over to @nikic :)

ext/oci8/oci8_interface.c

ext/oci8/oci8.stub.php

jensdenies · 2020-06-22T19:57:02Z

Thanks again @kocsismate. I committed the last fixes @nikic.

nikic

I did not check this in detail, but looks sensible :)

kocsismate

I found a couple of smaller issues

kocsismate · 2020-06-23T15:03:30Z

ext/oci8/oci8.stub.php

+
+function oci_lob_is_equal(OCI_Lob $lob_descriptor_first, OCI_Lob $lob_descriptor_second): bool {}
+
+function oci_lob_export(OCI_Lob $lob_descriptor, $path, int $start = UNKNOWN, int $length = UNKNOWN): bool {}


the string type of the $path parameter is missing

kocsismate · 2020-06-23T15:03:38Z

ext/oci8/oci8.stub.php

+ * @alias oci_lob_export
+ * @deprecated
+ */
+function ociwritelobtofile(OCI_Lob $lob_descriptor, $path, int $start = UNKNOWN, int $length = UNKNOWN): bool {}


same as before

jensdenies · 2020-06-24T21:13:11Z

@kocsismate Fixed, thanks.

kocsismate

After a last full round of review, I found a few minor issues. When fixing them, please rebase your branch to current master (so that the stub file will have a hash + your ZPP fixes are there), and run the tests. If they pass, then I'll merge the PR :)

ext/oci8/oci8.stub.php

jensdenies · 2020-07-02T19:52:27Z

@kocsismate Hi, i applied the requested changes and i think i've rebased master. Did i do it correct? I've ran the tests and not all of them pass, but i dont think it's related to my changes,.

kocsismate · 2020-07-03T07:03:08Z

@jensdenies The fixes look good! However, I don't think you rebased to current master, because there should be a hash in the beginning of the arginfo file now. That possibly means you have to fetch current master from the upstream remote:

git pull upstream master --rebase

P.S. You don't need --rebase if you have configured git to use rebase by default

If you haven't done that before, you can set up the upstream remote like below:

git remote add upstream [email protected]:php/php-src.git

…ated function entries.

jensdenies · 2020-07-03T19:08:47Z

Thanks @kocsismate, i think it worked. There's a hash in the file now.

kocsismate · 2020-07-04T09:01:07Z

@jensdenies I've just merged your work! Congrats! As a follow-up, the oci_internal_debug() method should be removed. You can also try to fix the UNKNOWN defaults, if you're up to a new challenge. ^^

jensdenies · 2020-07-04T09:03:47Z

Thanks @kocsismate for all your help! I noticed the oci_interna_debug function and emailed @cjbj about it. So i will make a new PR to remove that function, i’ll also make a PR to deprecate ocifetchinto, since i forgot it while deprecating the other aliases.

carusogabriel added the Stubs label Jun 11, 2020

kocsismate reviewed Jun 11, 2020

View reviewed changes

kocsismate reviewed Jun 12, 2020

View reviewed changes

jensdenies mentioned this pull request Jun 13, 2020

Refactored some ZPP calls in OCI8. #5714

Closed

kocsismate reviewed Jun 15, 2020

View reviewed changes

jensdenies marked this pull request as draft June 16, 2020 22:28

jensdenies requested a review from kocsismate June 16, 2020 22:33

jensdenies marked this pull request as ready for review June 17, 2020 07:05

kocsismate reviewed Jun 17, 2020

View reviewed changes

kocsismate reviewed Jun 22, 2020

View reviewed changes

jensdenies requested a review from nikic June 22, 2020 19:57

nikic approved these changes Jun 23, 2020

View reviewed changes

kocsismate reviewed Jun 23, 2020

View reviewed changes

jensdenies requested a review from kocsismate June 24, 2020 21:14

kocsismate reviewed Jun 28, 2020

View reviewed changes

jensdenies added 7 commits July 3, 2020 21:01

Added stubs for oci8.

3aeea94

Replaces arginfo names.

7392bcc

Removed return types from oci8 class stubs.

14898d6

Added stubs for deprecated functions, added alias docblocks and gener…

18a6fc0

…ated function entries.

Fixed some default values.

6df7bd5

Fixed some stubs and some adjustments to oci8_statement.

59b4119

Various changes.

c493b2e

jensdenies added 4 commits July 3, 2020 21:01

Fix.

ee59439

Fixes.

fcabd58

Fixes.

9e5ca87

Fixes.

7c81f38

php-pulls closed this in 58f51f8 Jul 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Created stubs for the OCI8 extension. #5701

Created stubs for the OCI8 extension. #5701

jensdenies commented Jun 11, 2020

kocsismate left a comment

kocsismate commented Jun 11, 2020

jensdenies commented Jun 12, 2020

kocsismate left a comment

jensdenies commented Jun 16, 2020 •

edited

Loading

kocsismate left a comment

jensdenies commented Jun 18, 2020

kocsismate left a comment

jensdenies commented Jun 22, 2020

nikic left a comment

kocsismate left a comment

kocsismate Jun 23, 2020

kocsismate Jun 23, 2020

jensdenies commented Jun 24, 2020

kocsismate left a comment

jensdenies commented Jul 2, 2020

kocsismate commented Jul 3, 2020 •

edited

Loading

jensdenies commented Jul 3, 2020

kocsismate commented Jul 4, 2020

jensdenies commented Jul 4, 2020


		function oci_lob_is_equal(OCI_Lob $lob_descriptor_first, OCI_Lob $lob_descriptor_second): bool {}

		function oci_lob_export(OCI_Lob $lob_descriptor, $path, int $start = UNKNOWN, int $length = UNKNOWN): bool {}

Created stubs for the OCI8 extension. #5701

Created stubs for the OCI8 extension. #5701

Conversation

jensdenies commented Jun 11, 2020

kocsismate left a comment

Choose a reason for hiding this comment

kocsismate commented Jun 11, 2020

jensdenies commented Jun 12, 2020

kocsismate left a comment

Choose a reason for hiding this comment

jensdenies commented Jun 16, 2020 • edited Loading

kocsismate left a comment

Choose a reason for hiding this comment

jensdenies commented Jun 18, 2020

kocsismate left a comment

Choose a reason for hiding this comment

jensdenies commented Jun 22, 2020

nikic left a comment

Choose a reason for hiding this comment

kocsismate left a comment

Choose a reason for hiding this comment

kocsismate Jun 23, 2020

Choose a reason for hiding this comment

kocsismate Jun 23, 2020

Choose a reason for hiding this comment

jensdenies commented Jun 24, 2020

kocsismate left a comment

Choose a reason for hiding this comment

jensdenies commented Jul 2, 2020

kocsismate commented Jul 3, 2020 • edited Loading

jensdenies commented Jul 3, 2020

kocsismate commented Jul 4, 2020

jensdenies commented Jul 4, 2020

jensdenies commented Jun 16, 2020 •

edited

Loading

kocsismate commented Jul 3, 2020 •

edited

Loading