LdapSource: Remove call to undefined fatal_error() function

[ctgraham]

The function fatal_error() is not defined; Disconnecting and returning to the caller is probably preferred anyway.

[chinpei215]

@ctgraham We are very sorry for not replying to your 3 pull requests. I will take a look at all of them as soon as possible.

[chinpei215]

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.

[ctgraham]

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?

[chinpei215]

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.

[ctgraham]

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?

[chinpei215]

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.

[ctgraham]

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