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

Fix loss of escape value and data in the model #4088

Merged
merged 2 commits into from
Jan 15, 2021

Conversation

michalsn
Copy link
Member

Description
This PR tries to fix some issues with current Model behavior.

The main change is that $tempData['escape'] is no longer a boolean value but an array that holds all of the escape values for data. Since this variable is internal and wasn't never really used anywhere (in events for example), then I think changing it is fine.

After all, we're fixing a bug here. But if anyone sees a better idea to solve this, I'm all ears.

Fixes #4087

Checklist:

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

Copy link
Collaborator

@iRedds iRedds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I still don't understand why use 3 methods for insert / update in two classes
Model::insert() -> BaseModel::insert() -> Model::doInsert()

system/Model.php Outdated
Comment on lines 336 to 338
protected function doUpdate($id = null, $data = null, ?bool $escape = null): bool
{
$escape = $this->escape;
$this->escape = null;
$this->escape = [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argument "?bool $escape = null" never used

When we use the foreach construct to call the set() method, we can use $this->escape directly and reset after the loop ends.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BaseModel Model->insert()

Public methods like this handles Object (Entity) to array conversion ,validation, protect fields, timestamp processing for created_at, updated_at, deleted_at and triggers the events,

Model doInsert() - Also defined as abstract in BaseModel

The actual database connection to read, insert, update data happens in methods prefixed with do which are called at the specific point from mentioned public methods

What's the reason behind all of this. If you wan't to create model for any other kind of database which was in my example MongoDB. You previously had to override or implement everything in the custom model. Now you can simply extend the BaseModel and do the custom db implementation by only implementing the abstract methods from the BaseModel. In practice this means that all the non DB related features will be available in the custom model including validation, protect fields, timestamps, conversion to entity objects, soft deletes, callbacks, pager ....

It might seam easier to complete model from scratch but this saves a lot of time by reusing already well documented and tested functionalities especially when you follow the complete DB driver structure using ConnectionInterface, QueryInterface and ResultInterface

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iRedds Thanks for the tip about the escape parameter I missed this one in #4072 so I submitted a new PR #4090

@michalsn can you check it out and rebase this one afterward ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, I want to change the BaseModel::update() method.
In this case, I need to inherit the new class from BaseModel and implement all abstract methods. Instead of just extending the Model with existing methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need to change the extend the update for SQL model you need to extend Model classs. Then you can either override the update or doUpdate depending on what you need

Extending BaseModel directly is only to be used when implementing Model class for custom database. eg. MongoDB, ElastecSearch, Firebase ...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @iRedds.

Thanks for making a PR @najdanovicivan, I missed it too. Please let me know if there are any issues with the proposed changes or if you see any place for improvements.

@MGatner MGatner merged commit 6ee89af into codeigniter4:develop Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Loss of escape value and data in the model
5 participants