No throw when Mage_Admin_Model_User::delete fails

[midlan]

https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/code/core/Mage/Admin/Model/Resource/User.php#L219

inside this catch is not thrown any error, so user is not deleted and you get green message:

The user has been deleted.

But the user still remains in database and also can login. It return false but the return value is ignored in controller: \Mage_Adminhtml_Permissions_UserController::deleteAction

It is bug or it is there for some purpose?

[Sekiphp]

@midlan In my opinion this is bug, because is ignored returned value from delete method on admin/user model.

I suggest that there should be (at least):

Use return value of model here https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/code/core/Mage/Adminhtml/controllers/Permissions/UserController.php#L187

            try {
                $model = Mage::getModel('admin/user');
                $model->setId($id);
                if ($model->delete()) {
                      Mage::getSingleton('adminhtml/session')->addSuccess($this->__('The user has been deleted.'));
                }
                else {
                      Mage::getSingleton('adminhtml/session')->addSuccess($this->__('Deleting of user was terminated!'));
                }
                $this->_redirect('*/*/');
                return;
            }
            catch (Exception $e) {
                Mage::getSingleton('adminhtml/session')->addError($e->getMessage());
                $this->_redirect('*/*/edit', array('user_id' => $this->getRequest()->getParam('user_id')));
                return;
            }

Log exception which is lost here https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/code/core/Mage/Admin/Model/Resource/User.php#L209:

        try {
            $conditions = array(
                'user_id = ?' => $uid
            );

            $adapter->delete($this->getMainTable(), $conditions);
            $adapter->delete($this->getTable('admin/role'), $conditions);
            $adapter->commit();
        } catch (Mage_Core_Exception $e) {
            $adapter->rollBack();
            throw $e;
        } catch (Exception $e) {
            Mage::logException($e);
            $adapter->rollBack();
            return false;
        }

[colinmollenhour]

I think instead of returning false on that second to last line of User.php the exception should simply be rethrown (throw $e;). This is the common pattern throughout Magento. The error message "Deleting of user was terminated!" doesn't help the user at all or even make sense so should just be removed and the return value not checked at all.

[midlan]

@colinmollenhour
I agree. But what about backcompatibility? Someone may rely on the return value and only catching exceptions of type Mage_Core_Exception .

[colinmollenhour]

It should be noted then as a BC change. We are still working out exactly how to deal with BC changes, though...

[elidrissidev]

Closing as fixed.

[addison74]

Today, for the first time, the number of reported issues is equal to the number of open PRs. 135 :))