XoopsBlock::store() should return int, but it calls XoopsBlockHandler::insert() which returns bool

[mambax7]

Originally store() returned int, but now, since it calls XoopsBlockHandler::insert(), it returns bool:

    /**
     * Store Block Data to Database
     *
     * @return int $id
     *
     * @deprecated
     */
    public function store()
    {
        /** @var XoopsBlockHandler $blkhandler */
        $blkhandler = xoops_getHandler('block');
        return $blkhandler->insert($this);
    }

This breaks cases like this one:

        $block = new \XoopsBlock($bid);
        $clone = $block->xoopsClone();
        //...
        $newid = $clone->store();

Should I change all these cases to something like:

        $block = new \XoopsBlock($bid);
        $clone = $block->xoopsClone();
        //...
        if ($clone->store()) {
           $newid = $clone->id();  //get the id of the cloned block
        }

[zyspec]

We'll have to let @geekwright weigh in but in my opinion store() should be fixed to return an int. This was changed between 2.5.10 (current release) and 2.5.11 (not released yet). The store() method is deprecated in 2.5.11 (I assume you should use insert() instead) but it shouldn't break backward compatibility. The values returned by XoopsHandler::insert() and XoopsPersistableHandler::insert() methods has been inconsistent between the two for quite a while.

Having said that - the 2.5.10 version of the store() method could have returned false. So in the case where there was an error, the assignment of $newid above would have been broken anyway. So, in effect, the change brought a latent bug to light...

[mambax7]

@zyspec, you're right that the store() method could have returned false

I found this code that deals with that in an old module Blocksadmin:

    $newid = $cblock->store();
    if (!$newid) {
        xoops_cp_header();
        $cblock->getHtmlErrors();
        xoops_cp_footer();
        exit();
    }

[zyspec]

It looks like the 'fix' in 2.5.11 XoopsBlock should be something like this:

    public function store()
    {
        /** @var XoopsBlockHandler $blkhandler */
        $blkhandler = xoops_getHandler('block');
        $bid = false;
        if (true === $blkhandler->insert($this)) {
            $bid = $this->getVar('bid');
        }
        return $bid;
    }

[mambax7]

I think, that should work!