-
Notifications
You must be signed in to change notification settings - Fork 9.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add new GraphQL query for single product retrieval #28316
base: 2.4-develop
Are you sure you want to change the base?
Changes from 1 commit
e5ae941
5d21f23
4e17fd7
7c02282
b689765
9539a75
578ea6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
<?php | ||
/** | ||
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
declare(strict_types=1); | ||
|
||
namespace Magento\CatalogGraphQl\Model\Resolver\Product\DataProvider; | ||
|
||
use Magento\Catalog\Api\Data\ProductInterface; | ||
use Magento\Catalog\Model\ProductFactory; | ||
use Magento\Catalog\Model\ResourceModel\Product as ProductResourceModel; | ||
|
||
/** | ||
* Product data provider, used for GraphQL resolver processing. | ||
*/ | ||
class ProductDataProvider implements ProductDataProviderInterface | ||
{ | ||
/** | ||
* @var ProductResourceModel | ||
*/ | ||
private $productResourceModel; | ||
|
||
/** | ||
* @var ProductFactory | ||
*/ | ||
private $productFactory; | ||
|
||
/** | ||
* ProductDataProvider constructor. | ||
* | ||
* @param ProductResourceModel $productResourceModel | ||
* @param ProductFactory $productFactory | ||
*/ | ||
public function __construct( | ||
ProductResourceModel $productResourceModel, | ||
ProductFactory $productFactory | ||
) { | ||
$this->productResourceModel = $productResourceModel; | ||
$this->productFactory = $productFactory; | ||
} | ||
|
||
/** | ||
* Get product data by ID with full data set | ||
* | ||
* @param int $productId | ||
* @param array $attributeCodes | ||
* @return ProductInterface|Product | ||
*/ | ||
public function getProductById(int $productId, array $attributeCodes) | ||
{ | ||
/** @var \Magento\Catalog\Model\Product $product */ | ||
$product = $this->productFactory->create(); | ||
$this->productResourceModel->load($product, $productId, $attributeCodes); | ||
|
||
return $product; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
<?php | ||
/** | ||
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
declare(strict_types=1); | ||
|
||
namespace Magento\CatalogGraphQl\Model\Resolver\Product\DataProvider; | ||
|
||
use Magento\Catalog\Api\Data\ProductInterface; | ||
|
||
/** | ||
* Provides product data by product ID. | ||
*/ | ||
interface ProductDataProviderInterface | ||
{ | ||
/** | ||
* Retrieve product by product ID. | ||
* | ||
* @param int $productId | ||
* @param array $attributeCodes | ||
* @return ProductInterface | ||
*/ | ||
public function getProductById(int $productId, array $attributeCodes); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
<?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\Model\Product; | ||
use Magento\Framework\GraphQl\Query\Resolver\IdentityInterface; | ||
|
||
/** | ||
* Identity for resolved product by ID | ||
*/ | ||
class ProductIdIdentity implements IdentityInterface | ||
{ | ||
/** | ||
* @var string | ||
*/ | ||
private $cacheTag = Product::CACHE_TAG; | ||
|
||
/** | ||
* Get product id for cache tag | ||
* | ||
* @param array $resolvedData | ||
* @return array | ||
*/ | ||
public function getIdentities(array $resolvedData): array | ||
{ | ||
return empty($resolvedData['id']) ? | ||
[] : [$this->cacheTag, $resolvedData['id']]; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
<?php | ||
/** | ||
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
declare(strict_types=1); | ||
|
||
namespace Magento\CatalogGraphQl\Model\Resolver; | ||
|
||
use Magento\CatalogGraphQl\Model\Resolver\Product\DataProvider\ProductDataProviderInterface; | ||
use Magento\CatalogGraphQl\Model\Resolver\Product\ProductFieldsSelector; | ||
use Magento\Framework\GraphQl\Config\Element\Field; | ||
use Magento\Framework\GraphQl\Exception\GraphQlInputException; | ||
use Magento\Framework\GraphQl\Query\ResolverInterface; | ||
use Magento\Framework\GraphQl\Schema\Type\ResolveInfo; | ||
|
||
/** | ||
* Product by ID resolver, used for GraphQL request processing. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note: Filename/Classname/Code comment all still reference the old behavior in this PR (before we switched to multiple IDs as input) |
||
*/ | ||
class ProductId implements ResolverInterface | ||
{ | ||
/** | ||
* @var ProductDataProviderInterface | ||
*/ | ||
private $productDataProvider; | ||
|
||
/** | ||
* @var ProductFieldsSelector | ||
*/ | ||
private $productFieldsSelector; | ||
|
||
/** | ||
* ProductId constructor. | ||
* | ||
* @param ProductDataProviderInterface $productDataProvider | ||
* @param ProductFieldsSelector $productFieldsSelector | ||
*/ | ||
public function __construct( | ||
ProductDataProviderInterface $productDataProvider, | ||
ProductFieldsSelector $productFieldsSelector | ||
) { | ||
$this->productDataProvider = $productDataProvider; | ||
$this->productFieldsSelector = $productFieldsSelector; | ||
} | ||
|
||
/** | ||
* @inheritdoc | ||
*/ | ||
public function resolve( | ||
Field $field, | ||
$context, | ||
ResolveInfo $info, | ||
array $value = null, | ||
array $args = null | ||
) { | ||
$productId = (int) ($args['id'] ?? 0); | ||
$fields = $this->productFieldsSelector->getProductFieldsFromInfo($info); | ||
|
||
if (!$productId) { | ||
throw new GraphQlInputException( | ||
__("'id' input argument is required.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming this validation code can go away once the input arg is required via https://github.com/magento/magento2/pull/28316/files#r438190331 |
||
); | ||
} | ||
|
||
/** @var \Magento\Catalog\Model\Product $product */ | ||
$product = $this->productDataProvider->getProductById($productId, $fields); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you tried the following scenario? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
$data = $product->getData(); | ||
$data['model'] = $product; | ||
|
||
return $data; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,6 +10,10 @@ type Query { | |||||
sort: ProductAttributeSortInput @doc(description: "Specifies which attributes to sort on, and whether to return the results in ascending or descending order.") | ||||||
): Products | ||||||
@resolver(class: "Magento\\CatalogGraphQl\\Model\\Resolver\\Products") @doc(description: "The products query searches for products that match the criteria specified in the search and filter attributes.") @cache(cacheIdentity: "Magento\\CatalogGraphQl\\Model\\Resolver\\Product\\Identity") | ||||||
product ( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this endpoint does not support multiple IDs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seconding this - input should be a non-nullable list type of IDs. Fun fact: GraphQL spec allows clients to pass a single value when the input is a list type, and it will coerce to a list length of 1 with that value: In other words, a list type will work fine for both use-cases, with some nice syntax sugar from GraphQL. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One other note: if we accept a list type as an input, we'll need a different name for this field that isn't singular. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure that How about something like |
||||||
id: Int @doc(description: "Id of the product.") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The argument should be required. Also, @kokoc what if we change the type to
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional, but don't really need this comment - the schema here is plenty declarative. If it's not, we can call the field |
||||||
): ProductInterface | ||||||
@resolver(class: "Magento\\CatalogGraphQl\\Model\\Resolver\\ProductId") @doc(description: "The product query searches for product that match the criteria specified in the search and filter attributes.") @cache(cacheIdentity: "Magento\\CatalogGraphQl\\Model\\Resolver\\Product\\ProductIdIdentity") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "that match the criteria specified in the search and filter attributes" doesn't seem correct if the query only has an attribute to specify the id of the product. "The product query searches for a product that matches the specified id." feels a bit more closer to what it should be, looking at the attribute. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for your suggestion, the description has been adjusted. |
||||||
category ( | ||||||
id: Int @doc(description: "Id of the category.") | ||||||
): CategoryTree | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, maybe this part was agreed previously. But I would use return types hinting since we use strict_types declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't agreed previously, I just missed the strict type here.
I've just added it.
Thanks!