Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thedavidmeister’s picture

Issue tags: +Novice
sandipmkhairnar’s picture

Deprecated drupal_json_decode() by Json::decode().

sandipmkhairnar’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, remove_deprecate_json_decode-2093173-2.patch, failed testing.

sandipmkhairnar’s picture

Status: Needs work » Needs review
FileSize
47.79 KB

Status: Needs review » Needs work

The last submitted patch, remove_deprecate_call-2093173-5.patch, failed testing.

damiankloip’s picture

Issue tags: +Needs reroll

Looks like this needs a reroll, plus there are no use statements anywhere for Drupal\Component\Utility\Json.

thedavidmeister’s picture

yeah, i linked to the documentation for "use" in the parent issue's summary.

rbayliss’s picture

Status: Needs work » Needs review
FileSize
28.68 KB
33.28 KB

Rerolled and added use statements. It looks like a bunch of calls to json_decode were switched over as well, including in vendor files, so I switched those back.

sandipmkhairnar’s picture

Its my fault :( Thanks rbayliss. I have test the patch its working fine for me.

dawehner’s picture

Is there a reason why we still have drupal_json_decode() in the following places left?

views_ui/lib/Drupal/views_ui/Tests/TagTest.php
53:    $matches = (array) json_decode($result->getContent());
59:    $matches = (array) json_decode($result->getContent());
68:    $matches = (array) json_decode($result->getContent());

block/tests/Drupal/block/Tests/CategoryAutocompleteTest.php
68:    $this->assertSame(MapArray::copyValuesToKeys($suggestions), json_decode($result->getContent(), TRUE));

system/lib/Drupal/system/Tests/Form/FormTest.php
437:    $values = json_decode($this->drupalPostForm('form-test/range', array(), 'Submit'));
467:      $result = json_decode($this->drupalPostForm('form-test/color', $edit, 'Submit'));

system/lib/Drupal/system/Tests/Form/CheckboxTest.php
68:    $results = json_decode($this->drupalPostForm('form-test/checkboxes-zero', array(), 'Save'));
73:    $results = json_decode($this->drupalPostForm('form-test/checkboxes-zero', $edit, 'Save'));

system/lib/Drupal/system/Tests/Database/SelectTableSortDefaultTest.php
41:      $data = json_decode($this->drupalGetContent());
69:      $data = json_decode($this->drupalGetContent());

system/lib/Drupal/system/Tests/Database/TemporaryQueryTest.php
42:    $data = json_decode($this->drupalGetContent());

system/lib/Drupal/system/Tests/Database/SelectPagerDefaultTest.php
48:      $data = json_decode($this->drupalGetContent());
82:      $data = json_decode($this->drupalGetContent());

ckeditor/lib/Drupal/ckeditor/Plugin/Editor/CKEditor.php
212:    $toolbar_settings['buttons'] = json_decode($toolbar_settings['buttons'], FALSE);

edit/lib/Drupal/edit/Tests/EditLoadingTest.php
256:      $ajax_commands = drupal_json_decode($response);
275:      $ajax_commands = drupal_json_decode($response);
damiankloip’s picture

Status: Needs review » Needs work

That looks like a definitive list to me.

Cyberschorsch’s picture

Are we going to replace json_decode() with Json::decode() as well?

thedavidmeister’s picture

Are we going to replace json_decode() with Json::decode() as well?

Maybe. Out of scope here though, I think. I guess first, we'd want to figure out if there's a reason that they're not already using Json::decode().

Cyberschorsch’s picture

Status: Needs work » Needs review
FileSize
36.01 KB

Alright. I attached the patch with the missing replacements.

Status: Needs review » Needs work

The last submitted patch, 2093173-remove_calls_drupal_json_decode-15.patch, failed testing.

Cyberschorsch’s picture

Status: Needs work » Needs review
FileSize
34.71 KB
dawehner’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.61 KB

Uploading an interdiff between #9 and #17 used for the review.

I agree that replacing json_decode itself would be out of scope.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Patch no longer applies.

Cyberschorsch’s picture

Status: Needs work » Needs review
FileSize
34.66 KB

Rerolled the patch.

Status: Needs review » Needs work
Issue tags: -Novice, -Needs reroll

The last submitted patch, 2093173-remove_calls_drupal_json_decode-20.patch, failed testing.

Cyberschorsch’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs reroll
mcrittenden’s picture

Issue tags: -Needs reroll

Tags

areke’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch doesn't apply anymore and needs to be re-rolled again.

forbesgraham’s picture

Assigned: Unassigned » forbesgraham
forbesgraham’s picture

Status: Needs work » Needs review
FileSize
35.49 KB

Re-rolling patch from comment 20.

Status: Needs review » Needs work

The last submitted patch, 26: 2093173-remove_calls_drupal_json_decode-21.patch, failed testing.

The last submitted patch, 26: 2093173-remove_calls_drupal_json_decode-21.patch, failed testing.

forbesgraham’s picture

My apologies, I think some typos are behind this. I'm submitting a new one shortly..

forbesgraham’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
36.13 KB

Trying this again.

Status: Needs review » Needs work

The last submitted patch, 30: 2093173-remove_calls_drupal_json_decode-30.patch, failed testing.

forbesgraham’s picture

Grr..sorry I'll take another stab at this tonight or tomorrow :(

forbesgraham’s picture

The re-roll failed when I tried to apply it and it's getting late. I'll get back to this soon.

damiankloip’s picture

Assigned: forbesgraham » Unassigned
Status: Needs work » Needs review
FileSize
44.17 KB
14.46 KB

Sorry, @forbesgraham, It's been a while :)

We need to keep the scope of this patch intact, so the changes to the expected values in the tests that were changes (like views-uiedit > views-ui-edit) break the tests. Even though they are indeed wrong, this is not the place the fix them.

Also, some missing conversions in tests.

sun’s picture

Issue tags: +@deprecated
dawehner’s picture

Just a tiny bit.

+++ b/core/modules/rest/lib/Drupal/rest/Tests/CreateTest.php
@@ -7,6 +7,9 @@
+use Drupal\rest\Tests\RESTTestBase;

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.php
--- a/core/modules/rest/lib/Drupal/rest/Tests/CreateTest.php
+++ b/core/modules/rest/lib/Drupal/rest/Tests/CreateTest.php

+++ b/core/modules/rest/lib/Drupal/rest/Tests/CreateTest.php
@@ -7,6 +7,9 @@
 namespace Drupal\rest\Tests;

This base class exists in the same namespace, so we don't have to add it.

dawehner’s picture

34: 2093173-34.patch queued for re-testing.

The last submitted patch, 34: 2093173-34.patch, failed testing.

InternetDevels’s picture

jsobiecki’s picture

Assigned: Unassigned » jsobiecki
Issue tags: +Needs reroll
jsobiecki’s picture

Issue tags: -Needs reroll

I rerolled patch. Now it should apply cleanly.

jsobiecki’s picture

Patch after reroll applies cleanly for me. I verified codebase by using cscope, and it haven't detected call to drupal_json_decode().
It seems that it works OK now.

At implementation level, it's only simple substitutions, nothing fancy here.

jsobiecki’s picture

Assigned: jsobiecki » Unassigned

Status: Needs review » Needs work

The last submitted patch, 42: replace-drupal-json-decode-2093173-41.patch, failed testing.

The last submitted patch, 42: replace-drupal-json-decode-2093173-41.patch, failed testing.

jlbellido’s picture

Assigned: Unassigned » jlbellido
Issue tags: +SprintWeekend 2014, +D8SVQ

I'll work on this.

jlbellido’s picture

Status: Needs work » Needs review

Tests pass locally, run test again.

jlbellido’s picture

YesCT’s picture

Issue tags: -SprintWeekend 2014 +SprintWeekend2014

fixing tag. sorry for noise.

ParisLiakos’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
longwave’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
45.75 KB
1.33 KB

Rerolled, removed a new reference in EditorSecurityTest, and a documentation reference in the JSON utility class itself.

Status: Needs review » Needs work

The last submitted patch, 51: 2093173-remove-drupal_json_decode.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
45.54 KB
ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

longwave’s picture

FileSize
45.54 KB

Previous patch only applied with fuzz, rerolled, still should be RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 55: 2093173-remove-drupal_json_decode-55.patch, failed testing.

longwave’s picture

Status: Needs work » Needs review
FileSize
45.43 KB

Rerolled, again

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Looks like we're missing a change notice about this: https://drupal.org/list-changes/published?keywords_description=drupal_js...

ianthomas_uk’s picture

Assigned: jlbellido » Unassigned
Status: Needs work » Needs review
Parent issue: » #2208607: Write script to automate replacement of deprecated functions where possible

This should be done as part of #2208607: Write script to automate replacement of deprecated functions where possible, so writing a patch should be postponed on that.

I've posted a draft change record at https://drupal.org/node/2219113 which needs review.

grom358’s picture

Using Function Replacer Part of Write script to automate replacement of deprecated functions where possible.

$ function_replace.php drupal_json_decode 'Drupal\Component\Utility\Json' UtilityJson decode
ianthomas_uk’s picture

Status: Needs review » Reviewed & tested by the community

Patch is a good reroll of the earlier (manual) patch. Most of the changes are either merge conflicts or the use statements being in a different order.

The one real change is that the scripted patch doesn't remove the @see drupal_json_decode() from the new function's docblock, but that's probably better removed with the function itself anyway.

Strictly speaking I don't think this requires a change record as we're not actually removing the old functions, but there is a draft at https://drupal.org/node/2219113 anyway.

damiankloip’s picture

Yeah, prob doesn't need a CR? As we are just converting the core usages of the function?

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 880f697 and pushed to 8.x. Thanks!

jessebeach’s picture

Issue tags: -Needs change record

The change record just needed to be published.

Removed 'Needs change record' tag.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.