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.
Problem
- All autocomplete and other responses are broken, because the kernel patch did not convert drupal_json_output().
- Regression: Some page callbacks have been converted to use Symfony's
JsonResponse
, which means they do not use the required drupal_json_encode()/drupal_json_decode() wrappers anymore.
Notes
- No idea how tests were able to pass with this — either we do not have any tests for autocomplete at all, or the kernel patch might have inappropriately changed the test expectations.
Comments
Comment #1
sunWanted to quickly whip up a patch, but now I see that batch.inc and other functions have been converted to
However: There's a very good reason for why we have the drupal_json_encode() + drupal_json_decode() wrappers. Omitting those is a serious regression. Thus, bumping to critical.
Comment #2
pounardHonest question, how bad is it broken? If Symfony doesn't have their own wrapper for this and don't have a problem, it probably means we have a problem ourselves elsewhere? I'd be curious to know in which circumstances it breaks and how it breaks.
EDIT: I think that in the end, the less we have those kind of wrappers, the happier will be developers. In the worst case scenario, the wrappers could be internal JsonResponse object logic instead, case in which we'd have just to extend the Symfony's one and loose two global scope functions, FTW.
Comment #3
sunWe wrongly assumed that PHP's native json_encode()/json_decode() would be correct and sufficient in the past already.
Just search for critical/major JSON issues: http://drupal.org/project/issues/search/drupal?text=JSON&priorities[]=1&priorities[]=4
Most notably: #479368: D7: Create RFC compliant HTML safe JSON which took an enormous amount of work to figure out, verify, and also test. (And I've still no idea why the existing tests did not bail out - looks like they're not asserting any of the actual RFC stuff that the issue fixed.)
I have zero interest in repeating those mistakes and repeating that discussion.
I'd agree that extending JsonResponse into our own probably makes sense, although we likely need to retain the procedural PHP wrapper functions, since those are general utility functions that are used in many more places other than HTTP responses.
Comment #4
pounardMakes sense, thanks for the answer.
Comment #5
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedIf we can figure out what is required to fix JsonResponse and can back that up with an RFC, it shouldn't be too hard to fix that upstream, no?
Really, the only special thing drupal_json_output() seams to do via drupal_json_encode() is:
Where JsonResponse does the same thing without all the flags.
I'd open a pull request for Symfony. Is this everything?
Comment #6
aspilicious CreditAttribution: aspilicious commentedI agree with Niklas, there is a lot of extra code in D7 but thats for php versions < 5.3 which aren't allowed in D8. :)
I agree this should be fixed upstream in Symfony, they don't mind making stuff safer if it's RFC compliant.
Comment #7
Crell CreditAttribution: Crell commentedI agree with fixing this upstream. No sense having Drupal-specific handling here.
Niklas, can you open a PR?
Comment #8
pounardI agree on Crell's suggestion, from PHP 5.3 Drupal is not using its own JSON layer anymore, it's just passing options to the json_encode() and json_decode() calls. Ideally we could actually get rid of those wrappers.
Comment #9
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOpened a PR: https://github.com/symfony/symfony/pull/4510.
Comment #10
sunThe PR landed :) (although, what worries me is that it didn't contain tests, so at this point, the code might change without notice at any point in time)
Anyway, this means we need to update JsonResponse (and thus, the entire
HttpKernelHttpFoundation?) to the latest version.Comment #11
aspilicious CreditAttribution: aspilicious commentedIs that critical (the updating part)? Symfony will soon release a 2.1 version we can update than I guess...
I suggest fixing the critical part of this bug so we can close it :)
Comment #12
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedMhh ... yeah. Once we updated we can at least downgrade this to a major task. Should we have a Drupal-land test for this, or another upstream PR?
What is the procedure for updating Symfony? Do we need a patch?
(Btw. I remember that some JSON related tests were adjusted to the new model, but not the expectations about the delivered data or the way it's escaped.)
Comment #13
aspilicious CreditAttribution: aspilicious commentedRobloach has a patch to update symfony automagicly with composer when we want. So there are issues for that.
Comment #14
Niklas Fiekas CreditAttribution: Niklas Fiekas commented#1424924: Use Composer for updating Symfony components (without removing Symfony code from repo).
Comment #15
Niklas Fiekas CreditAttribution: Niklas Fiekas commented@fabpot committed a unit test to Symfony: https://github.com/symfony/symfony/commit/b84b46ba1a4e1197d1e8874c395bcf....
Comment #16
sun1) There is no "simple fix" for this. If we don't update HttpFoundation, then we would have to introduce a custom JsonResponse class that extends Symfony's, but contains the encoding fix. Doing that would be stupid, given that Symfony has been adjusted already.
2) In turn, this means we either have to manually update the HttpFoundation to its latest code, or we need to wait until #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo) is resolved. The remaining blocker for that issue is to not replace the autoloader with Composer's. I'd personally prefer to finally resolve that issue, but that may still take some weeks.
3) While Symfony will have tests for RFC-compliant JSON responses, we still need tests for our functional autocomplete code. This entire bug should not have existed in the first place. Thus, restoring tag.
4) We need to go over the entire code base to verify that all JSON responses are using JsonResponse. At the same time, drupal_json_output() can be removed from core, because it is obsolete. Thus, adding tags.
Comment #17
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedGonna create a patch that removes drupal_json_output(), replaces the remaining occurences and maybe adds some tests.
Comment #18
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedHere's a patch that removes drupal_json_output(). That some form tests use that in submit handlers and then quickly drupal_exit() is unfortunate, because that doesn't allow for replacement with kernel responses. That should be figured out in #1623114: Remove drupal_exit(), though. Using print drupal_json_encode() for now, that is just as bad.
But really:
The only thing that makes this issue critical are point (1) and (2): Update Symfony. And please don't let there be some stupid rule that prevents us from doing that.
Then (3) is a major task to add testcoverage.
And (4), which is my patch, isn't even nescessary to fix this problem. Just nice to have. But since drupal_json_output() is not broken, using it is no regression.
Comment #19
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedComment #20
sunAwesome patch, thanks :)
I'm fairly sure that all of those form test module responses can simply be converted into JsonResponses, too. It's possible that form processing should be adjusted to properly account for a response object returned from a form submit handler (and thus skip the attempt to perform a redirect after submission), but that's for another issue.
Comment #22
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedHehe ... lol. Look sun, we do have testcoverage. drupal_json_output() is working, JsonResponse is not. The test case that ensures
responses are properly escaped, was just still using drupal_json_output().
So my current patch that replaces all instances is failing and should be passing once we update Symfony. Boom. Removing Needs tests again. But if you think that is not enough coverage, please re-add it.
Comment #23
sunWell, I do not doubt that we have some tests.
But just simply try to autocomplete a user name on the node add/edit form. What you get is a JS error/alert that shows an Ajax response.
I've bumped this to critical, because there is no way we can release with that brokenness ;) So, yes, it's not only about correcting the code base with regard to JsonResponses, but it's also about adding at least one test (e.g., to Node module) that performs more robust assertions for autocomplete expectations. If such a test would have existed previously, then we would have noticed this brokenness for the Kernel patch already.
Comment #24
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOk, but: Isn't the test that we have exactly that: Take a random auto-complete (taxonomy term, instead of node or user) and check if some crazy data is properly escaped? Unless we test those low level stuff on all auto-completes, we might always miss problems, when we convert only some to a new system. What else would you want to assert? (I am not arguing against this issue beeing critical. That's pretty obvious. Let's update Symfony as soon as is possible - or as soon as it makes sense, when we're so close to a new release.)
Comment #25
sunHm. Yeah, you're probably right. So the actual bug is that drupal_json_output() was not converted by the kernel patch.
I still have a suspicion on #22 though... since taxonomy autocomplete wasn't converted either, it returns an Ajax response -- I almost guess the assertRaw() only passes, because the JSON string is found within the Ajax response garbage.
Comment #26
Niklas Fiekas CreditAttribution: Niklas Fiekas commented(I've been doing a quick check with a debug() in there. The result is OK. But that is certainly a weak point of the test.)
Comment #27
sunPostponing on #1424924: Use Composer for updating Symfony components (without removing Symfony code from repo)
Comment #28
RobLoachMade it in! This issue is a blocker for #675446: Use jQuery UI Autocomplete.
Comment #29
aspilicious CreditAttribution: aspilicious commented#18: 1623114-json-response-conversion-18.patch queued for re-testing.
Comment #30
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedReuploading #18 that passes after the Symfony update. Before that our tests caught the problem (as discussed in #24), once we converted everything. I also (see the interdiff) made it more obvious, that the valid output is not just contained in some wierd output, but is instead the whole thing.
So ... I think we're fine now. Btw. Symfony also added tests for this.
Comment #31
sunOne of my favorite patches currently. It shows and proves a very successful collaboration between Symfony and Drupal.
(Admittedly, that upstream fix happened "just in time" and the situation might look different, now that sf 2.1 is in beta.)
Anyway. Thanks for championing this issue + upstream change, @Niklas Fiekas! Truly awesome :)
Lastly, to clarify: The interdiff in #30 is the test improvement I had in mind; i.e., verifying the total response instead of an arbitrary chunk of text within it, so we can be sure the response is 100% what it is supposed to be. While earlier comments give the impression that this won't be a real 100% protection against future regressions, I'm fine with committing this patch. This bug/regression blocks some other issues currently. I'd like to see progress on those, instead of being blocked on a regression, which might cease to exist anyway.
Comment #32
webchickAwesome to see this fixed! :D Great work, Niklas!
I'm happy to commit this now, but think I'll leave it overnight in case Crell wants to chime in.
Comment #33
Crell CreditAttribution: Crell commentedThere's a bunch of these. Why are we not converting these to JsonResponse as well? They look like they're all unit tests, but that's no excuse for leaving an exit in place. :-)
Comment #34
aspilicious CreditAttribution: aspilicious commentedComment #35
Crell CreditAttribution: Crell commentedTalked a bit in IRC. Those will get cleaned up as part of #1623114: Remove drupal_exit(). With the comments added in #34 I'm cool with this.
Thanks everyone! Back over to you, webchick.
Comment #36
webchickGreat, thanks for taking a look!
Committed and pushed to 8.x. Thanks!
Comment #37
webchickActually, looks like we need a change notice here about:
Comment #38
aspilicious CreditAttribution: aspilicious commentedhttp://drupal.org/node/1665684
Comment #39
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks aspilicious! I think the example makes it pretty clear, what we did here.
Comment #40
andypostThis commit reverts previously fixed #7881: Add support to drupal_http_request() for proxy servers (http not https)
Comment #41
BerdirI think we don't have any change record coverage for response objects yet other than this. Do we want to make this the general one and make the json related changes just one of two examples or do we want a general one that states "There are now response objects" and cross-reference them? (sorry for bringing that up again ;))
Comment #42
aspilicious CreditAttribution: aspilicious commentedIt depends on the search filter of the change notices ;). If I notice that a function is gone I would search it in the change notices. If it searches in text and title I can live with a big one. Else I would make seperate ones and link them.
Comment #43
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThere will be a more general conversion to response objects. We can write the general change notification then.
As for #40: Thanks @andypost, well spotted and RTBC seconded.
Comment #44
pwolanin CreditAttribution: pwolanin commentedI'm assuming that partial reversion was accidental?
Comment #45
webchickWhoops! Thanks. Not sure how that happened.
Committed and pushed the reversion to that accidental reversion to 8.x.
Comment #46.0
(not verified) CreditAttribution: commentedUpdated issue summary.