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.
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.
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.
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.
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.
Comments
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedComment #2
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedDeprecated drupal_json_decode() by Json::decode().
Comment #3
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedComment #5
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedComment #7
damiankloip CreditAttribution: damiankloip commentedLooks like this needs a reroll, plus there are no use statements anywhere for Drupal\Component\Utility\Json.
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commentedyeah, i linked to the documentation for "use" in the parent issue's summary.
Comment #9
rbayliss CreditAttribution: rbayliss commentedRerolled 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.
Comment #10
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedIts my fault :( Thanks rbayliss. I have test the patch its working fine for me.
Comment #11
dawehnerIs there a reason why we still have drupal_json_decode() in the following places left?
Comment #12
damiankloip CreditAttribution: damiankloip commentedThat looks like a definitive list to me.
Comment #13
CyberschorschAre we going to replace json_decode() with Json::decode() as well?
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commentedMaybe. 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().
Comment #15
CyberschorschAlright. I attached the patch with the missing replacements.
Comment #17
CyberschorschComment #18
dawehnerUploading an interdiff between #9 and #17 used for the review.
I agree that replacing json_decode itself would be out of scope.
Comment #19
alexpottPatch no longer applies.
Comment #20
CyberschorschRerolled the patch.
Comment #22
Cyberschorsch#20: 2093173-remove_calls_drupal_json_decode-20.patch queued for re-testing.
Comment #23
mcrittenden CreditAttribution: mcrittenden commentedTags
Comment #24
areke CreditAttribution: areke commentedThe patch doesn't apply anymore and needs to be re-rolled again.
Comment #25
forbesgraham CreditAttribution: forbesgraham commentedComment #26
forbesgraham CreditAttribution: forbesgraham commentedRe-rolling patch from comment 20.
Comment #29
forbesgraham CreditAttribution: forbesgraham commentedMy apologies, I think some typos are behind this. I'm submitting a new one shortly..
Comment #30
forbesgraham CreditAttribution: forbesgraham commentedTrying this again.
Comment #32
forbesgraham CreditAttribution: forbesgraham commentedGrr..sorry I'll take another stab at this tonight or tomorrow :(
Comment #33
forbesgraham CreditAttribution: forbesgraham commentedThe re-roll failed when I tried to apply it and it's getting late. I'll get back to this soon.
Comment #34
damiankloip CreditAttribution: damiankloip commentedSorry, @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.
Comment #35
sunComment #36
dawehnerJust a tiny bit.
This base class exists in the same namespace, so we don't have to add it.
Comment #37
dawehner34: 2093173-34.patch queued for re-testing.
Comment #39
InternetDevels CreditAttribution: InternetDevels commentedDo not created interdiff, because can`t apply old patch.
Comment #40
jsobiecki CreditAttribution: jsobiecki commentedComment #41
jsobiecki CreditAttribution: jsobiecki commentedI rerolled patch. Now it should apply cleanly.
Comment #42
jsobiecki CreditAttribution: jsobiecki commentedPatch 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.
Comment #43
jsobiecki CreditAttribution: jsobiecki commentedComment #46
jlbellidoI'll work on this.
Comment #47
jlbellidoTests pass locally, run test again.
Comment #48
jlbellido42: replace-drupal-json-decode-2093173-41.patch queued for re-testing.
Comment #49
YesCT CreditAttribution: YesCT commentedfixing tag. sorry for noise.
Comment #50
ParisLiakos CreditAttribution: ParisLiakos commentedComment #51
longwaveRerolled, removed a new reference in EditorSecurityTest, and a documentation reference in the JSON utility class itself.
Comment #53
longwaveComment #54
ParisLiakos CreditAttribution: ParisLiakos commentedThanks!
Comment #55
longwavePrevious patch only applied with fuzz, rerolled, still should be RTBC.
Comment #56
catch55: 2093173-remove-drupal_json_decode-55.patch queued for re-testing.
Comment #58
longwaveRerolled, again
Comment #59
ParisLiakos CreditAttribution: ParisLiakos commentedComment #60
webchickLooks like we're missing a change notice about this: https://drupal.org/list-changes/published?keywords_description=drupal_js...
Comment #61
ianthomas_ukThis 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.
Comment #62
grom358 CreditAttribution: grom358 commentedUsing Function Replacer Part of Write script to automate replacement of deprecated functions where possible.
Comment #63
ianthomas_ukPatch 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.
Comment #64
damiankloip CreditAttribution: damiankloip commentedYeah, prob doesn't need a CR? As we are just converting the core usages of the function?
Comment #65
alexpottCommitted 880f697 and pushed to 8.x. Thanks!
Comment #66
jessebeach CreditAttribution: jessebeach commentedThe change record just needed to be published.
Removed 'Needs change record' tag.