Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xeniak’s picture

Assigned: Unassigned » xeniak
Status: Active » Needs review
FileSize
7.19 KB

OK, did it. This is my first patch; so please let me know if I did anything wrong.

Issue #2093161 by thedavidmeister: Remove and deprecate all calls to drupal_json_encode() in favour of \Drupal\Component\Utility\Json::encode().

Status: Needs review » Needs work

The last submitted patch, drupal8.base-system.2093161-1.patch, failed testing.

thedavidmeister’s picture

+<?xml version="1.0" encoding="UTF-8"?>
+<classpath>
+	<classpathentry kind="src" path=""/>
+	<classpathentry kind="con" path="org.eclipse.wst.jsdt.launching.JRE_CONTAINER"/>
+	<classpathentry kind="con" path="org.eclipse.wst.jsdt.launching.WebProject">
+		<attributes>
+			<attribute name="hide" value="true"/>
+		</attributes>
+	</classpathentry>
+	<classpathentry kind="con" path="org.eclipse.wst.jsdt.launching.baseBrowserLibrary"/>
+	<classpathentry kind="output" path=""/>
+</classpath>

Hey, thanks for the patch. Aside from the fails that you can see from the testbot, I don't think this stuff should be in the patch.

sidharthap’s picture

The common.inc file contains the
function drupal_json_encode($var) {
return Json::encode($var);
}

so we need to use directly \Drupal\Component\Utility\Json::encode() and remove this function "drupal_json_encode" from common.inc file ?
There is a discussion going on https://drupal.org/node/2093143

sandipmkhairnar’s picture

sandipmkhairnar’s picture

Status: Needs work » Needs review
xeniak’s picture

Yeah, I'm not sure what that is - I think it's something Zend Studio adds. I'll have to investigate how to get it excluded from the patch.

sandipmkhairnar’s picture

Thanks @xeniak for comment. Not sure but, I think it is removing all deprecate all calls drupal_json_encode() to json_encode.

Status: Needs review » Needs work

The last submitted patch, remove_deprecate_call_2093161-5.patch, failed testing.

sandipmkhairnar’s picture

Status: Needs work » Needs review
FileSize
10.54 KB

Status: Needs review » Needs work

The last submitted patch, remove_deprecate_call_2093161-10.patch, failed testing.

xeniak’s picture

Assigned: xeniak » Unassigned
thedavidmeister’s picture

Title: Remove and deprecate all calls to drupal_json_encode() in favour of \Drupal\Component\Utility\Json::encode() » Remove all calls to drupal_json_encode() in favour of \Drupal\Component\Utility\Json::encode()
thedavidmeister’s picture

@sandipmkhairnar, if you don't put use \Drupal\Component\Utility\Json at the top of the files where you want to call Json::encode() then PHP won't know what to do.

http://www.php.net/manual/en/language.namespaces.rationale.php
http://www.php.net/manual/en/language.namespaces.importing.php

+ $element['#value'] = 'var drupalSettings = ' . json::encode(drupal_merge_js_settings($js_asset['data'])) . ";";
return json::encode($commands);

Json, not json.

rbayliss’s picture

Status: Needs work » Needs review
FileSize
6.62 KB
12.95 KB

Added those use statements and corrected the capitalization of the class name.

Status: Needs review » Needs work

The last submitted patch, 2093161-remove_calls_drupal_json_encode-15.patch, failed testing.

rbayliss’s picture

Status: Needs work » Needs review
FileSize
6.76 KB
12.96 KB

Let's try this again... Same patch, hopefully better formatting.

thedavidmeister’s picture

Status: Needs review » Reviewed & tested by the community

I applied this patch.

Code standards look fine.

Site installs and clicking around a bit produces no fatal errors.

Didn't find any more calls to drupal_json_encode() after applying the patch.

The remaining docs references to drupal_json_encode() are probably fine as they're only on drupal_json_decode() and Json::decode().

RTBC.

I'd like to use this review as bonus (#2094585: [policy, no patch] Core review bonus) for #1987396: Refactor & Clean-up comment.module theme functions.

alexpott’s picture

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

Patch no longer applies.

sidharthap’s picture

Status: Needs work » Needs review
FileSize
15.22 KB

There are some occurrence of json_encode() in vendor/* directory. Will it required an attention too with this patch.

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 no longer applies; it needs to be re-rolled again.

Sumeet.Pareek’s picture

Status: Needs work » Needs review
FileSize
19.91 KB

Here is a re-rolled version of the patch.

Status: Needs review » Needs work

The last submitted patch, 23: 2093161-remove_calls_drupal_json_encode-21.patch, failed testing.

haithem_pro’s picture

Assigned: Unassigned » haithem_pro
Issue tags: +TUNIS_2013_DECEMBER

start working on issue

haithem_pro’s picture

haithem_pro’s picture

Status: Needs work » Needs review
FileSize
12.93 KB

Status: Needs review » Needs work

The last submitted patch, 27: remove_calls_drupal_json_encode-2093161-26.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 27: remove_calls_drupal_json_encode-2093161-26.patch, failed testing.

The last submitted patch, 26: remove_calls_drupal_json_encode-2093161-26.patch, failed testing.

The last submitted patch, 27: remove_calls_drupal_json_encode-2093161-26.patch, failed testing.

The last submitted patch, 26: remove_calls_drupal_json_encode-2093161-26.patch, failed testing.

InternetDevels’s picture

sun’s picture

Issue tags: +@deprecated
idflood’s picture

Issue tags: -Needs reroll
FileSize
6.77 KB

Patch in #34 wasn't applying anymore so here is a reroll.

ianthomas_uk’s picture

Reroll, plus add @ingroup php_wrappers back to Json::decode. That was removed in #34, but I don't understand why (it's still a wrapper around a PHP function, even if it isn't called drupal_$foo any more).

Other than that the patch looks good and should be an easy review, so it would be good to get this to RTBC.

thedavidmeister’s picture

ParisLiakos’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
error: patch failed: core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermTest.php:8
error: core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermTest.php: patch does not apply

Shouldn't we also remove the function here?

longwave’s picture

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

Rerolled.

Not sure we should remove drupal_json_encode() while its companion function drupal_json_decode() still exists?

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

They are different functions and there is already a similar issue #2093173: Remove all calls to drupal_json_decode(), use \Drupal\Component\Utility\Json::decode()

I just dont see the point of removing usage but not functions that are already deprecated for months now.
We will have to remove them anyway and not doing it here means more issues + more maintainers and reviewers time.

But i hate coming here so late and re-scoping the issue, so i ll let a maintainer decide what would be best

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6383d18 and pushed to 8.x. Thanks!

alexpott’s picture

Yep all usages should be removed before the function. Less to revert if removing the function falls or breaks something unexpectedly.

ParisLiakos’s picture

ah, i see, that makes sense:)
Thanks!

Status: Fixed » Closed (fixed)

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