Moving JResponse to its own package and adding JResponseJson #1596

Closed
wants to merge 1 commit into
from

Projects

None yet

7 participants

@Chraneco
Contributor

Hi,

this pull request is a replacement for #1470.

It introduces a new class JResponseJson which is able to simplify things with Ajax requests.

  • Using the flag success the JavaScript code (whether Mootools JRequest.JSON or jQuery.getJSON) can check whether the task was successful or not and react accordingly (If the request itself failes the error event of the JavaScript API can be used).
  • Then, the actual response data (if any) can be retrieved from data in the JSON object and
  • optionally a main response message from message.
  • Additionally all gathered messages in the JApplication message queue are also automatically sent back in messages. I'm not sure what will happen with that part because the message queue is only in the legacy application class at the moment without any replacement, but it's very useful at this point. The dependencies are checked afore.

Another advantage is that no $app->close(); is necessary anymore if the Ajax request is done with 'format=json' because the already existing API handles the rest. Just echo the response object at the end of the task.

With this pull request the class can be added to the platform so that it can be used for any application.
For example, in the CMS it could replace the current code in the installer and the language override manager. I saw that Finder and the media manager could also be adapted for using this class. If this pull request is accepted I can standardize this in the CMS.

I'm not sure whether the unit tests are written as expected (it's the first time I coded some).

@Chraneco
Contributor

@eddieajau

Can you add a reference to where this is being used in the CMS?

As @piotr-cz already mentioned, there are several places where this strategy is used.
Since there are different developers using such a class I think it's a good reason to integrate it into the platform:

  • For seeing it in action please have a look at the strings.json.php-Controller of the language manager in the CMS and at the JavaScript file overrider.js.
  • In the installation application nearly the same class is used: InstallationJsonResponse (defined directly in the controller file).
  • In the finder component an extended and specialised version of the class is used: FinderIndexerResponse (defined directly in the controller file).
  • For example, the controller file.json.php can easily be adapted for simplifying its code together with JResponseJson.

t's probably worth looking at this GSOC project for how a JSON response is handled

The benefit of this class against the handling in that application is that not only data is encoded and returned but also a success flag is set automatically depending on the arguments. For example, the class can handle normal objects and arrays but it can also handle an exception with which success is set to false and the message is appended to the response.
For the various ways to use this class please have a look at the unit test file.
I just want to add an additional example here. Assuming that the following is a task of a controller:

  public function execute()
  {
    try
    {
      $model = $this->getModel('example');
      $result = $model->createSomething();

      // Some more code

      echo new JResponseJson($result);
    }
    catch(Exception $e)
    {
      echo new JResponseJson($e);
    }
  }

If everything goes right the success flag will be set to true and the requested data will be returned. If an exception is thrown the success flag will be set to false and the message of the exception will be returned.

for example, not every platform application will necessarily have a message queue

This is already covered by using is_callable. In applications without a message queue, it is treated as if $ignoreMessages is set to true.

I'm not sure this adds a lot to JResponse but I'd certainly like to see this class extending it if it's to be used at all.

At the moment I don't see any benefit of that or are there other reasons for that? This class actually doesn't use any code from JResponse.
For making it clear: This class just creates the JSON string for the response which in turn usually would then be inserted into the body of JResponse (like it's happening automatically in the CMS) or another response object like in JApplicationWeb, for example.

@piotr-cz
Contributor

I'll just add that the advantages of having such class shows up when you have more than one JSON controller or outputting JSON response in different places: one client-side parser will understand response from any extension/ application

@eddieajau
Contributor

@Chraneco I think what you are describing is a stand alone helper that's been design very much with the CMS in mind (or in other words, it's application specific). There's nothing wrong with that, but in terms of the Platform, I'm just not seeing it as generically useful (and I go back to my comments that the message queue is a 'CMS thing'). Maybe I'm looking at the problem it's trying to solve the wrong way, but I just don't get how it give the controller any added value over $this->app->setBody(json_encode($data));. Whatever the case, I do feel strongly that if a class such as JResponseJson is to exist, it should extend JResponse and add value to that base class. I also think that errors should not be coupled to the response object - the error/exception handler should set the response (not the response set the error).

I'll leave this open for a while longer to see what others think.

@LouisLandry
Contributor

If you move the package into the legacy tree then I'd be supportive of merging it.

@LouisLandry
Contributor

Upon thinking this through I think it best if this pull request is sent to the CMS tree anyway. We plan to deprecate JResponse in the near future in favor of just using the methods built into JApplicationWeb. This is one of the major functions of the new web application class. In terms of the CMS landscape I do think the idea and implementation has merit, but it isn't really in line with where we are wanting to go with the platform for the reasons I've already discussed.

Because I think this should be submitted to the CMS instead, and because it is no longer mergeable I'm going to close this. Thanks for the time, thoughts, and work regardless!

@piotr-cz
Contributor

@LouisLandry
You mean /libraries/cms/response/ ?

@LouisLandry
Contributor

Correct.

https://github.com/joomla/joomla-cms/tree/master/libraries/cms is where I would put this stuff. I don't disagree with the notion of having JResponseJson extend JResponse though, Andrew is on to something there.

@laoneo
Member
laoneo commented Mar 22, 2013

What happens now with this pull request? Is it merged?

@dongilbert
Contributor

It is not merged. @elinw would know (maybe?) if this is something the CMS would use.

In the meantime, and this is what I do, when sending JSON responses as the application output, you can use this in your view:

$app = JFactory::getApplication();
$items = $this->get('Items');

// do post processing if need be

$app->close(json_encode($items));
@elinw
Contributor
elinw commented Mar 22, 2013

I like the idea a lot and I think it makes sense in the CMS libraries where we are doing some interesting things and have a couple of gsoc ideas that could use this plus we are not yet using JApplicationWeb so Louis's point about that also makes me think it makes sense to do given that we are potentially looking at 2.5 years of support for JApplication. I pretty much always trust Andrew's instincts on what should extend what, but we have also added a folder /cms/helper that could work if it is a standalone helper.

@eddieajau
Contributor

@laoneo given the changes between Platform and Framework I'd make this request against the CMS tree. I don't think it makes sense putting anything new in this Platform as we are about to freeze it.

@laoneo
Member
laoneo commented Mar 25, 2013

@eddieajau means what? Should there be made a new pull request on the CMS? Can this pull request be moved over to the CMS?

For others if you want to do it https://groups.google.com/forum/?hl=en-GB&fromgroups=#!topic/joomla-dev-general/LLosuO2w6LU

@eddieajau
Contributor

Yes, make a new pull request against the CMS repo. No, this pull request can't be moved.

@piotr-cz piotr-cz referenced this pull request in joomla/joomla-cms Aug 31, 2013
Closed

Add JResponseJson as a CMS library class #1383

@Chraneco Chraneco deleted the Chraneco:json2 branch Sep 13, 2013
@Chraneco
Contributor

The JResponseJson class was recently merged into the CMS libraries.

I added a documentation article about it here:
http://docs.joomla.org/JSON_Responses_with_JResponseJson
Any feedback is very welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment