Skip to content
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

showing warning oci8 install through pecl #7736

Closed
saifulferoz opened this issue Nov 27, 2021 · 13 comments
Closed

showing warning oci8 install through pecl #7736

saifulferoz opened this issue Nov 27, 2021 · 13 comments

Comments

@saifulferoz
Copy link

installing oci8 extension through pecl
sudo pecl -d php_suffix=8.1 install oci8

In function ‘php_oci_column_to_zval’:
/tmp/pear/temp/oci8/oci8.c:1708:72: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘zend_long’ {aka ‘long int’} [-Wformat=]
1708 | php_error_docref(NULL, E_WARNING, "Unable to find LOB descriptor #%d", column->descid->handle);

@cmb69
Copy link
Contributor

cmb69 commented Nov 27, 2021

That's caused by 14f599e, so PECL/oci8 needs to be updated.

However, this bug tracker is about documentation issues, and this one clearly isn't. Not quite sure what to do, since PECL/oci8 doesn't have its own bug tracker.

Anyhow, @cjbj, are you planning to publish a new PECL/oci8 version which is compatible with PHP 8.1?

@cjbj
Copy link
Contributor

cjbj commented Nov 28, 2021

@cmb69 I was hoping to release a PECL OCI8 version that supported several PHP 8 versions, however @nikic's 902d64390e4 from https://wiki.php.net/rfc/deprecate_dynamic_properties has just borked OCI8 tests that use OCILob or OCICollection, which will delay things.

How do I pass #[AllowDynamicProperties] from oci8.stub.php? It seems to be ignored. And is there a way to have the stub generator create version-dependent #ifdef code so that oci8_arginfo.h can be used with, e.g. 8.2 and 8.1?

@cmb69
Copy link
Contributor

cmb69 commented Nov 28, 2021

Do OCILob and OCICollection use dynamic properties? If so, can't that be changed so they have declared properties or use read_property/write_property handlers. If that's not possible, I think that you need to add ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES to the ce_flags. I don't know whether #[AllowDynamicProperties] will be supported for the stubs.

@nikic
Copy link
Member

nikic commented Nov 28, 2021

From a quick look at the code, OCILob only has a $descriptor property, which can be declared. Though in this particular case, $descriptor probably shouldn't be exposed as a property at all -- it looks like a purely internal resource that the user can't interact with. Normally, we'd store something like this in a custom zend_object extension rather than an opaque resource. (Would also help with the migration away from resources.)

Though in any case, I think it's a bit early to think about an 8.2 compatible extension release, given how 8.2 is still a year away.

@cjbj
Copy link
Contributor

cjbj commented Nov 28, 2021

Thanks for jumping in. I need to do a PECL release for 8.1, and I wanted to add the newest performance optimization feature which landed in 8.2.

The class code is all very old (pre dating me) and I haven't reviewed it for a long time. I don't know the answers to @cmb69's questions. Are the ce_flags controls via stubs?

@nikic can you expand on what 'which can be declared' means?

Thanks !
PS I'm out for a few days.

@cmb69
Copy link
Contributor

cmb69 commented Nov 28, 2021

Instead of calling add_property_resource(), you'd call zend_declare_property_ex(), but as Nikita said, this property would better be avoided in favor of a custom zend_object extension (e.g. like https://github.com/php/php-src/blob/php-8.1.0/ext/gd/gd.c#L162-L165).

The ce_flags are a member of zend_class_entry; I don't think you can mark a class to allow dynamic properties via the stubs (yet).

@nikic
Copy link
Member

nikic commented Nov 28, 2021

A sample of the required change for soap, which was using the exact same pattern: c5d6f59 (and still is, for three more resources...)

@cjbj
Copy link
Contributor

cjbj commented Nov 28, 2021

Excellent. I'll take a look when I'm back.

@cmb69 cmb69 transferred this issue from php/doc-en Dec 8, 2021
@cjbj
Copy link
Contributor

cjbj commented Dec 11, 2021

I just pushed OCI8 3.2 to PECL. Give it a whirl. It is for PHP 8.1 only.

@cjbj cjbj closed this as completed Dec 11, 2021
@cmb69
Copy link
Contributor

cmb69 commented Dec 11, 2021

OCI8 3.2 uses OCI_ATTR_LOBPREFETCH_LENGTH and OCI_ATTR_LOBPREFETCH_SIZE which are apparently not defined in instaclient 10.2; should I disable 10.2 builds for Windows?

@cjbj
Copy link
Contributor

cjbj commented Dec 11, 2021

@cmb69 I should have made the build cleaner but, yes, it's time to move forward and stop building with ancient Oracle Client libraries. Thanks for identifying this. Builds with 11.2+ should work.

cmb69 added a commit to php/web-rmtools that referenced this issue Dec 11, 2021
@cmb69
Copy link
Contributor

cmb69 commented Dec 11, 2021

Yes, 11.2+ builds succeed. I've updated rmtools and the PECL build machine, and triggered new builds. Thanks for the swift reply! :)

@cjbj
Copy link
Contributor

cjbj commented Dec 12, 2021

I'll release a OCI8 3.2.1 version which removes support for linking with Oracle Client 10. I don't have Windows so won't touch the config.w32 file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants