Form validation some fixes #12466

mbrostami · 2016-12-09T18:37:40Z

Hello!

Type: bug fix
Link to issues (this will fix all of the below issues):
Form setValidation is not work correctly #12465
[2.1.x] $form->isValid() doesn't merge validators (separate Validation class) #11500
Form validation for field not possible #11135
Phalcon\Forms\Element hasMessages() always returns false #3167

In raising this pull request, I confirm the following (please check boxes):

I have read and understood the Contributing Guidelines?
I have checked that another pull request for this purpose does not exist.
I wrote some tests for this PR.

Small description of change:
setValidation for form is not work correctly as I explained at #12465.
Elements messages doesn't set if custom validation fails.

Thanks

sergeyklay · 2016-12-09T19:21:11Z

phalcon/forms/form.zep

+
+        if !(validation instanceof Validation) {
+            // Create an implicit validation
+            validation = new Validation();


The let statement is used to mutate variables, properties and arrays. Variables are by default immutable and this instruction makes them mutable:

let validation = new Validation();

sergeyklay · 2016-12-09T19:21:34Z

phalcon/forms/form.zep

+
+        let validation = this->getValidation();
+
+        if !(validation instanceof Validation) {


here does not need braces

sergeyklay · 2016-12-09T19:23:30Z

phalcon/forms/form.zep

+            for elementMessage in messages {
+                this->get(elementMessage->getField())->appendMessage(elementMessage);
+            }
+            validationStatus = false;


let validationStatus = false;

sergeyklay · 2016-12-09T20:15:21Z

phalcon/forms/form.zep

+
+        if !(validation instanceof Validation) {
+            // Create an implicit validation
+            let validation = new Validation();


Well, I think I misled you. The exclamation point has a higher priority. So you should use:

if !(validation instanceof Validation) {

But actually I think that more flexibility was to use the interface but not concrete class

Yeah! You're right. Thanks for comments.

mbrostami · 2016-12-10T21:38:35Z

phalcon/forms/form.zep

-			if typeof messages != "array" {
-				return new Group();
-			}
-			return messages;


I think, returning multiple type of value is not a good idea (see line 378 old - return type).

sergeyklay · 2016-12-10T21:46:21Z

@mbrostami Could you please split this PR into parts: one that can be merged to the 3.0.x branch without backward compatibility breaking and another one that need to be sent to the 4.0.x branch. Also I would like to see a more detailed explanation of your changes.

Thanks

mbrostami · 2016-12-10T21:46:24Z

phalcon/forms/form.zep

@@ -712,7 +684,11 @@ class Form extends Injectable implements \Countable, \Iterator
 	public function rewind() -> void
 	{
 		let this->_position = 0;
-		let this->_elementsIndexed = array_values(this->_elements);


If you create a form without any element, and iterate that, php warning will be shown, because this->_elements is null. I added some code to prevent that. https://github.com/phalcon/cphalcon/blob/3.0.x/tests/unit/Forms/FormTest.php#L77

mbrostami · 2016-12-10T22:09:05Z

I think using _messages as a single type (Group|null) would be better, and will avoid of complexity.
In old code, you can see _messages as an array and somewhere else as a Group object!
Other issue was about a bug for getMessagesFor method, which didn't return correct value(#11135). So inside isValid method, after validation, setting error messages to each element fixes this issue and also after that you can get each element messages from this->get(elementName)->getMessages() and doesn't need to iterate _messages.(would be cleaner code I think).
Other issue is injecting CustomValidation to a Form by setValidation method, it didn't work, now is fixed. (#12465 I don't know if it was a bug or not!).

sergeyklay

Cc: @niden @andresgutierrez Could you please take a look

sergeyklay · 2016-12-11T19:47:58Z

@mbrostami Could you please rebase, update CHANGELOG.md and cover issues by small test you successfully solved

mbrostami · 2016-12-13T07:13:18Z

@sergeyklay Sure.

mbrostami · 2016-12-13T09:31:04Z

@sergeyklay can you please check appveyor error? Is it my bad!?

Jurigag · 2016-12-13T09:34:41Z

Inner Exception: Unable to connect to the remote server no

mbrostami · 2016-12-13T18:34:38Z

This issue (#12395) will also be fixed by this PR.

sergeyklay

Could you please take a look at comment for getMessagesFor and update CHANGELOG.md by adding last issue.

Thanks

sergeyklay · 2016-12-13T21:54:50Z

phalcon/forms/form.zep

-			this->_messages[name] = group;
-
-		return group;
+        return this->get(name)->getMessages();


Well, in general looks good to me. But it bothers me that previously this method always returned the Group but now it can throw Exception. I think this behaviour should be the same now.

Right, It should be backward compatible, I missed that.

mbrostami · 2016-12-14T08:04:29Z

@sergeyklay It's ready.

sergeyklay · 2016-12-18T23:35:42Z

@mbrostami Could you please rebase onto 3.0.x

mbrostami · 2016-12-20T12:23:16Z

@sergeyklay Done

Jurigag · 2016-12-20T13:06:06Z

Squash please.

sergeyklay · 2016-12-20T15:04:33Z

Thanks

sergeyklay suggested changes Dec 9, 2016

View reviewed changes

sergeyklay added this to the 3.0.3 milestone Dec 9, 2016

mbrostami force-pushed the validation-fix branch from b4e9364 to 16f36ac Compare December 9, 2016 20:14

sergeyklay reviewed Dec 9, 2016

View reviewed changes

mbrostami force-pushed the validation-fix branch 2 times, most recently from 2919cda to ce6440b Compare December 9, 2016 20:18

mbrostami commented Dec 10, 2016

View reviewed changes

sergeyklay reviewed Dec 11, 2016

View reviewed changes

mbrostami force-pushed the validation-fix branch from e43345e to 0d43455 Compare December 13, 2016 08:19

sergeyklay suggested changes Dec 13, 2016

View reviewed changes

sergeyklay approved these changes Dec 18, 2016

View reviewed changes

mbrostami added 7 commits December 20, 2016 11:27

fix form isValid method

3221d00

fix zephir syntax erros

ace3ded

fix iterator - fix getMessageFor - unused getMessages byItemName

8d22a60

changelog + add unit tests

399c0aa

add test validation messages equals to form message

885950a

prevent exception in getMessagesFor

c6b4bfe

add test getMessagesFor for non existing element

6082da3

mbrostami force-pushed the validation-fix branch from 146549a to 6082da3 Compare December 20, 2016 08:00

sergeyklay merged commit c505c68 into phalcon:3.0.x Dec 20, 2016

mbrostami deleted the validation-fix branch December 20, 2016 18:40

sergeyklay self-assigned this Dec 24, 2016

sergeyklay mentioned this pull request Feb 5, 2018

Broken behavior of the Phalcon\Forms\Form::getMessages #13294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Form validation some fixes #12466

Form validation some fixes #12466

mbrostami commented Dec 9, 2016 •

edited by sergeyklay

Loading

sergeyklay Dec 9, 2016 •

edited

Loading

sergeyklay Dec 9, 2016

sergeyklay Dec 9, 2016

sergeyklay Dec 9, 2016 •

edited

Loading

mbrostami Dec 9, 2016

mbrostami Dec 10, 2016

sergeyklay commented Dec 10, 2016 •

edited

Loading

mbrostami Dec 10, 2016

mbrostami commented Dec 10, 2016

sergeyklay left a comment

sergeyklay commented Dec 11, 2016

mbrostami commented Dec 13, 2016

mbrostami commented Dec 13, 2016

Jurigag commented Dec 13, 2016

mbrostami commented Dec 13, 2016

sergeyklay left a comment

sergeyklay Dec 13, 2016

mbrostami Dec 14, 2016

mbrostami commented Dec 14, 2016 •

edited

Loading

sergeyklay commented Dec 18, 2016

mbrostami commented Dec 20, 2016

Jurigag commented Dec 20, 2016

sergeyklay commented Dec 20, 2016


		let validation = this->getValidation();

		if !(validation instanceof Validation) {

Form validation some fixes #12466

Form validation some fixes #12466

Conversation

mbrostami commented Dec 9, 2016 • edited by sergeyklay Loading

sergeyklay Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

sergeyklay Dec 9, 2016

Choose a reason for hiding this comment

sergeyklay Dec 9, 2016

Choose a reason for hiding this comment

sergeyklay Dec 9, 2016 • edited Loading

Choose a reason for hiding this comment

mbrostami Dec 9, 2016

Choose a reason for hiding this comment

mbrostami Dec 10, 2016

Choose a reason for hiding this comment

sergeyklay commented Dec 10, 2016 • edited Loading

mbrostami Dec 10, 2016

Choose a reason for hiding this comment

mbrostami commented Dec 10, 2016

sergeyklay left a comment

Choose a reason for hiding this comment

sergeyklay commented Dec 11, 2016

mbrostami commented Dec 13, 2016

mbrostami commented Dec 13, 2016

Jurigag commented Dec 13, 2016

mbrostami commented Dec 13, 2016

sergeyklay left a comment

Choose a reason for hiding this comment

sergeyklay Dec 13, 2016

Choose a reason for hiding this comment

mbrostami Dec 14, 2016

Choose a reason for hiding this comment

mbrostami commented Dec 14, 2016 • edited Loading

sergeyklay commented Dec 18, 2016

mbrostami commented Dec 20, 2016

Jurigag commented Dec 20, 2016

sergeyklay commented Dec 20, 2016

mbrostami commented Dec 9, 2016 •

edited by sergeyklay

Loading

sergeyklay Dec 9, 2016 •

edited

Loading

sergeyklay Dec 9, 2016 •

edited

Loading

sergeyklay commented Dec 10, 2016 •

edited

Loading

mbrostami commented Dec 14, 2016 •

edited

Loading