search

OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
870 stars 436 forks source link

Anonymous relevant blocks #521

Closed rafaelpatro closed 2 years ago

rafaelpatro commented 6 years ago

Hi guys! Do you know why the hell some important grids in backend were developed using anonymous blocks? There is no _adminhtml_sales_shipmentgrid, or _adminhtml_sales_invoicegrid, _or adminhtml_sales_creditmemogrid. Instead of we have anonymous blocks in these controllers... https://github.com/OpenMage/magento-lts/blob/f78f0c2421555d3c3f0f18f17a86cdd2a7315972/app/code/core/Mage/Adminhtml/Controller/Sales/Shipment.php#L65

On the other hand we have _adminhtml_sales_ordergrid perfectly designed. So we can easily apply class block methods... add export type, remove columns, add columns... https://github.com/OpenMage/magento-lts/blob/1.9.3.x/app/design/adminhtml/default/default/layout/sales.xml#L32

Example: add a custom column...

<reference name="sales_order.grid">
    <action method="addColumnAfter">
        <columnId>my_order_field</columnId>
        <arguments module="sales" translate="header">
            <header>My Field</header>
            <index>entity_id</index>
            <type>text</type>
            <width>110px</width>
        </arguments>
        <after>grand_total</after>
    </action>
</reference>

Is there a sensible reason why that blocks were not designed as order block? A security risk or something else? Could it be an improvement for this project?

colinmollenhour commented 6 years ago

I really don't think it was for any security reason. I think it could be changed and it would be an improvement, but then it might also break some extensions which assume it was done the old way..

tmotyl commented 6 years ago

I dont consider it breaking more than any bugfix. I dont see a usecase where somebody would rely on block being anonym.

colinmollenhour commented 6 years ago

It would take a lot of effort, probably resulting in a controller rewrite in many cases which wouldn't get broken since the update would be effectively bypassed. So I'm with you on accepting the change, but my personal sample size of extensions is small; does anyone have any examples of extensions updating grids that would be harmed by this change?

fballiano commented 2 years ago

I'm closing this issue since it doesn't have feedback since 3 years, also it would be a huge amount of work to fix it, for a really small benefit. You're welcome to reopen it if you feel so, possible with a PR.