Cakephp: 3.0 - Define request query value if the result is null

[btaens]

Hello,
I just have a convenience suggestion.
Frequently we have to handle request queries.
It often becomes a bother to check if the return value is null, and then define a default value for it with empty() or === null, or something like that.
Symfony handles this really nicely by simply being able to pass a second parameter to the query function, as so:

$limit = $this->request->query('limit', 20);

This would essentially return 20 if limit was not given as a request query parameter, thereby making the code more readable, and programming faster. It also wouldn't break code written in older versions, as if a second parameter is not given, it just returns null as it does now.

[htstudios]

Should the same be there for data()?

[dereuromark]

Sounds reasonable. But you have to be careful in general, empty() would false positive '0' which it should not as it is a valid "not empty" value being passed in - pretty much all cases anyway. So better checking for "null" and empty string only and specifically either way.

[ADmad]

Simply using ternary would have worked if PHP did not equate "0" to false :)

$limit = $this->request->query('limit') ?: 20;

[markstory]

I can see how having defaults for both data() and query() could be useful. I'm not sure param() should support defaults though. That seems like a place where you'd want to error out instead of provide fallbacks.

[dereuromark]

@markstory data() is not that easy as it is both setter and getter.

[markstory]

@dereuromark Bugger, I missed that as I only looked at the signature.

[htstudios]

I think this should be considered also /together/ with the tocket aiming for an easier and more convenient way to move data from requests to the orm.

[markstory]

@jhli For sure, but this looks like it is aiming to make reading data simpler without shortcuts whereas the other issue is focused more on shortcuts.

[burzum]

This sounds similar to the ticket that was proposed recently by @ravage84 or @ionas, he wanted to have some kind of short cut to the request data as well.

Can't we do something like this?

$entity = $this->request->getEntity('EntityClassNameHere', 'query');
$entity = $this->request->patchEntity($entity, 'query');

Second arg would be the request data (query, or post) type. We could do this as well:

$entity = $this->request->query->getEntity('EntityClassNameHere');
$entity = $this->request->data->getEntity('EntityClassNameHere');
$entity = $this->request->params->getEntity('EntityClassNameHere');

This will construct the entity with the given data.

[markstory]

I am not sure that making the request aware of the orm is wise. How would the request be extended for alternative datasource implementations like elasticsearch?

[josegonzalez]

@burzum Thats like giving a sanctioned alternative to mass assignment.

[burzum]

@markstory true, forget about it, it was just a suggestion.

@josegonzalez yes, but how does that differ from doing the same task manually?

[ionas]

@markstory agreed, I think the request/response/controller domain should be as agonistic towards the ORM as possible to allow re-uses against different model architectures (e.g. non ORM).
Thus my idea was to have a shortcut method to getting data out of the request instead of mangling together stuff with the ORM.

[ionas]

https://github.com/cakephp/cakephp/issues/7233 is the ticket in question.
Maybe we can attach the shortcut methods to the controller itself?

// In Controller Context

// by $this->request->data['Articles']['category']
// would use some Hash/Collection functions to fetch the data out of the request array
// matching to 'general' if empty
$this->Articles->find()->where($this->data('Articles.category', 'general')); 

// by query string /articles/index?Articles[category]=foo
// matching to 'general' if empty
$this->Articles->find()->where($this->query('Articles.category', 'general'));

In #7233 you would be able to specify multiple parameters. In this purposal the methods suggested do less than that: They only return an array with value and key for ONE modelfield or query parameter.
Both use some Hash:: or Collection:: function inside to filter out the data so that the methods could be reused to get data out of the request objects in a convenient way even when not using the ORM.
The second parameter would specify the default, as suggested in this ticket here.

// would still fetch the suggested /articles/index?limit=20 - independent of the ORM
$this->query('limit', 20);

[ionas]

And: If you wanted to filter by multiple fields you would use the query composer:

$this->Emails->find()
    ->where($this->query('Email.is_new', true))
    ->andWhere($this->query('Email.is_read', true));

Would get you all emails filtered by is_new and is_read booleans, e.g. /messages/index?Email[is_new]=1&Email[is_read]=0, in case one filter is not set it would set those to true by default.

[jeremyharris]

I personally read those as a setter, like it's modifying the request, and would prefer the pure PHP implementation @ADmad mentioned earlier. Part of what I really like about Cake 3 is that it feels more like writing PHP code than framework code.

[ionas]

@jeremyharris so either the method name is not good or you want to see an options list thus that it is clear what's going on:

// postdata and querystring methods on Controller
$this->Emails->find()
    ->where($this->postdata('Email.is_new', ['default' => true]))
    ->andWhere($this->postdata('Email.is_read', ['default' => true]));

$this->Emails->find()
    ->where($this->querystring('Email.is_new', ['default' => true]))
    ->andWhere($this->querystring('Email.is_read', ['default' => true]));

// pd and sq shortcut methods on Request
$this->Emails->find()
    ->where($this->request->pd('Email.is_new', ['default' => true]))
    ->andWhere($this->request->pd('Email.is_read', ['default' => true]));

$this->Emails->find()
    ->where($this->request->qs('Email.is_new', ['default' => true]))
    ->andWhere($this->request->qs('Email.is_read', ['default' => true]));

// EDIT: get method with parameters on Request
$this->Emails->find()
    ->where($this->request->get('query', 'Email.is_new', ['default' => true]))
    ->andWhere($this->request->get('query', 'Email.is_read', ['default' => true]));

$this->Emails->find()
    ->where($this->request->get('data', 'Email.is_new', ['default' => true]))
    ->andWhere($this->request->get('data', 'Email.is_read', ['default' => true]));

[ionas]

@jeremyharris and then... https://github.com/cakephp/cakephp/issues/7328#issuecomment-136409937 does not work cause of phps weak types, does it? So that's not an option. Any ideas on your behalf?

[josegonzalez]

@burzum as a developer, you have to go out of your way to be insecure ;)

[jeremyharris]

@ionas you're right the simple ternary won't work, I didn't consider that. How about a separate method, similar to how the $_defaultConfig var works on InstanceConfigTrait, that sets default query args to check and use if necessary?

$this->request->setDefaultQuery([
  'Email.is_read' => true,
  'Email.is_new' => false
]);

Something like that is more readable (to me) and makes more sense (to me), personally. Similar methods could be used for other defaults. I feel like it would result in cleaner controller code as well, since those would be set up in one place. Just a thought.

[htstudios]

Same would be required for data (and possibly even params) - If you are willing to setup a PR I'll remove the defaults stuff from my PR that deals mosly with extraction. Though I like the idea of throwing errors. And throwing errors might be better to be handled when the request variable is actually needed?

[htstudios]

Having thought about this again are more of you in favour of setting defaults for request query (and maybe data) separatly from extracing it. If so, wouldn't it the be also a good place for exceptions?- see PR #7353 .

[ionas]

@jeremyharris Is this a better concept for defaults + exceptions?
https://gist.github.com/ionas/d8b3c5d437cfba33bbfe

[jeremyharris]

@ionas I think the complex part is outside the scope of this RFC, and I suppose if you wanted to get really crazy you could use the Validation classes. I'm not entirely sure what the use case for throwing an error if a query arg was missing would be, at that point wouldn't one use a passed arg and check it in the method?

Anyway, I'd still prefer very specific functions, as option keys lead to typos and possibly aren't future proof. I'd like to see something like this method signature:

public function defaultQuery($query, $merge = false);
public function defaultData($data, $merge = false);
public function defaultParams($params, $merge = false);

It's easily discoverable and understandable, and could help prevent future breaks in case data keys moved or changed. Internally, the could all call a method similar to what you've defined, for DRY and stuff:

protected function _defaults($paramName, $params, $merge = false);

I added merge because I was thinking about it and if the initialize() hook sets some defaults, a method may want to add to those rather than just remove them. Or maybe that's outside this scope too :)

[ionas]

@jeremyharris Now I have been doing web services lately and those often require specific sets of get parameters. So generally speaking:

• HTTP POST/PUT/PATCH wants you to modify data. We can rely on validation/rules or other model logic here, for the most part.
• Sometimes you don't have a fully fledged model to treat POST data - for PRG you want a set of POST parameters and if its not there you should have an easy way to throw http errors
• Almost always almost when exposing interfaces by GET you want to have a specific set of parameters be submitted alongside some optional parameters. Some throw errors, other have defaults. This for instance is the case for web service calls as well.

So one thing your suggestion is missing, is treatment of exceptions. But one could argue that that could be a separate feature all-together that takes precedence before setting defaults.

As for your suggestion, which I really like, the only thing I don't understand is what goes into $query, etc. $data and $params now. For $params I can see that arrays are just fine. But for query and data I would want to have dot notation as it is a lot faster to write.
Utilizing dot notation though would mean that there would be live checks against those defaults based on dot notation going against $this->request->data[] or $this->request->query[] whenever $this->request->data() or $this->request->query() is being called.

What's your stance on dot notation? Can you supply an example of how to actually feed your suggested methods with information (e.g. example "how to use code")?

[jeremyharris]

@ionas Thanks for explaining the use cases for exceptions. Makes sense. I think they belong in a request validation layer at some point, though, not defaults.

I'm fine with dot notation. It's a familiar syntax around Cake. Here's how you would pass it in my example:

$this->request->defaultQuery([
  'Users.Emails.is_read' => true,
  'Users.Emails.is_new' => false
]);
// to merge with existing defaults, using existing merge strategy in Hash
$this->request->defaultQuery([
  'Users.Emails.is_read' => false,
], true);

The same would go for all other default*() methods. Basically, you set the same keys you would use to access the data from the query(), data() and param() methods. They would pass to the protected method, which would actually do the work. That's up to the implementation, though, it was just a suggestion.

[dereuromark]

I think most of that stuff doesn't have to to into the core.
As a component this would be perfectly fine, as well, would it not?

$this->MyRequest->defaultQuery(...)

or whatever you like to use to merge with defaults.

Right now I see a lot of different approaches, and not really a clear line where this would go to.

[jeremyharris]

I tend to agree, despite contributing to the signature of the possible methods. I usually just merge or += an array when I need a default. In the times where it's a necessity for an app, a component would do just fine. Like I said in my first comment, I like that Cake 3 feels more like programming PHP than programming a framework. Maybe I should stick by that -1, but I wanted to get my opinion in _in case_ it makes it in the core.

[ADmad]

I too think this feature could first start out as a plugin and then later once we get a clear picture and there is demand from community for including it in core we can do so.

[ionas]

So I did drop my purposal on concise request->orm data pushing (suggested request->extract()).
Two points, validating requests (not based on the ORM, including HTTP throws) and setting defaults (in case data/query is missing (empty would be fine)) are still left.

Request Validator (strictly: a new issue)

1. Rename Form (Modelless Validation) to Request Validator: See: http://book.cakephp.org/3.0/en/core-libraries/form.html
   Reason: This is the best entry point to validate request data/query before/without even sending it to the model layer. It is already there and should fulfill more/better.
2. Extend that for processing not only post/patch/put but also get/query data, if that does not work yet (untested)
   Reason: Some forms could use GET and/or webservices could use a lot of get parameters.
3. Extend that for adding exceptions (very useful with webservices / rest) and/or flash messages (depending on something is requested by a HTML client or a web service)
   Reason: If a bad request happens this should already be caught by the controller, cause the controller is managing the messaging (request/response) interface.
4. Internally uses validator, but allows to throw HTTP 4xx codes (All parts of the framework should be okay throwing 5xx errors, but 4xx is clearly part of a regular ('expected') request/response flow).
5. This _could_ also be used to validate the existence of parameters for view/edit/delete actions, by default.

Request Default Setter (this issue)

1. Set request data/query defaults by using Hash dotnotation. When the data is queried by using the methods (Request::data() and Request::query()) these defaults will be respected.
2. For consistency, make sure this works for both GET aka QUERY and POST/PUT/PATCH aka DATA (or even mixes like POST with Query String, see http://stackoverflow.com/a/612035 )

[dereuromark]

With the new path of explicit getters this will be easier to implement as 2nd argument (default value), consistently for all of them

• getQuery($value = null, $default = null)
• getData($value = null, $default = null)
• getParam($value = null, $default = null)
• getInput($value = null, $default = null)

etc, either for 3.4 with deprecations, or for 4.x.

[markstory]

@dereuromark I was thinking of adding similar methods. You also opened an issue around casting values read from the request. How would casting and defaults work together? Is the cast after the default value getQuery($key, $default = null, $cast = null)?

[inoas]

Do we want option array as the second param?

[dereuromark]

No Option Array. It should stay simple api IMO.
Symfony has its own cast methods for this besides the getters, e.g. getInt()

[inoas]

The issue with getQuery($key, $default = null, $cast = null) is that once you realise you are still missing certain params the signature will grow and grow.

I'd say either options or a builder. And I'd prefer latter.

Would an interface like this be possible and feasible?

$this->request->getQuery($value)->default($default)->getInt(); // ?

Edit: no, not possible like that, but I'd want to get somewhere close to that, if possible!

[dereuromark]

Other frameworks have an own object for each part, e.g. query:

$this->request->query->get($value, $default)
$this->request->query->getInt($value, $default)

This makes a simple and speaking API without the need for any complexity.

[ravage84]

Having separate objects for each part of the request sounds good to me.
Wil improve flexiblity of the API, while keeping it simple.

[markstory]

I'm not super keen on adding more public properties for each of the request data types. We _just_ deprecated them all.

[markstory]

Closing as #9742 implements default values. The consenus in #9670 was to not implement casting as casting to scalar values can be done in app code with existing PHP casts.