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

ErrorException Trying to get property 'affected_rows' of non-object #1559

Closed
nowackipawel opened this issue Nov 30, 2018 · 6 comments
Closed
Labels
database Issues or pull requests that affect the database layer

Comments

@nowackipawel
Copy link
Contributor

screenshot_2018-11-30_18-29-52

Not sure why this is happening.
Normal update action in one of the models method:
$this->update($id, ['column' => $value])
and then:
return $this->db->affectedRows() is causing this exception.

@nowackipawel
Copy link
Contributor Author

Ok it was because of validation rules.
I've got validation rules for every fields in my model, and I am updating only one field and.... there were errors in this operation.
However:

  • shouldn't affectedRows metod return 0 instead of exception?
  • shouldn't validation library check only fields which are updating?

nowackipawel added a commit to nowackipawel/ci4-old that referenced this issue Nov 30, 2018
According to codeigniter4#1559 I write some code which allows to filter validationRules to that elements which are provided in $data.

----

Allows to filter validation rules to only that elements which are in $data passed to update method
If you'd like to check only fields passed in $data during update just call:
$this->update($your_id, $your_data, true);


if there is 5 rules in your $validation_rules and  only two of them in $your_data array then only that two elements will be checked in validation process if you pass true as a 3rd arg (if you don't provide it or set to false then update will check every rule as is now). 

Useful: i.e. you want to only update user nickname or password instead of passing all user attributes.
nowackipawel added a commit to nowackipawel/ci4-old that referenced this issue Nov 30, 2018
codeigniter4#1559  - if there is no affected_rows set it is better to return 0 instead of exception :)
@natanfelles
Copy link
Contributor

natanfelles commented Nov 30, 2018

@nowackipawel

  • shouldn't affectedRows metod return 0 instead of exception?

It should not throw an exception. It seems that there is still no connection to the database when the affectedRows() method is called.

  • shouldn't validation library check only fields which are updating?

I think no, because if you set rules for a field, these rules must run.

If you allow to ignore certain fields, add the if_exist rule and you are fine.

If this rule is present, validation will only return possible errors if the field key exists, regardless of its value.

Works nice with Models, API updates, and HTML Forms.

Note that if you allow to ignore fields not sent in the Model, these can be ignored if you pass data direct from HTTP requests too...

@nowackipawel
Copy link
Contributor Author

You are right as far as we don't have methods which are specified to update only part of fields defined in model. I.e. I've got user panel and I want to allow user to change her/his nickname without passing all her/his data to database . In that case imho is better to update only one field ( in updateUserNickName() method ) instead of reading entire user record, update one field, and pass it to database . My solution in this case has better performance: less rules to check , less (not necessary to get current record) and simpler queries , etc.
That's why I wrote PR updated models update() method which allows to validate ONLY fields available in my $data. I think every developer know that it could be dangerous if is using incorrectly.

I know it shouldn't work in that way in insert but I think it should in update if developer specify that he/she know what he/she is doing.

@natanfelles
Copy link
Contributor

I like your approach. This way we will not need to use "if_exist" when we are sure which fields should be updated.

@nowackipawel
Copy link
Contributor Author

I think the same :) There is a way to do exactly the same with if_exist rule (with some bad hacks) but imho approach showed above is better.

@jim-parry jim-parry added the database Issues or pull requests that affect the database layer label Dec 10, 2018
@lonnieezell
Copy link
Member

Looks like the core issue here has already been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Issues or pull requests that affect the database layer
Projects
None yet
Development

No branches or pull requests

4 participants