New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move StoreView column renderer to Store module #10226

Closed
ldusan84 opened this Issue Jul 12, 2017 · 5 comments

Comments

Projects
None yet
7 participants
@ldusan84
Collaborator

ldusan84 commented Jul 12, 2017

When creating a custom grid using UI components, the "Store" column can be created like this:

<column name="store_id" class="Magento\Store\Ui\Component\Listing\Column\Store">
            <argument name="data" xsi:type="array">
                <item name="config" xsi:type="array">
                    <item name="bodyTmpl" xsi:type="string">ui/grid/cells/html</item>
                    <item name="sortable" xsi:type="boolean">false</item>
                    <item name="label" xsi:type="string" translate="true">Store View</item>
                </item>
            </argument>
        </column>

Where the class attribute value is the column renderer.

The Magento\Store\Ui\Component\Listing\Column\Store column renderer works correctly for the cases where the entity can belong to multiple store views such cms block, cms page, etc.

If you want your entity to belong to only one store view you need to use Magento\Search\Ui\Component\Listing\Column\StoreView as Magento\Store\Ui\Component\Listing\Column\Store doesn't work correctly in that case (shows blank for all store views).

I think that Magento\Search\Ui\Component\Listing\Column\StoreView belongs to the Store module instead of Search module.

There shouldn't be a need to have an additional dependency to Search module just to get the store view column renderer.

The naming can also be improved, maybe Magento\Store\Ui\Component\Listing\Column\MultipleStoreView and Magento\Store\Ui\Component\Listing\Column\SingleStoreView

Hope it makes sense.

@dmanners

This comment has been minimized.

Contributor

dmanners commented Jul 31, 2017

Hi @ldusan84 to me moving this into Magento\Store seems logical. I guess this one might just not have been migrated with the others like Magento\Store\Ui\Component\Listing\Column\Store and Magento\Store\Ui\Component\Listing\Column\Store\Options.

Also updating the naming could also help though I am not sure what that would mean with regards to BiC.

@korostii

This comment has been minimized.

korostii commented Aug 1, 2017

Also updating the naming could also help though I am not sure what that would mean with regards to BiC.

You can always create a new duplicate class (with new name) and mark the older one @deprecated instead of deleting it.

@piotrekkaminski

This comment has been minimized.

Contributor

piotrekkaminski commented Aug 22, 2017

Thank you for your submission.

We recently made some changes to the way we process GitHub submissions to more quickly identify and respond to core code issues.

Feature Requests and Improvements should now be submitted to the new Magento 2 Feature Requests and Improvements forum (see details here).

We are closing this GitHub ticket and have moved your request to the new forum.

@andkirby

This comment has been minimized.

andkirby commented Nov 15, 2017

Hi team,

Could you reopen this issue due to reason it really requires tiny fix?
I'll explain it.
That's about:

Magento\Store\Ui\Component\Listing\Column\Store doesn't work correctly in that case (shows blank for all store views).

My quick fix for this looks so:

class Store extends \Magento\Store\Ui\Component\Listing\Column\Store
{
    protected function prepareItem(array $item)
    {
        if (array_key_exists($this->storeKey, $item) && !is_array($item[$this->storeKey])) {
            $item[$this->storeKey] = [$item[$this->storeKey]];
        }

        return parent::prepareItem($item);
    }
}

So, I you can see it's converted into an array to make it work.

The method \Magento\Store\Ui\Component\Listing\Column\Store::prepareItem do the same, so it implies $item['store_id'] can be string or integer:

        if (!is_array($origStores)) {
            $origStores = [$origStores];
        }

In this the method has some contradiction for the case with admin store ID.
All what you need, just fix checking $item['store_id'] existence.

if (!empty($item[$this->storeKey])) {

I suggest to use array_key_exist() (or isset() if it's more suitable).

        if (array_key_exists($this->storeKey, $item)) {
            $origStores = $item[$this->storeKey];
        }

That's it. ;)

@andkirby

This comment has been minimized.

andkirby commented Nov 16, 2017

magento-engcom-team added the G1 Failed

Should I have added something?..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment