Removed dead code

[sreichel]

Description (*)

Removed dead code ...

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[tmotyl]

how does other grids behave? do they return string? or string or false? is there an interface?

[sreichel]

Other return string, for being clickable.

[ADDISON74]

Check this file https://github.com/OpenMage/magento-lts/blob/1.9.4.x/app/code/core/Mage/Adminhtml/Block/Urlrewrite/Grid.php. Maybe it should be included in the list.

line 113:

        //$this->addExportType('*/*/exportCsv', $this->__('CSV'));
        //$this->addExportType('*/*/exportXml', $this->__('XML'));
        return parent::_prepareColumns();
    }

    public function getRowUrl($row)
    {
        return $this->getUrl('*/*/edit', array('id'=>$row->getId()));
        //return $this->getUrl('*/*/view', array('id' => $row->getId()));

The export feature does not work and the last two lines seem to do the same thing.

[sreichel]

There is still commented out code in many places. This was only the beginning ... :)

[ADDISON74]

What do you think about the large number of TODOs that exist in the code? Will someone ever solve them? Those issues still exist?

[sreichel]

I thought about this ... e.g. deleted part. I'd keep most of todo comments (for now), but not within unfinished code. This was there since 1.1.

                //$m = $e->getMessage();
                //if ( eregi("^Resource '(.*)' not found", $m) ) {
                    // Deleting non existent resource rule from rules table
                    //$cond = $this->_write->quoteInto('resource_id = ?', $resource);
                    //$this->_write->delete(Mage::getSingleton('core/resource')->getTableName('admin/rule'), $cond);
                //} else {
                    //TODO: We need to log such exceptions to somewhere like a system/errors.log
                //}
            }
            /*
            switch ($rule['permission']) {
                case Mage_Admin_Model_Acl::RULE_PERM_ALLOW:
                    $acl->allow($role, $resource, $privileges, $assert);
                    break;

                case Mage_Admin_Model_Acl::RULE_PERM_DENY:
                    $acl->deny($role, $resource, $privileges, $assert);
                    break;
            }
            */

[github-actions]

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 1699468. ± Comparison against base commit cc320eb.

[Sdfendor]

@sreichel I wanted to personally thank you for one of the changes in this PR. In app/code/core/Mage/Admin/Model/Resource/Acl.php you replaced dead code and this todo

//TODO: We need to log such exceptions to somewhere like a system/errors.log

with code to log the exceptions. The method this belongs to loads all acl rules e.g. during a login. All rules from table admin_rules referencing resource_ids (acl nodes in the system.xml files) that no longer exist, raise exceptions that are logged by this code change.
Our store has seen a lot of modules come and go over its lifetime thus a quite a lot of rules were outdated and caused a lot exceptions accordingly. We never really noticed this because the exceptions had been ignored before and no other negative effects arose from unnecessary iterations in this foreach loop. But still we could clean up this mess 😀👍.

[luigifab]

We have also a lot of Zend_Acl_Exception: Resource 'admin/rewards/settings/action' not found in ... in our Sentry instance.