Skip to content

Fix GH-8756 : oci_new_descriptor() triggers dynamic property deprecation #8758

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Fix GH-8756 : oci_new_descriptor() triggers dynamic property deprecation #8758

wants to merge 5 commits into from

Conversation

adoy
Copy link
Member

@adoy adoy commented Jun 12, 2022

Fix GH-8756

@adoy
Copy link
Member Author

adoy commented Jun 12, 2022 •
edited
Loading

I don't know much about oci8 extension. In current stable PHP versions OCILob and OCICollection both use a dynamic property (descriptor and collection) whose values are resources. In order to clean this I have few questions for people who are more familiar with this extension than I am.

Are those properties usable ? I mean does it make sense to expose this ? From what I see the extension always use the classes OCILob and OCICollection as parameter types. So I think it would make more sense to hide thoses properties.

If making those resources visible make sense, it's currently possible to unset($ocilob->descriptor) or unset($ociCollection->collection). Does it make sense to make this property read write so that someone can overwrite it ? I think making it readonly would be better but maybe there is a use case I'm not aware of.

@cmb69
Copy link
Member

cmb69 commented Jun 12, 2022

Very good questions, @adoy! If we won't get good answers, we may consider to mark these classes with the AllowDynamicProperties attribute, and postpone the changes.

cc @cjbj

@cjbj
Copy link
Contributor

cjbj commented Jun 15, 2022

Good questions. Need to refresh my memory and investigate.

@adoy
Copy link
Member Author

adoy commented Jun 21, 2022

Hi @cjbj,

Just a quick follow up to make sure you didn't forget about this.

@cmb69 What do you think about adding the AllowDynamicProperties annotation to close the issue, and we keep this PR open to hopefully release a cleaner solution as part of 8.2 ?

@cmb69
Copy link
Member

cmb69 commented Jun 21, 2022

What do you think about adding the AllowDynamicProperties annotation to close the issue, […]

Makes sense to me.

adoy added a commit to adoy/php-src that referenced this pull request Jun 21, 2022
@adoy
Add AllowDynamicProperties on OCILob and OCICollection
885aa14 
Fix phpGH-8756 : oci_new_descriptor() triggers dynamic property
deprecation.

This fix should be temporary. At some point we should either define
those properties or just hide them since they should probably not be
used.

Better fix is here : php#8758 but
waiting feedback from Oracle team before going ahead.
@adoy adoy mentioned this pull request Jun 21, 2022
Merged
adoy added a commit to adoy/php-src that referenced this pull request Jun 22, 2022
@adoy
Add AllowDynamicProperties on OCILob and OCICollection
6b6e5f3 
Fix phpGH-8756 : oci_new_descriptor() triggers dynamic property
deprecation.

This fix should be temporary. At some point we should either define
those properties or just hide them since they should probably not be
used.

Better fix is here : php#8758 but
waiting feedback from Oracle team before going ahead.
@mvorisek
Copy link
Contributor

mvorisek commented Jul 4, 2022

6b6e5f3 should be reverted in this PR

kocsismate
kocsismate reviewed Jul 17, 2022
View reviewed changes
@kocsismate kocsismate mentioned this pull request Jul 17, 2022
Closed
@Girgias
Copy link
Member

Girgias commented Jan 25, 2023

Status of this PR, @adoy @cjbj ?

@adoy
Copy link
Member Author

adoy commented Jan 25, 2023

Status of this PR, @adoy @cjbj ?

@Girgias Thanks for bumping this thread. I'm still waiting for Oracle (@cjbj) to confirm if we can make those props private.

@adoy
Reuse old errors to ensure that tests are still working
1069dd6 
Next step will be to refactor the code to change those error messages
and also maybe to remove the useless temp variables.
kocsismate
kocsismate reviewed Feb 19, 2023
View reviewed changes
ext/oci8/oci8_interface.c
@@ -233,7 +236,8 @@ PHP_FUNCTION(oci_free_descriptor)
RETURN_THROWS();
}

if ((tmp = zend_hash_str_find(Z_OBJPROP_P(z_descriptor), "descriptor", sizeof("descriptor")-1)) == NULL) {
tmp = Z_OCILOB_DESCRIPTOR_P(z_descriptor);
Copy link
Member

Choose a reason for hiding this comment

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

Based on php_soap_deref added in https://github.com/php/php-src/commit/50484b59cd4ff5a1d32ce520fc7a7181b25ed0d9, I'd guess that these properties could benefit from dereferencing (pls double check it)

ext/oci8/oci8_statement.c
@@ -42,6 +42,9 @@ typedef ub8 oci_phpsized_int;
typedef ub4 oci_phpsized_int;
#endif

#define Z_OCILOB_DESCRIPTOR_P(zv) OBJ_PROP_NUM(Z_OBJ_P(zv), 0)
Copy link
Member

@kocsismate kocsismate Feb 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

IMO these macros should be added to the php_oci8_int.h header instead if they has to be shared internally.

kocsismate
kocsismate reviewed Feb 19, 2023
View reviewed changes
ext/oci8/oci8.stub.php
class OCILob {
/** @var resource */
public mixed $descriptor = null;
Copy link
Member

Choose a reason for hiding this comment

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

as far as I managed to understand, the property is private indeed as you guessed... Would be nice to double check with someone else and change the visibility accordingly. If someone later complains, we can still make it public again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep that's why I asked @cjbj for a confirmation. I'm not an OCI user and looking at some tests I see stuff like unset($clob->descriptor);.

Copy link
Contributor

Choose a reason for hiding this comment

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

I won't have time to look at this until next month :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjbj cjbj cjbj left review comments

@kocsismate kocsismate kocsismate left review comments

Assignees
No one assigned
Labels
Extension: oci8
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ext-oci8: oci_new_descriptor() triggers dynamic property deprecation
6 participants
@adoy @cmb69 @cjbj @mvorisek @Girgias @kocsismate