Fixed Thread:preSave() does not give expected type of error without a forum

X

Xon

Well-known member
Affected version
2.2.6
If $forum is null, then the code which uses $forumTypeHandler which is on all branches after fetching it from $forum; will be accessing a method on a null object instead of throwing/logging a useful/standard error message.

PHP: 
$forumTypeHandler = $forum ? $forum->TypeHandler : null;

if (!$this->discussion_type)
{
...
 
Yep you're right, but what are the specific circumstances where a thread would not have a forum relation?

It's possible for the code to fallover earlier, if the prefix_id has changed, for example.

I'm reluctant to bail out here with an error in case existing add-on code is relying on us not erroring in this case. Maybe we just return if there's no $forum?

Either way, useful to know how this manifested itself (true of many bug reports from many users... ;))
 
Chris D said:
Yep you're right, but what are the specific circumstances where a thread would not have a forum relation?
Click to expand...
Either from a bug from an old vBulletin importer or bugs from older XenForo versions (around deleting forums?).

Chris D said:
It's possible for the code to fallover earlier, if the prefix_id has changed, for example.

I'm reluctant to bail out here with an error in case existing add-on code is relying on us not erroring in this case. Maybe we just return if there's no $forum?
Click to expand...
I believe the previous behaviour in older XenForo versions was the forum relation being null would be silently skipped.

Chris D said:
Either way, useful to know how this manifested itself (true of many bug reports from many users... ;))
Click to expand...
Somehow SpaceBattles has a thousand or so threads which do not actually have a forum. Ever so often batch update threads manages to reach them.

I used this script to dump them into a non-public "recycled bin" forum where our spam-cleaned threads are moved to;
PHP: 
<?php

ignore_user_abort(true);
$dir = __DIR__ . '/html' ;
require($dir . '/src/XF.php');
XF::start($dir);
$app = XF::setupApp('XF\Pub\App');

$db = $app->db();

$junkForumId = 13;
$threadIds = $db->fetchAllColumn('select thread_id from xf_thread where node_id not in (select node_id from xf_node)');

echo 'Updating ' .count($threadIds). ' threads'."\n";

foreach ($threadIds as $threadId)
{
    $thread = $app->find('XF:Thread', $threadId);
if (!$thread) {
  continue;
}
    $thread->node_id = $junkForumId;
    $thread->saveIfChanged();
    \XF::triggerRunOnce();
    $app->em()->clearEntityCache();
}
 
Last edited:
Xon said:
I believe the previous behaviour in older XenForo versions was the forum relation being null would be silently skipped.
Click to expand...
Interestingly, this may have been broken for some time, but less common.

2.0.0-2.1.latest:

PHP: 
protected function _preSave()
{
   if ($this->prefix_id && ($this->isChanged(['prefix_id', 'node_id'])))
   {
      if (!$this->Forum->isPrefixValid($this->prefix_id))
      {
         $this->prefix_id = 0;
      }
   }
}

But yeah I think silently skipping is about the best we can do here.
 
Thank you for reporting this issue, it has now been resolved. We are aiming to include any changes that have been made in a future XF release (2.2.7).

Change log:
Skip some parts of the _preSave method in Thread entity if thread does not have a forum.
Click to expand...
There may be a delay before changes are rolled out to the XenForo Community.
 
Back
Top Bottom