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.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Priority: Major » Critical
Issue tags: +Regression

Wanted to quickly whip up a patch, but now I see that batch.inc and other functions have been converted to

use Symfony\Component\HttpFoundation\JsonResponse;

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.

pounard’s picture

Honest 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.

sun’s picture

We 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.

pounard’s picture

Makes sense, thanks for the answer.

Niklas Fiekas’s picture

If 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:

function drupal_json_encode($var) {
  // Encode <, >, ', &, and " using the json_encode() options parameter.
  return json_encode($var, JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT);
}

Where JsonResponse does the same thing without all the flags.

I'd open a pull request for Symfony. Is this everything?

aspilicious’s picture

I 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.

Crell’s picture

I agree with fixing this upstream. No sense having Drupal-specific handling here.

Niklas, can you open a PR?

pounard’s picture

I 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.

Niklas Fiekas’s picture

sun’s picture

The 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 HttpKernel HttpFoundation?) to the latest version.

aspilicious’s picture

Is 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 :)

Niklas Fiekas’s picture

Mhh ... 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.)

aspilicious’s picture

Robloach has a patch to update symfony automagicly with composer when we want. So there are issues for that.

Niklas Fiekas’s picture

Niklas Fiekas’s picture

Issue tags: -Needs tests
sun’s picture

1) 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.

Niklas Fiekas’s picture

Assigned: Unassigned » Niklas Fiekas

Gonna create a patch that removes drupal_json_output(), replaces the remaining occurences and maybe adds some tests.

Niklas Fiekas’s picture

Status: Active » Needs review
FileSize
9.32 KB

Here'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.

Niklas Fiekas’s picture

Assigned: Niklas Fiekas » Unassigned
sun’s picture

Awesome 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.

Status: Needs review » Needs work

The last submitted patch, 1623114-json-response-conversion-18.patch, failed testing.

Niklas Fiekas’s picture

Issue tags: -Needs tests

Hehe ... lol. Look sun, we do have testcoverage. drupal_json_output() is working, JsonResponse is not. The test case that ensures

 $this->assertRaw('{"\u0022' . $term_objects['term3']->name . '\u0022":"' . $term_objects['term3']->name . '"}', format_string('Autocomplete returns term %term_name after typing the first 3 letters.', array('%term_name' => $term_objects['term3']->name)));

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.

sun’s picture

Issue tags: +Needs tests

Well, 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.

Niklas Fiekas’s picture

Ok, 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.)

sun’s picture

Hm. 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.

Niklas Fiekas’s picture

(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.)

sun’s picture

RobLoach’s picture

Status: Postponed » Needs work

Made it in! This issue is a blocker for #675446: Use jQuery UI Autocomplete.

aspilicious’s picture

Status: Needs work » Needs review
Niklas Fiekas’s picture

Reuploading #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.

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

One 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.

webchick’s picture

Awesome 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.

Crell’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/system/tests/modules/form_test/form_test.module
@@ -280,7 +280,7 @@ function form_test_menu() {
 function _form_test_submit_values_json($form, &$form_state) {
-  drupal_json_output($form_state['values']);
+  print drupal_json_encode($form_state['values']);
   drupal_exit();
 }

There'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. :-)

aspilicious’s picture

Status: Needs work » Needs review
FileSize
12.9 KB
Crell’s picture

Status: Needs review » Reviewed & tested by the community

Talked 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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Great, thanks for taking a look!

Committed and pushed to 8.x. Thanks!

webchick’s picture

Title: All autocomplete and drupal_json_output() responses are broken (returning Ajax instead of JSON) » Change notice: All autocomplete and drupal_json_output() responses are broken (returning Ajax instead of JSON)
Category: bug » task
Status: Fixed » Active
Issue tags: +Needs change record

Actually, looks like we need a change notice here about:

+use Symfony\Component\HttpFoundation\JsonResponse;
...
-  drupal_json_output($options);
+  return new JsonResponse($options);
aspilicious’s picture

Status: Active » Needs review
Niklas Fiekas’s picture

Title: Change notice: All autocomplete and drupal_json_output() responses are broken (returning Ajax instead of JSON) » All autocomplete and drupal_json_output() responses are broken (returning Ajax instead of JSON)
Category: task » bug
Status: Needs review » Fixed
Issue tags: -Needs change record

Thanks aspilicious! I think the example makes it pretty clear, what we did here.

andypost’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
1.19 KB
Berdir’s picture

I 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 ;))

aspilicious’s picture

It 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.

Niklas Fiekas’s picture

There 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.

pwolanin’s picture

I'm assuming that partial reversion was accidental?

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Whoops! Thanks. Not sure how that happened.

Committed and pushed the reversion to that accidental reversion to 8.x.

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.