View Issue Details

IDProjectCategoryView StatusLast Update
0000166Adventure PHP FrameworkNeues Feature // New Featurepublic2015-10-12 12:20
Reporteruser15Assigned ToChristianAchatz 
PrioritynormalSeverityfeatureReproducibilityalways
Status closedResolutionfixed 
Product Version2.0 
Target Version2.1Fixed in Version2.1 
Summary0000166: AbstractFormControl::addValidator() should add validators to input fields, not execute the validation
DescriptionValidators are currently "added" (in fact they are executed, not added) via AbstractFormControl::addValidator(). After the call of this method the validator can never be accessed again.

This is no problem if the validators are assigned via "addvalidator" tags in a static APF template, but if one wants to add validators dynamically to an input field they are "lost" instantly after the "addValidator" call.

Suggestion:
- addValidator() adds a validator object to the validator chain (basically an object of AbstractFormValidator objects or - better - some kind of "ValidatorChain" object) of an AbstractFormControl
- AbstractFormControl::isValid iterates over the validator array and calls the validate() method on the current value of the input field (or directly calls the validate() method of the "ValidatorChain" object which will then execute the underlying validators)

The suggested implementation has some advantages:
- addValidator() would do what one would expect (adding a validator, not doing the validation)
- the validator objects would stay accessible after the addValidator() call, the developer could do something with those objects e.g. inside the transform() method of a custom FormTag object (e.g. re-using the information saved in the validators to generate code for client-side validation)
Tagsform, patch, validation
Codereferenz: ([Datei]:[Zeile])
Namespacetools

Relationships

related to 0000232 closedChristianAchatz AbstractFormControl::addValidator() should add validators to input fields only to allow form manipulation w/ controllers 

Activities

user15

2014-03-04 15:28

  ~0000244

Last edited: 2014-03-04 15:29

View 2 revisions

diff --git a/vendor/APF/tools/form/taglib/AbstractFormControl.php b/vendor/APF/tools/form/taglib/AbstractFormControl.php
index 70629d4..bf6bfc2 100644
--- a/vendor/APF/tools/form/taglib/AbstractFormControl.php
+++ b/vendor/APF/tools/form/taglib/AbstractFormControl.php
@@ -55,6 +55,11 @@ abstract class AbstractFormControl extends Document implements FormControl {
       'contenteditable', 'contextmenu', 'dir', 'draggable', 'dropzone');

    /**
+    * @var AbstractFormValidator[]
+    */
+   protected $validators = [];
+
+   /**
     * @since 1.11
     * @var boolean Indicates, whether the form control is valid or not.
     */
@@ -120,6 +125,15 @@ abstract class AbstractFormControl extends Document implements FormControl {
     * Version 0.1, 25.08.2009
     */
    public function isValid() {
+      $value = $this->getValue();
+      foreach ($this->validators as $validator) {
+         if ($validator->isActive() && $this->isMandatory($value)) {
+            if (!$validator->validate($value)) {
+               $validator->notify();
+            }
+         }
+      }
+
       return $this->controlIsValid;
    }

@@ -348,12 +362,7 @@ abstract class AbstractFormControl extends Document implements FormControl {
     * Version 0.2, 01.11.2010 (Added support for optional validators)
     */
    public function addValidator(AbstractFormValidator &$validator) {
-      $value = $this->getValue();
-      if ($validator->isActive() && $this->isMandatory($value)) {
-         if (!$validator->validate($value)) {
-            $validator->notify();
-         }
-      }
+      $this->validators[] = $validator;
    }

    /**


ChristianAchatz

2014-03-04 17:25

administrator   ~0000253

Last edited: 2014-03-04 18:52

View 2 revisions

@Andreas: thanks for providing a patch!

Change requires also changing the filter definition and execution model. E.g. addFilter() also adds to a filter chain and isValid() both triggers filtering and validation. This has to be changed anyway with the new APF parser coming up in 2.2 (see 0000143) since the parser slightly changes the document analysis order.

Besides, validation should be executed for both directly requesting $form->isValid() as well as just displaying/rendering the form via $form->transformOnPlace(). This might require a different concept (e.g. explicit validation trigger in $form->transformForm()).

ChristianAchatz

2014-03-27 22:59

administrator   ~0000282

As discusses in person, I originally wanted to go for collecting filters and validators within the form. This is no longer a good idea, since

- the API change is large and
- validators and filters added in document controllers will get us into trouble (again 2 mechanisms for executing validators needed as discussed for $form->isValid() vs. $form->transformOnPlace()).

Thus I revised the current implementation and did 2 implementations comparing API changes (and thus migration effort) vs. usability and benefit.

==> As your proposal (retrieving validators and filters directly from the control) both solves the validation timing issue and lets you define/add validators and filters dynamically within tags and controllers, I must admit it's the better solution. ;)

Another advantage is, that I'll add the feature to 2.1 already as it doesn't require the 2.2 parser any more.

ChristianAchatz

2014-03-27 23:03

administrator   ~0000283

Last edited: 2014-03-27 23:04

View 2 revisions

* Code checked in. Snapshot available under http://files.adventure-php-framework.org/snapshot/apf-2.1-snapshot-php5.tar.gz in +1h.
* Docs to be written.

Issue History

Date Modified Username Field Change
2014-03-03 17:59 user15 New Issue
2014-03-03 18:00 user15 Tag Attached: form
2014-03-03 18:00 user15 Tag Attached: validation
2014-03-04 15:28 user15 Note Added: 0000244
2014-03-04 15:29 user15 Note Edited: 0000244 View Revisions
2014-03-04 15:30 user15 Tag Attached: patch
2014-03-04 17:17 ChristianAchatz Assigned To => ChristianAchatz
2014-03-04 17:17 ChristianAchatz Status new => acknowledged
2014-03-04 17:17 ChristianAchatz Status acknowledged => confirmed
2014-03-04 17:17 ChristianAchatz Target Version => 3.0
2014-03-04 17:25 ChristianAchatz Note Added: 0000253
2014-03-04 18:52 ChristianAchatz Note Edited: 0000253 View Revisions
2014-03-27 22:59 ChristianAchatz Note Added: 0000282
2014-03-27 23:02 ChristianAchatz Target Version 3.0 => 2.1
2014-03-27 23:03 ChristianAchatz Note Added: 0000283
2014-03-27 23:03 ChristianAchatz Status confirmed => resolved
2014-03-27 23:03 ChristianAchatz Fixed in Version => 2.1
2014-03-27 23:03 ChristianAchatz Resolution open => fixed
2014-03-27 23:04 ChristianAchatz Note Edited: 0000283 View Revisions
2014-08-21 21:52 ChristianAchatz Issue cloned: 0000232
2014-08-21 21:52 ChristianAchatz Relationship added related to 0000232
2015-10-12 12:20 ChristianAchatz Status resolved => closed