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

model->refresh() should return all defined columns #13781

Closed
cnyyk opened this issue Jan 22, 2019 · 8 comments

Comments

Projects
3 participants
@cnyyk
Copy link

commented Jan 22, 2019

Expected and Actual Behavior

model->refresh() should return all defined columns(fields).

Create a new model, set not null fields, and some fields not assignment any value(using db default value), then save it. and change value, then save it, will throw an exception The record doesn't have a valid data snapshot. Then model->refresh(), and save, PDO exception raised, some fields has default db value, but still notice to set null value: Column 'flag' cannot be null.

SQL schema

DROP TABLE IF EXISTS `demos`;
CREATE TABLE `demos`
(
  `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT COMMENT 'ID',
  `flag` int(10) NOT NULL DEFAULT '1' COMMENT 'Flag',
  `name` varchar(190) COLLATE utf8mb4_unicode_520_ci NOT NULL COMMENT 'Name',
 PRIMARY KEY (`id`) USING BTREE
) ENGINE = InnoDB
  DEFAULT CHARSET = utf8mb4
  COLLATE = utf8mb4_unicode_520_ci COMMENT ='Demos';

Provide minimal script to reproduce the issue

class Demos extends \Phalcon\Mvc\Model {
}
$demo = new Demos();
$demo->name = 'test name';
$result = $demo->save(); // $result === true, everything goes well, record saved into db.

$demo->name = 'new test name';
$result = $demo->save(); // Exception: The record doesn't have a valid data snapshot

$demo->refresh();
$demo->name = 'new test name';
$result = $demo->save(); // Another Exception: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'flag' cannot be null

var_dump($demo->toArray());
// There still only has id and name field after a refresh(). flag field still missing(property does not existed).

MUST DOING THIS WAY TO AVOID EXCEPTION:

$demo = new Demos();
$demo->name = 'test name';
$result = $demo->save(); // $result === true, everything goes well

if ($result) {
  $demo = Demos::findFirst($demo->id);

  $demo->name = 'new test name';
  $result = $demo->save(); // $result === true
}

If some code defined in afterSave() in model, when created then save it:

  1. The record doesn't have a valid data snapshot, must be refresh() it.
  2. Even after refresh() it, when save it, will using null value to update not existed property, and avoided all default value.

SO, IS THERE A WAY TO REAL REFRESH MODEL WAY?

Details

  • Phalcon version: 3.4.2
  • PHP Version: 7.2.14
  • Operating System: macOS 10.14.2
  • Installation type: Compiling from source
  • Zephir version (if any):
  • Server: Apache
  • Other related info (Database, table schema):

@niden niden added the Bug - Medium label Feb 2, 2019

@cnyyk

This comment has been minimized.

Copy link
Author

commented Feb 13, 2019

@niden thanks for you tagging this issue. hope this can be fixed soon.

@niden niden added this to To do in 4.0 Release via automation Feb 15, 2019

@zsilbi

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

I ran the same test using the the 4.0.x branch and the exceptions are gone, refresh() works as intented.

But the validation during the second save() fails when the orm.not_null_validations setting is on, because the default values from the database are not present in the Model attributes after the insert.

We could copy the unset default values from the MetaData to the Model after a successful insert, like I did it here: zsilbi@a0c3184, or should we make the validation aware of these defaults?
Currently the validation in _preSave() only allows this when the record doesn't exist.

@cnyyk

This comment has been minimized.

Copy link
Author

commented May 17, 2019

@zsilbi Thanks for your great work on this framework.
I think let db default value as the field value should be better after refresh().
In most circumstances, db field will define a default value for better performance(at least with MySQL).
So refresh() to retrieve all db default values for all fields is what we expected.

@zsilbi

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

In most circumstances, db field will define a default value for better performance(at least with MySQL).
So refresh() to retrieve all db default values for all fields is what we expected.

@cnyyk the default values are present in the MetaData cache even before the insert happens, so we wouldn't really need to refresh()

@niden

This comment has been minimized.

Copy link
Member

commented May 17, 2019

@zsilbi I think the best approach on this would be to copy the unset default values from the MetaData to the Model as you mentioned above. This will solve this issue.

Do you want to do this or should I?

@zsilbi

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Do you want to do this or should I?

@niden I am on it, thanks 👍

zsilbi pushed a commit to zsilbi/cphalcon that referenced this issue May 17, 2019

niden added a commit that referenced this issue May 17, 2019

@niden

This comment has been minimized.

Copy link
Member

commented May 17, 2019

Resolved in #14088

@niden niden closed this May 17, 2019

4.0 Release automation moved this from To do to Done May 17, 2019

@cnyyk

This comment has been minimized.

Copy link
Author

commented May 18, 2019

Thanks again for your great work.
As you mentioned above, the best way is copy all default value from metadata, for less db read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.