No feedback.

Can you please look at the LDAP driver in forwards HEAD (which will be 
released soon to work with Horde 3.0) and see if any of this still 
applies? I know a bunch of work has been done already on it.

I did -- and I also tried hard to copy the coding conventions from the 
"neighboring" code.  But I can easily believe that I missed or forgot 
something.   I will re-read the doc and review my code.  In the 
meantime, is there some particular blunder that you caught?



The only intentional exceptions were the "// :ptf:" notes -- and these 
were designed to be excised from the production code.  That's why I 
tagged them with a string that's easy to find.  I only put them in to 
make a complex patch easier to review.
I would have thought so -- but that doesn't appear to be the case.   
Again, I might have missed something, but there's that annoying 
empirical evidence:  If you comment out that function, the module fails.

A few quick notes - first, please take a read through 
horde/docs/CODING_STANDARDS. Second, there's no need for 
Forwards_Driver_ldap - it doesn't do anything different from the 
parent function that it automatically inherits.

I offer no warrantee on this code -- I'm not a native PHP speaker.   
But it seems to work so far.



The patches below address several issues that I found in the LDAP 
driver for Forwards.  Each of the main issues is noted in the code 
with a comment beginning // :ptf:  (PTF is my initials).



First, a small patch to forwards/main.php.  It seems to me that 
"forwarding is enabled" should not be an error.



--- main.php.orig       2004-09-07 20:01:11.904287000 -0400

+++ main.php    2004-09-07 17:01:47.684079000 -0400

@@ -85,8 +85,9 @@

  // this fails, it could be because it is disabled, or just because we

  // can't tell, so just be quiet about it.

  if ($driver->isEnabledForwarding($user, $realm, 
Auth::getCredential('password'))) {

+            // :ptf: Why was this an ERROR?

              Horde::raiseMessage( _("Forwarding is currently enabled."),

-                    HORDE_ERROR);

+                    HORDE_SUCCESS);

  }



      include_once $registry->getTemplatePath('horde') . 
'/javascript/open_help_win.js';







Now for the meat of it -- changes to forwards/lib/Driver/ldap.php.  In 
summary:

1)  There was no function Forwards_Driver_ldap()

2)  The local delivery flag was ignored.  Added a config param 
(localEmail) to support this function.

3)  Errors such as "insufficient access" from the EnableForwarding() 
function were not properly returned to the calling routine

4)  isEnabledForwarding() needed to return Boolean values, not 'Y' and 'N'

5)  Renamed _getForwards to _getFirstValue, since the name is more 
apt, and changed filter from 'uid=*' to 'objectClass=*'

6)  Added a utility function to remove the 'count' from arrays 
returned by the ldap_get() functions.



Local Delivery still needs a bit of work.  I added an attribute 
(localEmail) to the config file to support this function.  (The 
alternative -- building an address out of the user and realm -- 
doesn't work well if the realm is 'default'.)



To properly handle local delivery, I need (1) to properly identify if 
the local address is part of the forwarding string, and (2) if it is 
there, strip it out when displaying the page and check off the "local 
delivery" box.  I should also handle the case where local delivery is 
the only remaining address on the list.





--- ldap.php.orig       2004-09-07 16:13:51.668142000 -0400

+++ ldap.php    2004-09-07 19:44:26.845636000 -0400

@@ -22,6 +22,17 @@

       */

      var $_ds;



+    // :ptf:  The function Forwards_Driver_ldap was missing ... (?)

+    /**

+    * Constructs a new ldap Forwards_Driver object.

+    *

+    * @param array  $params        A hash containing connection parameters.

+    */

+    function Forwards_Driver_ldap($params = array())

+    {

+        $this->_params = $params;

+    }

+

      /**

       * Check if the realm has a specific configuration.  If not, try to fall

       * back on the default configuration.  If still not a valid configuration

@@ -41,6 +52,7 @@

          }



          // If still no host/port, then we have a misconfigured module.

+

          if (empty($this->_params[$realm]['host']) ||

              empty($this->_params[$realm]['port']) ) {

              $this->_error = _("The module is not properly configured!");

@@ -55,7 +67,7 @@

       * @param string $user       The username to enable forwards for.

       * @param string $realm      The realm of the user.

       * @param string $target     The message to install.

-     * @param string $keeplocal  Never used.

+     * @param string $keeplocal  Deliver message to box and forward 
it as well.

       *

       * @return boolean  Returns true on success, false on error.

       */

@@ -84,17 +96,64 @@

              return false;

          }



+

+        // :ptf:  Handle "Local Delivery" flag

+       // Local delivery?

+        if ($keeplocal === 'on') {

+

+            // What is the local mailbox?

+            if ( $this->_params[$realm]['localEmail']) {

+                $attribs = array($this->_params[$realm]['localEmail']);

+                $local_target = $this->_getFirstValue($userdn, $attribs);

+            //

+            } else {

+                $res = PEAR::raiseError(_("Cannot determine local mailbox"));

+                $this->err_str = "Cannot determine local mailbox";

+                $this->_disconnect();

+                return false;

+            }

+            // An alternative would be to guess what the local box

+            // might be (see below).  But guessing badly is a bad thing.

+            //    } elseif ($realm == 'default' ) {

+            //        $local_target = $user;

+            //    } else {

+            //        $local_target = $user . '@' . $realm;

+            //    }

+            //

+

+

+            // :ptf:  LocalDelivery handling still has a two bugs

+            // BUG:  I copied this code from the SQL driver -- but

+            //       it ain't perfect:

+            //          if the local target is foo@bar.com, and the

+            //          forwards list contains bigfoo@bar.com,  then

+            //          it will get the wrong answer

+            //

+            // Assumes that Forwards attribute is comma-delimited list

+            if($local_target AND !strstr($message,$local_target)) {

+                $message .= ', ' . $local_target;

+            }

+

+        } else {   // keeplocal is off

+            // BUG:  We need to pull out the local target(s) if

+            //       keeplocal is off

+        }

+

+

          // Change the user's forwards.

          $newDetails[$this->_params[$realm]['forwards']] = $message;

          $res = ldap_mod_replace($this->_ds, $userdn, $newDetails);

+

+        // :ptf:  Errors here were being ignored

          if (!$res) {

              $res = PEAR::raiseError(ldap_error($this->_ds));

+            $this->err_str = ldap_error($this->_ds);

+            $this->_disconnect();

+            return false;

+        } else {

+            $this->_disconnect();

+            return true;

          }

-

-        // Disconnect from the ldap server.

-        $this->_disconnect();

-

-        return true;

      }



      /**

@@ -133,7 +192,7 @@



          // Delete the user's forwards.

          $attribs = array($this->_params[$realm]['forwards']);

-        $value = $this->_getForwards($userdn, $attribs);

+        $value = $this->_getFirstValue($userdn, $attribs);

          if (!$value) {

              // Nothing to delete, so treat as success.

              return true;

@@ -158,9 +217,10 @@

       *

       * @param string    $realm    The realm of the user to check.

       *

-     * @return mixed    Returns 'Y' if forwarding is enabled for the

-     *          user or 'N' if forwarding is currently

-     *          disabled.

+     * // :ptf: This function needs to return Boolean values, not strings

+     * @return boolean    Returns true if forwarding is enabled for the

+     *          user or false if forwarding is currently disabled or if

+     *          an error occurs

       */

      function isEnabledForwarding($user, $realm, $pass)

      {

@@ -175,7 +235,7 @@

          } else {

              $userdn = $this->_lookupdn($user, $realm);

              if (is_a($userdn, 'PEAR_Error')) {

-                return $userdn;

+                return false;

              }

          }



@@ -184,18 +244,18 @@

          if (is_a($res, 'PEAR_Error')) {

              $this->_disconnect();

              if ($res->getMessage() == _("Could not bind to ldap server")) {

-                return PEAR::raiseError(_("Incorrect Password"));

+                $res = PEAR::raiseError(_("Incorrect Password"));

              }

-            return $res;

+            return false;

          }



          $attribs = array($this->_params[$realm]['forwards']);

-        if ($this->_getForwards($userdn, $attribs)) {

+        if ($this->_getFirstValue($userdn, $attribs)) {

              $this->_disconnect();

-            return 'Y';

+            return true;

          } else {

              $this->_disconnect();

-            return 'N';

+            return false;

          }

      }



@@ -236,7 +296,7 @@



          $attribs = array($this->_params[$realm]['forwards']);



-        return $this->_getForwards($userdn, $attribs);

+        return $this->_getFirstValue($userdn, $attribs);

      }



      /**

@@ -275,9 +335,20 @@

          return $userdn;

      }



-    function _getForwards($userdn, $attribs)

+    //  :ptf: Renamed this function

+    /**

+     * Get the first available attribute value for a particular DSN

+     *

+     * @param  $userdn   The ldap basedn.

+     * @param  $attribs  A list whose first entry is the attribute we seek

+     *

+     * @return string    The value of the attribute, or false if not found

+     *

+     */

+    function _getFirstValue($userdn, $attribs)

      {

-        $sr = ldap_search($this->_ds, $userdn, 'uid=*', $attribs);

+        //  :ptf: Using objectClass, since we don't know if 'uid' exists

+        $sr = ldap_search($this->_ds, $userdn, 'objectClass=*', $attribs);

          $entry = ldap_first_entry($this->_ds, $sr);

          $res = ldap_get_attributes($this->_ds, $entry);

          if ($res['count'] == 0) {

@@ -288,6 +359,32 @@

          return $values[0];

      }



+

+    // :ptf:  Handy function to remove 'count' from ldap_get() results

+    /**

+     * Recursively remove the 'count' entry from an array

+     *

+     *   Posted by mack@incise.org

+     *   http://us4.php.net/manual/en/function.ldap-get-entries.php

+     *

+     * @param  $arr      The source array

+     *

+     * @return mixed     The same array with no 'count' entries

+     *

+     */

+    function _rCountRemover($arr)

+    {

+        foreach($arr as $key=>$val) {

+            # (int)0 == "count", so we need to use ===

+            if($key === "count")

+                unset($arr[$key]);

+            elseif(is_array($val))

+                $arr[$key] = _rCountRemover($arr[$key]);

+            }

+            return $arr;

+    }

+

+

      /**

       * Do an ldap connect and bind as the guest user or as the 
optional userdn.

       *