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 rules for default values #420

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Model rules for default values #420

wants to merge 20 commits into from

Conversation

uldisn
Copy link
Contributor

@uldisn uldisn commented Nov 16, 2019

Q A
Is bugfix? yes
New feature? yes
Breaks BC? no
Tests pass? yes
Fixed issues -

Bug

CREATE TABLE `contract` (
  `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `contract_number` varchar(50)  NOT NULL DEFAULT 'NoNumber' COMMENT 'Number',
  `contract_date` date DEFAULT NULL COMMENT 'Contract date',
  `contract_place` varchar(250) DEFAULT NULL COMMENT 'Contract place',
  `notes` text COMMENT 'Notes',
  PRIMARY KEY (`id`)
) ENGINE=InnoDB AUTO_INCREMENT=1 DEFAULT CHARSET=utf8

rules

    /**
     * @inheritdoc
     */
    public function rules()
    {
        return [
            [['contract_number'], 'required'],
            [['contract_date'], 'safe'],
            [['notes'], 'string'],
            [['contract_number'], 'string', 'max' => 50],
            [['contract_place'], 'string', 'max' => 250],
        ];
    }

If in rules no defined default value for field contract_number and not initiated, validation fired on require rule.

Example of generated rules:

    /**
     * {@inheritdoc}
     */
    public function rules()
    {
        return [
            [['name', 'address', 'profile_id'], 'default', 'value' => null],
	    [['status'], 'integer', 'default', 'value' => 0],
            [['email'], 'required'],
            [['address'], 'string'],
            [['status', 'profile_id'], 'integer'],
            [['email', 'name'], 'string', 'max' => 128],
        ];
    

uldisn and others added 14 commits November 11, 2019 11:56
Signed-off-by: uldisn <uldisnelsons@gmail.com>
Signed-off-by: uldisn <uldisnelsons@gmail.com>
Signed-off-by: uldisn <uldisnelsons@gmail.com>
Signed-off-by: uldisn <uldisnelsons@gmail.com>
Fix yiisoft#416: Improved generation of model attributes and type annotations
Signed-off-by: uldisn <uldisnelsons@gmail.com>
Signed-off-by: uldisn <uldisnelsons@gmail.com>
Signed-off-by: uldisn <uldisnelsons@gmail.com>
@samdark samdark added the pr:missing usecase It is not clear what is the use case for the pull request. label Nov 18, 2019
@yii-bot
Copy link

yii-bot commented Nov 18, 2019

Thank you for putting effort in the improvement of the Yii framework.
We have reviewed your pull request.

Unfortunately a use case is missing. It is required to get a better understanding of the pull request and helps us to determine the necessity and applicability of the suggested change to the framework.

Could you supply us with a use case please? Please be as detailed as possible and show some code!

Thanks!

This is an automated comment, triggered by adding the label pr:missing usecase.

@schmunk42
Copy link
Contributor

I think this should either be enabled by a parameter or done in a 2.2.x release.

@uldisn
Copy link
Contributor Author

uldisn commented Nov 18, 2019

Realy is bug! See description.

@samdark samdark added type:bug Bug and removed pr:missing usecase It is not clear what is the use case for the pull request. labels Nov 18, 2019
@uldisn uldisn changed the title To model rules add default values Model rules for default values Nov 18, 2019
@samdark samdark requested a review from a team November 19, 2019 08:36
Copy link
Member

@samdark samdark left a comment

Choose a reason for hiding this comment

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

👍

src/generators/model/Generator.php Outdated Show resolved Hide resolved
src/generators/model/Generator.php Outdated Show resolved Hide resolved
src/generators/model/Generator.php Outdated Show resolved Hide resolved
src/generators/model/Generator.php Outdated Show resolved Hide resolved
@samdark samdark added the status:under development Someone is working on a pull request. label Nov 19, 2019
…NT_TIMESTAMP

Signed-off-by: uldisn <uldisnelsons@gmail.com>
@samdark samdark added status:code review The pull request needs review. and removed status:under development Someone is working on a pull request. labels Nov 19, 2019
@samdark samdark added this to the 2.1.2 milestone Nov 19, 2019
@samdark samdark removed the status:code review The pull request needs review. label Nov 19, 2019
@samdark samdark added the status:under development Someone is working on a pull request. label Nov 19, 2019
@schmunk42
Copy link
Contributor

I don't get it from the description, but apparently @samdark does, so it's fine 😄

Signed-off-by: uldisn <uldisnelsons@gmail.com>
@samdark samdark added status:under discussion and removed status:under development Someone is working on a pull request. labels Nov 19, 2019
@samdark
Copy link
Member

samdark commented Nov 19, 2019

Thinking more about it... @uldisn what's the use case for having explicit default rules if they match database ones? As far as I know, database silently uses default when value is not provided anyway...

@uldisn
Copy link
Contributor Author

uldisn commented Nov 19, 2019

Sometimes rules is fired, if not set default values. For example, if field required ( not null), in database is defined default value and in model not set. Then rule required fired.
For optimal indexing not null is very actual, but usually using default CURRENT_TIMESTAMP fired on require rule.

@samdark
Copy link
Member

samdark commented Nov 19, 2019

Isn't it better not to generate required rule in this case?

@uldisn
Copy link
Contributor Author

uldisn commented Nov 19, 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 other problems can occur.

For tuning masters yes. They can do it ( not to generate required rule in this case).

Additionally i see follow improvements:

  • default value define as model constant
  • create method "loadDefaultValues()" - useful for new record before showing in form.

@uldisn uldisn requested review from samdark and removed request for a team November 19, 2019 11:18
@samdark
Copy link
Member

samdark commented Nov 19, 2019

Currently my know problem is with require, but other problems can occur.

Any examples?

create method "loadDefaultValues()" - useful for new record before showing in form.

https://www.yiiframework.com/doc/api/2.0/yii-db-activerecord#loadDefaultValues()-detail is already there and works without any extra rules.

@samdark samdark removed this from the 2.1.2 milestone Nov 19, 2019
@uldisn
Copy link
Contributor Author

uldisn commented Nov 20, 2019

On issue #335 is problem

@samdark
Copy link
Member

samdark commented Nov 20, 2019

#335 seems to have another pull request https://github.com/yiisoft/yii2-gii/pull/421/files

@uldisn
Copy link
Contributor Author

uldisn commented Nov 20, 2019

Solutions:

  • database fields with not null and default value not include in require rule
  • create full list rules for default values
    • work correctly in any situation
    • huge code
  • create full list rules for default values excluding null default value
    • work correctly in any situation
    • no so huge code

@samdark samdark added this to the 2.1.4 milestone Nov 20, 2019
Signed-off-by: uldisn <uldisnelsons@gmail.com>
This reverts commit c83843f

Signed-off-by: uldisn <uldisnelsons@gmail.com>
@samdark samdark removed this from the 2.1.4 milestone Jan 17, 2020
@uldisn
Copy link
Contributor Author

uldisn commented Jan 17, 2020

Generation default value rules cover follow problems:

Only need adapt for PostgrSQL

@WinterSilence
Copy link
Contributor

WinterSilence commented Mar 8, 2021

that's magic in yii confuse me - why not define properties with defaults from db table?

class Contact extends ActiveRecord
{
    /**
     * @var int
     */
    public $id;

    /**
     * @var string Number
     */
    public $contract_number = 'NoNumber';

    ...
}
/**
 * @var {type} {comment}
 */
public ${column} ={default};

case Schema::TYPE_DATETIME:
case Schema::TYPE_TIMESTAMP:
if(strtoupper($column->defaultValue) === 'CURRENT_TIMESTAMP'){
$defaultValue = 'date(\'Y-m-d H:i:s\')';
Copy link
Contributor

@WinterSilence WinterSilence Apr 10, 2021

Choose a reason for hiding this comment

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

mssql/sql server:

DEFAULT definitions cannot be created on columns with a timestamp data type or columns with an IDENTITY property.

https://docs.microsoft.com/ru-ru/sql/t-sql/statements/create-table-transact-sql?view=sql-server-ver15

also it must be $defaultValue = 'time()';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For timestamp column, if default value not set, fired on require rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

@uldisn

if (strtoupper($column->defaultValue) === 'CURRENT_TIMESTAMP') {
     if ($driverName === 'mssql') {
          $defaultValue = $this->generateString($column->defaultValue);
     } else {
        $defaultValue = 'time()';
    }
} 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On MSSQL fired rule, who validate actual field value - time format.

Copy link
Contributor

Choose a reason for hiding this comment

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

@uldisn yes, my fail

if (strtoupper($column->defaultValue) === 'CURRENT_TIMESTAMP') {
     if ($driverName === 'mysql' && $column->type === Schema::TYPE_TIMESTAMP) {
          $defaultValue = 'time()';
     } else {
        $defaultValue = "date('Y-m-d H:i:s')";
    }
}

@uldisn
Copy link
Contributor Author

uldisn commented Apr 12, 2021

that's magic in yii confuse me - why not define properties with defaults from db table?

class Contact extends ActiveRecord
{
    /**
     * @var int
     */
    public $id;

    /**
     * @var string Number
     */
    public $contract_number = 'NoNumber';

    ...
}
/**
 * @var {type} {comment}
 */
public ${column} ={default};

For example, if default value is timestamp, it cannot set to property.

@WinterSilence
Copy link
Contributor

@uldisn why? timestamp is integer

$rules = [];
$driverName = $this->getDbDriverName();

if (in_array($driverName, ['mysql', 'sqlite'], true)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why only that's drivers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know only MySql.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sure just about MySql

Copy link
Contributor

@WinterSilence WinterSilence Apr 12, 2021

Choose a reason for hiding this comment

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

@uldisn that's types normalized check DB subfolders, also:

/**
 * @var string abstract type of this column. Possible abstract types include:
 * char, string, text, boolean, smallint, integer, bigint, float, decimal, datetime,
 * timestamp, time, date, binary, and money.
 */
public $type;
/**
 * @var string the PHP type of this column. Possible PHP types include:
 * `string`, `boolean`, `integer`, `double`, `array`.
 */
public $phpType;

as you can see, $phpType use only 5 types, you can detect numeric as if (in_array($column->phpType, ['integer', 'double', 'float'], true)) 'float' added because php dont have type 'double', i think it's used to set validation rule


foreach ($table->columns as $column) {

if ($column->defaultValue !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if ($column->defaultValue === null && !$column->allowNull) { continue; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not need "&& !$column->allowNull"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor

Choose a reason for hiding this comment

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

@uldisn no need $columnsDefaultNull set if ($column->allowNull) {$defaultValue = 'null';}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

by default all attributes has null value. Not need.

Copy link
Contributor

@WinterSilence WinterSilence Apr 12, 2021

Choose a reason for hiding this comment

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

@uldisn null !== 'null', echo null or echo false don't print null or false, in my packages I use <?=json_encode($var)?>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked MySql schema. If in MySql field no default value, then $column->defaultValue is null.
$column->allowNull use for rules "required"
Correct:

 if ($column->defaultValue === null) { continue; }

Copy link
Contributor

@WinterSilence WinterSilence Apr 12, 2021

Choose a reason for hiding this comment

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

@uldisn $column->allowNull is @param type|null $foo, function setFoo(?type $value) and function getFoo(): ?type.

If in MySql field no default value, then $column->defaultValue is null.

CREATE TABLE `demo` (`a` VARCHAR(255) NULL, `b` TEXT NULL);
INSERT INTO `demo` (`a`) VALUES (NULL); // (NULL, NULL)
CREATE TABLE ` demo` (
    `id` INT(11) UNSIGNED NOT NULL AUTO_INCREMENT,
    `a` TEXT NULL DEFAULT NULL,
    `b` VARCHAR(50) NULL DEFAULT 'is default'
    PRIMARY KEY (`id`) USING BTREE
) ENGINE=InnoDB;

INSERT INTO ` demo` (`a`, `b`) VALUES ('...', NULL);
INSERT INTO ` demo` (`a`, `b`) VALUES (NULL, '...');
INSERT INTO `demo` (`b`) VALUES (NULL);
INSERT INTO `demo` (`a`) VALUES ('...');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants