Fix GH-8756 : oci_new_descriptor() triggers dynamic property deprecation by adoy · Pull Request #8758 · php/php-src

adoy · 2022-06-12T17:28:20Z

adoy · 2022-06-12T17:50:30Z

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 · 2022-06-12T18:08:56Z

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 · 2022-06-15T12:02:14Z

Good questions. Need to refresh my memory and investigate.

adoy · 2022-06-21T10:55:37Z

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 · 2022-06-21T11:03:14Z

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

Makes sense to me.

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 · 2022-07-04T08:23:09Z

6b6e5f3 should be reverted in this PR

Girgias · 2023-01-25T19:48:34Z

Status of this PR, @adoy @cjbj ?

adoy · 2023-01-25T20:40:08Z

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.

Next step will be to refactor the code to change those error messages and also maybe to remove the useless temp variables.

kocsismate · 2023-02-19T22:11:41Z

 	}

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


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)

kocsismate · 2023-02-19T22:14:36Z

 typedef ub4 oci_phpsized_int;
 #endif

+#define Z_OCILOB_DESCRIPTOR_P(zv) OBJ_PROP_NUM(Z_OBJ_P(zv), 0)


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

kocsismate · 2023-02-19T22:41:59Z

-#[\AllowDynamicProperties]
 class OCILob {
+    /** @var resource */
+    public mixed $descriptor = null;


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.

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);.

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

github-actions Bot added the Extension: oci8 label Jun 12, 2022

adoy mentioned this pull request Jun 21, 2022

Add AllowDynamicProperties on OCILob and OCICollection #8844

Merged

kocsismate reviewed Jul 17, 2022

View reviewed changes

Comment thread ext/oci8/oci8.stub.php Resolved

kocsismate mentioned this pull request Jul 17, 2022

Test oci8 & pdo_oci in CI #8348

Closed

adoy added 4 commits January 25, 2023 20:20

Declare OCILob descriptor property

8155e1b

Declare OCICollection collection property

d509a70

Add type declaration in stub

cf4c315

Remove #[\AllowDynamicProperties]

2c56ef4

adoy force-pushed the bugfix/GH-8756 branch from f5856f4 to 2c56ef4 Compare January 26, 2023 01:22

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 reviewed Feb 19, 2023

View reviewed changes

Conversation

adoy commented Jun 12, 2022

Uh oh!

adoy commented Jun 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmb69 commented Jun 12, 2022

Uh oh!

cjbj commented Jun 15, 2022

Uh oh!

adoy commented Jun 21, 2022

Uh oh!

cmb69 commented Jun 21, 2022

Uh oh!

mvorisek commented Jul 4, 2022

Uh oh!

Uh oh!

Girgias commented Jan 25, 2023

Uh oh!

adoy commented Jan 25, 2023

Uh oh!

kocsismate Feb 19, 2023

Choose a reason for hiding this comment

Uh oh!

kocsismate Feb 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kocsismate Feb 19, 2023

Choose a reason for hiding this comment

Uh oh!

adoy Feb 19, 2023

Choose a reason for hiding this comment

Uh oh!

cjbj Feb 21, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

adoy commented Jun 12, 2022 •

edited

Loading

kocsismate Feb 19, 2023 •

edited

Loading