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

Fixed bug in FormHelper::error() #9105

Merged
merged 3 commits into from Jul 16, 2016
Merged

Fixed bug in FormHelper::error() #9105

merged 3 commits into from Jul 16, 2016

Conversation

oyas
Copy link
Contributor

@oyas oyas commented Jul 12, 2016

In FormHelper::error(), param array $text doesn't work.
Fixed it.

In FormHelper::error(), param array $text doesn't work.
Fixed it.
@ADmad
Copy link
Member

ADmad commented Jul 12, 2016

Please provide test case or example code to reproduce the issue.

@ADmad ADmad added this to the 3.2.13 milestone Jul 12, 2016
@thinkingmedia
Copy link
Contributor

That changes switches $e with the rule identifier and not the error message.

@markstory
Copy link
Member

What does this fix? How does someone reproduce the issue you're having?

@oyas
Copy link
Contributor Author

oyas commented Jul 13, 2016

I'm sorry for my little request message.

I tried to use this param array $text for setting validation error message that was not setted in model.

In CakePHP 2.x, default validation error message is rule name.
However, In CakePHP 3.x, default validation error message is 'The provided value is invalid'.

So, the output is different in spite of the same param $text.

For same output, you must:
In CakePHP2.x, use error() with $text = ['rule name' => 'new error message'].
In CakePHP3.x, use error() with $text = ['The provided value is invalid' => 'new error message'].

I suggest to fix it get same output with $text = [ 'rule name' => 'new error message' ].

Thank you.

Example code:

CakePHP 2.8:

$ cat app/Controller/TestController.php

<?php
App::uses('AppController', 'Controller');

class TestController extends AppController {
    public function a() {
        $this->Test->set($this->request->data);
        $this->Test->validates();
        debug( $this->Test->validationErrors );
    }
}

$ cat app/Model/Test.php

<?php
App::uses('Model', 'AppModel');

class Test extends AppModel {
    public $validate = [
        'field1' => [
            'rule1' => [
                'rule' => 'email',
                'allowEmpty' => true,
            ],
        ],
    ];
}

$ cat app/View/Test/a.ctp

<?= $this->Form->create('Test'); ?>

<?= $this->Form->input('field1'); ?>

<?php $test = ['rule1' => 'new error message']; ?>
<?= $this->Form->error('field1', $test); ?>

<?= $this->Form->submit(); ?>
<?= $this->Form->end(); ?>

result:

debug( $this->Test->validationErrors ) outputs:

array(
    'field1' => array(
        (int) 0 => 'rule1'
    )
)

Form::error() outputs:

new error message

CakePHP 3.2:

$ cat src/Controller/TestController.php

<?php
namespace App\Controller;

use Cake\Controller\Controller;
use Cake\ORM\TableRegistry;

class TestController extends AppController {
    public function a() {
        $Entity = TableRegistry::get('Test')->newEntity($this->request->data);
        debug( $Entity->errors() );
        $this->set('Entity', $Entity);
    }
}

$ cat src/Model/Table/TestTable.php

<?php
namespace App\Model\Table;

use Cake\ORM\Table;
use Cake\Validation\Validator;

class TestTable extends Table {
    public function validationDefault(Validator $validator) {
        $validator
            ->allowEmpty('field1')
            ->add('field1', [
                'rule1' => [
                    'rule' => 'email',
                ]
            ]);
        return $validator;
    }
}

$ cat src/Template/Test/a.ctp

<?= $this->Form->create($Entity); ?>

<?= $this->Form->input('field1'); ?>

<?php $test = ['rule1' => 'new error message']; ?>
<?= $this->Form->error('field1', $test); ?>

<?= $this->Form->submit(); ?>
<?= $this->Form->end(); ?>

result:

debug( $Entity->errors() ) outputs:

[
    'field1' => [
        'rule1' => 'The provided value is invalid'
    ]
]

Form::error() outputs:

The provided value is invalid

@markstory
Copy link
Member

markstory commented Jul 14, 2016

Thanks for providing more context @oyas. This change causes a number of tests to fail which indicates that is it breaking existing behavior.

It looks like the failing tests are preserving the current behavior that you'd like to change. Perhaps you could modify your changes to be compatible with allowing replacement by either the message, or the rule key?

In param $text, you can set the rule key or the message for array key.
The order of priority to apply is 1 rule key, 2 message.
foreach ($error as $k => $e) {
if (isset($text[$k])) {
$tmp[] = $text[$k];
} else if (isset($text[$e])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Usage of ELSE IF is discouraged; use ELSEIF instead
Usage of ELSE IF not allowed; use ELSEIF instead

@markstory
Copy link
Member

Would it be possible to get a test for the new behavior?

@oyas
Copy link
Contributor Author

oyas commented Jul 15, 2016

I modified this change.
And I found that I had mistakes.
If array $text has no replace message, it replace $e(= rule key). It was fixed.

This change is compatible with allowing replacement by either the message, or the rule key.
And the order of priority to apply is:

  1. 'rule key' => 'new error message'
  2. 'error message' => 'new error message'
  3. Otherwise, the error message is not replaced.

Example code:

$ cat src/Template/Test/a.ctp

<?= $this->Form->create($Entity); ?>

<?= $this->Form->input('field1'); ?>

<?php
    $test = [
        'rule1' => 'new error message 1',
        'The provided value is invalid' => 'new error message 2',
    ];
?>
<?= $this->Form->error('field1', $test); ?>

<?= $this->Form->submit(); ?>
<?= $this->Form->end(); ?>

Form::error() outputs:

new error message 1

@codecov-io
Copy link

codecov-io commented Jul 15, 2016

Current coverage is 94.93%

Merging #9105 into master will decrease coverage by <.01%

@@             master      #9105   diff @@
==========================================
  Files           368        368          
  Lines         26989      26994     +5   
  Methods        3232       3232          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          25624      25628     +4   
- Misses         1365       1366     +1   
  Partials          0          0          

Powered by Codecov. Last updated by 5319dd5...7c16fe5

@markstory markstory self-assigned this Jul 15, 2016
@oyas
Copy link
Contributor Author

oyas commented Jul 15, 2016

More detail example:

$ cat src/Template/Test/a.ctp

<?= $this->Form->create($Entity); ?>

<?= $this->Form->input('field1'); ?>

<?php
    $test1 = [
        'rule1' => 'new error message 1',
        'The provided value is invalid' => 'new error message 2',
    ];
    $test2 = [
        'The provided value is invalid' => 'new error message 2',
        'rule1' => 'new error message 1',
    ];
    $test3 = [
        'The provided value is invalid' => 'new error message 2',
    ];
    $test4 = [
    ];
?>
<?= $this->Form->error('field1', $test1); ?>
<?= $this->Form->error('field1', $test2); ?>
<?= $this->Form->error('field1', $test3); ?>
<?= $this->Form->error('field1', $test4); ?>

<?= $this->Form->submit(); ?>
<?= $this->Form->end(); ?>

Form::error() outputs:

new error message 1
new error message 1
new error message 2
The provided value is invalid

@markstory
Copy link
Member

Thanks for the example code @oyas I can turn that into automated regression tests 😄

@markstory markstory merged commit e38cda7 into cakephp:master Jul 16, 2016
markstory added a commit that referenced this pull request Jul 16, 2016
markstory added a commit that referenced this pull request Jul 16, 2016
@markstory
Copy link
Member

Thanks @oyas

@oyas
Copy link
Contributor Author

oyas commented Jul 16, 2016

Thank you very much.

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.

None yet

6 participants