ext/ldap: Refactor php_ldap_do_modify()
#16146
Conversation
|
CI failures seem legit |
Girgias
force-pushed
the
ldap-do-modify-refacto
branch
from
0d4ea3e to
89b8149
Compare
And also amend some existing tests which would duplicate coverage
Girgias
force-pushed
the
ldap-do-modify-refacto
branch
from
89b8149 to
0ef85bb
Compare
ext/ldap/ldap.c
Outdated
| } else { | ||
| SEPARATE_ARRAY(value); | ||
| num_values = zend_hash_num_elements(Z_ARRVAL_P(value)); | ||
| int num_values = zend_hash_num_elements(Z_ARRVAL_P(value)); |
There was a problem hiding this comment.
zend_hash_num_elements returns a uint32_t.
There was a problem hiding this comment.
Fixed in a follow-up commit
ext/ldap/ldap.c
Outdated
| convert_to_string(attribute_value); | ||
| if (EG(exception)) { | ||
| num_berval[i] = j; | ||
| num_berval[i] = (int)attribute_value_index; |
There was a problem hiding this comment.
int may cause problems, uint32_t certainly won't.
There was a problem hiding this comment.
Fixed in a follow-up commit.
ext/ldap/ldap.c
Outdated
| efree(ldap_mods[i]->mod_bvalues[j]); | ||
| for (LDAPMod **ptr = ldap_mods; *ptr != NULL; ptr++) { | ||
| LDAPMod *mod = *ptr; | ||
| if (mod->mod_type) { |
There was a problem hiding this comment.
efree doesn't need null checks.
| ZEND_HASH_FOREACH_STR_KEY_VAL(attributes_ht, attribute, attribute_values) { | ||
| if (attribute == NULL) { | ||
| php_error_docref(NULL, E_WARNING, "Unknown attribute in the data"); | ||
| zend_argument_value_error(3, "must be an associative array of attribute => values"); |
There was a problem hiding this comment.
The plural use is a bit strange here. I would write attribute => value or attributes => values but not mix them. WDYT?
There was a problem hiding this comment.
The thing is for a single attribute there might be multiple values attached to it
Agreed. |
2 participants
Commits should be reviewed in order.
I am planning on tackling the usage of
convert_to_string()too, but this was getting long enough.