Conversation IDs (again) no longer based on Notice ID

actions/conversation.php

@@ -49,24 +49,9 @@ class ConversationAction extends ManagedAction
     var $page        = null;
     var $notices     = null;
 
-    /**
+    protected function doPreparation()
-     * Initialization.
-     *
-     * @param array $args Web and URL arguments
-     *
-     * @return boolean false if id not passed in
-     */
-    protected function prepare(array $args=array())
     {
-        parent::prepare($args);
+        $this->conv = Conversation::getByID($this->int('id'));
-        $convId = $this->int('id');
-
-        $this->conv = Conversation::getKV('id', $convId);
-        if (!$this->conv instanceof Conversation) {
-            throw new ClientException('Could not find specified conversation');
-        }
-
-        return true;
     }
 
     /**
@@ -90,7 +75,7 @@ class ConversationAction extends ManagedAction
     function showContent()
     {
         if (Event::handle('StartShowConversation', array($this, $this->conv, $this->scoped))) {
-            $notices = $this->conv->getNotices();
+            $notices = $this->conv->getNotices($this->scoped);
             $nl = new FullThreadedNoticeList($notices, $this, $this->scoped);
             $cnt = $nl->show();
         }
@@ -108,7 +93,7 @@ class ConversationAction extends ManagedAction
         return array(new Feed(Feed::JSON,
                               common_local_url('apiconversation',
                                                array(
-                                                    'id' => $this->conv->id,
+                                                    'id' => $this->conv->getID(),
                                                     'format' => 'as')),
                               // TRANS: Title for link to notice feed.
                               // TRANS: %s is a user nickname.
@@ -116,7 +101,7 @@ class ConversationAction extends ManagedAction
                      new Feed(Feed::RSS2,
                               common_local_url('apiconversation',
                                                array(
-                                                    'id' => $this->conv->id,
+                                                    'id' => $this->conv->getID(),
                                                     'format' => 'rss')),
                               // TRANS: Title for link to notice feed.
                               // TRANS: %s is a user nickname.
@@ -124,7 +109,7 @@ class ConversationAction extends ManagedAction
                      new Feed(Feed::ATOM,
                               common_local_url('apiconversation',
                                                array(
-                                                    'id' => $this->conv->id,
+                                                    'id' => $this->conv->getID(),
                                                     'format' => 'atom')),
                               // TRANS: Title for link to notice feed.
                               // TRANS: %s is a user nickname.

classes/Conversation.php

@@ -43,7 +43,7 @@ class Conversation extends Managed_DataObject
     {
         return array(
             'fields' => array(
-                'id' => array('type' => 'int', 'not null' => true, 'description' => 'should be set from root notice id (since 2014-03-01 commit)'),
+                'id' => array('type' => 'serial', 'not null' => true, 'description' => 'Unique identifier, (again) unrelated to notice id since 2016-01-06'),
                 'uri' => array('type' => 'varchar', 'not null'=>true, 'length' => 191, 'description' => 'URI of the conversation'),
                 'created' => array('type' => 'datetime', 'not null' => true, 'description' => 'date this record was created'),
                 'modified' => array('type' => 'timestamp', 'not null' => true, 'description' => 'date this record was modified'),
@@ -63,25 +63,17 @@ class Conversation extends Managed_DataObject
      *
      * @return Conversation the new conversation DO
      */
-    static function create(Notice $notice, $uri=null)
+    static function create($uri=null, $created=null)
     {
-        if (empty($notice->id)) {
+        // Be aware that the Notice does not have an id yet since it's not inserted!
-            throw new ServerException(_('Tried to create conversation for not yet inserted notice'));
-        }
         $conv = new Conversation();
-        $conv->created = common_sql_now();
+        $conv->created = $created ?: common_sql_now();
-        $conv->id = $notice->id;
+        $conv->uri = $uri ?: sprintf('%s%s=%s:%s=%s',
-        $conv->uri = $uri ?: sprintf('%s%s=%d:%s=%s:%s=%x',
                              TagURI::mint(),
-                             'noticeId', $notice->id,
                              'objectType', 'thread',
-                             'crc32', crc32($notice->content));
+                             'nonce', common_random_hexstr(8));
-        $result = $conv->insert();
+        // This insert throws exceptions on failure
-
+        $conv->insert();
-        if ($result === false) {
-            common_log_db_error($conv, 'INSERT', __FILE__);
-            throw new ServerException(_('Failed to create conversation for notice'));
-        }
 
         return $conv;
     }
@@ -108,13 +100,8 @@ class Conversation extends Managed_DataObject
 
     static public function getUrlFromNotice(Notice $notice, $anchor=true)
     {
-        $conv = new Conversation();
+        $conv = Conversation::getByID($notice->conversation);
-        $conv->id = $notice->conversation;
+        return $conv->getUrl($anchor ? $notice->getID() : null);
-        if (!$conv->find(true)) {
-            common_debug('Conversation does not exist for notice ID: '.$notice->id);
-            throw new NoResultException($conv);
-        }
-        return $conv->getUrl($anchor ? $notice->id : null);
     }
 
     public function getUri()
@@ -125,18 +112,25 @@ class Conversation extends Managed_DataObject
     public function getUrl($noticeId=null)
     {
         // FIXME: the URL router should take notice-id as an argument...
-        return common_local_url('conversation', array('id' => $this->id)) .
+        return common_local_url('conversation', array('id' => $this->getID())) .
                 ($noticeId===null ? '' : "#notice-{$noticeId}");
     }
 
     // FIXME: ...will 500 ever be too low? Taken from ConversationAction::MAX_NOTICES
-    public function getNotices($offset=0, $limit=500, Profile $scoped=null)
+    public function getNotices(Profile $scoped=null, $offset=0, $limit=500)
     {
-        if ($scoped === null) {
+        $stream = new ConversationNoticeStream($this->getID(), $scoped);
-            $scoped = Profile::current();
-        }
-        $stream = new ConversationNoticeStream($this->id, $scoped);
         $notices = $stream->getNotices($offset, $limit);
         return $notices;
     }
+
+    public function insert()
+    {
+        $result = parent::insert();
+        if ($result === false) {
+            common_log_db_error($this, 'INSERT', __FILE__);
+            throw new ServerException(_('Failed to insert Conversation into database'));
+        }
+        return $result;
+    }
 }

classes/Notice.php

@@ -592,21 +592,25 @@ class Notice extends Managed_DataObject
                 $conv = Conversation::getKV('uri', $options['conversation']);
                 if ($conv instanceof Conversation) {
                     common_debug('Conversation stitched together from (probably) a reply to unknown remote user. Activity creation time ('.$notice->created.') should maybe be compared to conversation creation time ('.$conv->created.').');
-                    $notice->conversation = $conv->id;
                 } else {
-                    // Conversation URI was not found, so we must create it. But we can't create it
+                    // Conversation entry with specified URI was not found, so we must create it.
-                    // until we have a Notice ID because of the database layout...
+                    common_debug('Conversation URI not found, so we will create it with the URI given in the options to Notice::saveNew: '.$options['conversation']);
-                    // $options['conversation'] will be used later after the $notice->insert();
+                    // The insert in Conversation::create throws exception on failure
-                    common_debug('Conversation URI not found, so we have to create it after inserting this Notice: '.$options['conversation']);
+                    $conv = Conversation::create($options['conversation'], $notice->created);
                 }
-            } else {
+                $notice->conversation = $conv->getID();
-                // If we're not using the attached conversation URI let's remove it
+                unset($conv);
-                // so we don't mistake ourselves later, when creating our own Conversation.
-                // This implies that the notice knows which conversation it belongs to.
-                $options['conversation'] = null;
             }
         }
 
+        // If it's not part of a conversation, it's the beginning of a new conversation.
+        if (empty($notice->conversation)) {
+            $conv = Conversation::create();
+            $notice->conversation = $conv->getID();
+            unset($conv);
+        }
+
+
         $notloc = new Notice_location();
         if (!empty($lat) && !empty($lon)) {
             $notloc->lat = $lat;
@@ -662,17 +666,6 @@ class Notice extends Managed_DataObject
                     $notloc->notice_id = $notice->getID();
                     $notloc->insert();  // store the notice location if it had any information
                 }
-
-                // If it's not part of a conversation, it's
-                // the beginning of a new conversation.
-                if (empty($notice->conversation)) {
-                    $orig = clone($notice);
-                    // $act->context->conversation will be null if it was not provided
-
-                    $conv = Conversation::create($notice, $options['conversation']);
-                    $notice->conversation = $conv->id;
-                    $notice->update($orig);
-                }
             } catch (Exception $e) {
                 // Let's test if we managed initial insert, which would imply
                 // failing on some update-part (check 'insert()'). Delete if
@@ -872,14 +865,22 @@ class Notice extends Managed_DataObject
                 $conv = Conversation::getKV('uri', $act->context->conversation);
                 if ($conv instanceof Conversation) {
                     common_debug('Conversation stitched together from (probably) a reply activity to unknown remote user. Activity creation time ('.$stored->created.') should maybe be compared to conversation creation time ('.$conv->created.').');
-                    $stored->conversation = $conv->getID();
                 } else {
-                    // Conversation URI was not found, so we must create it. But we can't create it
+                    // Conversation entry with specified URI was not found, so we must create it.
-                    // until we have a Notice ID because of the database layout...
+                    common_debug('Conversation URI not found, so we will create it with the URI given in the context of the activity: '.$act->context->conversation);
-                    // $options['conversation'] will be used later after the $stored->insert();
+                    // The insert in Conversation::create throws exception on failure
-                    common_debug('Conversation URI from activity context not found, so we have to create it after inserting this Notice: '.$act->context->conversation);
+                    $conv = Conversation::create($act->context->conversation, $stored->created);
+                }
+                $stored->conversation = $conv->getID();
+                unset($conv);
             }
         }
+
+        // If it's not part of a conversation, it's the beginning of a new conversation.
+        if (empty($stored->conversation)) {
+            $conv = Conversation::create();
+            $stored->conversation = $conv->getID();
+            unset($conv);
         }
 
         $notloc = null;
@@ -939,15 +940,7 @@ class Notice extends Managed_DataObject
                     throw new ServerException('Unsuccessful call to StoreActivityObject '.$stored->getUri() . ': '.$act->asString());
                 }
 
-                // If it's not part of a conversation, it's the beginning
+                // If something changed in the Notice during StoreActivityObject
-                // of a new one (or belongs to a previously unknown URI).
-                if (empty($stored->conversation)) {
-                    // $act->context->conversation will be null if it was not provided
-                    common_debug('Creating a new conversation for stored notice ID='.$stored->getID().' with URI: '.$act->context->conversation);
-                    $conv = Conversation::create($stored, $act->context->conversation);
-                    $stored->conversation = $conv->getID();
-                }
-
                 $stored->update($orig);
             } catch (Exception $e) {
                 if (empty($stored->id)) {

plugins/ConversationTree/ConversationTreePlugin.php

@@ -26,7 +26,7 @@ if (!defined('GNUSOCIAL')) { exit(1); }
 class ConversationTreePlugin extends Plugin
 {
     public function onStartShowConversation(Action $action, Conversation $conv, Profile $scoped=null) {
-        $nl = new ConversationTree($conv->getNotices(), $action);
+        $nl = new ConversationTree($conv->getNotices($action->getScoped()), $action);
         $cnt = $nl->show();
         return false;
     }

plugins/GNUsocialPhotos/actions/photo.php

@@ -92,7 +92,7 @@ class PhotoAction extends ManagedAction
         $this->element('style', array(), "#notice-{$this->photo->notice_id} div { display: none } #notice-{$this->photo->notice_id} ol li div { display: inline }");
 
         if (Event::handle('StartShowConversation', array($this, $this->conv, $this->scoped))) {
-            $notices = $this->conv->getNotices();
+            $notices = $this->conv->getNotices($this->scoped);
             $nl = new FullThreadedNoticeList($notices, $this, $this->scoped);
             $cnt = $nl->show();
         }

plugins/TwitterBridge/lib/twitterimport.php

@@ -184,6 +184,12 @@ class TwitterImport
 
         if (Event::handle('StartNoticeSave', array(&$notice))) {
 
+            if (empty($notice->conversation)) {
+                $conv = Conversation::create();
+                common_log(LOG_INFO, "No known conversation for status {$statusId} so a new one ({$conv->getID()}) was created.");
+                $notice->conversation = $conv->getID();
+            }
+
             $id = $notice->insert();
 
             if ($id === false) {
@@ -191,14 +197,6 @@ class TwitterImport
                 common_log(LOG_ERR, __METHOD__ . ' - Problem saving notice.');
             }
 
-            if (empty($notice->conversation)) {
-                $orig = clone($notice);
-                $conv = Conversation::create($notice);
-                common_log(LOG_INFO, "No known conversation for status {$statusId} so a new one ({$conv->id}) was created.");
-                $notice->conversation = $conv->id;
-                $notice->update($orig);
-            }
-
             Event::handle('EndNoticeSave', array($notice));
         }