Skip to content

Refactored some ZPP calls in OCI8. #5714

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

Closed
wants to merge 4 commits into from
Closed

Refactored some ZPP calls in OCI8. #5714

wants to merge 4 commits into from

Conversation

jensdenies
Copy link
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.

@jensdenies jensdenies changed the title Refactored some ZPP calls in oci8 Refactored some ZPP calls in oci8. Jun 13, 2020
Copy link
Member

@kocsismate kocsismate left a comment

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

@kocsismate
Copy link
Member

ping @cjbj

@jensdenies jensdenies changed the title Refactored some ZPP calls in oci8. Refactored some ZPP calls in OCI8. Jun 14, 2020
Copy link
Contributor

@cjbj cjbj left a comment

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
Contributor

Do we have oci8 enabled in our CIs?

@kocsismate
Copy link
Member

kocsismate commented Jun 16, 2020

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

@cjbj
Copy link
Contributor

cjbj commented Jun 16, 2020

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

@nikic
Copy link
Member

nikic commented Jun 16, 2020

@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.

@nikic
Copy link
Member

nikic commented Jun 16, 2020

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 free to 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