Small image URL added to the product data #132

rogyar · 2018-07-26T08:26:58Z

Description

This PR adds an extra field to the ProductInterface with image URL. We already have the small_image field with information about the image path. However, the image path information is not very useful for PWA applications that build a list of products and require rather URLs than some data that needs to be parsed.

Fixed Issues (if relevant)

'small_image' in 'ProductInterface' does not have enough info #88 : 'small_image' in 'ProductInterface' does not have enough info

Manual testing scenarios

Create a new product with/without an image
Make the following request:

{
  products (search: "withimages") {
    items {
      small_image_url
    }
  }
}

You should see the small image URL in the response:

{
  "data": {
    "products": {
      "items": [
        {
          "small_image_url": "http://docker.graphql.loc:8087/media/catalog/product/cache/0c40cc075c35680261029fa6d255feee/1/6/16262-11.jpg"
        }
      ]
    }
  }
}

If the requested product does not have an image, a placeholder URL should be returned

vkublytskyi · 2018-07-26T14:55:03Z

app/code/Magento/CatalogGraphQl/etc/schema.graphqls

@@ -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")


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

@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.

Ticket for other image types #174

paliarush · 2018-09-04T21:53:02Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Product/SmallImageUrl.php

+        $product = $value['model'];
+
+        /* If small_image is not loaded for product, need to load it separately */
+        if (!$product->getSmallImage()) {


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

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.

paliarush · 2018-09-04T21:57:53Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Product/SmallImageUrl.php

+        $area->load(Area::PART_DESIGN);
+
+        $smallImageURL = $this->catalogImageHelper->init($product, 'product_small_image')->getUrl();
+        $product->getMediaAttributes();


Why is this needed?

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.

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

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).

paliarush · 2018-09-04T21:59:02Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Product/SmallImageUrl.php

+    /**
+     * Retrieves small image from the product gallery image
+     *
+     * @param $productImages


Please specify argument type.

paliarush · 2018-09-04T22:01:38Z

dev/tests/api-functional/testsuite/Magento/GraphQl/Catalog/MediaGalleryTest.php

+        $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']);


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

paliarush

Please address several comments above

paliarush · 2018-09-05T21:12:30Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Product/SmallImageUrl.php

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


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?

Sure. I will try to figure it out

paliarush · 2018-09-18T16:33:58Z

app/code/Magento/CatalogGraphQl/Model/Resolver/Product/Image.php

@@ -43,7 +44,7 @@ public function resolve(
        array $args = null
    ): array {
        if (!isset($value['model'])) {
-            throw new \LogicException(__("Cannot resolve entity model"));
+            throw new GraphQlInputException(__('"model" value should be specified'));


Why? Client knows nothing about the model. It must be passed from the parent resolver. I think before the change we had correct exception, the only change I would made is remove translation:
throw new \LogicException("Cannot resolve entity model");

#189
Has been created

…-url

…info -- Update benchmarks scenarios according to updated GraphQL Schema

rogyar added 2 commits July 19, 2018 16:06

Small image URL added to the product data

1288659

API-functional tests added

ca13aa8

vkublytskyi reviewed Jul 26, 2018

View reviewed changes

Use current store id instead of admin store id

f41eb3e

paliarush reviewed Sep 4, 2018

View reviewed changes

paliarush suggested changes Sep 4, 2018

View reviewed changes

rogyar added 2 commits September 5, 2018 16:00

API-functional test improvements

fe6d7f8

Added parameter type in DocBlock

a60e55d

misha-kotov mentioned this pull request Sep 5, 2018

Absolute image paths for Products #174

Closed

paliarush reviewed Sep 5, 2018

View reviewed changes

naydav added the Imported label Sep 11, 2018

magento-engcom-team assigned naydav Sep 11, 2018

magento-engcom-team added this to the Release: 2.3.0 milestone Sep 11, 2018

magento-engcom-team added Release Line: 2.3 Partner: Atwix partners-contribution labels Sep 11, 2018

naydav removed the Imported label Sep 11, 2018

rogyar added 3 commits September 12, 2018 13:48

Removed area simulation. Resolver improvements

6877798

Declared new type for product images

646f248

DocBlock comment adjustments

f8b59e7

naydav added the in-progress label Sep 13, 2018

Removed debug parts

a027ae7

paliarush approved these changes Sep 13, 2018

View reviewed changes

rogyar added 4 commits September 13, 2018 17:26

Removed placeholder test

c53bb4f

Making the resolver universal

5a674c7

Merge branch '2.3-develop' into 88-small-image-url

4de3a9e

Return scalar type instead of deferred

0ea7d50

naydav removed the broken-tests label Sep 18, 2018

paliarush approved these changes Sep 18, 2018

View reviewed changes

paliarush reviewed Sep 18, 2018

View reviewed changes

Valeriy Nayda added 2 commits September 20, 2018 17:28

Merge remote-tracking branch 'origin/2.3-develop' into 88-small-image…

45cc658

…-url

GraphQL-88: 'small_image' in 'ProductInterface' does not have enough …

dad6575

…info -- Update benchmarks scenarios according to updated GraphQL Schema

paliarush approved these changes Sep 20, 2018

View reviewed changes

magento-engcom-team modified the milestones: Release: 2.3.0, Release: 2.3.1 Sep 20, 2018

magento-engcom-team added the Component: Setup label Sep 20, 2018

naydav added Imported Complex PR and removed in-progress labels Sep 20, 2018

magento-engcom-team merged commit dad6575 into 2.3-develop Sep 26, 2018

magento-engcom-team pushed a commit that referenced this pull request Sep 26, 2018

ENGCOM-2948: Small image URL added to the product data #132

c5c8ea5

magento-engcom-team added Award: test coverage Award: complex labels Sep 26, 2018

naydav removed Complex PR Component: Setup Imported labels Sep 26, 2018

naydav mentioned this pull request Sep 26, 2018

'small_image' in 'ProductInterface' does not have enough info #88

Closed

naydav added backward-incompatible and removed 2.3.0-release labels Sep 26, 2018

zetlen mentioned this pull request Oct 2, 2018

Update queries for latest 2.3 schema and add validator script magento/pwa-studio#318

Merged

keharper added the requires-documentation label Nov 20, 2018

keharper mentioned this pull request Nov 21, 2018

GraphQL: Update product (catalog) endpoint docs for 2.3.1 magento/devdocs#3339

Closed

12 tasks

naydav removed this from the Release: 2.3.1 milestone Dec 7, 2018

cherdman mentioned this pull request Feb 26, 2019

DISCOVERY: Identify breaking changes and impact to PWA from 2.3.1 magento/pwa-studio#948

Closed

keharper added documentation-completed and removed requires-documentation labels Mar 2, 2019

naydav deleted the 88-small-image-url branch April 12, 2019 19:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Small image URL added to the product data #132

Small image URL added to the product data #132

rogyar commented Jul 26, 2018

vkublytskyi Jul 26, 2018

paliarush Sep 4, 2018

paliarush Sep 5, 2018

paliarush Sep 4, 2018

rogyar Sep 5, 2018

paliarush Sep 4, 2018

rogyar Sep 5, 2018

paliarush Sep 5, 2018

rogyar Sep 7, 2018

paliarush Sep 4, 2018

paliarush Sep 4, 2018

paliarush left a comment

paliarush Sep 5, 2018

rogyar Sep 7, 2018

paliarush Sep 18, 2018

naydav Sep 18, 2018

Small image URL added to the product data #132

Small image URL added to the product data #132

Conversation

rogyar commented Jul 26, 2018

Description

Fixed Issues (if relevant)

Manual testing scenarios

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paliarush left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment