Remove uses of deprecated drupal_merge_* functions [#2221773]

The following functions have been refactored into methods on the NestedArray class, all calls should be replaced with direct calls to the methods the wrappers are calling:

drupal_merge_js_settings($settings_items) becomes \Drupal\Component\Utility\NestedArray::mergeDeepArray($settings_items, TRUE) (note the extra parameter)
drupal_merge_attached() becomes \Drupal\Component\Utility\NestedArray::mergeDeep()

Comment	File	Size	Author
#17	drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-16.patch	12.13 KB	visabhishek
#17
#17	interdiff-2221773-11-16.txt	1.72 KB	visabhishek
#15	drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-15.patch	12.65 KB	visabhishek
#15
#15	interdiff-2221773-11-15.txt	2.01 KB	visabhishek
#11	drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-11.patch	10.57 KB	visabhishek
#11
#11	interdiff-2221773-09-11.txt	9.2 KB	visabhishek
#9	interdiff.txt	2.72 KB	sriharsha.uppuluri
#9	drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-09.patch	10.98 KB	sriharsha.uppuluri
#9
#4	drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-04.patch	10.21 KB	visabhishek
#4

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

ianthomas_uk

he/him

English

Brighton, UK

CreditAttribution: ianthomas_uk commented 21 March 2014 at 15:30

Issue summary:

View changes

Comment #2

ianthomas_uk

he/him

English

Brighton, UK

CreditAttribution: ianthomas_uk commented 21 March 2014 at 15:38

Status:

Active

» Postponed

This will be a small patch and will conflict with #2221755: Remove uses of deprecated Element functions, so let's do that one first.

Comment #3

ianthomas_uk

he/him

English

Brighton, UK

CreditAttribution: ianthomas_uk commented 1 April 2014 at 12:59

Status:

Postponed

» Active

That patch is in.

These are new functions in 8.x, so don't need their own change record, but I have updated https://drupal.org/node/1911578 to refer to the NestedArray instead of drupal_merge_js_settings.

Comment #4

visabhishek CreditAttribution: visabhishek commented 2 April 2014 at 07:17

Status:

Active

» Needs review

File	Size
drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-04.patch	10.21 KB

i am uploading the initial patch, Please review...

Comment #5

2 April 2014 at 07:50

Status:

Needs review

» Needs work

The last submitted patch, 4: drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-04.patch, failed testing.

Comment #6

vastav CreditAttribution: vastav commented 2 April 2014 at 10:57

Status:

Needs work

» Needs review

4: drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-04.patch queued for re-testing.

Comment #7

2 April 2014 at 11:30

Status:

Needs review

» Needs work

The last submitted patch, 4: drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-04.patch, failed testing.

Comment #8

ianthomas_uk

he/him

English

Brighton, UK

CreditAttribution: ianthomas_uk commented 2 April 2014 at 11:35

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -152,7 +152,7 @@ protected function ajaxRender(Request $request) {
+      $settings = \Drupal\Component\Utility\NestedArray::mergeDeepArray($scripts['settings']['data'], TRUE);

We should rely on 'use' statements, rather than having the fully qualified class name everywhere.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/MergeAttachmentsTest.php
@@ -221,10 +221,10 @@ function testJsSettingMerging() {
+    // retained, it's up to the rest of the system (\Drupal\Component\Utility\NestedArray::mergeDeepArray())

Comments should wrap at 80 characters

I've not looked into what is causing the failure, but the failing test is very relevant to the code we're changing so it does look like a problem with the patch rather than the testbot (there may have been a different failure before the retest though).

Comment #9

sriharsha.uppuluri CreditAttribution: sriharsha.uppuluri commented 4 April 2014 at 05:31

Status:

Needs work

» Needs review

File	Size
drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-09.patch	10.98 KB

interdiff.txt	2.72 KB

Updated the patch based on #8

Comment #10

4 April 2014 at 06:43

Status:

Needs review

» Needs work

The last submitted patch, 9: drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-09.patch, failed testing.

Comment #11

visabhishek CreditAttribution: visabhishek commented 7 April 2014 at 14:47

Status:

Needs work

» Needs review

File	Size
interdiff-2221773-09-11.txt	9.2 KB
drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-11.patch	10.57 KB

updated patch

Comment #12

7 April 2014 at 15:20

Status:

Needs review

» Needs work

The last submitted patch, 11: drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-11.patch, failed testing.

Comment #13

vastav CreditAttribution: vastav commented 7 April 2014 at 16:25

Status:

Needs work

» Needs review

11: drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-11.patch queued for re-testing.

Comment #14

ParisLiakos CreditAttribution: ParisLiakos commented 13 April 2014 at 14:57

Status:

Needs review

» Needs work

after applying the patch, i still find calls to drupal_merge_attached in drupal_pre_render_links, _drupal_render_process_post_render_cache and drupal_render_collect_attached

Comment #15

visabhishek CreditAttribution: visabhishek commented 13 April 2014 at 16:02

Status:

Needs work

» Needs review

File	Size
interdiff-2221773-11-15.txt	2.01 KB
drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-15.patch	12.65 KB

patch updated as per #14 and drupal_merge_attached () is also removed from common.inc

Comment #16

ParisLiakos CreditAttribution: ParisLiakos commented 13 April 2014 at 17:57

thanks!
i dont think we should remove drupal_merge_attached() itself in this issue though

Comment #17

visabhishek CreditAttribution: visabhishek commented 14 April 2014 at 05:56

File	Size
interdiff-2221773-11-16.txt	1.72 KB
drupal8.remove-uses-of-deprecated-drupal-merge-functions.2221773-16.patch	12.13 KB

patch update as per #16.

Comment #18

ParisLiakos CreditAttribution: ParisLiakos commented 14 April 2014 at 09:16

Status:

Needs review

» Reviewed & tested by the community

thank you!

Comment #19

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 16 April 2014 at 08:37

Status:

Reviewed & tested by the community

» Needs work

+++ b/core/lib/Drupal/Core/Ajax/AjaxResponse.php
@@ -152,7 +153,7 @@ protected function ajaxRender(Request $request) {
-      $settings = drupal_merge_js_settings($scripts['settings']['data']);
+      $settings = NestedArray::mergeDeepArray($scripts['settings']['data'], TRUE);

+++ b/core/lib/Drupal/Core/Asset/JsCollectionRenderer.php
@@ -68,7 +69,7 @@ public function render(array $js_assets) {
-          $element['#value'] = 'var drupalSettings = ' . Json::encode(drupal_merge_js_settings($js_asset['data'])) . ";";
+          $element['#value'] = 'var drupalSettings = ' . Json::encode(NestedArray::mergeDeepArray($js_asset['data'], TRUE)) . ";";

+++ b/core/modules/file/lib/Drupal/file/Controller/FileWidgetAjaxController.php
@@ -76,7 +76,7 @@ public function upload(Request $request) {
-    $settings = drupal_merge_js_settings($js['settings']['data']);
+    $settings = NestedArray::mergeDeepArray($js['settings']['data'], TRUE);

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -1770,7 +1770,7 @@ protected function drupalProcessAjaxResponse($content, array $ajax_response, arr
-          $drupal_settings = drupal_merge_js_settings(array($drupal_settings, $command['settings']));
+          $drupal_settings = NestedArray::mergeDeepArray(array($drupal_settings, $command['settings']), TRUE);

@Wim Leers was very adament about adding this as a separate function, so this should be left alone. At some point we should replace it with a non-procedural alternative, but that's for a separate issue.

Comment #20

ianthomas_uk

he/him

English

Brighton, UK

CreditAttribution: ianthomas_uk commented 21 April 2014 at 12:27

Status:

Needs work

» Closed (won't fix)

Thanks @tstockler. Actually he was adamant about having both functions as usability wrappers, see #2113015: Add drupal_merge_attached() function, to merge #attached assets, therefore this is a won't fix. Moving the wrappers into a class would be a separate issue.

I've changed the CR back to refer to the wrapper function.