Skip to content

ext/ldap: fix ldap_explode_dn result ordering and simplify count handling#22519

Open
arshidkv12 wants to merge 5 commits into
php:masterfrom
arshidkv12:ldap_explode_dn
Open

ext/ldap: fix ldap_explode_dn result ordering and simplify count handling#22519
arshidkv12 wants to merge 5 commits into
php:masterfrom
arshidkv12:ldap_explode_dn

Conversation

@arshidkv12

Copy link
Copy Markdown
Contributor

@come-nc come-nc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good to me, basically a revert of 30bc98c

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

Just move the add_assoc_long() call prior to the for loop

@Girgias

Girgias commented Jul 2, 2026

Copy link
Copy Markdown
Member

Also needs to target a lower branch.

Comment thread ext/ldap/ldap.c Outdated
array_init(return_value);
int i;

for (i = 0; ldap_value[i] != NULL; i++);

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.

What?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm a bit confused. Could you please clarify what you mean?

Is the following correct?

	add_assoc_long(return_value, "count", 0);

	for (i = 0; ldap_value[i] != NULL; i++) {
		add_index_string(return_value, i, ldap_value[i]);
	}
	add_assoc_long(return_value, "count", i);

@arshidkv12 arshidkv12 changed the base branch from master to PHP-8.4 July 2, 2026 11:56
@arshidkv12 arshidkv12 changed the base branch from PHP-8.4 to master July 2, 2026 11:56
@arshidkv12 arshidkv12 changed the base branch from master to PHP-8.5 July 2, 2026 12:01
@come-nc

come-nc commented Jul 6, 2026

Copy link
Copy Markdown

Just move the add_assoc_long() call prior to the for loop

The count cannot be added before it’s computed.

I find the implementation in cee392a better than the new one.

@Girgias

Girgias commented Jul 6, 2026

Copy link
Copy Markdown
Member

@come-nc I guess this is your new GitHub account compared to @MCMic ?

@come-nc

come-nc commented Jul 7, 2026

Copy link
Copy Markdown

@come-nc I guess this is your new GitHub account compared to @MCMic ?

No this is my work account, sorry for the confusion. I still use both (though not that active outside of work these days)

@Girgias

Girgias commented Jul 7, 2026

Copy link
Copy Markdown
Member

Right, considering you are more of a maintainer of this extension then I am, feel free to merge this. Do you have commit access (be that either on this account or MCMic)?

@MCMic

MCMic commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Right, considering you are more of a maintainer of this extension then I am, feel free to merge this. Do you have commit access (be that either on this account or MCMic)?

No it seems I do not have permissions to merge or approve the PR at #22550
Maybe I have permissions at git level, not sure (at least I used to have them for the ext/ldap folder, but not sure how that gets reflected in github).

[EDIT] Found out about https://wiki.php.net/vcs/gitworkflow , currently trying that, will keep you updated ^^

@MCMic

MCMic commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Right, considering you are more of a maintainer of this extension then I am, feel free to merge this. Do you have commit access (be that either on this account or MCMic)?

No it seems I do not have permissions to merge or approve the PR at #22550 Maybe I have permissions at git level, not sure (at least I used to have them for the ext/ldap folder, but not sure how that gets reflected in github).

[EDIT] Found out about https://wiki.php.net/vcs/gitworkflow , currently trying that, will keep you updated ^^

Nope, following gitworkflow did not help:

$ LC_ALL=en_US.utf8 git push --atomic origin PHP-8.5 master --verbose
Pushing to github.com:php/php-src.git
ERROR: Permission to php/php-src.git denied to MCMic.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.

@Girgias

Girgias commented Jul 10, 2026

Copy link
Copy Markdown
Member

Right, considering you are more of a maintainer of this extension then I am, feel free to merge this. Do you have commit access (be that either on this account or MCMic)?

No it seems I do not have permissions to merge or approve the PR at #22550 Maybe I have permissions at git level, not sure (at least I used to have them for the ext/ldap folder, but not sure how that gets reflected in github).
[EDIT] Found out about https://wiki.php.net/vcs/gitworkflow , currently trying that, will keep you updated ^^

Nope, following gitworkflow did not help:

Let me ask the relevant people to give you commit access.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants