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

Allow construction of products with custom_attributes in $data #24460

Merged
merged 4 commits into from
Feb 8, 2020

Conversation

Vinai
Copy link
Contributor

@Vinai Vinai commented Sep 5, 2019

This is a new PR because I accidentally deleted the previous remote branch for #24407


Description (*)

This patch does two things:

  1. Currently it is not possible to pass an array with custom_attributes
    to the \Magento\Catalog\Model\Product constructor (it causes a fatal error).
    The reason is because the filterCustomAttribute and eavConfig arguments are
    assigned after the call to parent::__construct.
    However, the properties are used during the parent::__construct calls.
    The flow of execution is as follows:
    1. Product::__construct -> Catalog\Model\AbstractModel::__construct
    2. Catalog\Model\AbstractModel::__construct -> AbstractExtensibleModel::__construct
    3. AbstractExtensibleModel::__construct -> AbstractExtensibleModel::filterCustomAttributes
    4. AbstractExtensibleModel::filterCustomAttributes -> AbstractExtensibleModel::getCustomAttributesCodes
      ...which is overridden by Product::getCustomAttributesCodes.
      getCustomAttributesCodes expects the filterCustomAttribute and eavConfig properties to be set if custom_attributes are present in $data, but they are still null because the Product::__construct method has not yet completed.

The fix this PR applies is to assign the properties before the call to parent::__construct.
The bug and fix are covered by the integration test:
\Magento\Catalog\Model\ProductTest::testConstructionWithCustomAttributesMapInData

  1. The method AbstractExtensibleModel::filterCustomAttribute expects the custom_attributes in $data to be a simple map from codes to values, e.g. ['category_ids => '1,2'].
    However, the method \Magento\Framework\Reflection\DataObjectProcessor::buildOutputDataArray generates a
    numerically indexed custom attributes array, where each custom attribute is a sub-array with a attribute_code and value record.
    This PR allows passing such an custom_attributes array into the Product model constructor.
    Currently it would be ignored, but with this patch the code checks if custom_attributes is numerically indexed, and if so, flattens the sub-arrays into the expected map format.
    To illustrate the difference of the custom_attributes array formats:
    // Map:
    [
        'custom_attributes' => [
            'category_ids' => '1,2',
            'tax_class_id'  => '3',
        ]
    ]
   // Numerically indexed array of sub-arrays:
    [
        'custom_attributes' => [
           [
                'attribute_code' => 'category_ids',
                'value'                => '1,2'
           ],
           [
               'attribute_code' => 'tax_class_id',
               'value'                 => '3'
           ],
    ]

This improvement is covered by the integration test
\Magento\Catalog\Model\ProductTest::testConstructionWithCustomAttributesArrayInData

Description (*)

Manual testing scenarios (*)

  1. Instantiate Magento\Catalog\Model\Product and pass a $data array with a custom_attributes key to the constructor. The custom_attributes array can be a map or a numerically indexed array, the exception is thrown either way.
    Examples of how this can be done can be seen in the integration tests in this PR.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@Vinai Vinai requested a review from orlangur September 5, 2019 09:22
@Vinai Vinai requested a review from akaplya as a code owner September 5, 2019 09:22
@m2-assistant
Copy link

m2-assistant bot commented Sep 5, 2019

Hi @Vinai. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

},
$websiteIds
);
$storeIds = array_merge($storeIds, ...$websiteStoreIds);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewers: This change is only in the PR because the static tests complained about the array_merge in a loop. I didn't write that loop, but I don't mind changing it to get the tests green.

@Vinai Vinai changed the title Allow construction of products with custom_attributes Allow construction of products with custom_attributes in $data Sep 5, 2019
@orlangur orlangur self-assigned this Sep 5, 2019
@sidolov sidolov added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Sep 11, 2019
This patch does two things:

1. Currently it is not possible to pass an array with `custom_attributes`
   to the `\Magento\Catalog\Model\Product` constructor (it causes a fatal error).
   The reason is because the `filterCustomAttribute` and `eavConfig` arguments are
   assigned after the call to `parent::__construct`.
   However, the properties are used during the `parent::__construct` calls.
   The flow of execution is as follows:
       Product::__construct -> Catalog\Model\AbstractModel::__construct
       Catalog\Model\AbstractModel::__construct -> AbstractExtensibleModel::__construct
       AbstractExtensibleModel::__construct -> AbstractExtensibleModel::filterCustomAttributes
       AbstractExtensibleModel::filterCustomAttributes -> AbstractExtensibleModel::getCustomAttributesCodes
       ...which is overridden by Product::getCustomAttributesCodes
       getCustomAttributesCodes expectes the `filterCustomAttribute` and `eavConfig` properties to be set if
       `custom_attributes` are present in `$data`, but they are still null because the `Product::__construct`
       method has not yet completed.
   The fix this PR applies is to assign the properties before the call to `parent::__construct`.
   The bug and fix are covered by the integration test:
   `\Magento\Catalog\Model\ProductTest::testConstructionWithCustomAttributesMapInData`

2. The method `AbstractExtensibleModel::filterCustomAttribute` expects the `custom_attributes` in `$data` to
   be a simple map from codes to values, e.g. `['category_ids => '1,2']`.
   However, the method `\Magento\Framework\Reflection\DataObjectProcessor::buildOutputDataArray` generates a
   numerically indexed custom attributes array, where each custom attribute is a sub-array with a `attribute_code`
   and `value` record.
   This PR allows passing such an `custom_attributes` array into the `Product` model constructor.
   Currently it would be ignored, but with this patch the code checks if `custom_attributes` is numerically indexed,
   and if so, flattens the sub-arrays into the expected map format.
   To illustrate the difference of the `custom_attributes` array formats:

   Map:
   [
       'custom_attributes' => [
           'category_ids' => '1,2',
           'tax_class_id' => '3',
       ]
   ]

   Numerically indexed array of sub-arrays:
   [
      'custom_attributes' => [
          [
              'attribute_code' => 'category_ids',
              'value'          => '1,2'
          ],
          [
              'attribute_code' => 'tax_class_id',
              'value'          => '3'
          ],

      ]
  ]

  This improvement is covered by the integration test
  `\Magento\Catalog\Model\ProductTest::testConstructionWithCustomAttributesArrayInData`
@Vinai
Copy link
Contributor Author

Vinai commented Sep 12, 2019

Resolved merge conflicts.

@Vinai
Copy link
Contributor Author

Vinai commented Sep 12, 2019

I do not know why the Database Compare check fails, as the PR does not include any storage related changes. Can someone with more insight help, please?

@magento-engcom-team
Copy link
Contributor

Hi @orlangur, thank you for the review.
ENGCOM-5836 has been created to process this Pull Request

@ghost ghost assigned sidolov Sep 16, 2019
@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-5836 has been created to process this Pull Request

@sidolov
Copy link
Contributor

sidolov commented Sep 23, 2019

@engcom-Foxtrot , please, take a look at the failed test on the Magento Commerce and B2B edition:
Magento\FunctionalTestingFramework.functional.AdminCustomerCustomAddressMultilineAttributeTest
@Vinai looks like current implementation does not work with multiline attributes, probably the best way to check it - use multi-line address for order on the backend.

@engcom-Foxtrot
Copy link
Contributor

@magento run all tests

1 similar comment
@sdzhepa
Copy link
Contributor

sdzhepa commented Dec 2, 2019

@magento run all tests

@sidolov sidolov changed the base branch from 2.3-develop to 2.4-develop December 5, 2019 17:20
# Conflicts:
#	app/code/Magento/Catalog/Model/Product.php
#	dev/tests/integration/testsuite/Magento/Catalog/Model/ProductTest.php
@engcom-Foxtrot
Copy link
Contributor

@magento run all tests

@slavvka
Copy link
Member

slavvka commented Jan 9, 2020

Internal BIC approval ticket https://jira.corp.magento.com/browse/MC-30325

@slavvka
Copy link
Member

slavvka commented Jan 17, 2020

BIC ia approved

@engcom-Kilo
Copy link
Contributor

✔️ QA Passed

@m2-assistant
Copy link

m2-assistant bot commented Feb 8, 2020

Hi @Vinai, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants