Order of how arguments are merged in multiple di.xml-files causes unexpected results

[kanduvisla]

Ok, so bear with me on this one. I have to explain some things first because I have no other way on explaining what's this issue is about, although in my opinion it's quite an important one.

How Magento handles constructor arguments

Take a look at the following example about 2 modules, module A and B, that want to add some arguments to an argument in module X:

Module A

<type name="Vendor\ModuleX\Model\Foo">
    <arguments>
        <argument name="bar" xsi:type="array">
            <item name="first" xsi:type="array">
                <item name="message" xsi:type="string">Hello</item>
        </argument>
    </arguments>
</type>

Module B

<type name="Vendor\ModuleX\Model\Foo">
    <arguments>
        <argument name="bar" xsi:type="array">
            <item name="second" xsi:type="array">
                <item name="message" xsi:type="string">World</item>
        </argument>
    </arguments>
</type>

The above 2 configurations both manipulate the argument named bar and add their own values to it. The result of this is that when class Foo of Module X is instantiated, the $bar-argument in it's constructor call will be:

$bar = [
   'first' => ['message' => 'Hello'],
   'second' => ['message' => 'World']
];

Under the hood

Let me explain some code that's currently in Magento 2: Take a look at the method \Magento\Framework\ObjectManager\Config\Config::_collectConfiguration():

/**
 * Collect parent types configuration for requested type
 *
 * @param string $type
 * @return array
 * @SuppressWarnings(PHPMD.CyclomaticComplexity)
 */
protected function _collectConfiguration($type)
{
    if (!isset($this->_mergedArguments[$type])) {
        if (isset($this->_virtualTypes[$type])) {
            $arguments = $this->_collectConfiguration($this->_virtualTypes[$type]);
        } elseif ($this->_relations->has($type)) {
            $relations = $this->_relations->getParents($type);
            $arguments = [];
            foreach ($relations as $relation) {
                if ($relation) {
                    $relationArguments = $this->_collectConfiguration($relation);
                    if ($relationArguments) {
                        $arguments = array_replace($arguments, $relationArguments);
                    }
                }
            }
        } else {
            $arguments = [];
        }
        if (isset($this->_arguments[$type])) {
            if ($arguments && count($arguments)) {
                $arguments = array_replace_recursive($arguments, $this->_arguments[$type]);
            } else {
                $arguments = $this->_arguments[$type];
            }
        }
        $this->_mergedArguments[$type] = $arguments;
        return $arguments;
    }
    return $this->_mergedArguments[$type];
}

This code is (or one of the parts of) responsible for smashing down the various di.xml-files scattered around and merge their arguments to use in dependency injection. The magic ingredient in this method that's responsible for this is array_replace_recursive.

However... This could have some unexpected side-effects.

A theoretical example

Let's take a look about order of preference / order of loading. We all know that we can use the <sequence>-tag in our module.xml to determine the loading order of modules. So let's say we have the following setup:

• Module A waits for Module X to load
• Module B waits for Module X and Module A to load.

So the order of loading would be: Module X -> Module A -> Module B.
This means that the order of dependency injection using the <type>-tag in di.xml would follow the same pattern. So when $bar is set in Module X you would expect it to be:

$bar = [
   'first' => ['message' => 'Hello'],
   'second' => ['message' => 'World']
];

However... It's not! It's actually:

$bar = [
   'second' => ['message' => 'World'],
   'first' => ['message' => 'Hello']
];

Explanation

Like I said, this is due to how the arrays are merged in \Magento\Framework\ObjectManager\Config\Config::_collectConfiguration(). It's this line:

$arguments = array_replace_recursive($arguments, $this->_arguments[$type]);

• When module A is loaded, the first array passed to array_replace_recursive is the one with the Hello-message. So the returning array (where $arguments is set to), only has this array. It's then later on set to $this->_arguments[$type].
• When module B is loaded, the first array passed to array_replace_recursive is the on with the World-message. However, due to the nature of array_replace_recursive, the array returned will have this array as it's first key.

So in short: loading a module later on will cause it's added values to appear earlier. This is kind of the opposite what you would expect.

A real world example

Not a big deal you might ask. However, this is a big deal if you're trying to do something where the order of the array matters. For example, I've recently was given the task to add a new renderer to the webapi module (your can read more about it here, and more about my implementation here).

By default, the renderers that the webapi module has are default, application_json, text_xml, application_xml and application_xhtml_xml. This results in an array with the following keys:

$this->_renders = [
    'default' => ...
    'application_json' => ...
    'text_xml' => ...
    'application_xml' => ...
    'application_xhtml_xml' => ...
];

Now, I've added my own custom renderer to this list (text_plain), but after what I've explained above (and I discovered today, the resulting array was not the above one with my renderer added to the end of it, but instead:

$this->_renders = [
    'text_plain' => ...       // NOOOOOOO!!!
    'default' => ...
    'application_json' => ...
    'text_xml' => ...
    'application_xml' => ...
    'application_xhtml_xml' => ...
];

Now take a look at \Magento\Framework\Webapi\Rest\Response\RendererFactory::_getRendererClass(), and specifically at the part where determined where the renderer is used:

foreach ($acceptTypes as $acceptType) {
    foreach ($this->_renders as $rendererConfig) {
        $rendererType = $rendererConfig['type'];
        if ($acceptType == $rendererType || $acceptType == current(
            explode('/', $rendererType)
        ) . '/*' || $acceptType == '*/*'
        ) {
            return $rendererConfig['model'];
        }
    }
}

If I make an request with Accept: */*, this whole foreach-loop will eventually find the OR-statement || $acceptType == '*/*' and it would just say "Whatever! I'll return my first entry". Now this would be perfect since the webapi sets it's first item as default (which is the JSON renderer by the way), but because I've added a renderer to this list the first result was my text/plain-renderer.

The result? All API-requests that had the Accept-header set to */* expected a JSON result, but instead they broke.

Conclusion

The way I see it this ticket can go in 2 directions:

• Change the way how dependency injection is handled by making sure that the order of arguments is the same as the loading order of the modules.
• Update the webapi module to explicitly choose the default-renderer when Accept = */*. However, this is not the reason of this bug, but more of a result.

Perhaps both deserve attention.

Sorry for the long ticket, but I don't know how else I could explain this.

[t-richards]

User agents that cannot really handle */* responses should be more explicit in what they request. For example, during checkout, when the estimate-shipping-methods API call on the frontend is only capable of accepting JSON -- it breaks the checkout process in weird ways if the response has an unexpected content type.

The originator of this request (in the example above, the mage/storage library) should specify the appropriate Accept: application/json header before sending the request.

...but also it would be nice if the merge order were more sane 😁

[magento-engcom-team]

@kanduvisla, thank you for your report.
Only second part of the issue should be fixed:

Update the webapi module to explicitly choose the default-renderer when Accept = */*. However, this is not the reason of this bug, but more of a result.

We've created internal ticket(s) MAGETWO-85299 to track progress on the issue.