Initial value for is_used_defined columns

[paugnu]

Hi,

I'm trying to create a product grid that could be used on any project which had custom product attributes (for example: I have a project where 50 extra product attributes have been created).

I would like to create the product grid with the following pre-conditions:

• By default, I use <include keepAllSourceColumns="true"> to get all product attributes
• All custom attributes that have been created, should be available in the column list
• All custom attributes should be hidden by default (but available to be displayed)
• Magento default attributes will be hidden/displayed depending on basic criteria (just following the default magento displayed attributes by default in the original Magento product grid). This is something that I'll set up in my product-grid.xml file.

Does this feature make sense to you? Basically, I would like to avoid having to hardcode all the attributes I want to display in the grid. If I do it that way, the module I'm creating (https://github.com/catgento/magento2-admin-grids) won't be suitable to be used for other projects.

I did some tests, but I wasn't able to get it to work. I've created a PR (in my own repo) to show you the things I tested:

https://github.com/paugnu/magento2-hyva-admin/pull/1/files

[Vinai]

Thanks for opening the issue and introducing your use case! I understand your need. I wonder for how many other developers that feature would be useful.

Maybe it would make sense try and make this more generic. Maybe we could add a setting to the columns config to supply a class. This class then can implement the custom logic to determine the initially hidden state, or it could also change other column properties. The goal would be that it can be applied to more than only your use case.

Something like

<include keepAllSourceColumns="true" columDefinitionPostProcessor="\My\Module\Model\ColumnPreProcessor"/>

The class have a method would receive each column definition instance and would have a chance to replace values based on custom logic. In your example the PHP code might look something like this:

public function process(ColumnDefinitionInterface $columnDefinition): ColumnDefinitionInterface
{
    return $this->isUserDefinedAttribute($columnDefinition->getKey())
        ? $this->columnDefinitionFactory->create(merge($columnDefinition->toArray(), ['initiallyHidden' => 'true']))
        : $columnDefinition;

}

private function isUserDefinedAttribute(string $code): bool
{
    $attribute = $this->eavConfig->getAttribute('catalog_product', $code);
    return $attribute && $attribute->getIsUserDefined();
}

Alternatively the same possibility could also be provided by a new custom event along the lines of 'hyva_grid_column_definition_build_after_' . $gridName. This would have the benefit that more than one post-processor could be used for further customizations.

[paugnu]

Thanks, @Vinai, I'll take a look and see if I can implement something like that :)

[Vinai]

I think the event/observer solution is better. What do you think?

[paugnu]

It actually is.

When I first thought of this feature, I was thinking of writing a preference or plugin in my module. But I think that using event/observer is the best way to customize in any possible while keeping the Hyva Admin base as simple as possible, only adding features that could be a 'general' need.

Where exactly should this event be placed? When we have all the data ready to be displayed?

[Vinai]

Probably \Hyva\Admin\ViewModel\HyvaGridViewModel::buildColumnDefinitions would be a good place. It also knows the gridName, so it can be included in the event code.

Check \Hyva\Admin\Model\GridSourcePrefetchEventDispatcher::getGridNameEventSuffix for an example of transforming the grid name so it is suitable as part of the event code.

[paugnu]

Thanks @Vinai, I'll take a look. This will take me some time, but I'm happy to work on it :)

[paugnu]

Hi @Vinai,

I've done the modifications and created the observer in my module. The problem I found was that if I wanted to access ColumnDefinition class variables (initiallyHidden, for example), I need these variables to be public (instead of private).

public function execute(\Magento\Framework\Event\Observer $observer)
{
    $columnDefinitions = $observer->getData('column_definitions');

    foreach ($columnDefinitions as $key => $columnDefinition) {
        if ($columnDefinition->getKey() == 'id') {
            $columnDefinitions[$key]->initiallyHidden = 'true';
        }
    }

    return $columnDefinitions;
}

Actually, I'd say that to be able to do modifications (of any type), with the observer, all the variables should be public. Or at least:

• label
• initiallyHidden
• template
• renderAsUnsecureHtml
• sortOrder

Should I just make these variables public or maybe have a different approach? (you'll see my current changes in https://github.com/paugnu/magento2-hyva-admin/commit/c19617e7eecd8c3d68852b46653797974eceab6b)

[Vinai]

Most objects in Hyva_Admin are immutable after instantiation. They might use memoization internally, but there is no way to alter properties from the outside. Instead of changing a column definition, create a new instance. You can see an example in \Hyva\Admin\Model\GridSource::mergeColumnDefinitions

    private function mergeColumnDefinitions(
        ColumnDefinitionInterface $extracted,
        ?ColumnDefinitionInterface $configured,
        bool $isVisible
    ): ColumnDefinitionInterface {
        $configuredArray = $configured ? filter($configured->toArray()) : [];
        $extractedArray  = $extracted->toArray();
        $isVisibleArray  = ['isVisible' => $isVisible];
        return $this->columnDefinitionFactory->create(merge($extractedArray, $configuredArray, $isVisibleArray));
    }

In your case it would be the initiallyHidden property that would need to be set instead of isVisible in the example above.

[Vinai]

Some thoughts: maybe it makes sense to create a single class to dispatch the different events? That way the logic to convert the grid name to an event suffix wouldn't be duplicated.

The event probably needs a container object so PHP passes it by reference. See \Hyva\Admin\Model\GridSourceType\RepositorySourceType\SearchCriteriaEventContainer for an example. Then the observers could replace the column definitions as needed, and afterwards the event dispatcher can get the values back from the container.

[paugnu]

• Regarding modifying column definitions, I'll check what you propose (in addition, it seems to me that I was proposing something ugly...)
• It makes sense creating one class to dispatch all events. I'll look into it as well :)

[Vinai]

in addition, it seems to me that I was proposing something ugly...

I don't think calling it "ugly" is true. It just is different. My goal when I decided to make the classes immutable was to make it easier to use the classes in the way I intended them to be used. I didn't think of the current use case. Anyhow, I still think the current design works, but probably there should be a method that contains the merging logic, so it is easier to use.

That would probably make it a bit easier to use the column definitions in the way they are intended.

[paugnu]

Some thoughts: maybe it makes sense to create a single class to dispatch the different events? That way the logic to convert the grid name to an event suffix wouldn't be duplicated. The event probably needs a container object so PHP passes it by reference. See \Hyva\Admin\Model\GridSourceType\RepositorySourceType\SearchCriteriaEventContainer for an example. Then the observers could replace the column definitions as needed, and afterwards the event dispatcher can get the values back from the container.

If I understood you well, this would mean something like creating a generic class for dispatchers, for example:

• Model/HyvaAdminEventDispatcher.php

Also creating a new [generic] class for containers:

• Hyva/Admin/Model/GridSourceType/HyvaAdminEventContainer.php

Or should we create a new class for each type of container? (I see that the methods will be the same constuct/replace/get)

Arguments for HyvaAdminEventDispatcher:

• string $gridName
• string $recordType
• HyvaAdminEventContainer $container // or specific container

Would this be a good way to go?

[Vinai]

Yes, that's what I was thinking of 👍

[paugnu]

Hi @Vinai,

I've been trying to get this to work. But I'm struggling with the HyvaAdminEventContainer. Basically, in the example of SearchCriteriaEventContainer, when we call the function «getSearchCriteria», the return type is SearchCriteriaInterface, which is ready to be used.

When I use HyvaAdminEventContainer with more 'generic' functions like «getContainer / replaceContainer», I don't know how to make this function return the type I need (SearchCriteriaInterface for the search observer, columdefinition for my case in particular).

Could you please help me with this? (just a clue would be enough so that I know which way to go)

[Vinai]

In php there is no way. Other languages or static code analysis tools like psalm for php) have this idea of generics to solve this problem. In PHP you can either omit the return type, or add one getFoo/setFoo method pair for each type. Without return types the caller can add a phpdoc type hint to the returned value to make static code analysis work again. In this case probably a generic det/set pair without type is simpler.

[Vinai]

I’m occupied by other work this week, sorry if this comes a bit late.

I just realized my earlier comments mean backward incompatible changes, since events and the event arguments are part of the public extensibility api.

I think it’s important to avoid backward incompatible changes, even if an earlier design decision turned out to be bad.

Because of this, please don’t remove the existing event container, but rather add the new generic one as a new class.

To be honest, I’m not sure if the generic container is the best way forward. I would want to try it out if I where building that. I wish I could give clearer guidance, but sometimes things only become visible for me when I actually see them.

[paugnu]

Hi @Vinai, no problem at all. Last days I've been so busy that I didn't have the time to look deeper :)

I won't remove the existing container. I'll try to create the new one without returning any type. If this didn't work, please let me know if you'd rather to look into it later on, or if I can just create a «specific» container for ColumnDefinition.

All this is because I have a particular need for creating product grids :)

[Vinai]

Your use case it a typical one: customizing the default values for a column based on some business requirements. So thanks for bringing this up and being willing to work with me :)

[paugnu]

https://github.com/hyva-themes/magento2-hyva-admin/commit/14adbfc7e718b815176fa1ac54bf755587c9050d