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

Update auth.php #1700

Merged
merged 1 commit into from Nov 13, 2016
Merged

Update auth.php #1700

merged 1 commit into from Nov 13, 2016

Conversation

itsho
Copy link
Contributor

@itsho itsho commented Sep 26, 2016

added debug prints to better figure out when LDAP has search issues.

added debug prints to better figure out when LDAP has search issues.
@mention-bot
Copy link

@itsho, thanks for your PR! By analyzing the annotation information on this pull request, we identified @splitbrain, @axel-angel and @adrianheine to be potential reviewers

@splitbrain splitbrain merged commit 611545d into dokuwiki:master Nov 13, 2016
@itsho itsho deleted the patch-1 branch November 23, 2016 10:54
// Don't accept more or less than one response
if ($result['count'] != 1) {
$this->_debug('LDAP search returned '.htmlspecialchars($result['count']).' results while it should return 1!', -1, __LINE__, __FILE__);
//for($i = 0; $i < $result["count"]; $i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having code in comments is not good practice. If it was supposed to not be there, better remove it. If it was supposed to be there for debugging purposes but only when actually debugging, remove the comment and add it only if $this->getConf('debug') or if if($this->getConf('debug')).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. do you want me to remove the code in comment? or you prefer to do it yourself?

Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove it in an additional commit.

Copy link
Contributor Author

@itsho itsho Mar 7, 2017

Choose a reason for hiding this comment

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

I might be wrong but seems like someone else already did it.

edit:
I've compared this version with the one in master and it seems that someone indeed already did it.

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.

None yet

4 participants