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

Use camelCase method names in OCICollection and OCILob #7405

Merged
merged 3 commits into from
Aug 30, 2021

Conversation

kocsismate
Copy link
Member

@kocsismate kocsismate commented Aug 26, 2021

The doc has been using assignElem() and there is also a OCICollection::getElem() method (which was accidentally renamed from getelem() when arginfo got generated from the stubs), so it would be nice to keep using camelCase all over OCICollection.

@nikic
Copy link
Member

nikic commented Aug 26, 2021

Looks like historically these were lowercase:

PHP_FALIAS(assignelem, oci_collection_element_assign, arginfo_oci_collection_element_assign_method)

But I agree with the change to camel-case, especially as it is backwards-compatible (method names are case-insensitive).

I'm wondering whether we should change the casing in OCILob at the same time though, as it also have methods like writetemporary rather than writeTemporary.

@kocsismate
Copy link
Member Author

Looks like historically these were lowercase:

Yeah, only getelem() got messed up to getElem(). That's why I thought that we could go one step further :)

I'm wondering whether we should change the casing in OCILob at the same time though, as it also have methods like writetemporary rather than writeTemporary.

I haven't checked OCILob, but I agree with you!

@kocsismate kocsismate changed the title Rename OCICollection::assignelem() to OCICollection::assignElem() Use camelCase method names in OCICollection and OCILob Aug 26, 2021
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 it builds, ship it!

@nikic
Copy link
Member

nikic commented Aug 30, 2021

If it builds, ship it!

Probably doesn't ^^ I think this needs to adjust at least the PHP_METHOD for writetemporary. We don't check the OCI8 build on PHP-8.0...

@kocsismate
Copy link
Member Author

Ahh, I forgot about it...

@kocsismate
Copy link
Member Author

@nikic Are we good now? As far as I saw, the other methods are aliases

@kocsismate kocsismate merged commit e94731f into php:PHP-8.0 Aug 30, 2021
@kocsismate kocsismate deleted the assignelem branch August 30, 2021 12:01
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.

3 participants