-
Notifications
You must be signed in to change notification settings - Fork 8k
ext/ldap: Value parsing of LDAP Attributes #16173
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
Conversation
To avoid duplications, make sure that we always use zend_strings for bv_val, which seems to be the case. |
Okay this is ingenious, I like it, might have a helper function for this tho as it is slightly exotic |
I agree with making a helper macro/function to make the conversion. I would also propose to add the macro only to ext/ldap and not to somewhere generic because this is very much a "beware there be dragons" macro. |
edba601
to
83b017a
Compare
I have implemented the suggestion and made some follow-up changes. I think this can be used in more places, but I need to do some preliminary refactorings to be able to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks almost good, 2 nits.
ext/ldap/ldap.c
Outdated
case IS_OBJECT: | ||
return zval_try_get_string(zv); | ||
case IS_TRUE: | ||
return zend_string_init("TRUE", strlen("TRUE"), 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use ZSTR_INIT_LITERAL
. Same for false.
ext/ldap/ldap.c
Outdated
return -1; | ||
} | ||
|
||
bool control_iscritical = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use false instead of 0.
Add a new API to free a zend_string via its char*
Rename function to not have a leading "_" at the same time
83b017a
to
0968aa4
Compare
I feel like using this API in other place will result in needing to duplicate a lot of strings, but not sure how to handle this otherwise. :/