Initial overall architecture review

Services registered in simple_ldap.services.yml seem logical, although the fact that both simple_ldap.server and simple_ldap.connection require @config.factory makes me want to look into that, and in fact both SimpleLdapConnection and SimpleLdapServer are loading $config_factory->get('simple_ldap.server'). It seems it would probably be best to add some methods to SimpleLdapConnection like isReadOnly or any of the other config properties needed. Even adding a getConfig() which returns SimpleLdapConnection->config.

This also leads me to wonder what the need is to have the separate SimpleLdapConnection and SimpleLdapServer objects. I would think that things could be simplified a bit by just adding the necessary logic from SimpleLdapConnection into SimpleLdapServer. I'm not sure where the model of splitting these came from (maybe Database vs DatabaseConnection), but the fact that there can only be one connection makes me think simplifying and merging would be better. If there were a need to have multiple LDAP connections, then splitting those apart would make more sense to me.

Looking at SimpleLdap, it looks like most of the purpose is to be a wrapper around the PHP ldap functions. If I was approaching this, I have a feeling that I would probably put these functions into a Trait, which then could be re-used by the SimpleLdapServer class, but not need to know any logic of the container:

trait SimpleLdapWrapper {
  
  /**
   * @var boolean
   */
  private $bound = FALSE;

  public function ldapBind($bind_rdn = NULL, $bind_password = NULL) {
    $bound = @ldap_bind($this->connection, $bind_rdn, $bind_password);
    $this->bound = $bound;
  }

  public function isBound() {
    return $this->bound;
  }

  public function setBound($bound) {
    $this->bound = $bound;
  }

}

class SimpleLdapServer {
  use SimpleLdapWrapper;

  ...

}

Part of me even thinks that with the simplification, all the ldap PHP wrapper methods could just be moved to the server class. I'm not sure what benefit the wrapper is giving us (is it reusable outside of SimpleLdapServer?), it's resulting in more indirection and I feel like it is ok that SimpleLdapServer grows a bit larger. If anything the methods that don't call PHP's ldap methods are the best candidates for having in a trait since those could be re-used by any class.

I'm trying to think through what potential problems would be for merging the classes together, and testing seems to be the only major issue. If you can separate out the methods enough in the server class, they should still be able to be mocked

In SimpleLdapServer, it seems odd that it would be catching exceptions in the connect() method. I believe it should throw them and that it's the responsibility of the upstream code to catch exceptions. This would remove the need for the wrapper connect() method, and that could be moved up to the Server's connect method.

It might be helper for the Server's methods to do a "return $this" more often, especially with connect and bind methods. Easier for calling code that could do $server = \Drupal::service('simple_ldap.server')->connect()->bind();

Make sure to try and use interfaces for everything, especially when passing around SimpleLdapServer.

Is there a reason that the server shouldn't attempt to run connect() when it is instantiated? Or maybe calling bind() should automatically call connect()?