com_ajax: Changes response format

[piotr-cz]

Original PR: #1778
Tracker: #31980

As per discussion: JGEN: Joomla Ajax Interface, now new class JResponseJson is used to build a structured response.

Response

{
    "success": true,
    "data": "any module/ plugin data that fits JSON format",
}

Exception in com_ajax or thrown/ returned by Module/ Plugin/ Application

{
    "success": false,
    "message": "Method get does not exist"
}

Additionally I've added options:

• ignoreMessages (boolean, defaults to true): Set the JResponseJson to ignore dumping system messages into response->messages
• close (boolean, defaults to false): Close the application

In this patch Ajax Plugins are not expected to return anything - in the previous version this evaluated to an error 'plugin returned no response'.

[Chraneco]

Yes, this change should definitely by merged into master!
Is there already a tracker item, so we can post testing results?

---

A bit offtopic: About the 'close' thing.
If you use format=json correctly it shouldn't be necessary to close the application because Joomla! automatically doesn't output anything else.
Additionally, it won't be necessary to execute

header('Content-Type: application/json');

because Joomla! does that for you (if you don't close the application).

[piotr-cz]

Couple of things I'd like to change:

• reflect @Chraneco comments: remove the close option and header mime. The response will flow down the pipe to JDocumentJSON which does that.
• set the default option of ignoreMessages to true.
  The reason is that if the developer doesn't include functionality of rendering the messages, messages will be lost (session messages are removed after being read).
• Change component Exception codes from 500 to 404, just like @b2z suggested in previous PR. This makes response more explicit if we decide to send HTTP response codes. Plugin/ Module remain free to create own Exceptions.

[betweenbrain]

These changes make sense to me. I was also thinking that a 404 makes more
sense, but that is somewhat open to interpretation.

Best,

Matt Thomas
Founder betweenbrain™
Lead Developer Construct Template Development Framework
Phone: 203.632.9322
Twitter: @betweenbrain
Github: https://github.com/betweenbrain

Sent from mobile. Please excuse any typos and brevity.
On Sep 11, 2013 4:29 PM, "Piotr" notifications@github.com wrote:

Couple of things I'd like to change:

reflect @Chraneco https://github.com/Chraneco comments: remove the
close option and header mime. The response will flow down the pipe to
JDocumentJSONhttps://github.com/joomla/joomla-cms/blob/master/libraries/joomla/document/json/json.php#L42which does that.

set the default option of ignoreMessages to true.
The reason is that if the developer doesn't include functionality of
rendering the messages, messages will be lost (session messages are removed
after being read).

Change component Exception codes from 500 to 404, just like @b2zhttps://github.com/b2zsuggested in previous PR. This makes response more explicit if we decide to
send HTTP response codes. Plugin/ Module remain free to create own
Exceptions.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1960#issuecomment-24273603
.

[piotr-cz]

I've decided to include HTTP response header in case Exception happened and format is other than JSON or debug.

When client would request other available formats (Feed, Image, XML or other), response in Raw could break the client-side parser and we don't know how to construct proper one.

[Chraneco]

Thanks for taking that with the app closing into account.

Maybe a bit more offtopic, but since Ajax requests with format=json will will most probably be used much more in future because of that I want to point to that discussion:
https://groups.google.com/forum/#!topic/joomlabugsquad/YhxiAL5iaf8
Unfortunately, no decision has been made there yet.
Solving the problem mentioned there is very important in my opinion.

[betweenbrain]

If app->close is to be removed due to JDocumentJson, I'd move it back into the other format types of the watch statement. We still need that for debug and raw.

[piotr-cz]

@betweenbrain Agree, debug is a non-standard format and without closing the application system would fall back to JDocumentHtml.

What if we would use it along with 'default' in the switch, so there would be:

• json
• raw + html (new idea, same output)
• debug + and default fallback (with $app->close)

like that:

switch ($format)
{
    // Echo JSON format and break
    case 'json':
        echo new JResponseJson($results, null, false, $input->get('ignoreMessages', true, 'bool')); 
        break;

    // Anything but JSON is just echoed
    default:
        // Format to string and add header if it's an exception
        echo $results;

    // Break for supported documents, these will be handled by JDocumentXxx
    case 'raw':
    case 'html':
       break;

    // Close app for non-supported formats
    case 'debug':
    default:
        $app->close();
}

[piotr-cz]

@Chraneco maybe @nikosdion may have idea about SEF and non-Html formats (ARS + XMLs)

[Bakual]

@Chraneco For json, this isn't such a big deal since those URLs usually aren't created as SEF URLs. They are hidden from the user in an AJAX call somewhere and don't need to be SEF friendly.
However I agree that is an issue and needs to be solved as it also prevents extensions from using custom document types (like using an xls document type for creating an Excel file).

[betweenbrain]

I do agree with @Chraneco that it needs to be resolved, but I don't think this is the place to discuss it. Is there a PR where we can discuss it directly?

[betweenbrain]

@piotr-cz for the debug format, I'd suggest:

case 'debug':
echo '<pre>' . print_r($results, TRUE) . '</pre>';
$app->close();
break;

As that provides a human-readable format. I've found that to be very useful during development.

[Bakual]

@betweenbrain PR for the DocumentType issue: #488

[piotr-cz]

So this is an updated version.

One thing to remember is that if you'll requesting an HTML format or any that doesn't have own JDocument renderer,
add &tmpl=component to the request, or you'll receive whole HTML page.

switch ($format)
{
    // Echo JSON format and break
    case 'json':
        echo new JResponseJson($results, null, false, $input->get('ignoreMessages', true, 'bool')); 
        break;

    // Pretty print 
    case 'debug':
        print_r($results, true);
        $app->close();
        break;

    // Anything else is just echoed
    default:
        // Format to string and add header if it's an exception
        echo $results;
}

[betweenbrain]

I'd need to test this further, but I think what you have at https://github.com/piotr-cz/joomla-cms/blob/0326129aef79e3c9c08d1388b58c23241c48eea0/components/com_ajax/ajax.php#L106 is great, except for needing $app->close(); after echo '<pre>' . print_r($results, true) . '</pre>'; and after

else
    {
    echo implode((array) $results);
}

Does that make sense?

[piotr-cz]

@betweenbrain yeah, I realized I'm back there.

Agree about closing application for 'debug' format, but not by default.
The point is to handle response over to JDocumentXxx render.

Let's say we'd like to receive an image with applied filters:

1. We send a request to com_ajax?plugin=imagefilter&format=image&imagepath=foo/bar.png&type=jpeg
2. Plugin is executed and returns a blob of manipulated image data.
3. com_ajax echoes the response
4. JDocumentImage takes over and sets proper mime type.

Now, maybe we have to close the app for HTML and non-supported formats or system will spit whole page with a template. I'm not sure we can rely on '&tmpl=component' because it's in control of template.

[betweenbrain]

Ah, okay. Yes, that makes sense and sounds good. I agree that we probably do need to still close the app for HTML and non-supported formats, as raw, but not by default.

Thanks!

[piotr-cz]

The switch part became quite ugly now.

[Chraneco]

I don't think that it is necessary to close the application for 'debug' and 'raw', in fact it is never necessary to close the application. Format 'raw' does not add any output to the one of the component and for generic formats like 'debug' (for which no document type is given in 'libraries/joomla/document/') there is a fallback to 'raw'.

The only case where we can think about adding the closing of the application is when the developer has forgotten to add 'format=anything' to the request URL (thus $format is empty) because then Joomla! assumes format=html.

If you ask me I would never add closing the application because forgetting format=anything is the developer's fault.

[piotr-cz]

@Chraneco That makes sense to me.
What's your opinion about presenting Exceptions/ error messages in non-JSON responses?

I think these should be shown only to extension developers, so I'd:

• send HTTP status code of Exception (usually 404 or the one that module/ plugin returns) so developer can find out there's something wrong right from the ajax handler (onError like events)
• in reponse body I'd use Exception type: Exception message in plain english, like:
  LogicException: The helper file does not exist

[betweenbrain]

It makes sense to me too, but at the same time want to provide raw as the default if the format variable is omitted. I do understand the argument for not closing the application. One way to put it is that my intention was to assume raw and provide it as a default, Ajax friendly format.

[betweenbrain]

In other words, it makes sense for Joomla to generally assume format=html, but in this case format=raw might be the best fit. Does that seem like a reasonable default?

@piotr-cz If I haven't expressed it clearly already, I like your new approach for the error/exception handling.

[piotr-cz]

@betweenbrain Yes, default to format=raw is best option, I think that's what it's made for.
The problem is that Joomla's default format is html and I'm not sure if you can change it.

The process is:

1. Application dispatch
2. gets the document JFactory::getDocument()
3. which defaults to html

The Factory::$document object is public so theoretically so we could swap it, but it's a hack.
Besides, plugins that are executed before dispatch will assume that the format is 'html' (even if application is closed instantly).

We could change the system default format to raw for ajax requests, and then default to raw: check for the X-Requested-With': 'XMLHttpRequest header, which is a de-facto standard for Ajax requests (at least for jQuery, Mootools, Dojo and Prototype). More info: David Walsh: Detect an AJAX Request in PHP.

So change the JFactory::createDocument:

// Check for explicit request format
$type = $input->get('format', null, 'word');

// When no format provided
if (!$type)
{
    // Use Html or Raw for ajax requests.
    $type = (strtolower($input->server->get('HTTP_X_REQUESTED_WITH', null, 'string')) != 'xmlhttprequest')
        ? 'html' 
        : 'raw';
}

[betweenbrain]

@piotr-cz Changing JFactory::createDocument to have a different default for Ajax requests is a very intriguing idea. It makes sense to me to have different defaults for different request types.

[Bakual]

I tried to change the document type once in my component and came to the conclusion that it's impossible to do. It was possible in J1.5, but isn't possible since 1.6 (I think). The only way to change it is using the format parameter in the request.

Imho, we should rely on the developer using com_ajax to use the format parameter properly. Closing the application early is basically a hack in itself. We should not try to fix errors from lazy developers :-)

[Chraneco]

The new approach for error/exception handling seems to be fine for me, too.

I also think that we shouldn't change JDocument or JFactory just for com_ajax. I still think that closing the app is not necessary (@Bakual: I fully agree here), but if the majority is for having format=raw the default, it will be the best to just add

if(!$format)
{
  $app->close();
}

below everything.

This way you can delete this part

// Check for compatible JDocument formats
if (!in_array($format, array('feed', 'image', 'json', 'opensearch', 'raw')))
{
  JResponse::sendHeaders();

  echo $out;

  $app->close();
}

and simplify the switch part again.

[betweenbrain]

Indeed, we need solid test results that demonstrate the benefit of these changes before they are committed.

Keep in mind that the code originally contributed, including the debug format and closing the app, is battle hardened code used in a real-world application. I do recognize that it doesn't necessarily adhere to Joomla standard conventions, but you do have to admit that it is simple and gets the job done 😉

I'm all for improving it and will try to do some tests today. I'd also suggest pinging the original tracker item for testers as I think a couple of people on that item have created extensions to use com_ajax.

[Bakual]

I would be in favor of doing it the Joomla way. Not because it is broken now, but because it is a core component now. And those component are also used as examples for developers how to write extensions. Having a simple component supporting multiple formats would be a welcome example addition to the core imho :-)

I don't think the debug format is really needed, but we sure can keep it.

I'd also suggest pinging the original tracker item for testers as I think a couple of people on that item have created extensions to use com_ajax.

An important point to consider: If people are already using com_ajax, they do it at their own risk. It's in an alpha state at the moment and if there is one time in the Joomla lifecycle where you can easily change the API and do backward compatibility breaks, it is now. If you want to do it the proper way, do it now. After release you will be stuck to it and can't do such changes anymore. I would even suggest to change it before beta or during early beta, as developers like me don't like changes in APIs shortly before release (they tend to get in without proper tests) :-)

[piotr-cz]

I've updated the tracker.
By the way, how do you feel about leaving the exception messages in English (non-translatable) ?
I've mentioned in before here, but nobody gave any reply.

[betweenbrain]

@Bakual Yes, my reason for referencing the other tracker was not about BC changes, but to get more testers and feedback. Now is definitely the time to make changes, and I'm happy to see it happening 👍

Don't gt me wrong, I firmly believe that core components need to demonstrate best practices. I don't think everything needs to be MVC, they can be MC, but that is a topic for another thread. I do agree that having a component in core that supports multiple formats would be a nice.

@piotr-cz Sorry, I must have missed that about the messages. I think they need to be translatable.

[piotr-cz]

@betweenbrain Why do you think so?
Joomla Exceptions, PHP Exceptions and PHP Errors are not being translated.

There is a difference to Joomla messages (like form errors) in that these won't be shown to a end user (unless (s)he decides to mess with the system).

[mbabker]

On the topic of the translated Exceptions, generally in the libraries, we've moved to static English for all Exceptions and log entries, with some exceptions. The primary reason is that these sources are geared more towards developers and troubleshooting and less towards front end users and seeing errors. Developers should be catching Exceptions and deciding what to do (is the Exception fatal to processing or can you go on with only a note about the failed condition?). Rarely should you be catching the Exception and directly throwing its message right back out to a user, but if you have a valid case where you're doing that, then make it translatable.

[betweenbrain]

@piotr-cz @mbabker Consider me convinced, I didn't realize those points. I retract my earlier statement about them needing to be translated. I had just assumed that was the de facto practice.

[mbabker]

It was for the longest time. Then the Platform came along and started decoupling dependencies between packages, with the big one being why was EVERY package dependent on JText. Personally, I think that (and moving away from JError in general) is the right move; it requires you to think about everything more in your code, including error handling.

[betweenbrain]

Thanks for the explanation, that helps allot. Frankly, I'm behind on most of this as nearly all of my work for the last year has been focused on 1.5, and will be for some time still.

[Chraneco]

@piotr-cz What do you think about replacing
getWord('format', 'html')
with
getWord('format')
and
if($format == 'html')
with
if(!$format)
?
This way the case format= (an empty format string) is also covered and the developer is able to explicitly add format=html if he wants that (I cannot think about any reason for this, but it is more flexibel).

Shouldn't the closing of the app for format=debugbe removed also?

[piotr-cz]

@Chraneco The code would definitely be more readable and as you say flexible.

[elinw]

That's also pretty similar to what i have been doing elsewhere. format=
is really a good way to send and i think in general we should stop always
assuming html is the format by default.

I don't know if you saw but I proposed a general controller helper which i
really am just using for components right now but it would be a good pace
to put other controller related methods if you wanted.

Elin

On Thu, Sep 19, 2013 at 6:52 AM, Piotr notifications@github.com wrote:

@Chraneco https://github.com/Chraneco The code would definitely be more
readable and as you say flexible.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1960#issuecomment-24730707
.

[piotr-cz]

@elinw Fedik suggested to rewrite the code to MVC and I'm personally very in favor of that (see his branch).

I'd like that to be separate PR after this one is merged, because it took some discussion to get into current state and we could discuss that thing separately.

PS send me some info about component helper you mentioned

[wilsonge]

Now @mbabker 's JApplication changes have gone through we shouldn't be using JResponse anymore but the JApplication alternative

[Fedik]

I think that the default format html is not so bad .... just problem that we cannot tell Joomla! to stop render the template, maybe something like JDocumentHtml::enable(disable)Template(); can be as solution?
just question :)

[Bakual]

Don't make it to complicate. I you need a html response and the tmpl=component parameter doesn't work you can always use format=raw and output whatever you need with the appropriate headers. If you would not render the template you will not get a valid HTML document anyway.

[piotr-cz]

@Fedik My explanation of raw format is that it's most of times same as html, just without template.

Using tmpl=component is for ajax requests bad idea, because it depends on the template provider - extension developer can't know what will get. It was designed for pop-up windows so it still contains whole and styling. Doesn't look like ajax response to me at all.

[mbabker]

Please resync this PR to master.

[piotr-cz]

Resynced to master