array_merge_recursive() is never what we want in Drupal: add a drupal_array_merge_recursive() function instead.

Changed the function name to drupal_array_merge_recursive() since it's a replacement for array_merge_recursive(), similar to drupal_strlen() being a replacement for strlen().

Implemented feedback from #3, except for the first one, since as #4 points out, we *are* renumbering the keys.

This needs to be checked: do we really want to passthru *all numeric keys* or just those that are in a numeric sequence? Probably only the latter.

The implementation here is the same as what array_merge_recursive() does with respect to that. I'm not sure it's worth changing that aspect of the function (performance implications of checking whether they key is sequential?). update_retrieve_dependencies() is an example where the renumbering of all numeric keys is unfortunate, but even if we made the drupal_array_merge_recursive() function smarter, we wouldn't be able to use it here, since this function has additional logic controlling the merge (only use latter value if it is greater).

Also I wonder whether this might have a performance impact, as it's used in module_invoke_all()...

Yes, it does. For example, a user with permission to view comments and access user profiles viewing a node with 50 comments, results in drupal_array_merge_recursive() running 80 times (that's based on using devel_generate, which makes some comments authorship "root", having a link to a user profile and therefore a call to drupal_array_merge_recursive(), while some comments authorship "devel_generate" which does not result in a call to drupal_array_merge_recursive()). And drupal_array_merge_recursive() has a minimum overhead of 2us (on my machine) compared to array_merge_recursive() (more depending on the arguments), so that contributes 160us. Also, module_invoke_all('element_info') is 260us with the patch compared to 160us with HEAD. That's done once per page request, contributing another 100us. I haven't tracked down the other major sources, but the node with 50 comments scenario is 0.5ms slower (196.0ms HEAD, 196.5ms patch, +0.25%). IMO: this is worth it, since the behavior in HEAD is a bug, but I'm not sure what other profiling we want to do before deciding that.

+++ includes/bootstrap.inc	7 May 2010 15:51:22 -0000
@@ -1783,6 +1783,52 @@ function drupal_hash_base64($data) {
+      // Renumber numeric keys as array_merge_recursive() does.
+      if (is_numeric($key)) {
+        $result[] = $value;
+      }

This patch replaces this with is_integer().

+++ includes/bootstrap.inc	7 May 2010 15:51:22 -0000
@@ -1783,6 +1783,52 @@ function drupal_hash_base64($data) {
+      // Recurse when both values are arrays.
+      elseif (is_array($value) && isset($result[$key]) && is_array($result[$key])) {
+        $result[$key] = drupal_array_merge_recursive($result[$key], $value);
+      }

This is an inefficient order. This patch moves is_array($value) to the end, resulting in shaving 20us from module_invoke_all('element_info').

84 critical left. Go review some!

The idea in #8 is interesting, but I wasn't able to come up with an implementation that's any faster than what's in this patch, at least for the module_invoke_all('element_info') case. I believe the reason is that most hooks like 'element_info' don't end up actually requiring recursion (multiple modules don't usually implement the same top-level key), but iteration through all the top-level keys and checking if the key is an integer or string is required in any solution, and that's the bulk of the time. Perhaps there are cases where #8 would be faster than this patch, but we'd need to determine what they are. But I think info hooks like module_invoke_all('element_info') are the ones that matter most, and this patch is the fastest for those.

This patch is a significant optimization relative to #6. It adds a drupal_array_merge_recursive_array() function to avoid having to call 'call_user_func_array' to construct Drupal.settings, and to avoid having to call func_get_args() during recursion. This also allows module_invoke_all() to call drupal_array_merge_recursive_array() just once, instead of drupal_array_merge_recursive() once for each module.

module_invoke_all('element_info') is 164us in HEAD, 184us with this patch.

+++ includes/module.inc	11 May 2010 17:42:14 -0000
@@ -719,14 +720,12 @@ function module_invoke_all() {
+  $return = drupal_array_merge_recursive_array($return);

This patch adds an if(!empty($return)) check to avoid an extra stack call for hooks that don't return anything.

87 critical left. Go review some!

@mtlhtml: Was #11 meant for a different issue?

@mtlhtml: I'm guessing you're refering to #794388: Clicking the "count" button triggers a SQL query error. I think you posted that issue correctly: as an issue for the project you think is where the problem is (Gallery Assist), marked as a "support request" as the category. If rather than seeking support, you believe you found a bug, you can change the category to "bug report". The difference is that a "support request" is something where you're not sure if something about your installation or the way you're using the module is responsible for the problem, whereas a "bug report" is where you can provide the module maintainer with specific steps about how he or she can reproduce the problem from a clean Drupal install.

If you're new to working with open source software, one of the things you may find frustrating is that you don't get timely answers to your questions. This is often also true with proprietary software, but at least when you're buying software from a company, there's a tech support phone line or email address. But in the case of Drupal, module maintainers and people who participate in the support forums and issue queue often are not paid to do so, and need to carve out time in which to contribute as something they do in addition to their "day job". So, 2 days going by without an answer to a support request is not at all unusual. You just need to be patient. That said, check out the "Get Help" column of http://drupal.org/support for links to other places you can try. In addition to the issue queue of the module you're struggling with, you can try the support forums, IRC, email lists, and local user groups (all linked to from that page). You can also help attract faster support by offering a bounty.

In any case, posting to unrelated issues is generally frowned upon. You'll get better results from those other channels, so please stick to those. Thanks! A way to think of the Drupal community is that it's decentralized. People help out where they want to help out. People helping out on one project's issue queue may not be interested in helping out on another one. So posting on an unrelated issue is basically the same thing as SPAM. The best way to get help is to solicit it from where it's relevant. If the decentralized nature of open source is frustrating, you can always buy support from Acquia or another company.

@Crell: I'm not sure the semantics of array_merge_recursive() is ever desirable, so I kind of do think of this as us needing to fix a broken PHP function, but I'll concede that that's opinion only, so your point is a good one. I renamed to drupal_array_merge_deep() and added it to the issue that originally inspired it: #823428: Impossible to alter node URLs efficiently.

So I'm postponing this issue until that one is resolved. Assuming that issue lands in some form, I'll re-open this issue to address the question of whether to use the new function for module_invoke_all() and Drupal.settings. I think we should, but as Damien points out in #3, it's not critical.

More support related to #20: #208611-29: Add drupal_array_merge_deep() and drupal_array_merge_deep_array() to stop drupal_add_js() from adding settings twice. If anyone is able to, please help that issue land, so we can then discuss where else we need to switch from array_merge_recursive() to drupal_array_merge_deep(). Thanks!