Part of #2093143: [meta] Remove calls to @deprecated and "backwards compatibility" procedural functions from core

CommentFileSizeAuthor
#40 2093161-remove-drupal_json_encode-40.patch5.96 KBlongwave
#36 remove-drupal-json-encode-2093161-36.patch6.77 KBidflood
#34 interdiff-2093161-27-34.txt7.46 KBinternetdevels
#34 remove-drupal-json-encode-2093161-34.patch6.77 KBinternetdevels
#27 remove_calls_drupal_json_encode-2093161-26.patch12.93 KBhaithem_pro
#26 remove_calls_drupal_json_encode-2093161-26.patch12.93 KBhaithem_pro
#23 2093161-remove_calls_drupal_json_encode-21.patch19.91 KBsumeet.pareek
#20 2093161-remove_calls_drupal_json_encode-20.patch15.22 KBsidharthap
#17 interdiff.txt6.76 KBrbayliss
#17 2093161-remove_calls_drupal_json_encode-17.patch12.96 KBrbayliss
#15 interdiff.txt6.62 KBrbayliss
#15 2093161-remove_calls_drupal_json_encode-15.patch12.95 KBrbayliss
#10 remove_deprecate_call_2093161-10.patch10.54 KBsandipmkhairnar
#5 remove_deprecate_call_2093161-5.patch4.66 KBsandipmkhairnar
#1 drupal8.base-system.2093161-1.patch7.19 KBxeniak
#37 remove-drupal-json-encode-2093161-37.patch5.96 KBianthomas_uk

Comments

xeniak’s picture

xeniak commented
Assigned: Unassigned » xeniak
Status: Active » Needs review
StatusFileSize
new drupal8.base-system.2093161-1.patch7.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

thedavidmeister commented 
+<?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

sidharthap
Primary language English
commented

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 commented
StatusFileSize
new remove_deprecate_call_2093161-5.patch4.66 KB
sandipmkhairnar’s picture

sandipmkhairnar commented
Status: Needs work » Needs review
xeniak’s picture

xeniak commented

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

sandipmkhairnar commented

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

sandipmkhairnar commented
Status: Needs work » Needs review
StatusFileSize
new remove_deprecate_call_2093161-10.patch10.54 KB

Status: Needs review » Needs work

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

xeniak’s picture

xeniak commented
Assigned: xeniak » Unassigned
thedavidmeister’s picture

thedavidmeister commented
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()

drupal_json_encode() is *already deprecated*.

https://api.drupal.org/api/drupal/core%21includes%21common.inc/function/...

thedavidmeister’s picture

thedavidmeister commented

@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

rbayliss commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt6.62 KB
new 2093161-remove_calls_drupal_json_encode-15.patch12.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

rbayliss commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt6.76 KB
new 2093161-remove_calls_drupal_json_encode-17.patch12.96 KB

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

thedavidmeister’s picture

thedavidmeister commented
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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

sidharthap’s picture

sidharthap
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 2093161-remove_calls_drupal_json_encode-20.patch15.22 KB

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

mcrittenden’s picture

mcrittenden commented
Issue tags: -Needs reroll

Tags

areke’s picture

areke commented
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

sumeet.pareek commented
Status: Needs work » Needs review
StatusFileSize
new 2093161-remove_calls_drupal_json_encode-21.patch19.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

haithem_pro commented
Assigned: Unassigned » haithem_pro
Issue tags: +TUNIS_2013_DECEMBER

start working on issue

haithem_pro’s picture

haithem_pro commented
StatusFileSize
new remove_calls_drupal_json_encode-2093161-26.patch12.93 KB
haithem_pro’s picture

haithem_pro commented
Status: Needs work » Needs review
StatusFileSize
new remove_calls_drupal_json_encode-2093161-26.patch12.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

damiankloip commented
Status: Needs work » Needs review

27: remove_calls_drupal_json_encode-2093161-26.patch queued for re-testing.

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

internetdevels commented
Status: Needs work » Needs review
StatusFileSize
new remove-drupal-json-encode-2093161-34.patch6.77 KB
new interdiff-2093161-27-34.txt7.46 KB
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue tags: +@deprecated
idflood’s picture

idflood commented
Issue tags: -Needs reroll
StatusFileSize
new remove-drupal-json-encode-2093161-36.patch6.77 KB

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

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Assigned: haithem_pro » Unassigned
StatusFileSize
new remove-drupal-json-encode-2093161-37.patch5.96 KB

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

thedavidmeister commented
Issue tags: +Core Review Bonus

#2094585: [policy, no patch] Core review bonus from https://drupal.org/comment/8464143#comment-8464143

ParisLiakos’s picture

ParisLiakos commented
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

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2093161-remove-drupal_json_encode-40.patch5.96 KB

Rerolled.

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

ParisLiakos’s picture

ParisLiakos commented
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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

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

ParisLiakos’s picture

ParisLiakos commented

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

Status: Fixed » Closed (fixed)

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