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

Gii sets wrong model rule for integer field with default value #335

Open
kjusupov opened this issue Feb 19, 2018 · 8 comments
Open

Gii sets wrong model rule for integer field with default value #335

kjusupov opened this issue Feb 19, 2018 · 8 comments
Labels

Comments

@kjusupov
Copy link

What steps will reproduce the problem?

create table parent (id serial primary key, code character varying(5));
insert into parent (code) values ('one');
insert into parent (code) values ('two');

Next, create child table with FK pointing to parent.id

create table child (
    id serial primary key, 
    parent_id integer references parent(id) not null default 1, 
    code character varying(5)
);

insert into child (parent_id, code) values (1, 'c 1');
insert into child (parent_id, code) values (2, 'c 2');
insert into child (code) values ('c 3');   -- parent_id will be defaulted to `1`

Next, generate models via Gii/model

What is the expected result?

    /**
     * @inheritdoc
     */
    public function rules()
    {
        return [
            [['parent_id'], 'default', 'value' => 1], <--- this is what I expected
            [['parent_id'], 'integer'],
            [['code'], 'string', 'max' => 5],
            [['parent_id'], 'exist', 'skipOnError' => true, 'targetClass' => Parent::className(), 'targetAttribute' => ['parent_id' => 'id']],
        ];
    }

What do you get instead?

    /**
     * @inheritdoc
     */
    public function rules()
    {
        return [
            [['parent_id'], 'default', 'value' => null], <--- this is what I get
            [['parent_id'], 'integer'],
            [['code'], 'string', 'max' => 5],
            [['parent_id'], 'exist', 'skipOnError' => true, 'targetClass' => Parent::className(), 'targetAttribute' => ['parent_id' => 'id']],
        ];
    }

Additional info

Q A
Yii version 2.0.13.1
Gii 2.0.6.0
PHP version 7.0
PostgreSQL version PostgreSQL 9.5.9 on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609, 64-bit
Operating system Ubuntu 16.04 LTS
@samdark samdark added type:bug Bug status:to be verified Needs to be reproduced and validated. labels Feb 19, 2018
@samdark samdark added this to the 2.0.7 milestone Feb 19, 2018
@samdark samdark modified the milestones: 2.0.7, 2.0.8 Apr 30, 2018
@sdlins
Copy link
Contributor

sdlins commented Jun 13, 2018

This was added for sake of #224.

@samdark samdark modified the milestones: 2.0.8, 2.1.1 Dec 8, 2018
@samdark samdark modified the milestones: 2.1.1, 2.1.2 Aug 13, 2019
@samdark samdark removed this from the 2.1.2 milestone Nov 19, 2019
@kjusupov
Copy link
Author

I submitted a PR, not sure how to test other than what I did below (manual tests):

My test DB tables:

create table brand (id serial primary key, code character varying(20));
create table country (id serial primary key, code character varying(20));
create table region (id serial primary key, code character varying(20));
create table state (id serial primary key, code character varying(20));

create table car (
    id serial primary key, 
    brand_id integer references brand(id) not null default 1, 
    country_id integer references country(id) not null, 
    region_id integer references region(id), 
    state_id integer references region(id) default 2, 
    original_code character varying(20) not null,
    country_specific_code character varying(20),
    internal_code character varying(20) not null default 'Secret-code',
    external_code character varying,
    media_code character varying(20) not null
);

Current gii output:

public function rules()
    {
        return [
            [['brand_id', 'country_id', 'region_id', 'state_id'], 'default', 'value' => null],
            [['brand_id', 'country_id', 'region_id', 'state_id'], 'integer'],
            [['country_id', 'original_code', 'media_code'], 'required'],
            [['external_code'], 'string'],
            [['original_code', 'country_specific_code', 'internal_code', 'media_code'], 'string', 'max' => 20],
            [['brand_id'], 'exist', 'skipOnError' => true, 'targetClass' => Brand::className(), 'targetAttribute' => ['brand_id' => 'id']],
            [['country_id'], 'exist', 'skipOnError' => true, 'targetClass' => Country::className(), 'targetAttribute' => ['country_id' => 'id']],
            [['region_id'], 'exist', 'skipOnError' => true, 'targetClass' => Region::className(), 'targetAttribute' => ['region_id' => 'id']],
            [['state_id'], 'exist', 'skipOnError' => true, 'targetClass' => Region::className(), 'targetAttribute' => ['state_id' => 'id']],
        ];
    }

After gii code is updated:

public function rules()
    {
        return [
            [['brand_id', 'country_id', 'region_id', 'state_id'], 'integer'],
            [['brand_id'], 'default', 'value' => 1],
            [['state_id'], 'default', 'value' => 2],
            [['country_id', 'original_code', 'media_code'], 'required'],
            [['external_code'], 'string'],
            [['internal_code'], 'default', 'value' => 'Secret-code'],
            [['original_code', 'country_specific_code', 'internal_code', 'media_code'], 'string', 'max' => 20],
            [['brand_id', 'country_id', 'state_id'], 'unique', 'targetAttribute' => ['brand_id', 'country_id', 'state_id']],
            [['brand_id'], 'exist', 'skipOnError' => true, 'targetClass' => Brand::className(), 'targetAttribute' => ['brand_id' => 'id']],
            [['country_id'], 'exist', 'skipOnError' => true, 'targetClass' => Country::className(), 'targetAttribute' => ['country_id' => 'id']],
            [['region_id'], 'exist', 'skipOnError' => true, 'targetClass' => Region::className(), 'targetAttribute' => ['region_id' => 'id']],
            [['state_id'], 'exist', 'skipOnError' => true, 'targetClass' => Region::className(), 'targetAttribute' => ['state_id' => 'id']],
        ];
    }

@samdark samdark removed the status:to be verified Needs to be reproduced and validated. label Nov 20, 2019
@samdark samdark added this to the 2.1.4 milestone Nov 20, 2019
@schmunk42
Copy link
Contributor

If default values are set in the database, why do you need it in your model?

@samdark
Copy link
Member

samdark commented Nov 20, 2019

@schmunk42 the issue was that

 [['parent_id'], 'default', 'value' => null], <--- this is what I get

was generated while default value was actually present in DB.

@schmunk42
Copy link
Contributor

I was referring to those two lines in the patched output

            [['brand_id'], 'default', 'value' => 1],
            [['state_id'], 'default', 'value' => 2],

@uldisn
Copy link
Contributor

uldisn commented Nov 20, 2019

But my view is mirroring full table requirements in model. That allow in validation rules trust on default values. Currently my know problem is with require, but any other problems can occur.

@schmunk42
Copy link
Contributor

If there's a default value in the DB, I think it's debatable whether you want that in your model. If you "just" change it in the DB, you have to regenerate your code, otherwise you might end up in setting the wrong default value in your app.

Even more special is the case if you have a default value and NOT NULL - actually it does not have to be required in the model, since the DB takes care about setting a value.

But I also understand your point, that's why I mentioned making it optional in #420 (comment).

@kjusupov
Copy link
Author

We had the same argument (db would handle default values), and the conclusion was:

  • db Vs model alignment
  • how does it handle null Vs empty string
  • if it's not required to set in the model, mind as well don't set null as default in the model
  • you shouldn't just change in the dB, if there a change, there should be a migration as well make sure to regenerate your model

@samdark samdark removed this from the 2.1.4 milestone Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants