Use foreign keys in validation rules

[saeideng]

Refs cakephp/cakephp#10355 cc @markstory

Revert #62 cc @markstory
old issue #61 cc @lorenzo

[codecov-io]

Codecov Report

Merging #327 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #327      +/-   ##
============================================
- Coverage     95.64%   95.63%   -0.02%     
+ Complexity      591      588       -3     
============================================
  Files            23       23              
  Lines          1884     1878       -6     
============================================
- Hits           1802     1796       -6     
  Misses           82       82

Impacted Files	Coverage Δ	Complexity Δ
src/Shell/Task/ModelTask.php	94.69% <100%> (-0.07%)	143 <0> (-3)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bfa50d8...938705e. Read the comment docs.

[markstory]

We also have the allowNullableNulls option now. Instead of reverting the earlier change, could bake enable this option when a nullable foreign key is found?

[saeideng]

generated for null schema

        $validator
            ->integer('product_id')
            ->allowEmpty('product_id', 'create');

for not null schema

        $validator
            ->integer('product_id')
            ->requirePresence('product_id', 'create')
            ->notEmpty('product_id');

[saeideng]

i think this change not related to allowNullableNulls option

[saeideng]

probably you say bake should bake like :

$rules->add($rules->existsIn(['parent_id', 'site_id'],'ParentNodes',['allowNullableNulls' => false]));

[markstory]

Right now we don't generate any validators for foreign keys, and instead rely on the ExistsIn rules. Enabling allowNullableNulls should allow null to be accepted as a valid value. From what I've understood saving nulls, which currently causes rules failures is the problem. I may be misunderstanding the problem though.

[saeideng]

problem is not allowEmpty or notEmpty rule
problem is requirePresence rule

[saeideng]

CREATE TABLE IF NOT EXISTS `comments` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `user_id` int(11) NOT NULL,
  `title` varchar(1000) NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB  DEFAULT CHARSET=utf8 ;

    public function validationDefault(Validator $validator)
    {
        $validator
            ->integer('id')
            ->allowEmpty('id', 'create');


        $validator
            ->requirePresence('title', 'create')
            ->notEmpty('title');

        return $validator;
    }

    public function buildRules(RulesChecker $rules)
    {
        $rules->add($rules->existsIn(['user_id'], 'Users'));

        return $rules;
    }
}

//controller
		$data=[
			'title'=>'new title',
			//'user_id'=>1 // user_id not present 
		];
		$entity=$this->Comments->newEntity($data);
		$this->Comments->save($entity);

BEGIN
INSERT INTO comments (title) 
VALUES 
  ('new title')
COMMIT

//database
id=an Id
user_id=0 //invalid
title="new title"

result of
$rules->add($rules->existsIn(['user_id'], 'Users'));
or
$rules->add($rules->existsIn(['user_id'], 'Users',['allowNullableNulls' => false]));
or
$rules->add($rules->existsIn(['user_id'], 'Users',['allowNullableNulls' => true]));
is some

we should validate it in validationDefault()

table:save() should be return false because user_id is NOT NULL but ... 😟

allowNullableNulls is not related to this problem

[saeideng]

and instead rely on the ExistsIn rules.

i think no, is not true
these lines executed in my exam
https://github.com/cakephp/cakephp/blob/master/src/ORM/Rule/ExistsIn.php#L109-L111

[saeideng]

Mentioned lines in core is invalid
That lines applyed in both save ,update
We can fix it for all programs (old,new) by fixing existsIn()
Or fixing bake for new programs
i think validator is good place to validation ,what you think?

[saeideng]

I think i found another problem in core that make conflict with this PR
We can not use inline setter like bellow code after merging this PR !?

$entity=patchEntity(....);
$entity->user_id=1;

I want to resolve not present field in core ,closing until that time

[saeideng]

Error will remove in setDirty after inline set
https://github.com/cakephp/cakephp/blob/master/src/Datasource/EntityTrait.php#L761
Reopen until found a way

[markstory]

Ok, I was under the impression you were having problems with nullable foreign keys. Not required foreign key columns. The code you linked to in ExistsIn cannot be removed as it will cause breakage in existing applications. It sounds like we should at least be adding the requirePresence('create') validation rules, which is part of what this pull request is doing.

I'm not sure we need to also generate the unique validators when the foreign key columns also have SQL constraints though. The existsIn rule should be enough, and I think was the problematic part we wanted to remove in #61

[saeideng]

The code you linked to in ExistsIn cannot be removed as it will cause breakage in existing application.

I want to fix it instead of removing for old programs or programs that not using bake
I have an idea that i will open PR as soon

[saeideng]

I think you can merge this
Or you need just adding requirePresence() rule ?

[markstory]

@saeideng Right now bake will start re-adding validateUnique validators for foreign keys that have SQL constraints. I don't think that is desirable based on the confusion it caused earlier.

[saeideng]

ping

some of people not using foreign key constraints ,because cakephp handle it

[dereuromark]

@markstory What shall we do here?

[markstory]

Is the difference with this implementation vs the old one that keys can be blank on create?

[saeideng]

looks for null able column validation is not necessary , but for not null is important

[saeideng]

we can remove validation when foreign key was null able
but for not null column we should add bellow validation IMO

        $validator
            ->requirePresence('category_id', 'create')
            ->notEmpty('category_id');
            //or
            ->notEmpty('category_id',  'create');

[dereuromark]

needs rebase.

[dereuromark]

@saeideng Shall we finish this PR?

[dereuromark]

Also refs cakephp/migrations#385

[dereuromark]

@saeideng Are u still interested in finishing this up?

[saeideng]

let me to check again whole changes in next few days

[dereuromark]

@saeideng ping
We should close otherwise.

[saeideng]

I think it is valid yet , but whole PR needs to re create with change on command class
I want to close now maybe until next concrete reason