Skip to content

Refactored some ZPP calls in OCI8. - #5714

Closed
jensdenies wants to merge 4 commits into
php:masterfrom
jensdenies:zpp-fix-oci
Closed

Refactored some ZPP calls in OCI8.#5714
jensdenies wants to merge 4 commits into
php:masterfrom
jensdenies:zpp-fix-oci

Conversation

@jensdenies

Copy link
Copy Markdown
Contributor

Thanks to the comments of @kocsismate on #5701 i refactored some ZPP calls to the "zend_parse_method_parameters" variant, which ensures ZPP is always called on method/function invocation.

I also moved the "old_oci_close_semantics" check beneath the ZPP check.

@jensdeniesjensdenies changed the title Refactored some ZPP calls in oci8Refactored some ZPP calls in oci8.Jun 13, 2020

@kocsismatekocsismate left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for your work! All in all, the PR looks good to me as-is. I only had a few small nits, but I'm not 100% sure it's worth to address them right now. So let's now wait for other reviews.

if (ZEND_NUM_ARGS() == 2) {
data_len = MIN((zend_long) data_len, write_len);
}
if (getThis() && ZEND_NUM_ARGS() == 2) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These consecutive checks everywhere could be optimized a tiny little bit by either:

  • using else if clauses
  • extracting getThis() in an outer condition so that this function is not called multiple times

However, all the ZEND_NUM_ARGS() == ... checks indicate that the default value handling is not right - which is something we want to fix later. That's why I'm also ok with leaving these conditions as you have them currently.

Comment threadext/oci8/oci8_interface.c Outdated
Comment threadext/oci8/oci8_interface.c Outdated
@kocsismate

Copy link
Copy Markdown
Member

ping @cjbj

@jensdeniesjensdenies changed the title Refactored some ZPP calls in oci8.Refactored some ZPP calls in OCI8.Jun 14, 2020

@cjbjcjbj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the tests pass, LGTM

@carusogabriel

Copy link
Copy Markdown
Contributor

Do we have oci8 enabled in our CIs?

@kocsismate

kocsismate commented Jun 16, 2020

Copy link
Copy Markdown
Member

@carusogabriel Nope. :( I believe Jens run the tests locally. @jensdenies, so the tests pass, right?

@cjbj

cjbj commented Jun 16, 2020

Copy link
Copy Markdown
Contributor

Traditionally only gcov ran Oracle DB. But I guess http://gcov.php.net/ is obsolete?

@nikic

Copy link
Copy Markdown
Member

@cjbj Yes, gcov is no longer used. To test oci, we'd need to have it on either Travis, Azure Pipelines or AppVeyor. It does look like installing it on travis is possible: https://github.com/Vincit/travis-oracledb-xe No idea how compatible that is with licensing.

@nikicnikic mentioned this pull request Jun 16, 2020
@nikic

Copy link
Copy Markdown
Member

I've opened php/php-tasks#5 to track testing oci8 on CI, though from a cursory look, it doesn't look like a simple thing right now.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jensdenies@kocsismate@carusogabriel@cjbj@nikic