From 0fd2ad649e8572b10102ff2e14861e7558685c69 Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Wed, 6 Jan 2016 13:58:46 +0100 Subject: [PATCH] Conversation IDs (again) no longer based on Notice ID --- actions/conversation.php | 27 ++------ classes/Conversation.php | 52 +++++++-------- classes/Notice.php | 63 +++++++++---------- .../ConversationTreePlugin.php | 2 +- plugins/GNUsocialPhotos/actions/photo.php | 2 +- plugins/TwitterBridge/lib/twitterimport.php | 14 ++--- 6 files changed, 65 insertions(+), 95 deletions(-) diff --git a/actions/conversation.php b/actions/conversation.php index b1cb50abac..84178982e6 100644 --- a/actions/conversation.php +++ b/actions/conversation.php @@ -49,24 +49,9 @@ class ConversationAction extends ManagedAction var $page = null; var $notices = null; - /** - * Initialization. - * - * @param array $args Web and URL arguments - * - * @return boolean false if id not passed in - */ - protected function prepare(array $args=array()) + protected function doPreparation() { - parent::prepare($args); - $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; + $this->conv = Conversation::getByID($this->int('id')); } /** @@ -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. diff --git a/classes/Conversation.php b/classes/Conversation.php index 9ef1e06b76..3bad400174 100644 --- a/classes/Conversation.php +++ b/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)) { - throw new ServerException(_('Tried to create conversation for not yet inserted notice')); - } + // Be aware that the Notice does not have an id yet since it's not inserted! $conv = new Conversation(); - $conv->created = common_sql_now(); - $conv->id = $notice->id; - $conv->uri = $uri ?: sprintf('%s%s=%d:%s=%s:%s=%x', + $conv->created = $created ?: common_sql_now(); + $conv->uri = $uri ?: sprintf('%s%s=%s:%s=%s', TagURI::mint(), - 'noticeId', $notice->id, 'objectType', 'thread', - 'crc32', crc32($notice->content)); - $result = $conv->insert(); - - if ($result === false) { - common_log_db_error($conv, 'INSERT', __FILE__); - throw new ServerException(_('Failed to create conversation for notice')); - } + 'nonce', common_random_hexstr(8)); + // This insert throws exceptions on failure + $conv->insert(); return $conv; } @@ -108,13 +100,8 @@ class Conversation extends Managed_DataObject static public function getUrlFromNotice(Notice $notice, $anchor=true) { - $conv = new Conversation(); - $conv->id = $notice->conversation; - 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); + $conv = Conversation::getByID($notice->conversation); + return $conv->getUrl($anchor ? $notice->getID() : 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) { - $scoped = Profile::current(); - } - $stream = new ConversationNoticeStream($this->id, $scoped); + $stream = new ConversationNoticeStream($this->getID(), $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; + } } diff --git a/classes/Notice.php b/classes/Notice.php index c99da34c73..138c9f1b64 100644 --- a/classes/Notice.php +++ b/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 - // until we have a Notice ID because of the database layout... - // $options['conversation'] will be used later after the $notice->insert(); - common_debug('Conversation URI not found, so we have to create it after inserting this Notice: '.$options['conversation']); + // Conversation entry with specified URI was not found, so we must create it. + common_debug('Conversation URI not found, so we will create it with the URI given in the options to Notice::saveNew: '.$options['conversation']); + // The insert in Conversation::create throws exception on failure + $conv = Conversation::create($options['conversation'], $notice->created); } - } else { - // If we're not using the attached conversation URI let's remove it - // 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; + $notice->conversation = $conv->getID(); + unset($conv); } } + // 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,16 +865,24 @@ 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 - // until we have a Notice ID because of the database layout... - // $options['conversation'] will be used later after the $stored->insert(); - common_debug('Conversation URI from activity context not found, so we have to create it after inserting this Notice: '.$act->context->conversation); + // Conversation entry with specified URI was not found, so we must create it. + common_debug('Conversation URI not found, so we will create it with the URI given in the context of the activity: '.$act->context->conversation); + // The insert in Conversation::create throws exception on failure + $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; if ($act->context instanceof ActivityContext) { if ($act->context->location instanceof Location) { @@ -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 - // 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(); - } - + // If something changed in the Notice during StoreActivityObject $stored->update($orig); } catch (Exception $e) { if (empty($stored->id)) { diff --git a/plugins/ConversationTree/ConversationTreePlugin.php b/plugins/ConversationTree/ConversationTreePlugin.php index 5532adb66e..042677f824 100644 --- a/plugins/ConversationTree/ConversationTreePlugin.php +++ b/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; } diff --git a/plugins/GNUsocialPhotos/actions/photo.php b/plugins/GNUsocialPhotos/actions/photo.php index 6e807b4ccc..ecd64919c5 100644 --- a/plugins/GNUsocialPhotos/actions/photo.php +++ b/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(); } diff --git a/plugins/TwitterBridge/lib/twitterimport.php b/plugins/TwitterBridge/lib/twitterimport.php index a7cac7fd0d..74495d7ba3 100644 --- a/plugins/TwitterBridge/lib/twitterimport.php +++ b/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)); } -- 2.39.5