more subscription refactoring BROKEN

now the actual sending of bulk messages (digest, list) is reimplemented
and partially tested.

Still not complete

4 files changed, 238 insertions, 140 deletions

diff --git a/_test/tests/inc/subscription.test.php b/_test/tests/inc/subscription.test.php
index 77bf3e830..b3d49a533 100644
--- a/_test/tests/inc/subscription.test.php
+++ b/_test/tests/inc/subscription.test.php
@@ -2,7 +2,7 @@
 
 class subscription_test extends DokuWikiTest {
 
-    function test_regexp(){
+    function test_regexp() {
         // data to test against
         $data = array(
             "casper every\n",
@@ -19,13 +19,13 @@ class subscription_test extends DokuWikiTest {
             array('casper', null, null, 1),
             array('nope', null, null, 0),
             array('lights', null, null, 0),
-            array(array('Cold Fusion','casper','nope'), null, null, 2),
+            array(array('Cold Fusion', 'casper', 'nope'), null, null, 2),
             array(null, 'list', null, 1),
             array(null, 'every', null, 2),
             array(null, 'digest', null, 3),
             array(null, array('list', 'every'), null, 3),
             array('casper', 'digest', null, 0),
-            array('casper', array('digest','every'), null, 1),
+            array('casper', array('digest', 'every'), null, 1),
             array('zioth', 'list', '1344691369', 1),
             array('zioth', null, '1344691369', 1),
             array('zioth', 'digest', '1344691369', 0),
@@ -34,7 +34,7 @@ class subscription_test extends DokuWikiTest {
         $sub = new MockupSubscription();
 
         $row = 0;
-        foreach($tests as $test){
+        foreach($tests as $test) {
             $re = $sub->buildregex($test[0], $test[1], $test[2]);
             $this->assertFalse(empty($re), "test line $row");
             $result = preg_grep($re, $data);
@@ -44,13 +44,13 @@ class subscription_test extends DokuWikiTest {
         }
     }
 
-    function test_addremove(){
+    function test_addremove() {
         $sub = new MockupSubscription();
 
         // no subscriptions
         $this->assertArrayNotHasKey(
             'wiki:dokuwiki',
-            $sub->subscribers('wiki:dokuwiki', null, array('every','list','digest'))
+            $sub->subscribers('wiki:dokuwiki', null, array('every', 'list', 'digest'))
         );
 
         // add page subscription
@@ -59,7 +59,7 @@ class subscription_test extends DokuWikiTest {
         // one subscription
         $this->assertArrayHasKey(
             'wiki:dokuwiki',
-            $sub->subscribers('wiki:dokuwiki', null, array('every','list','digest'))
+            $sub->subscribers('wiki:dokuwiki', null, array('every', 'list', 'digest'))
         );
 
         // remove page subscription
@@ -68,7 +68,7 @@ class subscription_test extends DokuWikiTest {
         // no subscription
         $this->assertArrayNotHasKey(
             'wiki:dokuwiki',
-            $sub->subscribers('wiki:dokuwiki', null, array('every','list','digest'))
+            $sub->subscribers('wiki:dokuwiki', null, array('every', 'list', 'digest'))
         );
 
         // add namespace subscription
@@ -77,7 +77,7 @@ class subscription_test extends DokuWikiTest {
         // one subscription
         $this->assertArrayHasKey(
             'wiki:',
-            $sub->subscribers('wiki:dokuwiki', null, array('every','list','digest'))
+            $sub->subscribers('wiki:dokuwiki', null, array('every', 'list', 'digest'))
         );
 
         // remove (non existing) page subscription
@@ -86,7 +86,7 @@ class subscription_test extends DokuWikiTest {
         // still one subscription
         $this->assertArrayHasKey(
             'wiki:',
-            $sub->subscribers('wiki:dokuwiki', null, array('every','list','digest'))
+            $sub->subscribers('wiki:dokuwiki', null, array('every', 'list', 'digest'))
         );
 
         // change namespace subscription
@@ -95,17 +95,66 @@ class subscription_test extends DokuWikiTest {
         // still one subscription
         $this->assertArrayHasKey(
             'wiki:',
-            $sub->subscribers('wiki:dokuwiki', null, array('every','list','digest'))
+            $sub->subscribers('wiki:dokuwiki', null, array('every', 'list', 'digest'))
         );
 
         // check contents
         $this->assertEquals(
             array('wiki:' => array('testuser' => array('digest', '1234567'))),
-            $sub->subscribers('wiki:dokuwiki',  null, array('every','list','digest'))
+            $sub->subscribers('wiki:dokuwiki', null, array('every', 'list', 'digest'))
+        );
+
+        // change subscription data
+        $sub->add('wiki:', 'testuser', 'digest', '7654321');
+
+        // still one subscription
+        $this->assertArrayHasKey(
+            'wiki:',
+            $sub->subscribers('wiki:dokuwiki', null, array('every', 'list', 'digest'))
+        );
+
+        // check contents
+        $this->assertEquals(
+            array('wiki:' => array('testuser' => array('digest', '7654321'))),
+            $sub->subscribers('wiki:dokuwiki', null, array('every', 'list', 'digest'))
         );
     }
 
+    function test_bulkdigest(){
+        $sub = new MockupSubscription();
+
+        // let's start with nothing
+        $this->assertEquals(0, $sub->send_bulk('sub1:test'));
+
+        // create a subscription
+        $sub->add('sub1:', 'testuser', 'digest', '978328800'); // last mod 2001-01-01
+
+        // now create change
+        $_SERVER['REMOTE_USER'] = 'someguy';
+        saveWikiText('sub1:test', 'foo bar', 'a subscription change', false);
 
+        // should trigger a mail
+        $this->assertEquals(1, $sub->send_bulk('sub1:test'));
+        $this->assertEquals(array('arthur@example.com'), $sub->mails);
+
+        $sub->reset();
+
+        // now create more changes
+        $_SERVER['REMOTE_USER'] = 'someguy';
+        saveWikiText('sub1:sub2:test', 'foo bar', 'a subscription change', false);
+        saveWikiText('sub1:another_test', 'foo bar', 'a subscription change', false);
+
+        // should not trigger a mail, because the subscription time has not been reached, yet
+        $this->assertEquals(0, $sub->send_bulk('sub1:test'));
+        $this->assertEquals(array(), $sub->mails);
+
+        // reset the subscription time
+        $sub->add('sub1:', 'testuser', 'digest', '978328800'); // last mod 2001-01-01
+
+        // we now should get mails for three changes
+        $this->assertEquals(3, $sub->send_bulk('sub1:test'));
+        $this->assertEquals(array('arthur@example.com','arthur@example.com','arthur@example.com'), $sub->mails);
+    }
 
 }
 
@@ -113,9 +162,27 @@ class subscription_test extends DokuWikiTest {
  * makes protected methods visible for testing
  */
 class MockupSubscription extends Subscription {
-     public function buildregex($user = null, $style = null, $data = null) {
-         return parent::buildregex($user, $style, $data);
-     }
+    public $mails; // we keep sent mails here
+
+    public function __construct(){
+        $this->reset();
+    }
+
+    /**
+     * resets the mail array
+     */
+    public function reset(){
+        $this->mails = array();
+    }
+
+    public function buildregex($user = null, $style = null, $data = null) {
+        return parent::buildregex($user, $style, $data);
+    }
+
+    protected function send($subscriber_mail, $replaces, $subject, $id, $template){
+        $this->mails[] = $subscriber_mail;
+        return true;
+    }
 }
 
 //Setup VIM: ex: et ts=4 :
diff --git a/inc/common.php b/inc/common.php
index ac7e744d8..29940d8a6 100644
--- a/inc/common.php
+++ b/inc/common.php
@@ -107,9 +107,11 @@ function pageinfo() {
     $info['isadmin']   = false;
     $info['ismanager'] = false;
     if(isset($_SERVER['REMOTE_USER'])) {
+        $sub = new Subscription();
+
         $info['userinfo']   = $USERINFO;
         $info['perm']       = auth_quickaclcheck($ID);
-        $info['subscribed'] = get_info_subscribed();
+        $info['subscribed'] = $sub->user_subscription();
         $info['client']     = $_SERVER['REMOTE_USER'];
 
         if($info['perm'] == AUTH_ADMIN) {
diff --git a/inc/subscription.php b/inc/subscription.php
index 856836cd5..804776ced 100644
--- a/inc/subscription.php
+++ b/inc/subscription.php
@@ -253,24 +253,23 @@ class Subscription {
      * @return array
      * @author Adrian Lang <lang@cosmocode.de>
      */
-    function user_subscription($id='', $user='') {
+    function user_subscription($id = '', $user = '') {
         global $ID;
         global $conf;
         if(!$conf['subscribers']) return false;
 
-        if(!$id)   $id = $ID;
+        if(!$id) $id = $ID;
         if(!$user) $user = $_SERVER['REMOTE_USER'];
 
-
         $subs = $this->subscribers($id, $user);
         if(!count($subs)) return false;
 
         $result = array();
-        foreach($subs as $target => $data) {
+        foreach($subs as $target => $info) {
             $result[] = array(
                 'target' => $target,
-                'style'  => $data[$user][0],
-                'data'   => $data[$user][1]
+                'style'  => $info[$user][0],
+                'data'   => $info[$user][1]
             );
         }
 
@@ -278,52 +277,100 @@ class Subscription {
     }
 
     /**
-     * Return a string with the email addresses of all the
-     * users subscribed to a page
+     * Send digest and list subscriptions
      *
-     * This is the default action for COMMON_NOTIFY_ADDRESSLIST.
+     * This sends mails to all subscribers that have a subscription for namespaces above
+     * the given page if the needed $conf['subscribe_time'] has passed already.
      *
-     * @author Steven Danz <steven-danz@kc.rr.com>
-     * @author Adrian Lang <lang@cosmocode.de>
+     * This function is called form lib/exe/indexer.php
      *
-     * @todo this does NOT return a string but uses a reference to write back, either fix function or docs
-     * @param array $data Containing $id (the page id), $self (whether the author
-     *                    should be notified, $addresslist (current email address
-     *                    list)
-     * @return string
+     * @param string $page
+     * @return int number of sent mails
      */
-    function subscription_addresslist(&$data) {
-        global $conf;
+    public function send_bulk($page) {
         /** @var auth_basic $auth */
         global $auth;
+        global $conf;
+        global $USERINFO;
+        $count = 0;
 
-        $id          = $data['id'];
-        $self        = $data['self'];
-        $addresslist = $data['addresslist'];
+        $subscriptions = $this->subscribers($page, null, array('digest', 'list'));
 
-        if(!$conf['subscribers'] || $auth === null) {
-            return '';
-        }
-        $pres = array('style' => 'every', 'escaped' => true);
-        if(!$self && isset($_SERVER['REMOTE_USER'])) {
-            $pres['user'] = '((?!'.preg_quote_cb($_SERVER['REMOTE_USER']).
-                '(?: |$))\S+)';
-        }
-        $subs   = subscription_find($id, $pres);
-        $emails = array();
-        foreach($subs as $by_targets) {
-            foreach($by_targets as $sub) {
-                $info = $auth->getUserData($sub[0]);
-                if($info === false) continue;
-                $level = auth_aclcheck($id, $sub[0], $info['grps']);
-                if($level >= AUTH_READ) {
-                    if(strcasecmp($info['mail'], $conf['notify']) != 0) {
-                        $emails[$sub[0]] = $info['mail'];
+        // remember current user info
+        $olduinfo = $USERINFO;
+        $olduser  = $_SERVER['REMOTE_USER'];
+
+        foreach($subscriptions as $target => $users) {
+            if(!$this->lock($target)) continue;
+
+            foreach($users as $user => $info) {
+                list($style, $lastupdate) = $info;
+
+                $lastupdate = (int) $lastupdate;
+                if($lastupdate + $conf['subscribe_time'] > time()) {
+                    // Less than the configured time period passed since last
+                    // update.
+                    continue;
+                }
+
+                // Work as the user to make sure ACLs apply correctly
+                $USERINFO               = $auth->getUserData($user);
+                $_SERVER['REMOTE_USER'] = $user;
+                if($USERINFO === false) continue;
+                if(!$USERINFO['mail']) continue;
+
+                if(substr($target, -1, 1) === ':') {
+                    // subscription target is a namespace, get all changes within
+                    $changes = getRecentsSince($lastupdate, null, getNS($target));
+                } else {
+                    // single page subscription, check ACL ourselves
+                    if(auth_quickaclcheck($target) < AUTH_READ) continue;
+                    $meta    = p_get_metadata($target);
+                    $changes = array($meta['last_change']);
+                }
+
+                // Filter out pages only changed in small and own edits
+                $change_ids = array();
+                foreach($changes as $rev) {
+                    $n = 0;
+                    while(!is_null($rev) && $rev['date'] >= $lastupdate &&
+                        ($_SERVER['REMOTE_USER'] === $rev['user'] ||
+                            $rev['type'] === DOKU_CHANGE_TYPE_MINOR_EDIT)) {
+                        $rev = getRevisions($rev['id'], $n++, 1);
+                        $rev = (count($rev) > 0) ? $rev[0] : null;
+                    }
+
+                    if(!is_null($rev) && $rev['date'] >= $lastupdate) {
+                        // Some change was not a minor one and not by myself
+                        $change_ids[] = $rev['id'];
                     }
                 }
+
+                // send it
+                if($style === 'digest') {
+                    foreach($change_ids as $change_id) {
+                        $this->send_digest(
+                            $USERINFO['mail'], $change_id,
+                            $lastupdate
+                        );
+                        $count++;
+                    }
+                } elseif($style === 'list') {
+                    $this->send_list($USERINFO['mail'], $change_ids, $target);
+                    $count++;
+                }
+                // TODO: Handle duplicate subscriptions.
+
+                // Update notification time.
+                $this->add($target, $user, $style, time());
             }
+            $this->unlock($target);
         }
-        $data['addresslist'] = trim($addresslist.','.implode(',', $emails), ',');
+
+        // restore current user info
+        $USERINFO               = $olduinfo;
+        $_SERVER['REMOTE_USER'] = $olduser;
+        return $count;
     }
 
     /**
@@ -337,7 +384,7 @@ class Subscription {
      * @param array  $id              The ID
      * @param int    $lastupdate      Time of the last notification
      */
-    function subscription_send_digest($subscriber_mail, $id, $lastupdate) {
+    protected function send_digest($subscriber_mail, $id, $lastupdate) {
         $n = 0;
         do {
             $rev = getRevisions($id, $n++, 1);
@@ -360,7 +407,7 @@ class Subscription {
             $replaces['OLDPAGE'] = 'none';
             $replaces['DIFF']    = rawWiki($id);
         }
-        subscription_send(
+        $this->send(
             $subscriber_mail, $replaces, $subject, $id,
             'subscr_digest'
         );
@@ -377,14 +424,14 @@ class Subscription {
      * @param array  $ids             Array of ids
      * @param string $ns_id           The id of the namespace
      */
-    function subscription_send_list($subscriber_mail, $ids, $ns_id) {
+    protected function send_list($subscriber_mail, $ids, $ns_id) {
         if(count($ids) === 0) return;
         global $conf;
         $list = '';
         foreach($ids as $id) {
             $list .= '* '.wl($id, array(), true).NL;
         }
-        subscription_send(
+        $this->send(
             $subscriber_mail,
             array(
                  'DIFF'      => rtrim($list),
@@ -414,7 +461,7 @@ class Subscription {
      * @param string $template        The name of the mail template
      * @return bool
      */
-    function subscription_send($subscriber_mail, $replaces, $subject, $id, $template) {
+    protected function send($subscriber_mail, $replaces, $subject, $id, $template) {
         global $lang;
 
         $text = rawLocale($template);
@@ -433,4 +480,58 @@ class Subscription {
         return $mail->send();
     }
 
+
+
+    // FIXME no refactoring below, yet
+
+    /**
+     * Return a string with the email addresses of all the
+     * users subscribed to a page
+     *
+     * This is the default action for COMMON_NOTIFY_ADDRESSLIST.
+     *
+     * @author Steven Danz <steven-danz@kc.rr.com>
+     * @author Adrian Lang <lang@cosmocode.de>
+     *
+     * @todo this does NOT return a string but uses a reference to write back, either fix function or docs
+     * @param array $data Containing $id (the page id), $self (whether the author
+     *                    should be notified, $addresslist (current email address
+     *                    list)
+     * @return string
+     */
+    function subscription_addresslist(&$data) {
+        global $conf;
+        /** @var auth_basic $auth */
+        global $auth;
+
+        $id          = $data['id'];
+        $self        = $data['self'];
+        $addresslist = $data['addresslist'];
+
+        if(!$conf['subscribers'] || $auth === null) {
+            return '';
+        }
+        $pres = array('style' => 'every', 'escaped' => true);
+        if(!$self && isset($_SERVER['REMOTE_USER'])) {
+            $pres['user'] = '((?!'.preg_quote_cb($_SERVER['REMOTE_USER']).
+                '(?: |$))\S+)';
+        }
+        $subs   = subscription_find($id, $pres);
+        $emails = array();
+        foreach($subs as $by_targets) {
+            foreach($by_targets as $sub) {
+                $info = $auth->getUserData($sub[0]);
+                if($info === false) continue;
+                $level = auth_aclcheck($id, $sub[0], $info['grps']);
+                if($level >= AUTH_READ) {
+                    if(strcasecmp($info['mail'], $conf['notify']) != 0) {
+                        $emails[$sub[0]] = $info['mail'];
+                    }
+                }
+            }
+        }
+        $data['addresslist'] = trim($addresslist.','.implode(',', $emails), ',');
+    }
+
+
 }
\ No newline at end of file
diff --git a/lib/exe/indexer.php b/lib/exe/indexer.php
index e149770c0..270341fe6 100644
--- a/lib/exe/indexer.php
+++ b/lib/exe/indexer.php
@@ -166,92 +166,20 @@ function runSitemapper(){
  * @author Adrian Lang <lang@cosmocode.de>
  */
 function sendDigest() {
-    echo 'sendDigest(): started'.NL;
-    global $ID;
     global $conf;
+    global $ID;
+
+    echo 'sendDigest(): started'.NL;
     if (!$conf['subscribers']) {
         echo 'sendDigest(): disabled'.NL;
         return false;
     }
-    $subscriptions = subscription_find($ID, array('style' => '(digest|list)',
-                                                  'escaped' => true));
-    global $auth;
-    global $lang;
-    global $conf;
-    global $USERINFO;
-
-    // remember current user info
-    $olduinfo = $USERINFO;
-    $olduser  = $_SERVER['REMOTE_USER'];
-
-    foreach($subscriptions as $id => $users) {
-        if (!subscription_lock($id)) {
-            continue;
-        }
-        foreach($users as $data) {
-            list($user, $style, $lastupdate) = $data;
-            $lastupdate = (int) $lastupdate;
-            if ($lastupdate + $conf['subscribe_time'] > time()) {
-                // Less than the configured time period passed since last
-                // update.
-                continue;
-            }
-
-            // Work as the user to make sure ACLs apply correctly
-            $USERINFO = $auth->getUserData($user);
-            $_SERVER['REMOTE_USER'] = $user;
-            if ($USERINFO === false) {
-                continue;
-            }
-
-            if (substr($id, -1, 1) === ':') {
-                // The subscription target is a namespace
-                $changes = getRecentsSince($lastupdate, null, getNS($id));
-            } else {
-                if(auth_quickaclcheck($id) < AUTH_READ) continue;
-
-                $meta = p_get_metadata($id);
-                $changes = array($meta['last_change']);
-            }
-
-            // Filter out pages only changed in small and own edits
-            $change_ids = array();
-            foreach($changes as $rev) {
-                $n = 0;
-                while (!is_null($rev) && $rev['date'] >= $lastupdate &&
-                       ($_SERVER['REMOTE_USER'] === $rev['user'] ||
-                        $rev['type'] === DOKU_CHANGE_TYPE_MINOR_EDIT)) {
-                    $rev = getRevisions($rev['id'], $n++, 1);
-                    $rev = (count($rev) > 0) ? $rev[0] : null;
-                }
-
-                if (!is_null($rev) && $rev['date'] >= $lastupdate) {
-                    // Some change was not a minor one and not by myself
-                    $change_ids[] = $rev['id'];
-                }
-            }
-
-            if ($style === 'digest') {
-                foreach($change_ids as $change_id) {
-                    subscription_send_digest($USERINFO['mail'], $change_id,
-                                             $lastupdate);
-                }
-            } elseif ($style === 'list') {
-                subscription_send_list($USERINFO['mail'], $change_ids, $id);
-            }
-            // TODO: Handle duplicate subscriptions.
-
-            // Update notification time.
-            subscription_set($user, $id, $style, time(), true);
-        }
-        subscription_unlock($id);
-    }
+    $sub = new Subscription();
+    $sent = $sub->send_bulk($ID);
 
-    // restore current user info
-    $USERINFO = $olduinfo;
-    $_SERVER['REMOTE_USER'] = $olduser;
+    echo "sendDigest(): sent $sent mails".NL;
     echo 'sendDigest(): finished'.NL;
-    return true;
+    return (bool) $sent;
 }
 
 /**