Skip to content

Conversation

thomas-kl1
Copy link
Member

This PR allows to add handlers thanks to the di.xml configuration file.

Description (*)

Today we can't add easily handlers to manage the output of a product attribute by using the output helper.
The solution is add a plugin on getHandlers methods, but you have to check in your side that you didn't already added the handler.
An other solution is to add the handlers once on the instantiation of the helper.

Fixed Issues (if relevant)

  1. Adding a handler to the Magento\Catalog\Helper\Output Class #12371: Adding a handler to the Magento\Catalog\Helper\Output Class

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Sep 2, 2019

Hi @thomas-kl1. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@sky-hub
Copy link
Member

sky-hub commented Sep 2, 2019

This implementation has a design issue and will not work as intended.

  1. Given the following di.xml configuration:
<type name="Magento\Catalog\Helper\Output">
        <arguments>
            <argument name="handlers" xsi:type="array">
                <item name="my-attribute-handler" xsi:type="string">MyProject\MyModule\Model\Product\Attribute\MyAttributeHandler</item>
            </argument>
        </arguments>
    </type>

the implementation will fail because method get handlers will return always empty because variable $method will always be equal to productAttribute @see \Magento\Catalog\Helper\Output::process() and method getHandler() returns return $this->_handlers[strtolower($method)] ?? [];

  1. Given the following di.xml configuration:
<type name="Magento\Catalog\Helper\Output">
        <arguments>
            <argument name="handlers" xsi:type="array">
                <item name="productAttribute" xsi:type="string">MyProject\MyModule\Model\Product\Attribute\MyAttributeHandler</item>
            </argument>
        </arguments>
    </type>

the handler will return correct class but only one handler can be added, meaning you can not have different handlers for different attributes, or adding handlers from multiple modules without replacing the existing handlers.

Can you please provide your di.xml configuration?

Update:
A solution to make this implementation work will be to use $handler['productAttribute'] as composite class and add handlers for each attribute in the composite class.
Example:

<type name="Magento\Catalog\Helper\Output">
        <arguments>
            <argument name="handlers" xsi:type="array">
                <item name="productAttribute" xsi:type="object">MyProject\MyModule\Model\Product\Attribute\ProductAttributeHandlerComposite</item>
            </argument>
        </arguments>
    </type>
<type name="MyProject\MyModule\Model\Product\Attribute\ProductAttributeHandlerComposite">
        <arguments>
            <argument name="my-attribute-code" xsi:type="object">MyProject\MyModule\Model\Product\Attribute\MyAttributeHandler</argument>
            <argument name="my-second-attribute-code" xsi:type="object">MyProject\MyModule\Model\Product\Attribute\MyAttributeHandler</argument>
        </arguments>
    </type>

but is not very clear for end developers how the implementation works.

@thomas-kl1
Copy link
Member Author

@sky-hub Actually it works as following, no need to make it more complex:

<type name="Magento\Catalog\Helper\Output">
    <arguments>
        <argument name="handlers" xsi:type="array">
            <item name="productattribute" xsi:type="array">
                <item name="handlerA" xsi:type="object">Vendor\Module\Handler\A</item>
                <item name="handlerB" xsi:type="object">Vendor\Module\Handler\B</item>
            </item>
        </argument>
    </arguments>
</type>

However a new handler interface should be introduced, method_exists is the worst.

@ghost ghost assigned sidolov Sep 3, 2019
@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-5757 has been created to process this Pull Request
✳️ @sidolov, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@@ -94,7 +102,7 @@ protected function _getTemplateProcessor()
*/
public function addHandler($method, $handler)
{
if (!is_object($handler)) {
if (!\is_object($handler)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please import it similar to strtolower.

After changes are applied and all builds are green, please squash them into a single commit so that we have perfectly clean history 😉

@thomas-kl1 thomas-kl1 requested a review from akaplya as a code owner September 24, 2019 08:32
@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-5757 has been created to process this Pull Request
✳️ @orlangur, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@engcom-Bravo
Copy link
Contributor

✔️ QA Passed

@VladimirZaets VladimirZaets added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Oct 17, 2019
@m2-assistant
Copy link

m2-assistant bot commented Oct 18, 2019

Hi @thomas-kl1, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@nahall
Copy link

nahall commented Jan 28, 2021

Is there documentation somewhere about how to now create a product attribute output helper since this code change was made? I see code samples from before the change that don't work but no code sample of how it is actually now designed to work. Thanks.

@thomas-kl1
Copy link
Member Author

thomas-kl1 commented Jan 29, 2021

@nahall No there is no documentation as far as I know,

<type name="Magento\Catalog\Helper\Output">
    <arguments>
        <argument name="handlers" xsi:type="array">
            <item name="productattribute" xsi:type="array">
                <item name="myHandler" xsi:type="object">Vendor\Module\Model\Handler\Output</item>
            </item>
            <item name="categoryattribute" xsi:type="array">
                <item name="myHandler" xsi:type="object">Vendor\Module\Model\Handler\Output</item>
            </item>
        </argument>
    </arguments>
</type>

Where Vendor\Module\Model\Handler\Output is:

namespace  Vendor\Module\Model\Handler;

use Magento\Catalog\Helper\Output as HelperOutput;

class Output
{
    public function productAttribute(HelperOutput $output, string $attributeHtml, array $params): string
    {
        // $params: ['product' => $product, 'attribute' => $attributeName]
        // todo your output handler
        return $attributeHtml;
    }

    public function categoryAttribute(HelperOutput $output, string $attributeHtml, array $params): string
    {
        // $params: ['category' => $category, 'attribute' => $attributeName]
        // todo your output handler
        return $attributeHtml;
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Component: Catalog Progress: accept Release Line: 2.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants