Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Response format control through filter #2034

Closed
creocoder opened this issue Jan 18, 2014 · 32 comments
Closed

Response format control through filter #2034

creocoder opened this issue Jan 18, 2014 · 32 comments
Assignees
Labels
status:under development Someone is working on a pull request. status:under discussion
Milestone

Comments

@creocoder
Copy link
Contributor

Currently we have possiblity to do this job operativelly:

    public function actionSomething()
    {
        ...
        Yii::$app->response->format = Response::FORMAT_JSON;
        ...
    }

How about creating filter to avoid code like this? Something like:

public function behaviors()
{
    return [
        'verbs' => [
            'class' => \yii\web\ResponseFilter::className(),
            'actions' => [
                'something'  => [
                    'format' => Response::FORMAT_JSON,
                ],
            ],
        ],
    ];
}

Also since we have traits maybe its not needed to intruduce new class and just impove Response class to have possibility to work as filter. Not sure about this filter syntax yet. This is just idea about operative => declarative, since we have operative => declarative possibilities for many other features.

@qiangxue
Copy link
Member

I don't think this is needed. Normally you would use the same response format for ALL actions of a controller (e.g. REST api). Sometimes the format could be conditional (e.g. format negotiation based on header info). In this case, your proposal doesn't work either.

@creocoder
Copy link
Contributor Author

@qiangxue

Normally you would use the same response format for ALL actions of a controller

Usually i faced with situation that no, for example you have TagController. You have actions index, create, update, delete which response with html and action list which response with json. Also ok, for example we have controller like you say. Need to write init() with Yii::$app->response->format = Response::FORMAT_JSON; ? For me this code in any place looks like approach is not complete.

@creocoder
Copy link
Contributor Author

@qiangxue For example we have possibility to manage tags. Also we need action like list which used for jquery plugins to autosuggest tags. Can you show best practice then? As i understand you suggest to separate controllers into TagController with crud operations and ???Controller with list operation? This way i agree with:

you would use the same response format for ALL actions of a controller

Can you confirm about separation?

@cebe
Copy link
Member

cebe commented Jan 18, 2014

@creocoder what is the list action used for?

Do not really see why it is bad to set response format in one action that is different.

@creocoder
Copy link
Contributor Author

@cebe I do not say its bad. index action and list action is absoluttely different. So this is another case. And this is why i ask need i separate controller? Index action used for grid view while list action used for generating json for some jquery widgets.

@creocoder
Copy link
Contributor Author

@cebe And this is why this issue created. I just want to avoid code like Yii::$app->response->format = Response::FORMAT_JSON; inside action and use declarative form of it.

@samdark
Copy link
Member

samdark commented Jan 18, 2014

I'm against it. Yii::$app->response->format = Response::FORMAT_JSON; is short and the message is clear. If you don't want it inside action, use it inside init.

@creocoder
Copy link
Contributor Author

@samdark Using it inside init() cause this format will be used for all controller actions.

is short and the message is clear

Following this logic something like:

if (Yii::$app->user->checkAccess('some')) {
    ...
}

is too short and clean, but we have AccessFilter for it. So why not have filter for Responsible. Where is logic?

@qiangxue
Copy link
Member

I'm against this too. It's making simple thing complicated.

checkAccess is different because it is only part of the AccessFilter.

@creocoder
Copy link
Contributor Author

@qiangxue Ok, then lets remove Verb filter than? It too make things complicated. You can do imperative checks for verbs. This ticket about imperative VS declarative. I see no any complication here.

@qiangxue
Copy link
Member

Can you try to write plain PHP code without using verb filter? You will see it's not as trivial as this one-liner, and because of this, you want to use the verb filter to avoid lot of duplicated code.

@Ragazzo
Copy link
Contributor

Ragazzo commented Jan 18, 2014

Against it too, no real profit, VerFilter is not the case to compare.

@creocoder
Copy link
Contributor Author

Ok, saw that another frameworks have declarative feature for control response type, but ok, convienced.

@cebe
Copy link
Member

cebe commented Jan 18, 2014

Ok, saw that another frameworks have declarative feature for control response type, but ok, convienced.

could you post some links?

@creocoder
Copy link
Contributor Author

@cebe Ofrouse for example Symfony 2 has both imperative and declarative ways. Read this for example:

http://stackoverflow.com/questions/9146460/symfony2-echoing-json-from-a-controller-for-use-in-an-extjs-4-grid

Declarative way is:

# src/Scope/YourBundle/Resources/config/routing.yml

ScopeYourBundle_people_list:
    pattern:  /people
    defaults: { _controller: ScopeYourBundle:People:list, _format: json } <- look at this

Format controlled via router configuration. Anyway another frameworks has similar or different approaches to set response type. The main thing they have declarative in addition to imperative way. We have only imperative way, which is enough for home grown blog, but smells like bad practice for other types of applications.

@cebe
Copy link
Member

cebe commented Jan 19, 2014

Reopening this. Will check what we can do about it. It is not suiteable as an action filter imo but there need to be simple ways to configure this kind of behaviors.

@cebe cebe reopened this Jan 19, 2014
@ghost ghost assigned cebe Jan 19, 2014
@qiangxue
Copy link
Member

@creocoder The code you show here isn't exactly the declarative way of setting response type. It's actually similar to what you do with Yii URL rule: '<controller>/<action>.<_format>' => '<controller>/<action>' (you can also set default value of _format in Yii 2.0).

_format is just one way of requesting the response type. You may also get this information from accept type header. And once you get this information, you still need to check if the action supports the requested format. This process is called content type negotiation, which doesn't exist in Yii yet. Yes, this is something we can support.

@creocoder
Copy link
Contributor Author

@qiangxue I'm fine with any solution which will allow not write Yii::$app->response->format = Response::FORMAT_JSON; inside controller action.

@Ragazzo
Copy link
Contributor

Ragazzo commented Jan 20, 2014

@qiangxue any docs on this feature or example?

'/.<_format>' => '/' (you can also set default value of _format in Yii 2.0).

I think that simple check of if isset _format in web\Controller::init() + setting response formatter if needed, would be just the case.

@danschmidt5189
Copy link
Contributor

This process is called content type negotiation, which doesn't exist in Yii yet. Yes, this is something we can support.

@qiangxue I am for supporting this. Very important for APIs that expose JSON and XML, a common use-case.

To clarify the problem here, I think we essentially want Symfony FOSRestBundle's format-agnostic controllers. Actions would be expected to set Response::$data, and another component would map that into an HTML/JSON/whatever view. (Basically MVVM pattern.)

With this setup, it's possible to determine before the action is run whether the app can return the type requested by the client. That means we can HTTP 400/500 all over the place before hitting the database. (BIG WIN!)

Seems like a great idea to me.

Index action used for grid view while list action used for generating json for some jquery widgets.

@creocoder This gets at how distasteful it is to (essentially) combine your API with your view server. It feels better to have one format (or one conditional) per controller because it is, and because that's exactly how our APIs work. Nice to know I'm not the only one who does it and cringes afterwards :)

That said... does this belong in core?

@Ragazzo
Copy link
Contributor

Ragazzo commented Jan 20, 2014

@danschmidt5189 we already have issues for web-api, check them.

@danschmidt5189
Copy link
Contributor

@Ragazzo
Copy link
Contributor

Ragazzo commented Jan 31, 2014

Any news on this one? i guess simple rule with '<controller>/<action>.<format>' => '<controller>/<action>' in boilerplates config by default + check and set needed format in beforeAction would be just the case. Should this be implemented in core?

@danschmidt5189
Copy link
Contributor

@creocoder @Ragazzo I use a filter for content-negotiation. Is this what you're referring to?

It checks request parameters to see if a format was requested. (Supports checking headers, query, and body.) Details are in the comments below.

I could PR if there's interest.

Example configuration:

// Sets the application Response::$format
'format' => [
    'class' => '\app\filters\FormatNegotiationFilter',

    // List of supported actions
    'actions' => ['*'],

    // Formats supported by the configured actions
    // Defaults to array_keys(Response::$formatters)
    'supports' => ['json', 'xml', 'view'],

    // If no requested format is supported
    'unacceptableCallback' => function ($accepted, $supported) {
        throw new NotAcceptableHttpException();
    },

    // Format negotiation rules (source => param)
    // Source can be 'query', 'body', or 'header'. The param specifies the name
    // of the parameter in the given source to be checked for a format.
    // 
    // E.g.
    // ~~~
    // 'header' => 'X-Format' : $request->headers->get('X-Format')
    // 'query' => 'format' : $request->query->get('format')
    // 'body' => '_format' : $request->body->get('_format')
    // ~~~
    // 
    // Rules are checked in order. The first supported format is set as the
    // response format. If a format is not specified, the response is not modified.
    // If a format is specified but not supported, invokes the unacceptableCallback.
    'negotiation' => [
        'header' => 'X-Request-Format',
        'query' => 'format',
    ],
],

@Ragazzo
Copy link
Contributor

Ragazzo commented Jan 31, 2014

yeah, but this is too big for fw itself, so simple check in beforeAction is just the case.

@danschmidt5189
Copy link
Contributor

Same argument applies to AccessControl and VerbFilter. They're all basically the same class. Just different rules and HTTP errors.

@danschmidt5189
Copy link
Contributor

@qiangxue @Ragazzo Have you heard of Rails' respond_to' and 'respond_with methods?

respond_to registers a list of formats (Content-Types) that your controller/actions are able to return. This is invoked pre-action; if the request wants an unavailable format, the controller throws a 406 Not Acceptable HTTP Exception.

Here's the official Responder documentation.

respond_with is called in your actions to register callbacks that return data in different formats.

These are highly relevant to Issue #303 and very powerful for developers, allowing them to write a single controller per resource, including custom formats (e.g. versioned ones). Qiang's idea to replace the ModelSerializer with interfaces (e.g. toJson, toXml?) might fit well with this.

@Ragazzo
Copy link
Contributor

Ragazzo commented Feb 19, 2014

@danschmidt5189 yes, but we are not going that way, we will have rest support in other way. however i dont see good solution now for this issue with your suggested methods, because we dont have good syntax like do end.

@danschmidt5189
Copy link
Contributor

@Ragazzo No, but we have callbacks. What's the problem?

Controller::respondTo($formats, $options = []) registers a format the controller can return. It can be invoked as a method, or in an accessRule-like filter.

Controller::respondWith($data = null, $options = []) registers the returned data (e.g. a model) along with optional overrides for rendering the different formats.

Essentially what I want is for a single action to be able to return multiple formats with as little code/pain as possible. In 95% of cases those formats are:

  1. A full view, for text/html non-Ajax requests
  2. A partial view, for text/html Ajax-requests
  3. JSON, for application/json requests
  4. XML, for application/xml requests

I'm not sure how that's possible in the current setup, and especially not if the REST controller/actions are different classes from ordinary controllers.

@Ragazzo
Copy link
Contributor

Ragazzo commented Feb 19, 2014

yes, we have callbacks, but we loose readability and syntax, not sure about that. Anyway PR will be better to review, if you have time)

@cebe
Copy link
Member

cebe commented Mar 6, 2014

@qiangxue this is realated to API implementation, can you check whether it got obsolete since API branch is merged?

@cebe cebe assigned qiangxue and unassigned cebe Mar 6, 2014
@qiangxue
Copy link
Member

qiangxue commented Mar 6, 2014

Let's keep it open. Current REST implementation is based on a controller method. Will think about abstracting this out as a class so that it can be used elsewhere.

cebe added a commit that referenced this issue Apr 10, 2014
* 'master' of github.com:yiisoft/yii2: (79 commits)
  Refactored app bootstrap logic.
  Update authorization.md
  Fixes #3052: Fixed the issue that cache dependency data is not reused when `reusable` is set true
  start debug logging only if debug runs when bootstrap.
  Update finnish translation
  Add ODBC support to yii\db\Connection
  updated error handler and requirement checker links.
  fixed broken API links [skip ci]
  added more doc [skip ci]
  update class map.
  Fixes #2034: Added `ContentNegotiator` to support response format and language negotiation
  renamed attributes to attributeNames in model
  updated phpdoc
  Removed `Application::preload` in favor of `Application::bootstrap`
  Update module-debug.md
  Update model.md
  Fixes
  Update basics.md
  typo fix [skip ci]
  Added `HtmlResponseFormatter` and `JsonResponseFormatter`
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:under development Someone is working on a pull request. status:under discussion
Projects
None yet
Development

No branches or pull requests

6 participants