Skip to content

LdapSource: Remove call to undefined fatal_error() function #118

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.

Sign up for GitHub

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

Merged
chinpei215 merged 1 commit into from
Oct 25, 2017
Merged

LdapSource: Remove call to undefined fatal_error() function #118

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Model/Datasource/LdapSource.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ public function connect($bindDN = null, $passwd = null) {
if ($config['tls']) {
if (!ldap_start_tls($this->database)) {
$this->log("Ldap_start_tls failed", 'ldap.error');
fatal_error("Ldap_start_tls failed");
return $this->disconnect();
Copy link
Contributor

@chinpei215 chinpei215 Oct 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I would prefer throwing Exception here over returning false. For example, if it were Mysql connection, it would throw PDOException if it failed to connect to the server.

Copy link
Contributor Author

@ctgraham ctgraham Oct 9, 2017

Choose a reason for hiding this comment

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

Agreed, but nowhere else in this project currently throws an Exception. There are various places in this class where we could:

datasources/Model/Datasource/LdapSource.php

Lines 548 to 549 in 84cc8d0

$this->log("Error updating $dn: " . ldap_error($this->database) . "\nHere is what I sent: " . print_r($entry, true), 'ldap.error');
return false;

datasources/Model/Datasource/LdapSource.php

Lines 597 to 598 in 84cc8d0

$this->log("Failed Trying to delete: $dn \nLdap Erro:$errMsg", 'ldap.error');
return false;

datasources/Model/Datasource/LdapSource.php

Lines 748 to 749 in 84cc8d0

$this->log( "LDAP schema filter $schemaFilter is invalid!", 'ldap.error');
return array();

Shouldn't refactoring to use Exceptions be a different Issue/PR?

Copy link
Contributor

@chinpei215 chinpei215 Oct 10, 2017

Choose a reason for hiding this comment

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

Fair point. Now I am working on test cases for LdapSource. After I finished the work, I will merge this PR and #114. But I am not sure if I can merge #115, as it contains some conflicts. If your are still interested in it, then please rebase it and make sure test cases pass.

Copy link
Contributor Author

@ctgraham ctgraham Oct 10, 2017

Choose a reason for hiding this comment

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

I'm not seeing the merge conflict for #115. Each of these PRs does have a failed test, but it looked to me like it is all the same failure, outside of the scope of this class:

  1. XmlrpcSourceTest::testRequest
    Failed asserting that -32300 matches expected -32700.

Can you point me to any checks or conflicts that should be addressed?

Copy link
Contributor

@chinpei215 chinpei215 Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I have already merged #114 on my local environment to write test cases. And next, I tried to merge #115, but then it reported several conflicts.

Copy link
Contributor Author

@ctgraham ctgraham Oct 11, 2017

Choose a reason for hiding this comment

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

Ah, I see. When you've merged #114 into GitHub, I will be able to rebase #115 to resolve the conflicts.

}
}
//So little known fact, if your php-ldap lib is built against openldap like pretty much every linux
Expand Down