Add getters to product image builder

[VincentMarmiesse]

Hello,

Description

I have added the attributes getter into the product image builder, class Magento\Catalog\Block\Product\ImageBuilder in order to get them in a plugin for example. We may want to add some attributes and use $subject->getProduct() to get product's data.

Fixed Issues (if relevant)

N/A.

Manual testing scenarios

N/A.

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 on Travis CI are green)

[ihor-sviziev]

In general builder should not have getters, but it leads to issues with using plugins or replace builder with your one, that is worst because risk of conflicting with another modules is much higher. We also got this issue recently, we were doing some workarounds.

I'd like to add some description to phpdoc blocks of these getters that they are designed to be used in plugins only (I don't like this idea too much)
OR add product as additional parameter to list of attributes that are passed to imageFactory in https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Catalog/Block/Product/ImageBuilder.php#L139-L151, so you wouldn't need to have any plugin for builder

[orlangur]

Hi @VincentMarmiesse,

Same as #10792 (comment) - please provide exact customization scenario you trying to achieve with plugin.

Breaking class encapsulation is not a good idea in general and especially attaching plugins to getters as they can be called arbitrary amount of times being public.

[ihor-sviziev]

@orlangur in my case I was trying to add data- attributes to the image that are depends on the product. Reason of adding plugin in my case - we are loosing product instance in the image block during creating it in https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Catalog/Block/Product/ImageBuilder.php#L139-L151

[VincentMarmiesse]

Hi @orlangur,

I wanted to add a 2x image on my products so I needed the 2x image url, width and height exactly like the current image and add this data to the returned array of the create function.

As @ihor-sviziev said, I was unable to get product information in the plugin.

[orlangur]

@VincentMarmiesse you mean add a second image for each product?

[VincentMarmiesse]

@orlangur not really a second image but 2x data, with srcset attribute, so it renders like that

<img src="<1x url with standard Magento data>" alt="..." srcset="<2x url with my plugin data>" width="<1x width with standard Magento data>" height=<1x height with standard Magento data>" />

[ihor-sviziev]

I don't like idea to add getters for builder, it's not good idea to get plugins for builder. Better idea would be send needed data to block and write plugins for block if it's needed.

My vision is following:

1. add product to list of attributes that will be passed to Image Block Factory in https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Catalog/Block/Product/ImageBuilder.php#L139-L151
2. Add additional check to \Magento\Catalog\Block\Product\Image::__construct if we have $data['product'] - then set value to $this->product (property already exists, so let's fill it)
3. Add getter for $product property in \Magento\Catalog\Block\Product\Image that allow you to customize block as you want

[ihor-sviziev]

@VincentMarmiesse please wait for answer from @orlangur before doing requested changed, might be he has better ideas

[orlangur]

Hi @VincentMarmiesse, together with @ihor-sviziev we came to conclusion to just add 'product_id' into https://github.com/magento/magento2/blob/2.2-develop/app/code/Magento/Catalog/Block/Product/ImageBuilder.php#L139-L151 instead of such getters and instead of product model (then product instance in plugin may be obtained via repository).

Please share your thoughts regarding suggested implementation approach, if it covers scenarios you faced with.

If so, please apply necessary changes and force push as a single commit.

[VincentMarmiesse]

Hi @orlangur,

Yes it covers my needs, I update my PR ASAP.

[ihor-sviziev]

Hi @VincentMarmiesse,
Are you interested in finalizing your PR? Do you need any help?

[VincentMarmiesse]

Hi @ihor-sviziev @orlangur,

I have updated my PR according to your recommandations.

Just to know, we are adding the product_id so when we'll want to get the product, we'll have to load it, is there any reason to no add the product model directly?

Thanks.

[ihor-sviziev]

@VincentMarmiesse becasue view should not know about product, it should know minimum data that it needs to render block

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-865 has been created to process this Pull Request

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-865 has been created to process this Pull Request

[ihor-sviziev]

Hi @VincentMarmiesse, thank you for your contribution. Your changes were merged. Could you please create new PR to 2.3-develop branch with the same changes?