🔃 [EngCom] Public Pull Requests - 2.3-develop

Accepted Public Pull Requests: - #16838: [Forwardport] Correctly save Product Custom Option values (by @JeroenVanLeusden) - #16829: [Forwardport] Fix responsive tables showing broken heading (by @ronak2ram) - #16834: [Forwardport] Smallest codestyle fix in Option/Type/Text.php (by @mage2pratik) - #16817: [Forwardport] Fix zero price simple failed to resolve as default (by @mage2pratik) - #16808: [forwardport] fix #16764 Rating Star issue on Product detail Page. (by @Karlasa) - #16803: [Forwardport] #16685 Updated security issues details (by @quisse) - #16792: [Forwardport] Add sort order to user agent rules table headers (by @JRhyne) - #16413: [Forwardport] Fixing a Mistype Error (by @tiagosampaio) - #16767: [Forwardport] Improve attribute checking (by @FreekVandeursen) - magento-engcom/import-export-improvements#110: magento-engcom/import-export-improvements#44: Update the sample customer csv (by @dmanners) - magento-engcom/import-export-improvements#112: magento-engcom/import-export-improvements#88: Set store id on import product category initialization to 0 (by @pogster) - magento-engcom/import-export-improvements#113: magento-engcom/import-export-improvements#30: Validation should fail when a required field is supplied but is empty after trimming (by @pogster) Fixed GitHub Issues: - #5067: Custom option values do not save correctly (reported by @samtay) has been fixed in #16838 by @JeroenVanLeusden in 2.3-develop branch Related commits: 1. 955fbf2 2. db95d8a - #16764: Rating Star issue on Product detail Page. (reported by @Sathishkumar8731) has been fixed in #16808 by @Karlasa in 2.3-develop branch Related commits: 1. 1dd2017 - #16703: User Agent Rules table headers do match content of rows. (reported by @JRhyne) has been fixed in #16792 by @JRhyne in 2.3-develop branch Related commits: 1. 0430946
magento · Jul 18, 2018 · 9bfb67e · 9bfb67e
2 parents 5a186a3 + f73b119
commit 9bfb67e
Show file tree

Hide file tree

Showing 21 changed files with 212 additions and 70 deletions.
diff --git a/README.md b/README.md
@@ -58,7 +58,7 @@ To learn more about issue gate labels click [here](https://github.com/magento/ma
 
 <h2>Reporting security issues</h2>
 
-To report security vulnerabilities in Magento software or web sites, please e-mail <a href="mailto:[email protected]">[email protected]</a>. Please do not report security issues using GitHub. Be sure to encrypt your e-mail with our <a href="https://info2.magento.com/rs/magentoenterprise/images/security_at_magento.asc">encryption key</a> if it includes sensitive information. Learn more about reporting security issues <a href="https://magento.com/security/reporting-magento-security-issue">here</a>.
+To report security vulnerabilities in Magento software or web sites, please create a Bugcrowd researcher account <a href="https://bugcrowd.com/magento">there</a> to submit and follow-up your issue. Learn more about reporting security issues <a href="https://magento.com/security/reporting-magento-security-issue">here</a>.
 
 Stay up-to-date on the latest security news and patches for Magento by signing up for <a href="https://magento.com/security/sign-up">Security Alert Notifications</a>.
 

diff --git a/app/code/Magento/Backend/view/adminhtml/ui_component/design_config_form.xml b/app/code/Magento/Backend/view/adminhtml/ui_component/design_config_form.xml
@@ -92,7 +92,7 @@
                         </select>
                     </formElements>
                 </field>
-                <actionDelete template="Magento_Backend/dynamic-rows/cells/action-delete" sortOrder="50">
+                <actionDelete template="Magento_Backend/dynamic-rows/cells/action-delete">
                     <argument name="data" xsi:type="array">
                         <item name="config" xsi:type="array">
                             <item name="fit" xsi:type="boolean">false</item>

diff --git a/app/code/Magento/Catalog/Model/Product/Option.php b/app/code/Magento/Catalog/Model/Product/Option.php
@@ -8,6 +8,7 @@
 
 use Magento\Catalog\Api\Data\ProductCustomOptionInterface;
 use Magento\Catalog\Api\Data\ProductCustomOptionValuesInterface;
+use Magento\Catalog\Api\Data\ProductCustomOptionValuesInterfaceFactory;
 use Magento\Catalog\Api\Data\ProductInterface;
 use Magento\Catalog\Model\Product;
 use Magento\Catalog\Model\ResourceModel\Product\Option\Value\Collection;
@@ -102,6 +103,11 @@ class Option extends AbstractExtensibleModel implements ProductCustomOptionInter
      */
     private $metadataPool;
 
+    /**
+     * @var ProductCustomOptionValuesInterfaceFactory
+     */
+    private $customOptionValuesFactory;
+
     /**
      * @param \Magento\Framework\Model\Context $context
      * @param \Magento\Framework\Registry $registry
@@ -114,6 +120,7 @@ class Option extends AbstractExtensibleModel implements ProductCustomOptionInter
      * @param \Magento\Framework\Model\ResourceModel\AbstractResource $resource
      * @param \Magento\Framework\Data\Collection\AbstractDb $resourceCollection
      * @param array $data
+     * @param ProductCustomOptionValuesInterfaceFactory|null $customOptionValuesFactory
      * @SuppressWarnings(PHPMD.ExcessiveParameterList)
      */
     public function __construct(
@@ -127,12 +134,16 @@ public function __construct(
         Option\Validator\Pool $validatorPool,
         \Magento\Framework\Model\ResourceModel\AbstractResource $resource = null,
         \Magento\Framework\Data\Collection\AbstractDb $resourceCollection = null,
-        array $data = []
+        array $data = [],
+        ProductCustomOptionValuesInterfaceFactory $customOptionValuesFactory = null
     ) {
         $this->productOptionValue = $productOptionValue;
         $this->optionTypeFactory = $optionFactory;
         $this->validatorPool = $validatorPool;
         $this->string = $string;
+        $this->customOptionValuesFactory = $customOptionValuesFactory ?:
+            \Magento\Framework\App\ObjectManager::getInstance()->get(ProductCustomOptionValuesInterfaceFactory::class);
+
         parent::__construct(
             $context,
             $registry,
@@ -390,20 +401,21 @@ public function beforeSave()
      */
     public function afterSave()
     {
-        $this->getValueInstance()->unsetValues();
         $values = $this->getValues() ?: $this->getData('values');
         if (is_array($values)) {
             foreach ($values as $value) {
-                if ($value instanceof \Magento\Catalog\Api\Data\ProductCustomOptionValuesInterface) {
+                if ($value instanceof ProductCustomOptionValuesInterface) {
                     $data = $value->getData();
                 } else {
                     $data = $value;
                 }
-                $this->getValueInstance()->addValue($data);
-            }
 
-            $this->getValueInstance()->setOption($this)->saveValues();
-        } elseif ($this->getGroupByType($this->getType()) == self::OPTION_GROUP_SELECT) {
+                $this->customOptionValuesFactory->create()
+                    ->addValue($data)
+                    ->setOption($this)
+                    ->saveValues();
+            }
+        } elseif ($this->getGroupByType($this->getType()) === self::OPTION_GROUP_SELECT) {
             throw new LocalizedException(__('Select type options required values rows.'));
         }
 
@@ -804,7 +816,7 @@ public function setImageSizeY($imageSizeY)
     }
 
     /**
-     * @param \Magento\Catalog\Api\Data\ProductCustomOptionValuesInterface[] $values
+     * @param ProductCustomOptionValuesInterface[] $values
      * @return $this
      */
     public function setValues(array $values = null)

diff --git a/app/code/Magento/Catalog/Model/Product/Option/Type/Text.php b/app/code/Magento/Catalog/Model/Product/Option/Type/Text.php
@@ -84,7 +84,7 @@ public function validateUserValue($values)
      */
     public function prepareForCart()
     {
-        if ($this->getIsValid() && strlen($this->getUserValue()) > 0) {
+        if ($this->getIsValid() && ($this->getUserValue() !== '')) {
             return $this->getUserValue();
         } else {
             return null;

diff --git a/app/code/Magento/Catalog/Model/Product/Option/Value.php b/app/code/Magento/Catalog/Model/Product/Option/Value.php
@@ -76,6 +76,7 @@ class Value extends AbstractModel implements \Magento\Catalog\Api\Data\ProductCu
      * @param \Magento\Framework\Model\ResourceModel\AbstractResource $resource
      * @param \Magento\Framework\Data\Collection\AbstractDb $resourceCollection
      * @param array $data
+     * @param CustomOptionPriceCalculator|null $customOptionPriceCalculator
      */
     public function __construct(
         \Magento\Framework\Model\Context $context,
@@ -89,6 +90,7 @@ public function __construct(
         $this->_valueCollectionFactory = $valueCollectionFactory;
         $this->customOptionPriceCalculator = $customOptionPriceCalculator
             ?? \Magento\Framework\App\ObjectManager::getInstance()->get(CustomOptionPriceCalculator::class);
+
         parent::__construct(
             $context,
             $registry,
@@ -201,19 +203,15 @@ public function getProduct()
      */
     public function saveValues()
     {
+        $option = $this->getOption();
+
         foreach ($this->getValues() as $value) {
             $this->isDeleted(false);
-            $this->setData(
-                $value
-            )->setData(
-                'option_id',
-                $this->getOption()->getId()
-            )->setData(
-                'store_id',
-                $this->getOption()->getStoreId()
-            );
-
-            if ($this->getData('is_delete') == '1') {
+            $this->setData($value)
+                ->setData('option_id', $option->getId())
+                ->setData('store_id', $option->getStoreId());
+
+            if ((bool) $this->getData('is_delete') === true) {
                 if ($this->getId()) {
                     $this->deleteValues($this->getId());
                     $this->delete();
@@ -222,7 +220,7 @@ public function saveValues()
                 $this->save();
             }
         }
-        //eof foreach()
+
         return $this;
     }
 

diff --git a/app/code/Magento/CatalogImportExport/Model/Import/Product/CategoryProcessor.php b/app/code/Magento/CatalogImportExport/Model/Import/Product/CategoryProcessor.php
@@ -75,6 +75,7 @@ protected function initCategories()
             $collection->addAttributeToSelect('name')
                 ->addAttributeToSelect('url_key')
                 ->addAttributeToSelect('url_path');
+            $collection->setStoreId(\Magento\Store\Model\Store::DEFAULT_STORE_ID);
             /* @var $collection \Magento\Catalog\Model\ResourceModel\Category\Collection */
             foreach ($collection as $category) {
                 $structure = explode(self::DELIMITER_CATEGORY, $category->getPath());

diff --git a/app/code/Magento/CatalogImportExport/Model/Import/Product/Type/AbstractType.php b/app/code/Magento/CatalogImportExport/Model/Import/Product/Type/AbstractType.php
@@ -28,6 +28,13 @@ abstract class AbstractType
      */
     public static $commonAttributesCache = [];
 
+    /**
+     * Maintain a list of invisible attributes
+     *
+     * @var array
+     */
+    public static $invAttributesCache = [];
+
     /**
      * Attribute Code to Id cache
      *
@@ -278,7 +285,14 @@ protected function _initAttributes()
             }
         }
         foreach ($absentKeys as $attributeSetName => $attributeIds) {
-            $this->attachAttributesById($attributeSetName, $attributeIds);
+            $unknownAttributeIds = array_diff(
+                $attributeIds,
+                array_keys(self::$commonAttributesCache),
+                self::$invAttributesCache
+            );
+            if ($unknownAttributeIds || $this->_forcedAttributesCodes) {
+                $this->attachAttributesById($attributeSetName, $attributeIds);
+            }
         }
         foreach ($entityAttributes as $attributeRow) {
             if (isset(self::$commonAttributesCache[$attributeRow['attribute_id']])) {
@@ -303,37 +317,45 @@ protected function _initAttributes()
     protected function attachAttributesById($attributeSetName, $attributeIds)
     {
         foreach ($this->_prodAttrColFac->create()->addFieldToFilter(
-            'main_table.attribute_id',
-            ['in' => $attributeIds]
+            ['main_table.attribute_id', 'main_table.attribute_code'],
+            [
+                ['in' => $attributeIds],
+                ['in' => $this->_forcedAttributesCodes]
+            ]
         ) as $attribute) {
             $attributeCode = $attribute->getAttributeCode();
             $attributeId = $attribute->getId();
 
             if ($attribute->getIsVisible() || in_array($attributeCode, $this->_forcedAttributesCodes)) {
-                self::$commonAttributesCache[$attributeId] = [
-                    'id' => $attributeId,
-                    'code' => $attributeCode,
-                    'is_global' => $attribute->getIsGlobal(),
-                    'is_required' => $attribute->getIsRequired(),
-                    'is_unique' => $attribute->getIsUnique(),
-                    'frontend_label' => $attribute->getFrontendLabel(),
-                    'is_static' => $attribute->isStatic(),
-                    'apply_to' => $attribute->getApplyTo(),
-                    'type' => \Magento\ImportExport\Model\Import::getAttributeType($attribute),
-                    'default_value' => strlen(
-                        $attribute->getDefaultValue()
-                    ) ? $attribute->getDefaultValue() : null,
-                    'options' => $this->_entityModel->getAttributeOptions(
-                        $attribute,
-                        $this->_indexValueAttributes
-                    ),
-                ];
+                if (!isset(self::$commonAttributesCache[$attributeId])) {
+                    self::$commonAttributesCache[$attributeId] = [
+                        'id' => $attributeId,
+                        'code' => $attributeCode,
+                        'is_global' => $attribute->getIsGlobal(),
+                        'is_required' => $attribute->getIsRequired(),
+                        'is_unique' => $attribute->getIsUnique(),
+                        'frontend_label' => $attribute->getFrontendLabel(),
+                        'is_static' => $attribute->isStatic(),
+                        'apply_to' => $attribute->getApplyTo(),
+                        'type' => \Magento\ImportExport\Model\Import::getAttributeType($attribute),
+                        'default_value' => strlen(
+                            $attribute->getDefaultValue()
+                        ) ? $attribute->getDefaultValue() : null,
+                        'options' => $this->_entityModel->getAttributeOptions(
+                            $attribute,
+                            $this->_indexValueAttributes
+                        ),
+                    ];
+                }
+
                 self::$attributeCodeToId[$attributeCode] = $attributeId;
                 $this->_addAttributeParams(
                     $attributeSetName,
                     self::$commonAttributesCache[$attributeId],
                     $attribute
                 );
+            } else {
+                self::$invAttributesCache[] = $attributeId;
             }
         }
     }

diff --git a/...code/Magento/CatalogImportExport/Test/Unit/Model/Import/Product/Type/AbstractTypeTest.php b/...code/Magento/CatalogImportExport/Test/Unit/Model/Import/Product/Type/AbstractTypeTest.php
@@ -134,8 +134,28 @@ protected function setUp()
             ->expects($this->any())
             ->method('addFieldToFilter')
             ->with(
-                'main_table.attribute_id',
-                ['in' => ['attribute_id', 'boolean_attribute']]
+                ['main_table.attribute_id', 'main_table.attribute_code'],
+                [
+                    [
+                        'in' =>
+                            [
+                                'attribute_id',
+                                'boolean_attribute',
+                            ],
+                    ],
+                    [
+                        'in' =>
+                            [
+                                'related_tgtr_position_behavior',
+                                'related_tgtr_position_limit',
+                                'upsell_tgtr_position_behavior',
+                                'upsell_tgtr_position_limit',
+                                'thumbnail_label',
+                                'small_image_label',
+                                'image_label',
+                            ],
+                    ],
+                ]
             )
             ->willReturn([$attribute1, $attribute2]);
 

diff --git a/app/code/Magento/ConfigurableProduct/Pricing/Price/ConfigurablePriceResolver.php b/app/code/Magento/ConfigurableProduct/Pricing/Price/ConfigurablePriceResolver.php
@@ -64,7 +64,7 @@ public function resolvePrice(\Magento\Framework\Pricing\SaleableInterface $produ
 
         foreach ($this->lowestPriceOptionsProvider->getProducts($product) as $subProduct) {
             $productPrice = $this->priceResolver->resolvePrice($subProduct);
-            $price = $price ? min($price, $productPrice) : $productPrice;
+            $price = isset($price) ? min($price, $productPrice) : $productPrice;
         }
 
         return (float)$price;

diff --git a/...ode/Magento/ConfigurableProduct/Test/Unit/Pricing/Price/ConfigurablePriceResolverTest.php b/...ode/Magento/ConfigurableProduct/Test/Unit/Pricing/Price/ConfigurablePriceResolverTest.php
@@ -55,24 +55,31 @@ protected function setUp()
      * situation: one product is supplying the price, which could be a price of zero (0)
      *
      * @dataProvider resolvePriceDataProvider
+     *
+     * @param $variantPrices
+     * @param $expectedPrice
      */
-    public function testResolvePrice($expectedValue)
+    public function testResolvePrice($variantPrices, $expectedPrice)
     {
-        $price = $expectedValue;
-
         $product = $this->getMockBuilder(
             \Magento\Catalog\Model\Product::class
         )->disableOriginalConstructor()->getMock();
 
         $product->expects($this->never())->method('getSku');
 
-        $this->lowestPriceOptionsProvider->expects($this->once())->method('getProducts')->willReturn([$product]);
-        $this->priceResolver->expects($this->once())
+        $products = array_map(function () {
+            return $this->getMockBuilder(\Magento\Catalog\Model\Product::class)
+                ->disableOriginalConstructor()
+                ->getMock();
+        }, $variantPrices);
+
+        $this->lowestPriceOptionsProvider->expects($this->once())->method('getProducts')->willReturn($products);
+        $this->priceResolver
             ->method('resolvePrice')
-            ->with($product)
-            ->willReturn($price);
+            ->willReturnOnConsecutiveCalls(...$variantPrices);
 
-        $this->assertEquals($expectedValue, $this->resolver->resolvePrice($product));
+        $actualPrice = $this->resolver->resolvePrice($product);
+        self::assertSame($expectedPrice, $actualPrice);
     }
 
     /**
@@ -81,8 +88,40 @@ public function testResolvePrice($expectedValue)
     public function resolvePriceDataProvider()
     {
         return [
-            'price of zero' => [0.00],
-            'price of five' => [5],
+            'Single variant at price 0.00 (float), should return 0.00 (float)' => [
+                $variantPrices = [
+                    0.00,
+                ],
+                $expectedPrice = 0.00,
+            ],
+            'Single variant at price 5 (integer), should return 5.00 (float)' => [
+                $variantPrices = [
+                    5,
+                ],
+                $expectedPrice = 5.00,
+            ],
+            'Single variants at price null (null), should return 0.00 (float)' => [
+                $variantPrices = [
+                    null,
+                ],
+                $expectedPrice = 0.00,
+            ],
+            'Multiple variants at price 0, 10, 20, should return 0.00 (float)' => [
+                $variantPrices = [
+                    0,
+                    10,
+                    20,
+                ],
+                $expectedPrice = 0.00,
+            ],
+            'Multiple variants at price 10, 0, 20, should return 0.00 (float)' => [
+                $variantPrices = [
+                    10,
+                    0,
+                    20,
+                ],
+                $expectedPrice = 0.00,
+            ],
         ];
     }
 }