search

markshust / magento2-module-ordergrid

The Order Grid module adds more details to the order grid in the admin.
MIT License
85 stars 30 forks source link

Extend correct parent to fix third-party column filtering #9

Closed ghost closed 3 years ago

ghost commented 3 years ago

Fixes #8

The re-definition of the collection in di.xml can prevent third party modules (and even some commerce modules) from displaying or filtering values correctly.

Ideally the default collection source shouldn't be replaced as it currently is in di.xml and everything should be done via plugin(s) to prevent overriding the default collection. I attempted to use plugins but would sadly require further work.

<item name="sales_order_grid_data_source" xsi:type="string">MarkShust\OrderGrid\Model\ResourceModel\Order\Grid\Collection</item>

For now, changing the MarkShust\OrderGrid\Model\ResourceModel\Order\Grid\Collection class to extend the default class defined in the sales module, means that any other modules that add filtering and columns now work correctly, since the plugins they add to the default class now also process correctly from the parent.

robaimes commented 3 years ago

Above PR was me, was in the process of switching accounts, sorry!

roby0404 commented 3 years ago

Hi everyone!

Is there an ETA on when this PR will be merged into master?

We encountered the same issue as this: https://github.com/markshust/magento2-module-ordergrid/issues/8 where columns in select don't have 'main_table' alias assigned to it, hence using filters fails with this error for example:

.CRITICAL: SQLSTATE[23000]: Integrity constraint violation: 1052 Column 'store_id' in where clause is ambiguous

We can always apply a patch to either extend the original class, or to just add this bit in '_initSelect':

$tableDescription = $this->getConnection()->describeTable($this->getMainTable());
        foreach ($tableDescription as $columnInfo) {
            $this->addFilterToMap($columnInfo['COLUMN_NAME'], 'main_table.' . $columnInfo['COLUMN_NAME']);
        }

But I assume that cleanest solution would be to resolve this inside the module.

Thanks and best regards!

robaimes commented 3 years ago

Is there an ETA on when this PR will be merged into master?

I think @markshust would be best to answer that 😅

markshust commented 3 years ago

@robaimes @roby0404 so sorry on the delay of this! I tested it and everything seems to work properly. Merging in. Thanks!