Some cleanup in OCI8 extension for PHP 8 #5603

Girgias · 2020-05-20T14:27:44Z

While working on #5526 I saw there was some version handling which stems from the PECL version.

Except if I'm mistaken, this shouldn't be needed any more as it lies within php-src

KalleZ · 2020-05-20T18:58:09Z

This is probably a no-go because of how Oracle manages the extension, they support a wide range of unsupported PHP versions and the extension is dual released with PECL.

Maybe @cjbj can comment further on this.

Girgias · 2020-05-20T20:14:41Z

This is probably a no-go because of how Oracle manages the extension, they support a wide range of unsupported PHP versions and the extension is dual released with PECL.

Maybe @cjbj can comment further on this.

Ah so it's still dual released? If it's indeed true this is a no go I'll close this, however I'd hope it would be possible to fix the potential [-Wundef] warnings which may arise in theory, just to be pedantic.

KalleZ · 2020-05-20T20:20:23Z

While its been sometime since its last release, I do think it is still the case. the [-Wundef] fixes should be a non issue to commit tho. I think zip and oci8 (potentially PDO_OCI8 too, tho I don't think Oracle ever officially took it over) are the only two extensions in the core that have "regular" PECL releases, but I could be wrong, better wait for @cjbj to have an opinion regarding legacy support before closing :)

cjbj · 2020-05-21T05:36:51Z

OCI8 does land on PECL so it needs to compile with a range of PHP 7+ versions, and give graceful failure messages when people try to install with older versions. The #if -> #ifdef change should be fine.

PDO_OCI is under our wing, but doesn't have a separate PECL release.

camporter · 2020-05-21T18:23:26Z

Sorry, @cjbj somewhat related, but I had tried to work on the php-based stubbing of the OCI8 extension in anticipation of PHP 8, but I'm not sure if that will be backwards compatible to do. Do you think it will be possible to add stubs for oci8 or no?

nikic · 2020-05-21T20:34:06Z

Just as a general policy point: All extensions bundled with php-src generally only need to be compatible with master. As a courtesy, we do allow retaining some compatibility code, but we do not actively maintain it ourselves. For example, the current code of ext/oci8 is not compatible with older versions, because it uses some PHP 8 APIs like RETURN_THROWS(). The responsibility of making this code compatible with older versions lies with the PECL maintainer, and they will have to figure out how they want to handle it (either with more #if's, or maybe by cutting a new major extension version for PHP 8, dunno).

So to answer your question @camporter: Yes, you can add stubs and I'd say this is even a de-facto requirement for PHP 8. If desired, the PECL maintainer can then restore compatibility with older versions by defining necessary polyfill macros and/or adding #if's to the stubs that select signatures based on PHP version. There is no fundamental incompatibility of stubs with older PHP versions (as we commit the generated files, and those files are mostly backwards-compatible). Just requires the usual work of maintaining extensions across versions :)

cjbj · 2020-05-22T05:48:20Z

I appreciate the value-add that you are all putting into OCI8. The number of unread emails in my PHP folders is a reminder that I don't have the luxury of working with just one language anymore!

Girgias · 2020-07-20T16:43:14Z

@cjbj Is there an easy way to compile OCI8 locally? As I'm pretty sure the callable ZPP change here would (or at least should) affect tests. Or you can just pull this branch locally to test and merge it?

cjbj · 2020-07-21T05:27:55Z

ext/oci8/oci8_interface.c

@@ -152,18 +128,10 @@ PHP_FUNCTION(oci_define_by_name)
 	/* if (zend_hash_add(statement->defines, name, name_len, define, sizeof(php_oci_define), (void **)&tmp_define) == SUCCESS) { */
 	zvtmp = zend_string_init(name, name_len, 0);
 	if ((define = zend_hash_add_new_ptr(statement->defines, zvtmp, define)) != NULL) {
-#if PHP_VERSION_ID < 70300
 		zend_string_release(zvtmp);


Was keeping the non *_ex() calls deliberate?

Actually no, let me change those back

cjbj · 2020-07-21T05:52:20Z

@cjbj Is there an easy way to compile OCI8 locally? As I'm pretty sure the callable ZPP change here would (or at least should) affect tests. Or you can just pull this branch locally to test and merge it?

The callable changes are fine in the functional (manual) testing I ran. Yes, the error handling now gives a fatal error instead of a warning, but that's OK to me.

I wasn't sure why some of the macro cleanup changes kept the old code branch. Was that deliberate?

Girgias · 2020-07-21T11:09:41Z

@cjbj Is there an easy way to compile OCI8 locally? As I'm pretty sure the callable ZPP change here would (or at least should) affect tests. Or you can just pull this branch locally to test and merge it?

The callable changes are fine in the functional (manual) testing I ran. Yes, the error handling now gives a fatal error instead of a warning, but that's OK to me.

Good to know :)

I wasn't sure why some of the macro cleanup changes kept the old code branch. Was that deliberate?

I don't remember if I did that on purpose or just my brain didn't do the correct thing, so I think I fixed all relevant issues.

ext/oci8/oci8_interface.c

cjbj · 2020-07-21T12:12:25Z

I'll review it again tomorrow. Thanks!

cjbj · 2020-07-22T00:15:38Z

ext/oci8/oci8_statement.c

-#if PHP_VERSION_ID < 70300
 		zend_string_release(zvtmp);
-#else
-		zend_string_release_ex(zvtmp, 0);
-#endif


Should the new branch be retained?

cjbj · 2020-07-22T00:15:49Z

ext/oci8/oci8_statement.c

-#if PHP_VERSION_ID < 70300
 	zend_string_release(zvtmp);
-#else
-	zend_string_release_ex(zvtmp, 0);
-#endif


Should the new branch be retained?

Yes, I missed those two when I did the other file.
Hopefully I fixed them all now. :)

cjbj · 2020-07-22T07:38:23Z

I've merged this to master - thank you again.

The https://qa.php.net/pulls system doesn't seem to let me close the PR; it's silently doing nothing. Maybe I don't have access.

remicollet · 2020-07-22T07:53:49Z

@cjbj closing as merged

cjbj · 2020-07-22T08:02:00Z

@remicollet thank you.

cjbj · 2020-07-22T08:02:14Z

@remicollet thank you.

Girgias mentioned this pull request May 20, 2020

Enable -Wundef compiler warning #5526

Closed

87 tasks

Girgias force-pushed the oci8-cleanup-for-php-8 branch from ee9ca14 to 20992de Compare July 20, 2020 16:35

cjbj reviewed Jul 21, 2020

View reviewed changes

Girgias force-pushed the oci8-cleanup-for-php-8 branch from 20992de to d9e378e Compare July 21, 2020 11:08

nikic reviewed Jul 21, 2020

View reviewed changes

ext/oci8/oci8_interface.c Outdated Show resolved Hide resolved

Girgias force-pushed the oci8-cleanup-for-php-8 branch from d9e378e to 97d5f9f Compare July 21, 2020 11:14

cjbj reviewed Jul 22, 2020

View reviewed changes

Girgias added 2 commits July 22, 2020 02:10

Some cleanup in OCI8 extension for PHP 8

7d9e760

Use ZPP callable check for oci_register_taf_callback()

f5d6189

Girgias force-pushed the oci8-cleanup-for-php-8 branch from 97d5f9f to f5d6189 Compare July 22, 2020 01:11

remicollet closed this Jul 22, 2020

Girgias deleted the oci8-cleanup-for-php-8 branch July 22, 2020 10:07

Some cleanup in OCI8 extension for PHP 8 #5603

Some cleanup in OCI8 extension for PHP 8 #5603

Uh oh!

Conversation

Girgias commented May 20, 2020

Uh oh!

KalleZ commented May 20, 2020

Uh oh!

Girgias commented May 20, 2020

Uh oh!

KalleZ commented May 20, 2020

Uh oh!

cjbj commented May 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

camporter commented May 21, 2020

Uh oh!

nikic commented May 21, 2020

Uh oh!

cjbj commented May 22, 2020

Uh oh!

Girgias commented Jul 20, 2020

Uh oh!

cjbj Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

Girgias Jul 21, 2020

Choose a reason for hiding this comment

Uh oh!

cjbj commented Jul 21, 2020

Uh oh!

Girgias commented Jul 21, 2020

Uh oh!

Uh oh!

cjbj commented Jul 21, 2020

Uh oh!

cjbj Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

cjbj Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

Girgias Jul 22, 2020

Choose a reason for hiding this comment

Uh oh!

cjbj commented Jul 22, 2020

Uh oh!

remicollet commented Jul 22, 2020

Uh oh!

cjbj commented Jul 22, 2020

Uh oh!

cjbj commented Jul 22, 2020

Uh oh!

Uh oh!

cjbj commented May 21, 2020 •

edited

Loading