This was recently fixed in Panels and CTools suffers from the same basic problem: #1708230: panels/ajax does not respond with the 'application/json' Content-Type header

Files: 
CommentFileSizeAuthor
#20 views-list-page-do-not-test.patch477 byteseffulgentsia
#20 interdiff.txt452 byteseffulgentsia
#20 ctools-ajax-deliver-1710710-20.patch7.94 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]
#19 ctools-ajax-deliver-1710710-18.patch7.78 KBjaperry
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]
#17 ctools-ajax-deliver-1710710-17.patch7.78 KBjaperry
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/ctools/ctools.module.
[ View ]
#3 ctools-ajax-1710710-3.patch4.93 KBericduran
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

Comments

asb’s picture

Probably I'm in the totally wrong issue queue here, but #1708230 was the closest match for my search keywords (so my apologies upfront).

Correlating with the release of ctools 6.x-1.9 (D6 branch!), I have become unable to edit certain Panels content; attempts to edit these parts of a Panel page result in a browser dialog to download a file of the type application/json. This file is filled with something which looks like JSON:

[{"command":"settings","argument":{"basePath":"\/","fivestar":{"titleUser":"Your rating: ","titleAverage":"Average: ","feedbackSavingVote":"Saving your vote...","feedbackVoteSaved":"Your vote has been saved.","feedbackDeletingVote":"Deleting your vote...","feedbackVoteDeleted":"Your vote has been deleted."},"admin_menu":{"margin_top":1},"colorbox"…

However, my problem happens under D6, and the correlation with the ctools update might be circumstantial (apologies again). Please advise if this is a unrelated issue, or has nothing to do with ctools or panels.

Thank you!

ericduran’s picture

Ha, so I kept arguing with a co-worker to stop using ajax_render and use delivery callbacks and kept referring to ctools as a good example but Ha, he actually used ctools but it was still wrong.

Anyways I'm making patch for this now, I'll try to fix the example and show how to account for both non-ajax and ajax form.

ericduran’s picture

Status:Active» Needs review
StatusFileSize
new4.93 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

OK, Here's a first pass at this. A couple of things.

Personally I do not like that Core doesn't provide you with a default mechanism to deliver both html and ajax page at the same path. What most people do is just call ajax_deliver() manually when they're returning ajax which personally I think it's bad practice. You can find examples like this in the Examples module.

What I did here was add a ctools_page_delivery_callback_alter() to alter the callback when a request comes in from ctools-use-modal. This way anyone using the ctools-use-modal class can delivery both html and ajax by doing what they'll expect to do which is If($ajax) {return array('type' => 'ajax', '#commands' => ....); } else { return render form etc ...}.

Actually using the delivery callback allows you to even simpler just return drupal_get_form with out much work since the delivery callback will account for none command base return.

I updated the ctools_ajax_sample module and remove all the print & exit to simply

return array(
      '#type' => 'ajax',
      '#commands' => $commands
    );

This only works for now with the ctools-use-modal class. I know there's other places in ctools that do

    print ajax_render();
    exit;

I'll need to play a bit with other modules using that functionality in ctools to make sure I do it right and don't brake things.

FYI, I tested all the modal examples to be sure they still work and did a quick run through with Views (Just to be 100% sure I didn't brake something by mistake).

Also I know this isn't a very straight forward change :-/

ericduran’s picture

Also, I know it sucks to use $_SERVER & $_POST but this is an extremely rare time where this information is actually needed. There is literally no-other data available to you in that hook. I also made sure to check for everything before trying to access the array. I was super careful ;-/

Kinda sucks, but pretty much everything related to delivery_callbacks has to do a horrible hack like that. Also I do not like that we have to use $_POST but sadly we can't add a header which would be nicer because that functionality didn't get into jquery till 1.5 :-/

sambonner’s picture

Issue summary:View changes
StatusFileSize
new8.19 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

Adding an updated version of this patch that uses the new delivery callback on the access control and context modal's. I'm setting the header manually for the delete callbacks as these don't seem to play nice with the delivery callback approach, not quite sure why.

Elijah Lynn’s picture

StatusFileSize
new2.36 KB

Interdiff between #3 and #5.

Elijah Lynn’s picture

Both patches (3 and 5) against latest dev are returning HTML, not JSON in ctools_ajax_sample module when attempting to delete some of the sample content (md5 hashes). I will attempt to resolve.

Elijah Lynn’s picture

StatusFileSize
new969 bytes
new7.35 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

Okay, the delivery_callback_alter() was only hitting if it was a modal but I am not sure why this was checking for a modal.

It works if you remove the check for the modal and I tested the sample and it works fine. @ericduran can you tell me the logic for checking if it is a modal, I am sure there was a reason for it but just not sure what it is, thanks!

I attached a patch for not checking the modal. Otherwise this is fairly RTBC'd I would say but I will wait to mark it as so.

SocialNicheGuru’s picture

the patch in 8 no longer applies cleanly to the newest ctools

ericduran’s picture

--
Edit. Sorry wrong issue.

sambonner’s picture

StatusFileSize
new7.98 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

Rerolled patch from #8 to apply on top of latest ctools 1.4 release.

sambonner’s picture

Status:Needs review» Needs work

OK so I think the change to ctools_page_delivery_callback_alter() to make the check much wider is negatively impacting on autocomplete form elements returning JSON + HTML using drupal_json_output(). Entityreference for example has entityreference_autocomplete_callback_get_matches() returning a JSON object with some HTML properties using drupal_json_output() for it's autocomplete entity reference fields. Currently its breaking and returning an AJAX error because drupal_json_output() passes it's output to drupal_deliver_html_page() which is obviously being intercepted and overridden into processing using ajax_deliver().

While it may technically be more correct to return an array for use with ajax_deliver I suspect there's too many modules using drupal_json_output at the moment for this to be practical/acceptable. I suggest we tighten the check to restrict the ajax_deliver callback swap to only fire within ctools, particularly since drupal_json_output does at least set the correct json response headers at the moment.

I can try rolling a patch but I'd like some input from others who are more expert than I in this area as to whether this is the correct path to take.

dvandusen’s picture

This issue is causing some confusion to those of us with weaker minds. It pretty strongly implies that "ajax_render should not be used; ajax_deliver should be used instead."

Still, there are posts that suggest that are significantly less firm.

I must presume that ajax_render will disappear in D8 from the seeming direction shown on this issue. Is that a fair assumption?

Please give some very firm direction. Also, please be clear regarding the actual state of / problems with each. This would help nubies.

Thx

D

Dave Reid’s picture

Regardless of how it's fixed, it's causing issue when we need the proper content type header in the response.

maximpodorov’s picture

So this should be fixed soon, right?

japerry’s picture

Priority:Major» Critical
Status:Needs work» Needs review

Having a lack of content type header in the response has been causing a slew of issues from many module maintainers, and that ambiguity needs to be fixed.

Lets do some testing with entityreference_prepopulate and a few others modules to see where this breaks.

japerry’s picture

StatusFileSize
new7.78 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/ctools/ctools.module.
[ View ]

Here is a re-roll against latest HEAD.

Status:Needs review» Needs work

The last submitted patch, 17: ctools-ajax-deliver-1710710-17.patch, failed testing.

japerry’s picture

Status:Needs work» Needs review
StatusFileSize
new7.78 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]

Ahh gah, when I merged the re-roll in I lost a {

Here lets try this patch instead.

effulgentsia’s picture

StatusFileSize
new7.94 KB
PASSED: [[SimpleTest]]: [MySQL] 121 pass(es).
[ View ]
new452 bytes
new477 bytes

#19 breaks the Enable/Disable row operations on /admin/structure/views. Here's a fix for that, but it only works if the attached Views patch is also applied.

pwolanin’s picture

Why does views break? It's now getting a render array?

japerry’s picture

Status:Needs review» Reviewed & tested by the community

Unless there are some other regressions (I haven't found any others), considering this RTBC. Now views just needs to change some of their code ;)