Setting image keep frame to false in theme's view.xml does not work

[brendanmckeown]

Steps to reproduce

1. In the current theme's view.xml file, add <frame>false</frame> to a specific image type as instructed in the docs: http://devdocs.magento.com/guides/v2.0/frontend-dev-guide/themes/theme-images.html . This image should also have a width and height setting and aspect_ratio set to true. For example:

            <image id="category_page_grid" type="small_image">
                <width>460</width>
                <height>460</height>
                <aspect_ratio>true</aspect_ratio>
                <frame>false</frame>
            </image>

1. Clear image cache, refresh a page with this specific image type.

Expected result

1. Image should keep its aspect ratio, not have any added white frame, and size should be within the configured width and height.

## Actual result 1. Image is forced to width and height with white frame, and does not keep original aspect ratio.

Cause:
The value from view.xml is passed to the method \Magento\Catalog\Model\Product\Image::setKeepFrame – which casts the string "false" to the boolean true. Therefore, it is not possible to set keep_frame to false from within the view.xml.

    /**
     * @param bool $keep
     * @return $this
     */
    public function setKeepFrame($keep)
    {
        $this->_keepFrame = (bool)$keep;
        return $this;
    }

After creating a plugin to convert the string value to an integer (which properly casts to a boolean), the image frame is not added.

    public function beforeSetKeepFrame($image, $keep)
    {
        if (is_string($keep)) {
            $keep = (strtolower($keep) === 'true') ? 1 : 0;
        }
        return [$keep];
    }

[leoquijano]

I can confirm the issue:

Setting the value to "false" in view.xml will make it true:
(bool) "false" = true

Setting the value to "0" in view.xml will make it empty:
empty("0") = true

Thanks @brendanmckeown for the workaround, it does work.

[zanilee]

Hi, @brendanmckeown!

Thank you for reporting this issue. Internal ticket MAGETWO-53411

[Yonn-Trimoreau]

+1

[markdavies]

This is still an issue months later, any update?

[PascalBrouwers]

@rgoncharuk Can you merge the pull request #7262 ?
It passed all checks, etc.

[ishakhsuvarov]

@brendanmckeown @markdavies @PascalBrouwers
PR #7262 is now merged into the develop branch. Issue should be fixed. Closing.

[stianmartinsen]

@brendanmckeown Before PR #7262 is live, do you have any idea why your fix works smoothly in dev mode, but not in production mode?

[tigerx7]

@stianmartinsen I may have found the reason why... by correcting the boolean value for keepFrame, it exposed a bug in https://github.com/magento/magento2/blob/2.1/app/code/Magento/Catalog/Model/Product/Image/ParamsBuilder.php in which build() checks the frame parameter with the empty function $frame = !empty($imageArguments['frame']) which will evaluate that if statement as false since empty will return true on a false value. Since the if statement is evaluated as false, ParamsBuilder::build() uses the classes default value for keepFrame which is true.

$frame = !empty($imageArguments['frame'])
    ? $imageArguments['frame']
    : $this->keepFrame;

So although the images are being created correctly now, since ParamsBuilder is returning the incorrect parameters that are used for the MD5 cache directory hash, the url path returned to the frontend gallery is pointing to the wrong cache directory.

This one was a frustrating bug to hunt down since it was giving the impression that the original problem fixed by PR #7262 was still occurring. I have no idea why empty() is being used to check the existence of the frame argument in build(). Perhaps isset() was meant to be used? I really wish Magento had a much more improved QA; too many critical bugs are still making their way into releases.