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

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.

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 GitHub#1960 (comment)
.

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.

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.

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.

@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();
}

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

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

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?

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

@betweenbrain PR for the DocumentType issue: #488

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;
}

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?

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

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!

The switch part became quite ugly now.

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.

@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

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.

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.

@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';
}

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

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

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.

about default format raw for global JDocument is good idea from the current point of view, but I not sure about other points of view ;)

I just remembered this http://docs.joomla.org/Xml-rpc_changes_in_Joomla!_1.6 :)
what about we will have real controllers for each type: eg: module, plugin ... means we make it in joomla proper way
and for request do:
index.php?option=com_ajax&task=plugin.call&format=json
index.php?option=com_ajax&task=module.call&format=json

and if format exist we use com_ajax/controller/module.json.php
if no format then com_ajax/controller/module.php

and by adding new controllers we can add new tricks in the future....

just thought....

@Fedik I'm against having controllers for modules and plugins, this should be reserved for Components.
In my perception modules are 'simple' widgets, plugins are 'modificators', but it's just my opinion.

At this point it's possible to execute custom module methods with com_ajax using method parameter.

@all
Le'ts leave the JFactory::createDocument how it is, I'm not entirely sure about the backward compatibility after the change. We can bring it up for J4.0, similar behavior is present in other frameworks (at least in Nette).

I think Fedik referred to the controller of com_ajax, not for the modules or plugins.

yes, I mean the controller of com_ajax

com_ajax/controller/module.php
com_ajax/controller/module.json.php
com_ajax/controller/plugin.php
com_ajax/controller/plugin.json.php
...
and so on...

and later can add other if need ...

If we are to close the application, we should send the content-type: text/html header and the charset.

How to get a charset? From the JDocument.

JResponse::setHeader('content-type', JFactory::getDocument()->getMetaData('content-type'));
JResponse::sendHeaders();

It gets complicated and I'm more rather favor of not closing.

yes, you are right, now I also think that closing not need ....
before that I had doubt in this :)

@Fedik, @Bakual sorry, I misunderstood you. I think this is great idea! It could solve our issue with formats.

something like that https://github.com/Fedik/joomla-cms/compare/com_ajax-controllers
a bit code duplication, but part of it can be moved to the helper file or make "main" controller for avoid duplication

Is the motivation for introducing controllers to simplify the code in com_ajax/ajax.php? I'm not necessarily opposed to the idea, but am not sure if I understand the benefit as opposed to complicating the code.

not really ;)
there couple things:

• no need to worry about the format, Joomla! will call the right controller automatically, depend from given format
• possibility to extend it in the future by adding the new controller, for support new "components" , if it will be need ... eg field

but, I think now we cannot avoid problem with default HTML format (Joomla! continue with template render)...
so com_ajax need always call with defined format RAW or JSON...

ah, in theory the current Document type can be changed force:
JFactory::document = JDocument::getInstance($format, $attributes);
but it tooo tricky no? :)

About closing the application and default format:
my new idea is to show an error, when format is not specified, or it's html:
Status code 404, content body: InvalidArgumentException: You have to specify output format (json, raw, etc).

The reasons are:

• closing application in component is bad (I think we all agree on this)
• not specifying a format results in fallback to html - which is useless and misleading for ajax component. If somebody really needs to retrieve whole page as a ajax response, it's better to call required page with ajax
• this way developers sees a meaningful hint, how to use the component

@betweenbrain
I've changed a little bit the way how we determine whether to execute module and/ or plugin:
before it was possible to execute both (&module=hello&plugin=world), now just one or another. Executing both could result in overwriting the $results variable and in my personal view it's a bad design, but if you did that in purpose, please comment.

@Fedik
I think your idea is good in theory, but I'm nor sure if it won't be complicated it in practice. We should provide subcontrollers for all formats (feed, image, json, opensearch, raw and xml) for both module and plugin.

A little trick of mine is to create subcontroller files for all formats, but in reality use just one (like helper you mentioned).
ie in module.json.php:

JLoader::register('AjaxControllerModule', __DIR__ . '/module.php');

Anyway, I think you should create new Pull Request so we can discuss relevant code there.
This PR is about response and I have the feeling that we cleared up most things and this part may be merged into core.

@all I do want to say that I'm very excited and encouraged to see so much interest in com_ajax. When I first created it, my goal was to simply stimulate conversation about how to best accomplish Ajax support in Joomla. I never expected it to become part of core, nor so much interest in it.

I also hope that none of my comments come across as defensive. Some of this is new to me and turning into a learning experience. I am certain that while the code I wrote gets the job done, it can be greatly improved. I am very interested in seeing how it will evolve and learn from all of you in the process.

In any case, I thank you!

@piotr-cz You're right about executing both plugins and modules. One thought that I've had regarding that would be to make $results an array or object so that we can return results from both a module and plugin group. I'm certain that would be more of an edge case, but could be quite interesting.

As an aside, here is a bit of interesting reading regarding closing the application - https://groups.google.com/forum/#!msg/joomla-dev-cms/WsC0nA9Fixo/Ur-gPqpqh-EJ. I have no objection to not closing it, but thought that was an interesting approach.

Keep up the great work everyone!

@piotr-cz I think outputting
InvalidArgumentException: You have to specify output format (json, raw, etc)
is a very good idea. This way we never have to close the application and the developer immediately knows what to do.

Small note because I'm not sure whether you know: There is still a 'close' for 'debug' in the pull request.

@betweenbrain There is no special note about the 'close' in that thread (if I haven't overlooked anything). So, I think it can also be omitted in the code of Louis (if the page was requested with 'format=json'). Most probably they just not thought about adding 'format=json' to the URL.

@betweenbrain what is the purpose of debug format?

As for the topic, I think this exact code is more to show the concept in custom component.
We need to think about whole system. For example when application is closed instantly, the module/ plugin cannot use JResponse::setHeader and cannot use events that are being fired after dispatch.

Besides, JDocumentJSON has been added to Joomla after mentioned discussion

@Piotr The debug format provides a human-readable output. I've found it
helpful when working with large or complex results, especially in JSON
format.

Best,

Matt Thomas
Founder betweenbrain™
Lead Developer Construct Template Development Framework
Phone: 203.632.9322
Twitter: @betweenbrain
Github: https://github.com/betweenbrain
On Sep 16, 2013 3:07 PM, "Piotr" notifications@github.com wrote:

@betweenbrain https://github.com/betweenbrain what is the purpose of
debug format?

As for the topic, I think this exact code is more to show the concept in
custom component.
We need to think about whole system. For example when application is
closed instantly, the module/ plugin cannot use JResponse::setHeader and
cannot use events that are being fired after dispatch.

Besides, JDocumentJSON has been added to Joomla after08d39b6mentioned discussion

—
Reply to this email directly or view it on GitHub#1960 (comment)
.

I'm not entirely convinced about the debug format and about imploding the raw results if these are an array/ object but indeed this might be helpful for debugging.

I think the Pull request in this state is ready for merge, what do you think? We will probably need some test results published on the tracker. When it's merged, we may discuss if/ how to transform component codebase to MVC pattern like @Fedik suggested.

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.

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

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.

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

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

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.

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

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.

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.

@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?

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

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 GitHub#1960 (comment)
.

@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

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

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.

@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

Please resync this PR to master.

Resynced to master