Add mergeData method to DataObject

[dersam]

It's not uncommon to have two arrays that you wish to merge together recursively, for later descent. \Magento\Framework\DataObject provides the retrieval method through getData, where a slash separated path can be used to descend into an array to retrieve a specific item, without having to extract the whole array and do a bunch of isset checks.

However, there's no equivalent for setting data. If a key already exists in the DataObject with an array value, setting another array will always overwrite the existing data completely.

This pull request proposes adding a new mergeData method. This method will perform a recursive merge between the current data in the given key and the provided data. If the key is an array, it will merge against the entire dataset. If the key does not currently exist in the DataObject, it will fall back to the standard behavior of setData.

Making this a separate method instead of adding it to addData or setData ensures that this functionality must be used explicitly, avoiding any backwards compatibility issues.

The merge method is from http://php.net/manual/en/function.array-merge-recursive.php#92195. It has been slightly refactored to adhere to the style guide.

[orlangur]

I don't think adding new functionality to Magento\Framework\DataObject is a good idea. We are moving from inheritance-based to interface-based object hierarchy, ideally eliminating all super-abstract classes like blocks and models at some point.

So, just put any additional logic you need in a neat single-responsible class.

As far as I understood the problem you are trying to solve something like $dataObject->setData(\Magento\Framework\Stdlib\ArrayUtils::merge($dataObject->getData(), $value)) would do pretty the same.

Also, there are existing methods \Magento\Framework\Stdlib\ArrayManager::merge and \Zend\Stdlib\ArrayUtils::merge which also do perform some array merging.

[dersam]

I can see why you wouldn't want to pollute DataObject with additional functionality. However, I don't think it's really out of scope for that class; it already provides ways to descend into keys that are storing arrays via getData, and the proposed changes not only allow merging the entire dataset, but individual top level items. It is essentially a way to set data to match the slash-paths allowed through getData

For example, $dataObject->merge('someArray', ['key'=>['key2'=>'value]]) will only merge the array stored in the someArray key. The merging of the entire dataset contained in the object is to remain consistent with the behavior of getData when the key is an array.

Is the future plan to eliminate DataObject completely? Should that class not be relied on for creating models at all to ensure future compatibility? If that's the case, what is the "Magento approved" way to create models that contain arbitrary getters and setters? The model system seems to directly rely on DataObject, so I find it surprising that DataObject would be slated for removal.

[orlangur]

slash-paths allowed through getData

This better to be removed actually for better performance, so that you can use such functionality explicitly by some getDataByPath and have simple key-value storage otherwise.

The merging of the entire dataset contained in the object is to remain consistent with the behavior of getData when the key is an array.

No, it's new functionality never existed in Varien_Object before. For different needs there could be different merge strategies, like in various existing methods I mentioned. There will be no problem introduced if you add functionality you need to core as separate class.

what is the "Magento approved" way

Let's wait for the official response ;)

[dersam]

No, it's new functionality never existed in Varien_Object before. For different needs there could be different merge strategies, like in various existing methods I mentioned. There will be no problem introduced if you add functionality you need to core as separate class.

My mistake, I meant to remain consistent with setData. Passing an array as the key parameter of setData will currently overwrite all the data in the current DataObject with the newly passed data. I felt that mergeData should have similar behavior for consistency.

I'd also argue that if mergeData is out of place in DataObject, so is something like getDataByPath, as both are operating on data contained within keys of the object, rather than DataObject acting as a simple key-value store. It really depends on what the overarching purpose of DataObject is intended to be, and its future in Magento. As you said, let's wait for the official response. :)

[ishakhsuvarov]

Hi @dersam
Thank you for your proposal.

I do not think we can accept this Pull Request in its current state, since DataObject already has sufficient methods for data manipulation.
As @orlangur proposed above, you may try implementing it with a single-responsible class, if there is no possibility to leverage existing mechanisms for merging.

[dersam]

If this functionality doesn't belong in DataObject, the existing methods that @orlangur mentioned are sufficient.

Closing, thanks for the feedback.

[ishakhsuvarov]

@dersam Thank you.