Cakephp: 4.0+ Reduce setter/getter Verbosity

[inoas]

This is a (multiple allowed):

• [x] enhancement
• [x] feature-discussion (RFC)
• CakePHP Version: 3.4.x

What you did

• Used the query builder
• used request/response objects

What happened

Realized how nice it is to have a concise interface over verbose setters/getters.

What you expected to happen

I was hoping there would be a distinction of stateful objects in core cakephp grouped into following categories in future:

1. builders
2. vanilla read/write objects
3. readers and immutable objects

Builders

Builders could drop the 'set' prefix from all setters and/or would not get them for 4.x.

Aka the ORM's query builder is a good example. It is 99% about using setters, then at some point firing off the query.

When using builders without the setter prefix we can get very close to the visual size of array options, too. Example:

function o($filter) { return OptionsFactory::create($filter); } // somewhere in bootstrap or so

$options = ['controller' => 'Articles', 'action' => 'view', 5, 'q' => ['show' => 'recent']];
// vs
$options = o('url')->controller('Articles')->action('view')->param(5)->q(['show' => 'recent']);
// __call() can be used as a generic setter or the setters can be declared functions to help with IDE support

Readers / Immutable Objects

Say working with request/response here there is no need to use the get prefix. To modify an object that is mostly being read one simply uses setFoo() or to copy on change a immuntable object withFoo().

Vanilla Read/Write Objects

Entities come to mind... their properties are often set and get and thus both prefixes make sense.

Ref: https://github.com/cakephp/cakephp/pull/10887#issuecomment-316465007
Ref: https://github.com/cakephp/cakephp/issues/9978#issuecomment-323011610

[davidyell]

I much prefer the verbosity as it makes my code easier to read and thus reason about without having to go and read method source code.

[markstory]

Don't builders add more complexity over vanilla objects? There would be 2 classes to learn, use update and write tests for.

[inoas]

I didn't mean to create two classes; just to categorize all classes that are instantiated as stateful objects into 2 or 3 groups.

Objects similar to the query builder would then not get setter prefixes (but 'could' have getter methods). Etc.

[davidyell]

I'd be worried about the provision of a consistent api interface with the classes if things started getting mixed up. It's quite possible that users could try using an object in a way it was not intended for and get confused.

[dereuromark]

Staying explicit makes typing and finding method JUST as fast as without it (typing just foo will find both set and get method and then you hit enter, its that easy), but as @davidyell states, it helps to be more precise and clear on what the method intends to do.
So we should always have a clear get and set verb with each method - no exceptions for a consistent and communicative API.

[ADmad]

Staying explicit makes typing and finding method JUST as fast as without it

Err no, one won't get any useful completion until you type at least 4 chars getX, setX.

[dereuromark]

Thats not true @ADmad
The is the most common misinterpretation of IDEs :)
They use the wildcard pattern, thus "foo" will show both "getFoo" and "setFoo" as I just said.
Making it clear what those two are differing in - in contrast to some of our classes and their methods where you cannot just know what they do, because they are missing the "action" verb.

In addition, just typing get or set will also give you a nice grouped list of what is possible in regarding of those actions :) Just a nice benefit on top.

[jeremyharris]

Anyone who knows me knows I usually prefer verbosity, however this is one of those instances where I'd like at least some of the classes to use the builder suggestion proposed (contain(), getContain()). As silly as this sounds to say, we should consider the marketing aspect of it, as it is arguably much "prettier."

I don't want to diminish the work being done to have a consistent API across all of Cake - I think that's very important. But there might be a nice compromise somewhere which is why I like these open discussions. The one proposed sounds pretty good to me, except that it might be seen as arbitrary if the rules for what a "builder" is aren't strictly defined. Would the viewBuilder() classify as such, and un-deprecate the ->layout() et al methods?

[dereuromark]

@jeremyharris Not being an expert in CakePHP you would have to learn first what it means to just see a "layout()" method somewhere, instead of always knowing it right by the name itself.

is it

• layout() to set and getLayout() to read?
• layout() to get and setLayout() to write?

everyone will always have to look this up, it also seems to change from class to class.

So while I can understand the char savers - I way more appreciate the lower entrance barrier for anyone - and everyone just quickly skimming some code snippet somewhere sure also does :)

• setLayout() - cool
• getLayout() - sure thing

[jeremyharris]

@dereuromark to me it's not at all about saving chars, it's about how nice it is to read.

[dereuromark]

I think most would argue that clear and precise is nicer to read than having to guess what it means :)

[cleptric]

Regarding all the work we have done to deprecate all mixed mode methods and introducing new getX/setX Methods, I seems odd to changes this again.
We made an exception with the Query Builder, which is ok IMO.
Personally, I like the "verbose" style, as it cleary showcases what to expect when calling the method.

[markstory]

One challenge with converting the existing mixed mode methods to stricter methods was that deprecating half of a method is challenging. That is one of the reasons we adopted the getX, setX pattern. We could more clearly deprecate entire methods and get stricter typing down the road.

[ADmad]

They use the wildcard pattern, thus "foo" will show both "getFoo" and "setFoo" as I just said.

@dereuromark phpstorm does but all IDEs don't. For e.g. in Eclipse PDT if you type $this->response->f the only completion it offers is the file() method.

Also not everyone uses IDEs, they type out the method names the old fashioned way :)

[dereuromark]

That argument is still weak IMO, as those not using the "ideal" tool here for fast dev are still highly adjusted to their way and thus are still as fast as they can be given the toolset available, and those chars won't slow them dont much either.
But reading and quickly understanding the code on the other side will always, and especially, be easier. And that is what counts :)
And I am not just talking inside the application, or with the source and framework code available in parallel, but also as code snippet on SO or pastebin or as an outsider quickly glancing at a method.

[davidyell]

Framework choices should never be led by developer tooling. I was told this when I said about getting rid of the PHP 4 convention of prefixed underscored protected and private methods.

I would still focus on how easy the code is to reason about. If I can read a line and it makes sense as a sentence, then that's ideal to me. It's one of the things I really liked about Ruby.

[inoas]

Framework choices should never be led by developer tooling. I was told this when I said about getting rid of the PHP 4 convention of prefixed underscored protected and private methods.

Under that assumption the argument to always have getters/setters because then you can type foo in PHPStorm to have a choice of setFoo/getFoo falls flat then.

It's one of the things I really liked about Ruby

In Ruby you would export setters/getters through attr_accessor and then access without get or set prefixes. In Ruby world they are considered "Java junk". So I cannot follow here.

But reading and quickly understanding the code on the other side will always, and especially, be easier. And that is what counts :)

Agreed, and if you are working with builders the set prefixes are nothing but verbosity. They do not add any information you do not already have anyway. If you are reading $q->where(['foo' => 'bar') you know you are adding a where clause to a query builder, unless you are a robot that does not understand context. If you are reading $this->request->data() you know it is a reading operation because request is immutable.

Do not forget, I am not talking about chaos. I am neither talking about having magic methods. Nor I am talking about removing setters and getters from all objects. I am merely talking about categorizing SOME objects are builders and SOME as immutable objects/readers. Those objects then can either drop the set/add prefix as they are 99% being used as setters anyway (builders) or drop their get prefix as they are most of the time being used as getters (unless explicitly told otherwise, as in withData()).

So where is the logical contradiction in this?

[dereuromark]

falls flat then

@ionas You cannot just take an argument, inverse it and make it sound again this way. That doesnt work.
The argument was against the issue of typing. It has absolutely 0 to do with reading code. As it was clearly pointed out numerous times now.
So please dont mix up those things.

As to your repeating over and over the same thing:
We got your point, we will consider it where it is applicable. Thanks for the input, maybe we can stop that discussion now.

[inoas]

This issue has absolutely 100% to do with reading code, please don't state wrong things. A reason many "web" java developers migrated to Ruby/Rails about 10 years ago had to do with mostly verbosity and boilerplate: Having a bad experience reading and maintaining code.

And I could easily stop repeating myself if you tried to counter the argument itself:

The suggestion about conceptually and consistently splitting the core's classes that are instantiated as as stateful objects into 2 or 3 groups so that we can drop some verbosity instead of being forced to add it say the query builder (for consistency).

[dereuromark]

I was talking about my argument. You inversed it and used it out of the context it was meant to be used in.

[robertpustulka]

If you are reading $this->request->data() you know it is a reading operation because request is immutable.

@ionas
I don't understand your point here. CakePHP implements PSR-7 interfaces, where getters are prefixed with get*. Are you suggesting that getters added by CakePHP shouldn't be prefixed (like data()), but still have prefixed PSR-7 methods? How is that any better, easier to read and understand?

[ADmad]

In case of PSR7 we have no choice, we have to implement the it's interfaces. That doesn't mean we need to go about adding get/set to all methods throughout the framework.

To differentiate between getters and setters using only either of "get" or "set" prefix is enough. We don't have to use both.

The ONLY valid argument for adding both was brought up by @markstory that it's problematic to deprecate only one usage of existing combined getter setter methods.

[inoas]

@robertpustulka well PSR7 made a bad choice there, because the object is immutable there is no reason to use get prefixes. But we have no choice there.

[robertpustulka]

I still don't quite understand. You want to have unprefixed getters for immutable objects and unprefixed setters for builders? How can it be more confusing? :)

[inoas]

It is not confusing at all. The mode of operation you are usually doing is how the interface should be designed.

Anyway... as a compromise, here is my suggestion:

• Keep an eye if a stateful object is more of a builder or more of a reader or both. Add that to doc blocks you encounter.
• For consistency and interoperability (PSR etc), continue to add prefixed setters and getters to all stateful objects' methods - including the query builder / orm and validator.
• After 4.0 hits we had a long time of deprecation and @markstory important argument about the difficulty of deprecating magic getters/setters if the method name stays around then becomes less important in far future
• We can then introduce shortcut methods that simply call/alias getFoo() or setFoo() depending on if it is a builder or reader. Say - as an example - $this->request()->data() being to getters that actually call $this->getRequest()->getData() to reduce verbosity. You are then free to use the verbose nomenclature or the concise, eye-friendly one. The query, validator and rules builders would be the only stateful object that already features this shortcut behavior (did I miss any?).

[chinpei215]

Adding alias methods for readability would be a reasonable compromise. And, it would erase some problems in future. If you are really confused when you see $view->layout() and if you are not confused when you see $view->getLayout(), then you will be confused when you see $view->helpers(). Non-combined getters/setters have not been renamed yet. You have to deprecate this kind of methods too. But if we can have aliases, we can add getHelpers() and make helpers() the alias without any deprecations. This example would not follow the conventions @inoas suggested though.

[jorisvaesen]

@inoas

We can then introduce shortcut methods that simply call/alias getFoo() or setFoo() depending on if it is a builder or reader. Say - as an example - $this->request()->data() being to getters that actually call $this->getRequest()->getData() to reduce verbosity. You are then free to use the verbose nomenclature or the concise, eye-friendly one. The query, validator and rules builders would be the only stateful object that already features this shortcut behavior (did I miss any?).

To me this looks like the way to go. I think the docs should use the verbose version to make the snippets more readable.

[hmic]

I need to catch up on the java boilerplate argument and php lacking like 5-10 years behind java development...

Please have a look at https://projectlombok.org/features/GetterSetter

This is a patch to add and to automate the process of writing required getter and setter functions for like everything.

Maybe we on the php side can be smarter now and not try to force it on everything without reason...

@inoas, @dereuromark I don't think this topic is able to be settled, the conflicting parties have arguments that can't be denied. It's just about a decision, not winning the argument!
:p

[ADmad]

Or maybe tuupola/witchcraft is the solution to our problems :smile:

[Oxicode]

WoW. +1

[xavier83ar]

I think that a few different things are being mixed up here, but I'll try to leave my humble opinion about this matter.

First of all, while I understand the general idea of @ionas I didn't get what's so wrong with setters and getters and its verbosity, if it's about reading code, I think that verbosity is better, if it's about characters saving ... IMHO it won't worth it.

I'm in favor of having a consistent and coherent api using set/get preffixes where it should be used, for many reasons:

• readability

// this does what it says it does, no matter what I know or don't know.
$this->request->getData();

$this->request->data();  
// while in this case, as someone said, you'd know that's a getter 
// if you know know that $request is an immutable object, so it requires some previuos knowledge.

• compatibilty and consistence
  If you take a look at other PSR's, they're taking this way, (and there is a lot of people behind those desisions), so this has two advantages, it makes the framework more open and compatible with others packages and libraries, and also makes easier for developers adopt CakePHP. Besides the fact that there is already PSR7 adopted.

• code generation
  It's pretty easy to automate the generation of setters/getters methods, many IDE's are able to do it right now.

I'm also in favor of using immutable interface where appropriate, with the "with" prefix, but I guess that most of you agree in this point or at least it's not being argued about it.

Finally, I'm in favor of leaving query builder as it is, because here is where I think that there some confusion, methods like where(..), select(..) aren't setters, if you take a look at its method signature most of them has 2 o 3 parameters, and it behavies lightly different from a pure set(), let's use where as an example:

public function where($conditions = null, $types = [], $overwrite = false)
    {
        if ($overwrite) {
            $this->_parts['where'] = $this->newExpr();
        }
        $this->_conjugate('where', $conditions, 'AND', $types);

        return $this;
    }

It's pretty different from a typical setter, for example:

public function setLayout($name)
    {
        $this->layout = $name;
    }

While those methods of QueryBuilder actually change the internal state of the object, aren't setters, as they do additional stuffs. And, like this, there is a lot of cases, I mean, not every method must be categorized as setter or getter, and for all those cases there will be necesary to design and specify an interface.

The interface of QueryBuilder it pretty clear, and I don't think that should treated as setters and get changed because of that.

So, to close my opinion, I like adopting standars, I like the idea of having setters and getters, and being verbose helps a lot understanding and reading code and I like interfaces like QueryBuilder as they are, and I do not think that not having "set" prefix is against the standar neither make insconsistent the api, as they aren't setters.

[dereuromark]

Having more deprecations here for 4.0 or 5.0 (just for the sake of changing the name) might be an additional burden for the ecosystem and people upgrading. Maybe we should keep those ideas in mind for new code for now at least.

[ADmad]

The decision to use get/set prefixes is pretty much set in stone now and we can't back track. Hence closing the issue.