Skip to content
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 fallback for Product_links position attribute if not set in request #12650

67 changes: 65 additions & 2 deletions app/code/Magento/Catalog/Model/Product/Link/SaveHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,27 @@ class SaveHandler
private $linkResource;

/**
* @var linkTypeProvider
*/
private $linkTypeProvider;

/**
* SaveHandler constructor.
* @param MetadataPool $metadataPool
* @param Link $linkResource
* @param ProductLinkRepositoryInterface $productLinkRepository
* @param \Magento\Catalog\Model\Product\LinkTypeProvider $linkTypeProvider
*/
public function __construct(
MetadataPool $metadataPool,
Link $linkResource,
ProductLinkRepositoryInterface $productLinkRepository
ProductLinkRepositoryInterface $productLinkRepository,
\Magento\Catalog\Model\Product\LinkTypeProvider $linkTypeProvider
) {
$this->metadataPool = $metadataPool;
$this->linkResource = $linkResource;
$this->productLinkRepository = $productLinkRepository;
$this->linkTypeProvider = $linkTypeProvider;
}

/**
Expand All @@ -55,17 +64,71 @@ public function execute($entityType, $entity)
{
$link = $entity->getData($this->metadataPool->getMetadata($entityType)->getLinkField());
if ($this->linkResource->hasProductLinks($link)) {
/** @var \Magento\Catalog\Api\Data\ProductInterface $entity*/
/** @var \Magento\Catalog\Api\Data\ProductInterface $entity */
foreach ($this->productLinkRepository->getList($entity) as $link) {
$this->productLinkRepository->delete($link);
}
}
$productLinks = $entity->getProductLinks();

// Build links per type
$linksByType = [];
foreach ($productLinks as $link) {
Copy link
Contributor

Choose a reason for hiding this comment

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

After your last refactoring, we don't need the variable $productLinks in line 72 anymore. Call $entity->getProductLinks() directly in line 72. By doing this, we adhere to the best practice that a variable shouldn't be changed anymore after initialisation.

$linksByType[$link->getLinkType()][] = $link;
}

// Do check
$hasPositionLinkType = $this->isPositionSet($linksByType);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's confusing that a variable name starting with has contains an array. The name suggests that it contains a boolean value.

Copy link
Contributor Author

@mohammedsalem mohammedsalem Dec 21, 2017

Choose a reason for hiding this comment

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

it contains an array of boolean values per link type, I had the case that the positions were set for the products in "Related" but not set for products under "Cross sell" and so on,

the same that under on Link type got the case of some products had position attribute and others don't, therefor I had to check if at least one product doesn't have a position under a product Link type all positions under this type will be ignored and user array index instead.

regarding the question whether we need that amount of iterations,
This came from too many use cases and variants of sending data, as I said about some link types aren't used at all others have positions but not for all products, another type with full defined products positions.


// Set array position as a fallback position if necessary
foreach ($hasPositionLinkType as $linkType => $hasPosition) {
if (!$hasPosition) {
array_walk($linksByType[$linkType], function ($productLink, $position) {
$productLink->setPosition(++$position);
});
}
}

// Flatten multi-dimensional linksByType in ProductLinks
$productLinks = array_reduce($linksByType, 'array_merge', []);

if (count($productLinks) > 0) {
foreach ($entity->getProductLinks() as $link) {
$this->productLinkRepository->save($link);
}
}
return $entity;
}

/**
* Check if the position is set for all product links per link type.
* array with boolean per type
*
* @param $linksByType
* @return array
*/
private function isPositionSet($linksByType)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking some more about it, it's confusing that a method name starting with is returns an array. It suggests the result should be boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Naming convention will be improved

{
$linkTypes = $this->linkTypeProvider->getLinkTypes();

// Initialize isPositionSet for existent link types
$isPositionSet = [];
foreach (array_keys($linkTypes) as $typeName) {
if (array_key_exists($typeName, $linksByType)) {
$isPositionSet[$typeName] = count($linksByType[$typeName]) > 0;
}
}

// Check if at least on link without position exists per Link type
foreach ($linksByType as $type => $links) {
foreach ($links as $link) {
if (!array_key_exists('position', $link->getData())) {
$isPositionSet[$type] = false;
break;
}
}
}

return $isPositionSet;
}
}