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

[bug] cancelOnFail not working properly since 3.0.4 #13149

Closed
Frost007 opened this issue Nov 8, 2017 · 14 comments

Comments

Projects
8 participants
@Frost007
Copy link

commented Nov 8, 2017

Expected And Actual Behavior

I try to add validation to a form field with "cancelOnFail" attribute.
In 3.0.0 it worked as expected. it stopped the field's validation it self, but in 3.0.4 somehow it stops the whole validation. So i don't get any validation messages from the fields that are placed after the field that has cancelOnFail.

My field validation looks like this.

        $element = new Text('lastName');
        $element->setLabel('user.lastName');
        $element->setFilters([
            "string",
            "striptags",
            "trim",
        ]);
        $element->addValidators([
            new PresenceOf([
                'message'        => 'user.lastName.presenceOf'
                , 'cancelOnFail' => true
            ])
            , new StringLength([
                'min'               => 3,
                'max'               => 255,
                'messageMaximum'    => 'user.lastName.max',
                'messageMinimum'    => 'user.lastName.min'
            ])
        ]);
        $this->add($element);

        $element = new Text('firstName');
        $element->setLabel('user.firstName');
        $element->setFilters([
            "string",
            "striptags",
            "trim",
        ]);
        $element->addValidators([
            new PresenceOf([
                'message'        => 'user.firstName.presenceOf'
                , 'cancelOnFail' => true
            ])
            , new StringLength([
                'min'               => 3,
                'max'               => 255,
                'messageMaximum'    => 'user.firstName.max',
                'messageMinimum'    => 'user.firstName.min'
            ])
        ]);
        $this->add($element);

Lets say i only have these two field in the form. If i don't fill any of them, i should get 2 PresenceOf validation messages. One for 'firstName' and one for 'lastName'. But it's not the case i only get one message for 'lastName' and the cancelOnFail stop the whole validation.

Details

  • Phalcon version: 3.0.4
  • PHP Version: 7.0
  • Operating System: ubuntu 16.04
  • Installation type: Compiling from source
  • Zephir version (if any):
  • Server: Apache
  • Other related info (Database, table schema):
@laffentaller

This comment has been minimized.

Copy link

commented Nov 16, 2017

Same issue here:

PHP Version 7.0.22-0ubuntu0.16.04.1
Phalcon: 3.2.4
Installation type: from deb source (https://packagecloud.io/install/repositories/phalcon/stable/script.deb.sh)
Zephir version: Version 0.10.4-11e39849b0
Server: Apache/2.4.18 (Ubuntu)

@trix87

This comment has been minimized.

Copy link

commented Nov 16, 2017

Hello,

I am having the same issue but with the latest version 3.2.4.
The structure is as in the first post.

Details

Phalcon version: 3.2.4
PHP Version: 7.0
Operating System: Ubuntu 16.04
Installation type: Compiling from source
Zephir version (if any):
Server: Apache/2.4.18 (Ubuntu)
Other related info (Database, table schema):

@sergeyklay sergeyklay self-assigned this Nov 19, 2017

@sergeyklay sergeyklay referenced this issue Nov 19, 2017

Merged

Tests canceling validation on first fail #13167

3 of 3 tasks complete
@sergeyklay

This comment has been minimized.

Copy link
Member

commented Nov 19, 2017

Could you guys provide a minimal script, with which I would reproduce the error.

@Frost007

This comment has been minimized.

Copy link
Author

commented Nov 24, 2017

For example Auth controller has an index action where the user can log in.

public function indexAction(){
        
        $form = new LoginForm();

        if ($this->request->isPost() && $form->isValid($this->request->getPost())) {
       
        }

       $this->view->form = $form;
}

In the auth/index.phtml file:

<form method="post" action="">

    <?php $form->renderDecorated(); ?>

    <input type="submit" value="login">

</form>

The form could be the one that i wrote above. If you leave both of the fields empty than the validation fails and you get the first field validated not both.

@sergeyklay

This comment has been minimized.

Copy link
Member

commented Nov 30, 2017

@Frost007 Did you see #13167

@Frost007

This comment has been minimized.

Copy link
Author

commented Jan 7, 2018

Yes, i saw that. what exactly dou you want me to provide?

@sergeyklay

This comment has been minimized.

Copy link
Member

commented Jan 7, 2018

As you can see the test passed. All works as expected. Could you please create a PR with test which will failed?

@sergeyklay

This comment has been minimized.

Copy link
Member

commented Jan 9, 2018

I'm closing this issue due to the lack of any reaction. I'll open it again if the need arises

@sergeyklay sergeyklay closed this Jan 9, 2018

chilimatic added a commit to chilimatic/cphalcon that referenced this issue Jan 15, 2018

@marcojetson

This comment has been minimized.

Copy link

commented Mar 16, 2018

test expectation is wrong, issue creator says he is expecting 2 PresenceOf, one for firstName and one for lastName

before 3.0.3 Form::isValid() was running Validation::validate() one element at a time so cancelOnFail was cancelling the validators per element
since 3.0.3 this behaviour changed since Form::isValid() runs the validation once for all fields

@laffentaller

This comment has been minimized.

Copy link

commented Apr 19, 2018

@sergeyklay : aggree with @marcojetson we should get two 'PresenceOf' messages in case firstName and lastName both leaved empty.

@sergeyklay sergeyklay reopened this Apr 19, 2018

@sergeyklay

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

@laffentaller Could you please create a PR with failed tests? Feel free use #13167 as example

@magosla

This comment has been minimized.

Copy link

commented Apr 19, 2018

The script below shows how cancelOnFail=true cancels the validation of the remaining unvalidated fields
in the form

Phalcon version: 3.3.2
Server Software: Apache/2.4.6 (CentOS) PHP/7.0.28
Zephir version: Version 0.10.7-2917ebe8ae

use Phalcon\Forms\Element\TextArea,
    Phalcon\Forms\Element\Text,
    Phalcon\Forms\Element\Email,
    Phalcon\Validation\Validator\PresenceOf,
    Phalcon\Validation\Validator\StringLength;

/**
 * Description of ContactForm
 *
 * @author Magnus Lator
 */
class ContactForm extends \Phalcon\Forms\Form {

    public function initialize($entity = null, $options = null)
    {
        $this->fullname();
        $this->email();
        $this->subject();
        $this->message();
    }

    private function email()
    {
        $this->add((new Email('email', []))->setLabel('Email')
                        ->addValidators([
                            new PresenceOf(['message' => 'valid :field is required', 'cancelOnFail' => true]),
        ]));
    }

    private function fullname()
    {
        $this->add((new Text('fullname', ['placeholder' => 'First & Last Name',]))
                        ->setLabel('Full name')
                        ->addValidators([
                            new PresenceOf(['message' => 'your :field is required',
                                'cancelOnFail' => true]),
                            new StringLength(
                                    [
                                'max' => 20, 'messageMaximum' => ':field too long',
                                'min' => 2, 'messageMinimum' => ':field too short'
                                    ]
                            )
        ]));
    }

    private function subject()
    {
        $this->add((new Text('subject'))
                        ->addValidators([new PresenceOf(['message' => 'your :field is required',
                                'cancelOnFail' => true]),])
                        ->setLabel('Subject:')
        );
    }

    private function message()
    {
        $this->add((new TextArea('message'))
                        ->addValidators([
                            new PresenceOf(['message' => 'Your message is required', 'cancelOnFail' => true]),
                            new StringLength(
                                    [
                                'max' => 300, 'messageMaximum' => sprintf('your message can\'t be longer the %s characters', 300),
                                'min' => 4, 'messageMinimum' => 'Tell us what we can do for you', 'cancelOnFail' => true
                                    ]
                            )
                        ])
                        ->setLabel('Message?')
        );
    }

}

$form = new ContactForm();

if (!$form->isValid(['fullname' => '', 'email' => "", 'subject' => '', 'message' => ''])) {
    print_r($form->getMessages());
} else {
    echo "valid data";
}

@stale stale bot added the stale label Jul 18, 2018

@stale stale bot closed this Jul 19, 2018

@sergeyklay sergeyklay reopened this Jul 19, 2018

@stale stale bot removed the stale label Jul 19, 2018

@sergeyklay sergeyklay added this to the unplanned milestone Aug 12, 2018

@sergeyklay sergeyklay assigned Jurigag and unassigned sergeyklay Aug 12, 2018

@niden niden added the Bug - High label Feb 15, 2019

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

@niden

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Also note suggestions in #13540

@niden niden removed this from the unplanned milestone May 16, 2019

@niden niden referenced this issue May 16, 2019

Merged

T13149 cancel on fail #14083

4 of 4 tasks complete

@niden niden added the Breaks BC label May 16, 2019

@niden niden moved this from To do to In progress in 4.0 Release May 16, 2019

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

[#13149] - Adjusted the code to fail on the first `cancelOnFail` vali…
…dator for an element but continue to the rest

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

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

@niden

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Resolved in #14083

@niden niden closed this May 16, 2019

4.0 Release automation moved this from In progress to Done May 16, 2019

@niden niden added the 4.0 label Jun 21, 2019

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.