Merge pull request #2461 from magento-performance/MAGETWO-90572

[performance] MAGETWO-90572: Optimize retrieving product attributes
magento · Apr 27, 2018 · e49ccf7 · e49ccf7
2 parents 35d1377 + e6994a1
commit e49ccf7
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 54 deletions.
diff --git a/app/code/Magento/Catalog/Model/Product.php b/app/code/Magento/Catalog/Model/Product.php
@@ -12,6 +12,7 @@
 use Magento\Catalog\Model\Product\Attribute\Backend\Media\EntryConverterPool;
 use Magento\Framework\Api\AttributeValueFactory;
 use Magento\Framework\App\Filesystem\DirectoryList;
+use Magento\Framework\App\ObjectManager;
 use Magento\Framework\DataObject\IdentityInterface;
 use Magento\Framework\Pricing\SaleableInterface;
 
@@ -270,6 +271,7 @@ class Product extends \Magento\Catalog\Model\AbstractModel implements
 
     /**
      * @var \Magento\Catalog\Api\ProductAttributeRepositoryInterface
+     * @deprecated Not used anymore due to performance issue (loaded all product attributes)
      */
     protected $metadataService;
 
@@ -346,6 +348,11 @@ class Product extends \Magento\Catalog\Model\AbstractModel implements
      */
     protected $linkTypeProvider;
 
+    /**
+     * @var \Magento\Eav\Model\Config
+     */
+    private $eavConfig;
+
     /**
      * Product constructor.
      * @param \Magento\Framework\Model\Context $context
@@ -383,7 +390,7 @@ class Product extends \Magento\Catalog\Model\AbstractModel implements
      * @param \Magento\Framework\Api\DataObjectHelper $dataObjectHelper
      * @param \Magento\Framework\Api\ExtensionAttribute\JoinProcessorInterface $joinProcessor
      * @param array $data
-     *
+     * @param \Magento\Eav\Model\Config|null $config
      * @SuppressWarnings(PHPMD.ExcessiveParameterList)
      * @SuppressWarnings(PHPMD.UnusedFormalParameter)
      */
@@ -422,7 +429,8 @@ public function __construct(
         EntryConverterPool $mediaGalleryEntryConverterPool,
         \Magento\Framework\Api\DataObjectHelper $dataObjectHelper,
         \Magento\Framework\Api\ExtensionAttribute\JoinProcessorInterface $joinProcessor,
-        array $data = []
+        array $data = [],
+        \Magento\Eav\Model\Config $config = null
     ) {
         $this->metadataService = $metadataService;
         $this->_itemOptionFactory = $itemOptionFactory;
@@ -461,6 +469,7 @@ public function __construct(
             $resourceCollection,
             $data
         );
+        $this->eavConfig = $config ?? ObjectManager::getInstance()->get(\Magento\Eav\Model\Config::class);
     }
 
     /**
@@ -474,12 +483,18 @@ protected function _construct()
     }
 
     /**
-     * {@inheritdoc}
+     * Get a list of custom attribute codes that belongs to product attribute set. If attribute set not specified for
+     * product will return all attribute codes
+     *
+     * @return string[]
      */
     protected function getCustomAttributesCodes()
     {
         if ($this->customAttributesCodes === null) {
-            $this->customAttributesCodes = $this->getEavAttributesCodes($this->metadataService);
+            $this->customAttributesCodes = array_keys($this->eavConfig->getEntityAttributes(
+                self::ENTITY,
+                $this
+            ));
             $this->customAttributesCodes = array_diff($this->customAttributesCodes, $this->interfaceAttributes);
         }
         return $this->customAttributesCodes;

diff --git a/app/code/Magento/Catalog/Plugin/Model/ResourceModel/ReadSnapshotPlugin.php b/app/code/Magento/Catalog/Plugin/Model/ResourceModel/ReadSnapshotPlugin.php
@@ -58,7 +58,9 @@ public function afterExecute(ReadSnapshot $subject, array $entityData, $entityTy
         $globalAttributes = [];
         $attributesMap = [];
         $eavEntityType = $metadata->getEavEntityType();
-        $attributes = (null === $eavEntityType) ? [] : $this->config->getEntityAttributes($eavEntityType);
+        $attributes = null === $eavEntityType
+            ? []
+            : $this->config->getEntityAttributes($eavEntityType, new \Magento\Framework\DataObject($entityData));
 
         /** @var \Magento\Eav\Model\Entity\Attribute\AbstractAttribute $attribute */
         foreach ($attributes as $attribute) {

diff --git a/app/code/Magento/Catalog/Test/Unit/Model/ProductTest.php b/app/code/Magento/Catalog/Test/Unit/Model/ProductTest.php
@@ -205,6 +205,11 @@ class ProductTest extends \PHPUnit\Framework\TestCase
      */
     private $cacheInterfaceMock;
 
+    /**
+     * @var \Magento\Eav\Model\Config|\PHPUnit_Framework_MockObject_MockObject
+     */
+    private $eavConfig;
+
     /**
      * @SuppressWarnings(PHPMD.ExcessiveMethodLength)
      */
@@ -364,6 +369,7 @@ protected function setUp()
             ->setMethods(['create'])
             ->getMock();
         $this->mediaConfig = $this->createMock(\Magento\Catalog\Model\Product\Media\Config::class);
+        $this->eavConfig = $this->createMock(\Magento\Eav\Model\Config::class);
 
         $this->extensionAttributes = $this->getMockBuilder(ProductExtensionInterface::class)
             ->setMethods(['getStockItem'])
@@ -401,7 +407,8 @@ protected function setUp()
                 'catalogProductMediaConfig' => $this->mediaConfig,
                 '_filesystem' => $this->filesystemMock,
                 '_collectionFactory' => $this->collectionFactoryMock,
-                'data' => ['id' => 1]
+                'data' => ['id' => 1],
+                'eavConfig' => $this->eavConfig
             ]
         );
     }
@@ -1269,39 +1276,32 @@ public function testGetCustomAttributes()
     {
         $priceCode = 'price';
         $colorAttributeCode = 'color';
-        $interfaceAttribute = $this->createMock(\Magento\Framework\Api\MetadataObjectInterface::class);
-        $interfaceAttribute->expects($this->once())
-            ->method('getAttributeCode')
-            ->willReturn($priceCode);
-        $colorAttribute = $this->createMock(\Magento\Framework\Api\MetadataObjectInterface::class);
-        $colorAttribute->expects($this->once())
-            ->method('getAttributeCode')
-            ->willReturn($colorAttributeCode);
-        $customAttributesMetadata = [$interfaceAttribute, $colorAttribute];
-
-        $this->metadataServiceMock->expects($this->once())
-            ->method('getCustomAttributesMetadata')
+        $customAttributesMetadata = [$priceCode => 'attribute1', $colorAttributeCode => 'attribute2'];
+
+        $this->metadataServiceMock->expects($this->never())->method('getCustomAttributesMetadata');
+        $this->eavConfig->expects($this->once())
+            ->method('getEntityAttributes')
             ->willReturn($customAttributesMetadata);
         $this->model->setData($priceCode, 10);
 
         //The color attribute is not set, expect empty custom attribute array
         $this->assertEquals([], $this->model->getCustomAttributes());
 
         //Set the color attribute;
-        $this->model->setData($colorAttributeCode, "red");
+        $this->model->setData($colorAttributeCode, 'red');
         $attributeValue = new \Magento\Framework\Api\AttributeValue();
         $attributeValue2 = new \Magento\Framework\Api\AttributeValue();
         $this->attributeValueFactory->expects($this->exactly(2))->method('create')
             ->willReturnOnConsecutiveCalls($attributeValue, $attributeValue2);
         $this->assertEquals(1, count($this->model->getCustomAttributes()));
         $this->assertNotNull($this->model->getCustomAttribute($colorAttributeCode));
-        $this->assertEquals("red", $this->model->getCustomAttribute($colorAttributeCode)->getValue());
+        $this->assertEquals('red', $this->model->getCustomAttribute($colorAttributeCode)->getValue());
 
         //Change the attribute value, should reflect in getCustomAttribute
-        $this->model->setCustomAttribute($colorAttributeCode, "blue");
+        $this->model->setCustomAttribute($colorAttributeCode, 'blue');
         $this->assertEquals(1, count($this->model->getCustomAttributes()));
         $this->assertNotNull($this->model->getCustomAttribute($colorAttributeCode));
-        $this->assertEquals("blue", $this->model->getCustomAttribute($colorAttributeCode)->getValue());
+        $this->assertEquals('blue', $this->model->getCustomAttribute($colorAttributeCode)->getValue());
     }
 
     /**

diff --git a/app/code/Magento/Eav/Model/ResourceModel/ReadHandler.php b/app/code/Magento/Eav/Model/ResourceModel/ReadHandler.php
@@ -5,6 +5,7 @@
  */
 namespace Magento\Eav\Model\ResourceModel;
 
+use Magento\Framework\DataObject;
 use Magento\Framework\EntityManager\MetadataPool;
 use Magento\Framework\EntityManager\Operation\AttributeInterface;
 use Magento\Framework\Model\Entity\ScopeInterface;
@@ -59,13 +60,29 @@ public function __construct(
      * @param string $entityType
      * @return \Magento\Eav\Api\Data\AttributeInterface[]
      * @throws \Exception if for unknown entity type
+     * @deprecated Not used anymore
+     * @see ReadHandler::getEntityAttributes
      */
     protected function getAttributes($entityType)
     {
         $metadata = $this->metadataPool->getMetadata($entityType);
         $eavEntityType = $metadata->getEavEntityType();
-        $attributes = (null === $eavEntityType) ? [] : $this->config->getAttributes($eavEntityType);
-        return $attributes;
+        return null === $eavEntityType ? [] : $this->config->getEntityAttributes($eavEntityType);
+    }
+
+    /**
+     * Get attribute of given entity type
+     *
+     * @param string $entityType
+     * @param DataObject $entity
+     * @return \Magento\Eav\Api\Data\AttributeInterface[]
+     * @throws \Exception if for unknown entity type
+     */
+    private function getEntityAttributes(string $entityType, DataObject $entity): array
+    {
+        $metadata = $this->metadataPool->getMetadata($entityType);
+        $eavEntityType = $metadata->getEavEntityType();
+        return null === $eavEntityType ? [] : $this->config->getEntityAttributes($eavEntityType, $entity);
     }
 
     /**
@@ -105,7 +122,7 @@ public function execute($entityType, $entityData, $arguments = [])
         $selects = [];
 
         /** @var \Magento\Eav\Model\Entity\Attribute\AbstractAttribute $attribute */
-        foreach ($this->getAttributes($entityType) as $attribute) {
+        foreach ($this->getEntityAttributes($entityType, new DataObject($entityData)) as $attribute) {
             if (!$attribute->isStatic()) {
                 $attributeTables[$attribute->getBackend()->getTable()][] = $attribute->getAttributeId();
                 $attributesMap[$attribute->getAttributeId()] = $attribute->getAttributeCode();

diff --git a/app/code/Magento/Eav/Test/Unit/Model/ResourceModel/ReadHandlerTest.php b/app/code/Magento/Eav/Test/Unit/Model/ResourceModel/ReadHandlerTest.php
@@ -110,7 +110,7 @@ public function testExecute($eavEntityType, $callNum, array $expected, $isStatic
         $attributeMock->method('getAttributeCode')
             ->willReturn('attributeCode');
         $this->configMock->expects($this->exactly($callNum))
-            ->method('getAttributes')
+            ->method('getEntityAttributes')
             ->willReturn([$attributeMock]);
         $this->assertEquals($expected, $this->readHandler->execute('entity_type', $entityData));
     }

diff --git a/app/code/Magento/Swatches/Model/Plugin/ProductImage.php b/app/code/Magento/Swatches/Model/Plugin/ProductImage.php
@@ -69,7 +69,7 @@ public function beforeGetImage(
             && ($location == self::CATEGORY_PAGE_GRID_LOCATION || $location == self::CATEGORY_PAGE_LIST_LOCATION)) {
             $request = $this->request->getParams();
             if (is_array($request)) {
-                $filterArray = $this->getFilterArray($request);
+                $filterArray = $this->getFilterArray($request, $product);
                 if (!empty($filterArray)) {
                     $product = $this->loadSimpleVariation($product, $filterArray);
                 }
@@ -99,16 +99,18 @@ protected function loadSimpleVariation(\Magento\Catalog\Model\Product $parentPro
      * Get filters from request
      *
      * @param array $request
+     * @param \Magento\Catalog\Model\Product $product
      * @return array
      */
-    protected function getFilterArray(array $request)
+    private function getFilterArray(array $request, \Magento\Catalog\Model\Product $product)
     {
         $filterArray = [];
-        $attributeCodes = $this->eavConfig->getEntityAttributeCodes(\Magento\Catalog\Model\Product::ENTITY);
+        $attributes = $this->eavConfig->getEntityAttributes(\Magento\Catalog\Model\Product::ENTITY, $product);
+
         foreach ($request as $code => $value) {
-            if (in_array($code, $attributeCodes)) {
-                $attribute = $this->eavConfig->getAttribute(\Magento\Catalog\Model\Product::ENTITY, $code);
-                if ($attribute->getId() && $this->canReplaceImageWithSwatch($attribute)) {
+            if (isset($attributes[$code])) {
+                $attribute = $attributes[$code];
+                if ($this->canReplaceImageWithSwatch($attribute)) {
                     $filterArray[$code] = $value;
                 }
             }

diff --git a/app/code/Magento/Swatches/Test/Unit/Model/Plugin/ProductImageTest.php b/app/code/Magento/Swatches/Test/Unit/Model/Plugin/ProductImageTest.php
@@ -77,7 +77,11 @@ public function testBeforeGetImage($expected)
             ->method('getParams')
             ->willReturn($expected['getParams']);
 
-        $this->getFilterArray($expected);
+        $this->eavConfigMock
+            ->method('getEntityAttributes')
+            ->with('catalog_product')
+            ->willReturn(['color' => $this->attributeMock]);
+
         $this->canReplaceImageWithSwatch($expected);
         $this->swatchesHelperMock
             ->expects($this->exactly($expected['loadVariationByFallback_count']))
@@ -94,24 +98,6 @@ public function testBeforeGetImage($expected)
         $this->assertEquals([$this->productMock, $expected['page_handle'], []], $result);
     }
 
-    protected function getFilterArray($expected)
-    {
-        $this->eavConfigMock
-            ->method('getEntityAttributeCodes')
-            ->with('catalog_product')
-            ->willReturn($expected['attribute_codes_array']);
-
-        $this->eavConfigMock
-            ->method('getAttribute')
-            ->with('catalog_product', $expected['attribute_code'])
-            ->willReturn($this->attributeMock);
-
-        $this->attributeMock
-            ->expects($this->exactly($expected['getId_count']))
-            ->method('getId')
-            ->willReturn($expected['getId']);
-    }
-
     protected function canReplaceImageWithSwatch($expected)
     {
         $this->swatchesHelperMock
@@ -152,7 +138,6 @@ public function dataForTest()
                 [
                     'page_handle' => 'category_page_grid',
                     'getParams' => ['color' => 31],
-                    'attribute_codes_array' => ['color'],
                     'attribute_code' => 'color',
                     'getId_count' => 1,
                     'getId' => 332,
@@ -171,7 +156,6 @@ public function dataForTest()
                 [
                     'page_handle' => 'category_page_grid',
                     'getParams' => ['color' => 31],
-                    'attribute_codes_array' => ['color'],
                     'attribute_code' => 'color',
                     'getId_count' => 1,
                     'getId' => 332,
@@ -190,7 +174,6 @@ public function dataForTest()
                 [
                     'page_handle' => 'category_page_grid',
                     'getParams' => ['color' => 31],
-                    'attribute_codes_array' => ['color'],
                     'attribute_code' => 'color',
                     'getId_count' => 1,
                     'getId' => 332,