Support from Acquia helps fund testing for Drupal Acquia logo

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
FileSize
4.93 KB

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
FileSize
8.19 KB

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

FileSize
2.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

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

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

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
FileSize
7.78 KB

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

Here lets try this patch instead.

effulgentsia’s picture

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

joelpittet’s picture

@japerry is there an issue for the code needing fixes in views?

effulgentsia’s picture

I don't know if there's an issue, but #20 has a patch.

joelpittet’s picture

joelpittet’s picture

This seems to break ckeditor, it may need a similar fix.

sam-elayyoub’s picture

The main issue is with Ckeditor, I did some tests and i found out after debugging

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /system/ajax
StatusText: OK
ResponseText:
Warning: Unknown: Input variables exceeded 3000. To increase the limit change max_input_vars in php.ini. in Unknown on line 0

// the reason for this issue is overload of ajax within the same page, so after testing all the ways to solve this issue, i found that ckeditor by Wysiwyg Settings is the only module that loading over 30 seconds, so if you turn ckeditor or any text editor in the page off, the page works fine.

sam-elayyoub’s picture

Another solution to turn off "Disable" the ckeditor auto enable from the ckeditor basic settings in content authoring configuration.

sam-elayyoub’s picture

japerry’s picture

Status: Reviewed & tested by the community » Needs review

So it appears that ckeditor compatibility is an issue then, coming from comment #27? Changing to needs review until someone can chime in confirming whether or not this specific patch causes regressions in ckeditor.

SocialNicheGuru’s picture

this patch caused my site to not be able to rebuild permissions.
autocomplete also did not work :(

rivimey’s picture

This issue seems to have got stuck in a mire.

I wonder, would it be useful to split it into two parts: one side being the changing of ajax_render calls into returning a render array, and the other part being the intercept of deliver page? I would think the ajax_render change is a lot less risky than the deliver page change, so perhaps we can make some progress there at least.

@sam-elayyoub regarding the ckeditor tests in #27 does 'overload of ajax' refer to more than one ckeditor widget, or was only one instance of ckeditor on the page needed to cause problems?

sam-elayyoub’s picture

@rivimey I'm sorry it took's me so long to reply, however it was for all widgets not just one, the meaning it was a general overload of ajax from the page in general not just a field, so to be more clear, the fields which using ckeditor are loading the max of what ajax can be afford for that page, so if you sometime disable one ckeditor on that page you can add another but without disabling one always it shows the ajax overloaded issue.

the simplest way to avoid this issue is to increase the ajax timeout or disable the ckeditor and make it enable by choice.

setTimeout(function() { /* Your Code */ }, 2000);

SocialNicheGuru’s picture

Status: Needs review » Needs work
FileSize
236.62 KB

Per #31 I am setting to needs work. When I uninstalled the patch, I was able to build permissions and autocomplete worked again.

sam-elayyoub’s picture

Social.. please try to do the permissions rebuild again using a different theme. One of the issues Ajax page and if you would like try first to turn off the overlay module and uninstall it then do the same process