Remove all uses of array_merge_recursive, or document why they are being used instead of NestedArray::mergeDeep()

So what we should do here is check these arrays for scalar elements that might be affected.

xpostses, precious.

Except, you're not. There's no ajax_render hunk in there.

or document why they are being used instead

Because drupal_array_merge_deep() is slower...?

FYI:
Blocking #1462410: Clientside validation is not working properly with file component upload button

I can verify that the changes to core/modules/file/file.module and core/includes/ajax.inc fix #1525784: Drupal.settings.content_lock.internal_urls is array instead of string, breaks file.module's JavaScript.

The change to common.inc is obsolete because the use of array_merge_recursive() in drupal_get_filetransfer_info() has been replaced with use of module_invoke_all().

I disagree with the change to user.module. The use of array_merge_recursive() when collecting a set of attributes together will properly gather, for example, multiple settings of the class attribute into an array which drupal_attributes() will automatically join together with spaces. Changing this behavior, so a new setting of an attribute overwrites a previous one, might not make sense. Now, I understand that if people set $user['attributes_array']['class'] = array('myclass');, drupal_array_merge_deep() will merge those nicely because the arrays use numeric indexes. But if a single module author puts $user['attributes_array']['class'] = 'myclass'; (a flat string), then drupal_array_merge_deep() will let the flat string completely override the previous settings for that attribute instead of accumulating them. Perhaps setting the class attribute using an array is best practice, but I think that a lot of modules still set classes using flat strings... and drupal6 still doesn't support arrays in its drupal_attributes(), so there is a lot of code migrated from drupal6 which would use attributes with flat strings still.

Here is a re-roll which fixes the patch not applying in core/includes/common.inc. This re-roll also drops the change regarding attributes, with the above reasoning.

Re #15. Setting a string for $element['#attributes']['class'] is not supported by design.
We need to ensure that you can do $element['#attributes']['class][] = 'my-new-class'; from a hook_form_alter() or other alter hooks. It is just one of many things you need to port from your Drupal 6 modules.

@ohnobinki: Thanks. An interdiff would have been helpful to see the changes since the last patch.

While @ohnobinki's reasoning for skipping damp() in template_process_username() is invalid (as @tstoeckler pointed out), attributes arrays are not deeply nested by design in the first place, so damp() is not really required there.

Second, template_process_username() is potentially called very often and thus in the critical theme/render path, so not using the slower user-space code when it's not strictly required is a good thing for performance.

Tentatively marking this RTBC.

...although I'm not really sure whether the damp() replacements that are fixing actual bugs being reported in other issues shouldn't be split out into those issues (e.g., #1525784: Drupal.settings.content_lock.internal_urls is array instead of string, breaks file.module's JavaScript [which seems to be a duplicate of another Ajax settings issue]). For known functional bugs, we should actually have tests to prevent them in the future. For the other plain damp() replacements without known bugs, we're just applying a code pattern throughout core that is known to merge associative arrays collated by modules in a compatible way.

This patch adds tests for the use of drupal_array_merge_deep() in the AJAX system.

This still excludes the changes to user.module with regard to @sun's reasoning. If user.module should still be switched to drupal_array_merge_deep(), I can add that to the patch again.

I'm not changing the issue property to avoid infuriating people watching the "bug report" queue, but at least some of the changes contained here are bugfixes, and they should definitely be backported.

Assuming this comes back green, let's get this in.
Before anyone complains on the usage of the deprecated procedural wrappers for NestedArray, please note that this fixes really, really (!) hard to dissect bugs (I personally was bitten by ajax.inc) that are *impossible* to fix in contrib. We also have lots of other usage of the drupal_array_ functions lying around in core, so this does not make the situation any worse.

Looked over everything, including the non-trivial parts (especially rdf.module) and it looks good to go.

Still looks good.

This seems to include all the fixes that were in #791860: array_merge_recursive() is never what we want in Drupal: add a drupal_array_merge_recursive() function instead., so I marked that other one as duplicate.

This looks like good clean-up. I had two questions while looking through this:

1) How would a normal person not deeply embroiled in the day-to-day commits of D8 ever make the mental leap of array_merge_recursive() => NestedArray::mergeDeepArray()? One would think that there would be the word "Recursive" in the method name to help, and it's confusing that it isn't. When I asked Alex that question, he referred me to the discussion at #208611: Add drupal_array_merge_deep() and drupal_array_merge_deep_array() to stop drupal_add_js() from adding settings twice but that was back when this was a global function, not a method off a class called "NestedArray" which clearly insinuates we're doing something "specialer" than raw PHP. Obviously, not the fault of this issue, but is probably worth a follow-up discussion.

2) Catch had a concern about tests for module_invoke_all() at #791860-33: array_merge_recursive() is never what we want in Drupal: add a drupal_array_merge_recursive() function instead.. However, the patch over there didn't have any test hunk changes and this one does, so I think that addresses it? When I asked tim about it, he said he couldn't think of what other tests he would add, at any rate.

Therefore, I don't see anything else to complain about so!

Committed and pushed to 8.x. Thanks!

Oops. Needs a change notice. And then a backport.

I would caution to backport only on a case by case basis, not in its entirety. NestedArray::mergeDeep() intentionally has different semantics (with respect to handling scalar values) than array_merge_recursive(), as documented by that function. Backporting such an API change to module_invoke_all() seems unwise. However, backporting the ajax_render() fix per #208611-43: Add drupal_array_merge_deep() and drupal_array_merge_deep_array() to stop drupal_add_js() from adding settings twice would be a clear bug fix IMO, and therefore, worth doing. I don't know about the other specific cases.

Linking to related issue, FWIW.
#66183: Add helper methods to inject items into a particular position in associative arrays

FYI, #36 doesn't apply to D7.43.

Backporting such an API change to module_invoke_all() seems unwise.

Yes it can be risky to change the behavior of module_invoke_all() in Drupal 7.

I ran into one bug caused by this behavior, see #2852809: Facetapi module now implement hook_i18n_string_info.
I did an experiment on a local D7 test site, where I have two versions of module_invoke_all() and compare the difference each time it is called. So far the only case where it was different was this bug.
This does not mean changing this is safe, of course.

100% agreed on case-by-case. That's the reason for the cautious title of this issue, because I was in a D7 mindset at the time.

I'd say we should open distinct issues for specific cases in D7, and link to them here.
Some of them might be closed as wontfix, but still this is better than having all the discussion here.

See #2865599: module_invoke_all() can have surprising results thanks to array_merge_recursive()

Just ran into this with #2870289: file_ajax_upload() causes malformed Drupal.settings too.

I agree that this should be handled in separate issues (as needed) considering that there are only a few places in 7.x that still use this.

So, moving this issue back to 8.x since this was primarily about NestedArray::mergeDeep() which isn't in 7.x.

Really wish I could mark as "Closed (fixed)", but I guess I'll just mark as "Fixed".