Skip to content
This repository was archived by the owner on Dec 19, 2019. It is now read-only.

Small image URL added to the product data #132

Merged
merged 19 commits into from
Sep 26, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\CatalogGraphQl\Model\Resolver\Product;

use Magento\Catalog\Helper\Image as CatalogImageHelper;
use Magento\Catalog\Model\Product;
use Magento\Catalog\Model\ResourceModel\Product\GalleryFactory as GalleryResourceFactory;
use Magento\Framework\App\Area;
use Magento\Framework\App\AreaList;
use Magento\Framework\GraphQl\Config\Element\Field;
use Magento\Framework\GraphQl\Query\Resolver\Value;
use Magento\Framework\GraphQl\Query\Resolver\ValueFactory;
use Magento\Framework\GraphQl\Query\ResolverInterface;
use Magento\Framework\GraphQl\Schema\Type\ResolveInfo;
use Magento\Store\Model\StoreManagerInterface;

/**
* Returns product's small image. If the small image is not set, returns a placeholder
*/
class SmallImageUrl implements ResolverInterface
{
/**
* @var GalleryResourceFactory
*/
private $galleryResourceFactory;

/**
* @var AreaList
*/
private $areaList;

/**
* @var CatalogImageHelper
*/
private $catalogImageHelper;

/**
* @var ValueFactory
*/
private $valueFactory;

/**
* @var StoreManagerInterface
*/
private $storeManager;

/**
* @param ValueFactory $valueFactory
* @param CatalogImageHelper $catalogImageHelper
* @param AreaList $areaList
* @param GalleryResourceFactory $galleryResourceFactory
* @param StoreManagerInterface $storeManager
*/
public function __construct(
ValueFactory $valueFactory,
CatalogImageHelper $catalogImageHelper,
AreaList $areaList,
GalleryResourceFactory $galleryResourceFactory,
StoreManagerInterface $storeManager
) {
$this->valueFactory = $valueFactory;
$this->catalogImageHelper = $catalogImageHelper;
$this->areaList = $areaList;
$this->galleryResourceFactory = $galleryResourceFactory;
$this->storeManager = $storeManager;
}

/**
* {@inheritdoc}
*/
public function resolve(
Field $field,
$context,
ResolveInfo $info,
array $value = null,
array $args = null
): Value {
if (!isset($value['model'])) {
$result = function () {
return null;
};
return $this->valueFactory->create($result);
}
/** @var Product $product */
$product = $value['model'];

/* If small_image is not loaded for product, need to load it separately */
if (!$product->getSmallImage()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use case for it to be not loaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's loaded when we specify both small_image and small_image_url in the GraphQl request. If we specify only small_image_url we don't have any information about the small image and cannot use catalogImageHelper for the image URL generation.

$galleryResource = $this->galleryResourceFactory->create();
$currentStoreId =$this->storeManager->getStore()->getId();
$productImages = $galleryResource->getProductImages($product, [$currentStoreId]);
$productSmallImage = $this->getSmallImageFromGallery($productImages);
$product->setSmallImage($productSmallImage);
}

/* Design area is necessary to return the correct storefront image URL (or a placeholder) */
$area = $this->areaList->getArea(Area::AREA_FRONTEND);
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to avoid area emulation. Instead we can copy configuration from storefront di.xml to graphql di.xml. @rogyar could you please check how much configuration need to be copied to make it work without area emulation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I will try to figure it out

$area->load(Area::PART_DESIGN);

$smallImageURL = $this->catalogImageHelper->init($product, 'product_small_image')->getUrl();
$product->getMediaAttributes();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this part to generate a correct URL to the image. The product media gallery does not contain any information about the product image URL (only relative path). Upon a product image URL generation, the system takes into account some factors (like static content signing). The loaded area is also taken into account (in the core image retrieving flow). That's why we need to use these operations.
By removing the area loading, for example, we will always have a path (incorrect) to the placeholder instead of image URL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need $product->getMediaAttributes() call specifically? It looks like temporal coupling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. It was an initial solution and I tried to reuse as much Magento core approaches as possible. Unfortunately, we have the temporal coupling here there in the core. I'm going to dig deeper and find more clear way for that. But, probably, we will need to introduce some new logic within a scope of this task for images URL generation that is different that we have in the code to avoid temporal coupling and other issues (especially performance issues).


$result = function () use ($smallImageURL) {
return $smallImageURL;
};

return $this->valueFactory->create($result);
}

/**
* Retrieves small image from the product gallery image
*
* @param array $productImages
* @return string|null
*/
private function getSmallImageFromGallery(array $productImages)
{
foreach ($productImages as $productImage) {
if ($productImage['attribute_code'] == 'small_image') {
return $productImage['filepath'];
}
}

return null;
}
}
1 change: 1 addition & 0 deletions app/code/Magento/CatalogGraphQl/etc/schema.graphqls
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,7 @@ interface ProductInterface @typeResolver(class: "Magento\\CatalogGraphQl\\Model\
meta_description: String @doc(description: "A brief overview of the product for search results listings, maximum 255 characters")
image: String @doc(description: "The relative path to the main image on the product page")
small_image: String @doc(description: "The relative path to the small image, which is used on catalog pages")
small_image_url: String @doc(description: "The small image URL, which is used on catalog pages") @resolver(class: "Magento\\CatalogGraphQl\\Model\\Resolver\\Product\\SmallImageUrl")

Choose a reason for hiding this comment

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

Presence of both small_image and small_image_url may confuse developer. We should find another names or provide more details in the description. Also seems like similar pairs should be added for image and thumbnail. As an alternative solution, a path to media files may be part of a config type

Copy link
Contributor

Choose a reason for hiding this comment

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

@rogyar how about having the following structure

small_image: ProductImage
type ProductImage
{
   url: String
   path: String
}

and then re-use it for other image types?

@misha-kotov please provide a reference to the related tickets for other image types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ticket for other image types #174

thumbnail: String @doc(description: "The relative path to the product's thumbnail image")
new_from_date: String @doc(description: "The beginning date for new product listings, and determines if the product is featured as a new product") @resolver(class: "Magento\\CatalogGraphQl\\Model\\Resolver\\Product\\NewFromTo")
new_to_date: String @doc(description: "The end date for new product listings") @resolver(class: "Magento\\CatalogGraphQl\\Model\\Resolver\\Product\\NewFromTo")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\GraphQl\Catalog;

use Magento\TestFramework\TestCase\GraphQlAbstract;

class MediaGalleryTest extends GraphQlAbstract
{
/**
* @var \Magento\TestFramework\ObjectManager
*/
private $objectManager;

protected function setUp()
{
$this->objectManager = \Magento\TestFramework\Helper\Bootstrap::getObjectManager();
}

/**
* @magentoApiDataFixture Magento/Catalog/_files/product_with_image.php
*/
public function testProductSmallImageUrlWithExistingImage()
{
$productSku = 'simple';
$query = <<<QUERY
{
products(filter: {sku: {eq: "{$productSku}"}}) {
items {
small_image_url
}
}
}
QUERY;
$response = $this->graphQlQuery($query);

self::assertArrayHasKey('small_image_url', $response['products']['items'][0]);
self::assertContains('magento_image.jpg', $response['products']['items'][0]['small_image_url']);
self::assertTrue($this->checkImageExists($response['products']['items'][0]['small_image_url']));
}

/**
* small_image_url should contain a placeholder when there's no small image assigned
* to the product
*
* @magentoApiDataFixture Magento/Catalog/_files/product_simple.php
*/
public function testProductSmallImageUrlWithNoImage()
{
$productSku = 'simple';
$query = <<<QUERY
{
products(filter: {sku: {eq: "{$productSku}"}}) {
items {
small_image_url
}
}
}
QUERY;
$response = $this->graphQlQuery($query);
self::assertArrayHasKey('small_image_url', $response['products']['items'][0]);
self::assertContains('placeholder/small_image.jpg', $response['products']['items'][0]['small_image_url']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we should verify that the image can be loaded using returned URL. Can be done just with curl request.

self::assertTrue($this->checkImageExists($response['products']['items'][0]['small_image_url']));
}

/**
* @param string $url
* @return bool
*/
private function checkImageExists(string $url): bool
{
$connection = curl_init($url);
curl_setopt($connection, CURLOPT_HEADER, true);
curl_setopt($connection, CURLOPT_NOBODY, true);
curl_setopt($connection, CURLOPT_RETURNTRANSFER, 1);
curl_exec($connection);
$responseStatus = curl_getinfo($connection, CURLINFO_HTTP_CODE);

return $responseStatus === 200 ? true : false;
}
}