BaseModel/Model - Attempt to rework escape parameter

[najdanovicivan]

Description
Here is one way of keeping escape parameter outside of method definitions. But the existing solution is much more elegant

@michalsn What's your opinion on this one ?

Should we keep the existing implementation and just update the docs or should this one be merged

Checklist:

• Securely signed commits
• Component(s) with PHPdocs
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[michalsn]

Well, for 4.0.x I would rather merge this. 4.1 is a different story, but I would like to hear other people's opinions about it.

I know we shouldn't introduce BC changes into a minor version, but it feels like an improvement I'd shut my eyes for (if it would come with a good docs update).

[iRedds]

Dubious elegance

$db->escape = false;
$db->insert([]);
//instead
$db->insert([], true, false);

A class property is usually added to persist state. But you are resetting its value with every insert/update query.
What then is the meaning of this property?

[najdanovicivan]

Dubious elegance

$db->escape = false;
$db->insert([]);
//instead
$db->insert([], true, false);

A class property is usually added to persist state. But you are resetting its value with every insert/update query.
What then is the meaning of this property?

The class property is to keep state which is set in insert/update methods and handle this state in doInsert/doUpdate methods where is reseted after getting the value.

It works completely fine as is but the issue here is that public method definititons were modified when BaseModel was introduced. So having those changes will break stuff if there are custom model classes where this methods are overriden

[iRedds]

The class property is to keep state which is set in insert/update methods and handle this state in doInsert/doUpdate methods where is reseted after getting the value.

It works completely fine as is but the issue here is that public method definititons were modified when BaseModel was introduced. So having those changes will break stuff if there are custom model classes where this methods are overriden

Oh, please don't reinvent the wheel.
QueryBuilder has a default set() method that can add data to the set without escaping.

[michalsn]

I think @iRedds have a good point here. Seems like public $escape can be removed entirely.

[najdanovicivan]

@iRedds the issue here is that QueryBuilder is called in doInsert/doUpdate but the value have to be processed in insert method to get the data from tempData which is only done if insert/update is called with empty data.

Since the parent insert/update methods handle conversion to array if data is empty it will throw an exception so we have to get the date from tempData before passing on to baseModel method.

This is all due to set method from QueryBuilder being captured in model so that data can be validated. So the escape parameter cannot be removed unless we have a way to pass the tempData['escape'] to doInsert/doUpdate

It can be done with tempData directly but the value should only be passed if insert/update was called with empty data so in such case we could instead of setting $this->tempData to empty array in insert/update we can unset $this->tempData['data'] and keep $this->tempData['escape'] and then get the escape value from tempData and reset it out in the do method

[michalsn]

Thanks for explanation @najdanovicivan. Maybe we could make $escape protected and introduce the setEscape() method? We could then set it in more elegant way with support for chaining.

[iRedds]

OMG. OMG. We will all die. =)
I want to give my opinion on all these dances with the model. Even if I use CI4 in the future, I will be writing myself basic model. Cause now I see crazy useless overhead.

In my current CI3 project, I am using a base model that implements search, save, delete and event handling methods. There are 400 lines of code in the model, not around 3000 like CI4.

Does the code work without your changes? Works.
Did your change improve something? No.
If someone cancels existing methods, he must be aware of the consequences. Alternatively, you can use the final key to prevent overriding the method.

But it seems to me that it is better to completely revise the classes and remove this insert + parent::insert + doInsert.

[najdanovicivan]

@iRedds I have MongoDB driver for which is mandatory to use BaseModel

The idea behinde BaseModel/Model is to separate the DB stuff from the other things model do. Validation, Pager, Dates ..,

[paulbalandan]

Thanks for explanation @najdanovicivan. Maybe we could make $escape protected and introduce the setEscape() method? We could then set it in more elegant way with support for chaining.

There is already a Model::set(mixed $key[, ?string $value = ''[, ?bool $escape = null]]): self method for this one.

[paulbalandan]

Looking at the version in master, I think using the class property is our only way for Model::doInsert to consume the $escape var from Model::insert.

[michalsn]

There is already a Model::set(mixed $key[, ?string $value = ''[, ?bool $escape = null]]): self method for this one.

Thanks @paulbalandan I missed that.