Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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, 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.
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.
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
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
Comments
Comment #1
xeniak CreditAttribution: xeniak commentedOK, 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().
Comment #3
thedavidmeister CreditAttribution: thedavidmeister commentedHey, 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.
Comment #4
sidharthapThe 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
Comment #5
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedComment #6
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedComment #7
xeniak CreditAttribution: xeniak commentedYeah, 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.
Comment #8
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedThanks @xeniak for comment. Not sure but, I think it is removing all deprecate all calls drupal_json_encode() to json_encode.
Comment #10
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedComment #12
xeniak CreditAttribution: xeniak commentedComment #13
thedavidmeister CreditAttribution: thedavidmeister commenteddrupal_json_encode() is *already deprecated*.
https://api.drupal.org/api/drupal/core%21includes%21common.inc/function/...
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commented@sandipmkhairnar, if you don't put
use \Drupal\Component\Utility\Json
at the top of the files where you want to callJson::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.
Comment #15
rbayliss CreditAttribution: rbayliss commentedAdded those use statements and corrected the capitalization of the class name.
Comment #17
rbayliss CreditAttribution: rbayliss commentedLet's try this again... Same patch, hopefully better formatting.
Comment #18
thedavidmeister CreditAttribution: thedavidmeister commentedI 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.
Comment #19
alexpottPatch no longer applies.
Comment #20
sidharthapThere are some occurrence of json_encode() in vendor/* directory. Will it required an attention too with this patch.
Comment #21
mcrittenden CreditAttribution: mcrittenden commentedTags
Comment #22
areke CreditAttribution: areke commentedThe patch no longer applies; it needs to be re-rolled again.
Comment #23
Sumeet.Pareek CreditAttribution: Sumeet.Pareek commentedHere is a re-rolled version of the patch.
Comment #25
haithem_pro CreditAttribution: haithem_pro commentedstart working on issue
Comment #26
haithem_pro CreditAttribution: haithem_pro commentedComment #27
haithem_pro CreditAttribution: haithem_pro commentedComment #29
damiankloip CreditAttribution: damiankloip commented27: remove_calls_drupal_json_encode-2093161-26.patch queued for re-testing.
Comment #34
InternetDevels CreditAttribution: InternetDevels commentedComment #35
sunComment #36
idflood CreditAttribution: idflood commentedPatch in #34 wasn't applying anymore so here is a reroll.
Comment #37
ianthomas_ukReroll, 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.
Comment #38
thedavidmeister CreditAttribution: thedavidmeister commented#2094585: [policy, no patch] Core review bonus from https://drupal.org/comment/8464143#comment-8464143
Comment #39
ParisLiakos CreditAttribution: ParisLiakos commentedShouldn't we also remove the function here?
Comment #40
longwaveRerolled.
Not sure we should remove drupal_json_encode() while its companion function drupal_json_decode() still exists?
Comment #41
ParisLiakos CreditAttribution: ParisLiakos commentedThey 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
Comment #42
alexpottCommitted 6383d18 and pushed to 8.x. Thanks!
Comment #43
alexpottYep all usages should be removed before the function. Less to revert if removing the function falls or breaks something unexpectedly.
Comment #44
ParisLiakos CreditAttribution: ParisLiakos commentedah, i see, that makes sense:)
Thanks!