Separate gets and sets methods of Request: query, param, cookie and post

[Girt]

For greater flexibility i think that is the right way.

[sergeyklay]

Then, for consistency need do the same with all methods in all classes (many of them at the same time getter and setters).

[arteymix]

This is a pattern built in Kohana from the very beginning. I think it's very elegant and should stay this way.

How would it permit greater flexibility?

[Girt]

For example, I want to extend base class and use second parameter in getter. It will not work, because setter using second parameter.

[arteymix]

You mean like using a default value?

If you extend the Request class, $_get, $_post, $_cookies are all defined protected.

[lenton]

A method shouldn't have more than one responsibility and it should have an obvious/clear name relating to its purpose. Using "get_cookie()" and "set_cookie()" makes for a lot more cleaner code than using "cookie()" everywhere.

Splitting methods up to ensure they have a single responsibility is a good thing to do throughout the whole of the Kohana code base.

[acoulton]

I agree, also the single getter/setter leads to a lot of boilerplate logic all over the place, has side effects (as currently implemented) such as not being able to set NULL, and plays havoc with IDEs because of every method having two possible return types.

This will be a big change, breaking a lot of third party modules as well as end-user code. Perhaps we could introduce new getter/setter methods as proxies and mark the old-style ones deprecated for this major release series?

[shadowhand]

+1 for separated methods. But this is just one more backwards-incompatible change purposed for Kohana.

[lenton]

The old methods can easily become aliases and deprecated so that all code which uses Kohana will continue to work until we properly remove them in the major version 4.0.

[acoulton]

@lenton the new methods should be the aliases, otherwise existing code that extends setters/getters will break.

[lenton]

@acoulton How would it? When extending a method you replace all the code so surely it wouldn't matter what the existing code is?

[acoulton]

@lenton suppose people have custom logic in the getter/setter (struggling to think of a valid reason why, but doesn't mean they won't have).

If we make get_query call query then that logic will still be called whichever getter you use.

If we make query call get_query then only calls to query will be calling the overridden method.

[lenton]

@acoulton Ah ok, I see.

[arteymix]

The getter/setter usually covers 4 responsabilities:

• assigning an array
• retrieving an array
• assigning a value to a key
• retrieving a value from a key

How would all these be splitted?

This is and complexify considerably the framework code and go against Kohana spirit. I really fear that we move toward some Java-like approach and end-up with distinct getter and setter all over the framework. Let's say I really like the simplicity and elegance the pattern provide.

@acoulton setting NULL is not particulary useful.

Also, I'm not a big fan of leaving that many methods in the framework deprecated.

[acoulton]

@arteymix well probably if you were designing from scratch then for example Request would just have a getter that returns some kind of ArrayObject and then replacing the array, assigning a single key, getting as array and getting a single key would be separate methods on that object.

But since we're not designing from scratch, I'd say the array properties should be just split to a setter (pass an array or single key value) and a getter (return a single key or return all of them).

Let's say I really like the simplicity and elegance the pattern provide.

It's fine-ish when it's strings, but I don't think it's simple or elegant that Request->client() may return either a Request_Client or $this depending what you pass into it. Not least because IDEs and code analysis tools generally can't figure that out, so you have real issues with chained method calls unless you then add explicit typehints or warning suppressions all over your code.

@acoulton setting NULL is not particulary useful.

That very much depends. It's not commonly useful particularly on an empty class, but it can be required if you want to overwrite an existing value. More importantly, it can cause unexpected fatal errors - consider for example this code:

public function action_external()
{
    $request = Request::factory('http://some.api')
        ->query('active_only', $this->request->post('active_only'))
        ->query('search', $this->request->post('search'))
        ->query('auth',  'token');
    $this->response->body($request->execute());
}

Looks simple and elegant enough. Imagine active_only is a checkbox on your form - if so, and it's not checked, then $this->request->post('active_only') will return NULL. That means that $this>query('active_only', $this->request->post('active_only')) acts as a getter and returns NULL. And that makes the next line fatal.

Sure, if you know about that then you can protect against invalid input, but it's not a remotely obvious failure case from just looking at the code. I don't think that's simple or elegant.

Also, I'm not a big fan of leaving that many methods in the framework deprecated.

Me either, but I'm a bigger fan than of leaving that many methods in the framework for end-users to change themselves and of breaking every third-party module they may be using. So if we do this, it needs to be done by deprecation.

[arteymix]

I'm aware of this and always provide default value where appliable.

I think it is a good idea to go forward though. The approach you will take on this will reflect all over the framework, so make a good decision.

Here are my thoughts:

• getter should always have a default value as a second argument defaulted to NULL
• getter must return the instance for builder syntax
• setter should always allow to set an array of value as first argument
• the change must be over all the framework and retrocompatible like you suggest

How long would the actual getter/setter be kept for retrocompatibility?