Files: 
CommentFileSizeAuthor
#36 array_merge_recursive-1356170-36.patch15.66 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,505 pass(es).
[ View ]
#34 array_merge_recursive-1356170-34.patch15.66 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch array_merge_recursive-1356170-34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 interdiff.txt815 bytestim.plunkett
#28 drupal-1356170-28.patch14.86 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,975 pass(es).
[ View ]
#27 drupal-1356170-27.patch55.35 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,334 pass(es).
[ View ]
#25 drupal-1356170-25.patch13.13 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,313 pass(es), 50 fail(s), and 12 exception(s).
[ View ]
#25 interdiff.txt3.88 KBtim.plunkett
#21 drupal-1356170-21.patch9 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 48,905 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#19 drupal-1356170-19-tests.patch2.8 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 37,297 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#19 drupal-1356170-19-combined.patch6.95 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 37,297 pass(es).
[ View ]
#18 drupal-1356170-18.patch7.75 KBohnobinki
PASSED: [[SimpleTest]]: [MySQL] 36,980 pass(es).
[ View ]
#15 drupal-1356170-15.patch4.87 KBohnobinki
PASSED: [[SimpleTest]]: [MySQL] 36,998 pass(es).
[ View ]
#10 drupal-1356170-10.patch5.36 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 34,132 pass(es).
[ View ]
#8 drupal-1356170-8.patch5.35 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/ajax.inc.
[ View ]
#5 drupal-1356170-5.patch2.78 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 34,118 pass(es).
[ View ]
#1 drupal-1356170-1.patch4.76 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 34,116 pass(es).
[ View ]

Comments

tim.plunkett’s picture

Status:Active» Needs review
StatusFileSize
new4.76 KB
PASSED: [[SimpleTest]]: [MySQL] 34,116 pass(es).
[ View ]

This is merely to see what the testbot says. I've not yet sat down to think about whether the replacement is appropriate :)

tim.plunkett’s picture

Issue tags:+Needs tests

I can't think of a single place in core where we'd want the value of a string key to become an array, or the value of a numeric key to become appended to a new value.

So I think each of these hunks is valid, and we just need tests to demonstrate why.

xjm’s picture

Issue tags:-Needs tests

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

xjm’s picture

Issue tags:+Needs tests

xpostses, precious.

tim.plunkett’s picture

StatusFileSize
new2.78 KB
PASSED: [[SimpleTest]]: [MySQL] 34,118 pass(es).
[ View ]

I restored #208611: Add drupal_array_merge_deep() and drupal_array_merge_deep_array() to stop drupal_add_js() from adding settings twice to its fixed state, and I'm adding that patch's hunk here.

Still needs tests.

tstoeckler’s picture

Status:Needs review» Needs work

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

sun’s picture

or document why they are being used instead

Because drupal_array_merge_deep() is slower...?

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new5.35 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/ajax.inc.
[ View ]

Whoops, the patch in #5 was for another issue. I mixed up git diff and git diff --staged. Sorry @tstoeckler.

@sun, of those changes, you could make the case that we're babysitting broken code. If a module tries to implement the same hook_hook_info for tokens and it blows up, that's their fault.

But there are times, like ajax_render, where it should be core's responsibility to take the input and return something useful, not an improperly merged array.

If some of these functions should continue to use array_merge_recursive because we don't care if someone breaks something, that's fine, but let's point that out or add a helpful error message or something.

Status:Needs review» Needs work

The last submitted patch, drupal-1356170-8.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new5.36 KB
PASSED: [[SimpleTest]]: [MySQL] 34,132 pass(es).
[ View ]

Missing parentheses.

tim.plunkett’s picture

Just some analysis about how these are called, or why they are needed.

+++ b/core/includes/ajax.incundefined
@@ -291,7 +291,7 @@ function ajax_render($commands = array()) {
+    array_unshift($commands, ajax_command_settings(drupal_array_merge_deep_array($settings['data']), TRUE));

Needed for the same reasons as drupal_add_js().

+++ b/core/includes/common.incundefined
@@ -7555,7 +7555,7 @@ function drupal_get_filetransfer_info() {
+          $info = drupal_array_merge_deep($info, $result);

Wrapped in drupal_static().

+++ b/core/includes/module.incundefined
@@ -737,7 +737,7 @@ function module_hook_info() {
+            $hook_info = drupal_array_merge_deep($hook_info, $result);

Wrapped by drupal_static().

+++ b/core/modules/file/file.moduleundefined
@@ -278,7 +278,7 @@ function file_ajax_upload() {
+  $settings = drupal_array_merge_deep_array($js['settings']['data']);

Needed for the same reasons as drupal_add_js().

+++ b/core/modules/node/node.pages.incundefined
@@ -171,7 +171,7 @@ function node_form($form, &$form_state, $node) {
+    $form = drupal_array_merge_deep($form, $extra);

There is a nice @todo here to remove hook_form entirely :)

+++ b/core/modules/system/system.moduleundefined
@@ -3759,7 +3759,7 @@ function _system_date_formats_build() {
-      $date_formats[$module_format['type']][$module_format['format']] = array_merge_recursive($date_formats[$module_format['type']][$module_format['format']], $module_format);

This is a helper function that is wrapped by drupal_static().

+++ b/core/modules/translation/translation.pages.incundefined
@@ -58,7 +58,7 @@ function translation_node_overview($node) {
+        $options[] = empty($links->links[$langcode]['href']) ? l($text, $path, $query) : l($text, $links->links[$langcode]['href'], drupal_array_merge_deep($links->links[$langcode], $query));

This is a crazy ternary. Wow.

+++ b/core/modules/user/user.moduleundefined
@@ -1497,7 +1497,7 @@ function template_process_username(&$variables) {
-    $variables['link_options']['attributes'] = array_merge_recursive($variables['link_attributes'], $variables['attributes_array']);

Not sure about this one.

0 days to next Drupal core point release.

tim.plunkett’s picture

The other idea I had was to switch the order of the arguments passed to drupal_array_merge_deep() when appropriate. I believe I did this in Views when fixing a similar bug.

If a contrib or custom module caused a key collision with array_merge_recursive, the array would be broken. But with this patch, the offending module would overwrite core, effectively acting like an alter.

In the case of $info = drupal_array_merge_deep($info, $result);, I suggest that this be switched to $info = drupal_array_merge_deep($result, $info);. I can reroll like that if anyone else agrees.

ohnobinki’s picture

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.

ohnobinki’s picture

StatusFileSize
new4.87 KB
PASSED: [[SimpleTest]]: [MySQL] 36,998 pass(es).
[ View ]

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.

tstoeckler’s picture

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.

sun’s picture

Status:Needs review» Reviewed & tested by the community

@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.

ohnobinki’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new7.75 KB
PASSED: [[SimpleTest]]: [MySQL] 36,980 pass(es).
[ View ]

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.

tim.plunkett’s picture

Issue tags:-Needs tests+needs backport to D7
StatusFileSize
new6.95 KB
PASSED: [[SimpleTest]]: [MySQL] 37,297 pass(es).
[ View ]
new2.8 KB
FAILED: [[SimpleTest]]: [MySQL] 37,297 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

I had to reroll, split the test into its own patch to prove it works.

Also, any reason not to backport this?

tstoeckler’s picture

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.

tim.plunkett’s picture

StatusFileSize
new9 KB
FAILED: [[SimpleTest]]: [MySQL] 48,905 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, drupal-1356170-21.patch, failed testing.

tstoeckler’s picture

Status:Needs work» Reviewed & tested by the community

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.

tim.plunkett’s picture

Status:Reviewed & tested by the community» Needs work

This is blocked by #1778410: Throw exception when RDF namespaces collide, and the assumptions rdf_get_namespaces() makes about module_invoke_all().

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new3.88 KB
new13.13 KB
FAILED: [[SimpleTest]]: [MySQL] 49,313 pass(es), 50 fail(s), and 12 exception(s).
[ View ]

Actually, I discussed that with linclark, and she said it would be appropriate to resolve duplicate namespaces by taking the first defined one.

In addition, to mirror #1705920: Convert all calls to procedural drupal_array_*() functions to Drupal\Component\Utility\NestedArray I've switched everything to using the method.

Status:Needs review» Needs work

The last submitted patch, drupal-1356170-25.patch, failed testing.

tim.plunkett’s picture

Title:Remove all uses of array_merge_recursive, or document why they are being used instead of drupal_array_merge_deep» Remove all uses of array_merge_recursive, or document why they are being used instead of NestedArray::mergeDeep()
Status:Needs work» Needs review
StatusFileSize
new55.35 KB
PASSED: [[SimpleTest]]: [MySQL] 49,334 pass(es).
[ View ]

Including the patch from the other issue.

tim.plunkett’s picture

StatusFileSize
new14.86 KB
PASSED: [[SimpleTest]]: [MySQL] 49,975 pass(es).
[ View ]

Yay, that other issue went in.

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs work
Issue tags:-needs backport to D7

The last submitted patch, drupal-1356170-28.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review

#28: drupal-1356170-28.patch queued for re-testing.

tim.plunkett’s picture

Issue tags:+needs backport to D7

#28: drupal-1356170-28.patch queued for re-testing.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new815 bytes
new15.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch array_merge_recursive-1356170-34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Updating for a recent commit, and marking back to RTBC per #29, that was a random failure.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, array_merge_recursive-1356170-34.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new15.66 KB
PASSED: [[SimpleTest]]: [MySQL] 49,505 pass(es).
[ View ]

Rerolled.

tstoeckler’s picture

Status:Needs review» Reviewed & tested by the community

Still looks good.

yched’s picture

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.

tim.plunkett’s picture

webchick’s picture

Status:Reviewed & tested by the community» Fixed

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!

webchick’s picture

Title:Remove all uses of array_merge_recursive, or document why they are being used instead of NestedArray::mergeDeep()» Change notice: Remove all uses of array_merge_recursive, or document why they are being used instead of NestedArray::mergeDeep()
Priority:Normal» Critical
Status:Fixed» Active
Issue tags:+Needs change record

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

effulgentsia’s picture

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.

tim.plunkett’s picture

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.

tim.plunkett’s picture

Version:8.x-dev» 7.x-dev
Priority:Critical» Normal
Status:Active» Patch (to be ported)

Added a change notice here: http://drupal.org/node/1887290

tim.plunkett’s picture

Title:Change notice: Remove all uses of array_merge_recursive, or document why they are being used instead of NestedArray::mergeDeep()» Remove all uses of array_merge_recursive, or document why they are being used instead of NestedArray::mergeDeep()
Issue tags:-Needs change record
tim.plunkett’s picture

Issue summary:View changes

link to functions