Fixed XF\Service\Thread\Editor does not use database transactions

[Xon]

Affected version

2.0.2

In XF\Pub\Controller\Post::actionEdit() setupPostEdit and setupFirstPostThreadEdit do not communicate, and thus don't share the same transaction.

This means post changes can be saved but thread changes fail to save (for whatever reason).

Worse, however is the Thread editor code path starting here;

$editor->save();

if ($threadEditor)
{
   $threadEditor->save();
}

In the class XF\Service\Thread\Editor

protected function _save()
{
   $thread = $this->thread;

   $thread->save(true, false);

   return $thread;
}

This call chain from $threadEditor->save(); does not setup a transaction! Initial checks show this applies to other call sites for Thread\Editor.

Note; It is fairly yuck at pushing Thread generated in setupPostEdit with the threadEditor that is sometimes floating around that code path.

 

[Mike]

This is fixed now. The thread editor service can now be passed into the post editor service which allows them to be validated and saved together.

The thread editor service also uses a transaction itself now.

 

[DragonByte Tech]

This is fixed now. The thread editor service can now be passed into the post editor service which allows them to be validated and saved together.

The thread editor service also uses a transaction itself now.

What sort of changes are required for 3rd party addons that interact with the thread editor service to take advantage of / be compatible with your changes?

Here's my _save method extension:

    /**
     * @return \XF\Entity\Thread
     * @throws \XF\PrintableException
     */
    protected function _save()
    {
        $thread = parent::_save();
        
        if ($this->ticketEditor)
        {
            $this->ticketEditor->save();
        }
        
        if ($this->ticketReassigner)
        {
            $this->ticketReassigner->finalizeReassign();
        }
        
        return $thread;
    }

Thanks!


Fillip

 

[Mike]

The only change to the thread editor is the removal of the "false" value passed to Entity::save() to prevent it from opening a transaction.

Independent of that change, strictly speaking, your extension should likely wrap the whole action in a transaction (which is roughly what this bug report is about) to ensure that an unexpected failure doesn't leave half saved data. (Arguably in this bug report, the thread and post edits are independent so doing one without the other is ok, though given the UI, it's clearer to have them behave together.)

 

[DragonByte Tech]

Independent of that change, strictly speaking, your extension should likely wrap the whole action in a transaction (which is roughly what this bug report is about) to ensure that an unexpected failure doesn't leave half saved data.

The ticket editor's _save method does have a transaction:

    protected function _save()
    {
        $ticket = $this->ticket;
        
        $db = $this->db();
        $db->beginTransaction();
        
        $ticket->save(true, false);
        
        \XF::runOnce('dbtechTicketCountsRebuild', function()
        {
            $this->repository('DBTech\Tickets:Ticket')->rebuildTicketCounts();
        });
        
        $db->commit();
        
        return $ticket;
    }

I was more wondering whether this change would mean I shouldn't begin a new transaction inside my Ticket Editor service and instead rely on an "over-arching transaction", if that makes sense.


Fillip

 

[Mike]

No, because of how you're extending this, your extensions aren't inside an existing transaction (it would be opened and closed in the parent save method).