Allow construction of products with custom_attributes in $data

[Vinai]

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:
   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.
   This improvement is covered by the integration test
   \Magento\Catalog\Model\ProductTest::testConstructionWithCustomAttributesArrayInData

   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 PR contains no backward incompatible changes.

Manual testing scenarios:

Create a product model instance passing in a $data array with custom_attributes (any format).
An example of this can be seen in the 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)

[m2-assistant]

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.

[orlangur]

Nice observation @Vinai! What was the use for custom_attributes in your particular scenarios taking into consideration extension_attributes is a recommended way to go?

[Vinai]

@orlangur I simply was passing an array created via \Magento\Framework\Reflection\DataObjectProcessor::buildOutputDataArray to \Magento\Catalog\Model\ProductFactory::create(['data' => $data]) and it threw an exception. It's only core custom attributes, no custom custom attributes :)
It's a bit surprising, so I thought it should work, or at least not throw an exception.

Also, custom attributes are so much more practical in 95% of all cases compared to extension attributes, they are still extremely common, and should work as expected, even if extension attributes are the recommended way to go.

[orlangur]

@Vinai,

and it threw an exception. It's only core custom attributes, no custom custom attributes :)

oh, that's a bit of shame .) thanks for enforcing this use case with tests.

[orlangur]

LGTM 👍

I don't like the unrelated to main changes type-casting-spacing part but we need to enforce and fix it everywhere anyway.

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-5769 has been created to process this Pull Request
✳️ @orlangur, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[Vinai]

Note: force pushed a commit that hopefully fixes the static tests.

[Vinai]

@orlangur Only just noticed you already approved the changes, otherwise I wouldn't have pushed commit that hopefully fixes the static tests.
I agree that the space changes don't have anything to do with the actual PR and I would have preferred not to add them, but because I anticipated the static tests to barf on those anyhow I decided to add them anyhow.

[Vinai]

This change is only 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.

[m2-assistant]

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.

[Vinai]

I closed this PR accidentally because I deleted the remote branch (typed git push -d instead of git push -f).
I reopened a new PR for the same patch: #24460

[orlangur]

@Vinai when you recreate a branch, reopen is still not available?

[Vinai]

@orlangur No, unfortunately I wasn't able to reopen the original PR.
The only option I have is to delete the fork.