Subforms: Implementing per-field filters and validation. #14189

Harmageddon · 2017-02-22T17:39:08Z

This pull request introduces the option to both filter and validate subforms field by field. The intention is that every single field inside a subform can have its own filter and validation parameters. Before this PR, these parameters would just be ignored.

Summary of Changes

Introduced a Parameter filter="subform" to filter a subform field by filtering its child fields individually.
Introduced a Parameter validate="subform" to validate a subform field by validating its child fields individually.
Moved the loading of the JForm instance and its data inside the subform field from the getInput method into two separate methods to avoid repeated code, because the subform has to be loaded for filtering and validation, too. This change should not change any present behavior of subform fields, because I only moved the existing code into two additional methods.

Testing Instructions

Test with a form containing a subform field.
- I created a small module for testing purposes on mod_demovalidation. It has no frontend, only the form in the module configuration in backend. Feel free to use this, but I'd recommend that you play around with different parameter combinations to not just replicate my tests.
- You can also use the testing instructions from New field Subform, and deprecate the Repeatable field #7829 and add some filters and validations...
- ... or those from Support inline subforms #12813, where an inline subform is used.
- If you are developing an extension using subforms, this is the perfect opportunity to improve it! ;-)
Test the impact of filters.
- Add filter="subform" to the subform field.
- For example, add filter="int" to a field inside the subform XML and try to save values like "abc" (should be filtered to 0) or "0002" (should be filtered to 2).
- Make sure that correct values are not filtered and everything is stored correctly.
Test the impact of validation.
- Add validate="subform" to the subform field.
- For example, add validate="email" to a field inside the subform XML and try to save values like "foo".
- Make sure that correct values are not marked as invalid and everything is stored correctly.
Make sure that the subform field behaves like before (apart from the added filtering and validation options).
Test everything with multiple="true", multiple="false" and without the multiple parameter for the subform field.

Expected result

Unfiltered values should be filtered, depending on the filters of the fields inside the subform XML.
Invalid values should be rejected, depending on the validation rules of the fields inside the subform XML.

If a validation fails, there are three options what to display as an error message:

"Invalid Field: Subform" (Title of the subform field containing at least one invalid value)
"Invalid Field: Mail" (Title of the first invalid single field inside the subform)
Like 2., but for all invalid fields.

I used option 2, with a fallback to 1. For a subform containing invalid values, only one warning message will be returned, containing the title of the first field with an invalid value. I'm preferring this over option 1, because it is more specific about where the error is. Option 3 would be possible using additional messages, but as the basic implementation of validation itself displays a maximum of 3 messages, I thought it would be sufficient to send only one here.

Documentation Changes Required

~~As I have no experience in the version numbering of the CMS libraries, I'd be thankful if someone could tell me which value should be added for the @since tag for the JFormRuleSubform class.~~
When the pull request is merged and released, I will add short documentation about the additional parameters on docs.joomla.org.

zero-24 · 2017-02-22T18:34:17Z

@Harmageddon with the last 3 commit i have just fixed a error from drone and minimal cs ;)

Harmageddon · 2017-02-22T19:03:02Z

@zero-24 Awesome, thank you! Didn't know about this macro, very useful!

…subform_filter_validation

Harmageddon · 2017-02-22T20:29:33Z

Added some small code cleanups, inspired by @matrikular. Thank you!

laoneo · 2017-09-05T15:07:51Z

Nice pr. Can you please fix the merge conflicts that we can test it?

Harmageddon · 2017-09-05T15:52:01Z

@laoneo Thank you! It should work now.

laoneo · 2017-09-06T07:06:14Z

libraries/joomla/form/fields/subform.php

+	/**
+	 * Loads the form instance for the subform.
+	 *
+	 * @return  JForm  The form instance.


Can you use the namespaced class? Thanks.

laoneo · 2017-09-06T07:06:25Z

libraries/joomla/form/fields/subform.php

+
+		// Prepare the form template
+		$formname = 'subform.' . ($this->group ? $this->group . '.' : '') . $this->fieldname;
+		$tmpl     = JForm::getInstance($formname, $this->formsource, array('control' => $control));


Can you use the namespaced class? Thanks.

laoneo · 2017-09-06T07:06:37Z

libraries/joomla/form/fields/subform.php

+	/**
+	 * Binds given data to the subform and its elements.
+	 *
+	 * @param   JForm  &$subForm  Form instance of the subform.


Can you use the namespaced class? Thanks.

laoneo · 2017-09-06T07:07:51Z

libraries/joomla/form/fields/subform.php

+	 *
+	 * @since   __DEPLOY_VERSION__
+	 */
+	private function loadSubFormData(&$subForm)


Can you type hint it?

laoneo · 2017-09-06T07:08:00Z

libraries/joomla/form/fields/subform.php

+		for ($i = 0; $i < $c; $i++)
+		{
+			$control  = $this->name . '[' . $this->fieldname . $i . ']';
+			$itemForm = JForm::getInstance($subForm->getName() . $i, $this->formsource, array('control' => $control));


Can you use the namespaced class? Thanks.

laoneo · 2017-09-06T07:08:10Z

libraries/joomla/form/rule/subform.php

+ *
+ * @since  __DEPLOY_VERSION__
+ */
+class JFormRuleSubform extends JFormRule


Can you use the namespaced class? Thanks.

laoneo · 2017-09-06T07:08:17Z

libraries/joomla/form/rule/subform.php

+	 *
+	 * @since   __DEPLOY_VERSION__
+	 */
+	public function test(SimpleXMLElement $element, $value, $group = null, Registry $input = null, JForm $form = null)


Can you use the namespaced class? Thanks.

laoneo · 2017-09-06T07:08:21Z

libraries/joomla/form/rule/subform.php

+	 *                                      For example if the field has name="foo" and the group value is set to "bar" then the
+	 *                                      full field name would end up being "bar[foo]".
+	 * @param   Registry          $input    An optional Registry object with the entire data set to validate against the entire form.
+	 * @param   JForm             $form     The form object for which the field is being tested.


Can you use the namespaced class? Thanks.

laoneo · 2017-09-06T07:09:49Z

Thanks, added a few comments to use the namespaced classes. If done I will test it.

Harmageddon · 2017-09-06T08:37:26Z

Done. Thanks in advance!

laoneo · 2017-09-06T08:38:07Z

Now you got me wrong, use the real classes like \Joomla\CMS\Form\Form, etc. Not just adding a backslash before them.

Harmageddon · 2017-09-06T08:45:47Z

Oh, sorry. Should I copy the rule to libraries/src/Form/Rule, too, then? Or does it work with this "mixed" form (the subform field and the rule in libraries/joomla/form and the form and the JFormFieldRule class in libraries/src/Form)?

laoneo · 2017-09-06T08:48:27Z

Yes please copy the rule also to libraries/src/Form/Rule. The field has to stay because of autoloading. In Joomla 4 we have moved it also to libraries/src/Form/Field.

Harmageddon · 2017-09-06T08:58:51Z

Okay, a little bit confusing. I hope I haven't messed up anything.

laoneo · 2017-09-06T09:01:29Z

libraries/joomla/form/rule/subform.php

@@ -0,0 +1,88 @@
+<?php


This one can be deleted.

The <?php tag? Why?

I meant the whole file as you have the namespaced one.

Ah, I see. Seems I have missed that the rules have been moved completely.

laoneo · 2017-09-06T13:02:58Z

libraries/joomla/form/fields/subform.php

 * @license     GNU General Public License version 2 or later; see LICENSE
 */

+use Joomla\CMS\Form\Form;


Minor cosmetics here, it belongs below the defined check.

laoneo · 2017-09-06T13:12:28Z

I have tested this item ✅ successfully on d379667

It worked. I'v tested it by changing the file plugins/fields/checkboxes/params/checkboxes.xml to

<?xml version="1.0" encoding="utf-8"?>
<form>
	<fields name="fieldparams">
		<fieldset name="fieldparams">
			<field
				name="options"
				type="subform"
				label="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_LABEL"
				description="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_DESC"
				layout="joomla.form.field.subform.repeatable-table"
				icon="list"
				multiple="true"
				filter="subform"
				validate="subform"
			>
				<form hidden="true" name="list_templates_modal" repeat="true">
					<field
						name="name"
						type="text"
						label="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_NAME_LABEL"
						size="30"
					/>

					<field
						name="value"
						type="text"
						label="PLG_FIELDS_CHECKBOXES_PARAMS_OPTIONS_VALUE_LABEL"
						size="30"
						validate="color"
					/>
				</form>
			</field>
		</fieldset>
	</fields>
</form>

After that I created a checkbox field and tried to create an option where the value is not a hex number. It failed. The I changed validate="color"to filter="int" and tried to save again, then it successfully saved it but turned the value into an integer.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14189.}

SharkyKZ · 2018-05-15T06:50:03Z

I have tested this item ✅ successfully on 6b79cea

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14189.}

ghost · 2018-05-15T09:09:37Z

@laoneo can you please retest?

ghost · 2018-05-24T09:39:12Z

Ready to Commit after two successful tests.

laoneo · 2018-05-24T12:54:01Z

Perhaps you should consider to make your own callback for filter instead of hardcoding the subform filter into the form class. More details can be found here #20569 (comment).

Harmageddon · 2018-05-24T13:38:14Z

Are there any other filters that could serve as an example? I assume that I'd have to create a new class? Where should this class be located? As far as I see, most core rules (like "access") are directly in the Form class.

I would use the method you suggest only for third-party extensions and keep the core filters in one place - either in the Form class as of now or each one in a dedicated class. If we keep some filters in one and others in another place, we risk inconsistencies (i.e. if the filter mechanism gets changed) and people will have a harder time to find all available filters when digging through the code. Personally, I'd support your approach to create classes for filters, because I remember debugging the filter method until I found the hardcoded filters in the Form class. But I'd prefer to either move all or none.

mbabker · 2018-05-24T13:43:28Z

What is hardcoded into the Form class' filterField method is really a bunch of special cases specific to the form data that don't belong to the InputFilter class. So, "core filters" don't need to be hardcoded into the Form class itself unless there is a strong reason why it must be part of the Form API and available at all times.

Harmageddon · 2018-05-24T16:19:29Z

Okay... So is there any best practice where else to put such a filter? And if I see this right, we'd then require users/developers to use a filter="SomeHelperClass:subform" instead of just filter="subform"?

SharkyKZ · 2018-05-24T17:17:49Z

IMO, form field filtering and validation should be implicit whenever possible to reduce XML form maintenance needs.

So Form's validateField and filterField logic could be moved to FormField. Subform field could then override parent methods and other fields could declare default filters and validation rules to be used when none is added in XML.

Here's a mockup based on your code and #12414 SharkyKZ@fa514ae.

mbabker · 2018-05-24T17:53:10Z

So Form's validateField and filterField logic could be moved to FormField. Subform field could then override parent methods and other fields could declare default filters and validation rules to be used when none is added in XML.

Not necessarily, because each part of the API has a separate concern.

FormField is in essence the visual representation of the field and handles the rendering, the class chain does not belong in the filtering/validation process at all.

FormRule defines some validation rules to be applied when validating the form's data, these are called during the validation process only.

Form is the data manager for a form instance and this is where the filtering and validation processes belong.

SharkyKZ · 2018-05-25T07:01:07Z

That's the way it is now but is it really correct?

FormField is in essence the visual representation of the field and handles the rendering, the class chain does not belong in the filtering/validation process at all.

Form fields also provide the data options and data type. So filtering and validation makes sense here. Filtering and validation is applied per-field anyways. That said, shouldn't FormRule be FormFieldRule in the first place?

List-based fields should be validated against options (except when custom option is allowed), numeric fields should be filtered accordingly and Subform should not accept other filters but its own (currently, adding a filter like string to subform field causes fatal error).

I think this functionality is vital and should be implemented one way or another.

mbabker · 2018-05-25T12:35:02Z

Rewrite the entire form package from scratch then? There is already a separation of concern regarding each of the object types in the package and an established pattern of use, if you feel this is inefficient feel free to propose a new package but the existing class chain can't just be changed out of the blue like this. By design, filtering and validation are not the responsibility of the FormField classes and refactoring to move responsibilities like this is no small thing. At the end of the day though, regardless of the class structure, what is responsible for rendering should be a different aspect of the API than what is responsible for data validation (if you look at any proper framework you'll find they have a standalone validation component that the form data is pushed through whereas the closest thing we have to that is the FormRule class and that is tightly coupled to the Form class) and if we're going to start talking refactoring the form API then I don't think we should be making one sort of "super" object that does all the things.

roland-d · 2018-07-22T20:34:01Z

@mbabker Is this something for 3.9?

Harmageddon · 2018-09-14T15:26:10Z

So with this PR being marked as RTC for almost 4 months now, I'd like to ask what is the status for this one? Is it going to be merged for some future release in the current state or do you prefer me to move the filter to some different location for architectural reasons?
@mbabker @wilsonge (pinging you as the release leads for 3.9/3.10)

mbabker · 2018-09-14T15:34:59Z

I'm hesitant to merge this because I don't think it is architecturally done correctly and inherently once it ships changing the API will never happen.

Harmageddon · 2018-09-14T15:48:08Z

I see. However, the problem with moving this method outside the Form class is, that we need the form field ($element) in order to load the subform and get the filters for the respective fields inside the subform. Callback filters as well as the InputFilter->clean function only get the value of the field as an argument, but not the whole field.
So a callback filter would get an array containing the values of the fields inside the subform without knowing which value to treat with which filter.

Harmageddon · 2018-10-03T10:21:46Z

Merged staging to include the changes from #17552.

@mbabker With the current API, I don't see any alternative to this implementation.

Harmageddon · 2018-10-03T10:22:58Z

(sorry, I accidentally hit the wrong button)

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14189.}

Hackwar · 2019-01-18T18:43:24Z

Please see #12414. That might have an impact on this PR.

Harmageddon · 2019-01-18T19:29:03Z

Thank you for your work there! It does indeed have an impact. I'm going to rewrite this PR for 4.0 then. And as this PR here probably won't be ever merged into 3.x after being RTC for 8 months, I'm now closing it and hope for better luck and API integration for 4.0.

wilsonge · 2019-03-21T17:17:32Z

Just for the record I'd definitely be interested in seeing this for Joomla 4!

Harmageddon · 2019-08-31T16:07:13Z

Apparently, the changes proposed in this PR have been added in b06019e and d34a3ec, so I assume they're going to be merged into 4.0, too, and there is no need for a further PR?
@HLeithner / @wilsonge what do you think?

zero-24 · 2019-08-31T16:45:58Z

Correct. All 3.x changes are merged up to 4.0 before the final release.

Subforms: Implementing per-field filters and validation.

700f74d

joomla-cms-bot added the PR-staging label Feb 22, 2017

zero-24 added 3 commits February 22, 2017 19:31

use __DEPLOY_VERSION__

985e6f5

fix drone and replace the version tag with __DEPLOY_VERSION__

3eb1ecf

just two spaces to much ;)

4118e9a

Harmageddon added 2 commits February 22, 2017 21:27

Cleaning up some code.

4c5bbe1

Merge remote-tracking branch 'origin/subform_filter_validation' into …

d279bc5

…subform_filter_validation

Merge branch 'staging' into subform_filter_validation

7dc7f77

laoneo requested changes Sep 6, 2017

View reviewed changes

Namespacing and typehinting.

a5a1577

Added rule to libraries/src, fixed namespaces.

93c9208

laoneo reviewed Sep 6, 2017

View reviewed changes

Removed old rule file.

d379667

laoneo reviewed Sep 6, 2017

View reviewed changes

Moved import below define check.

6b79cea

Fedik mentioned this pull request Oct 5, 2017

JForm : Problem whith rule in subforms (multiple) #18250

Closed

laoneo mentioned this pull request May 24, 2018

Filters are hardcoded in the form #20569

Closed

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 24, 2018

Merge branch 'staging' into subform_filter_validation

6401227

Harmageddon closed this Oct 3, 2018

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 3, 2018

Harmageddon reopened this Oct 3, 2018

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 3, 2018

Harmageddon closed this Jan 18, 2019

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 18, 2019

Fedik mentioned this pull request Mar 19, 2019

Timezone Errror in JForm layout Repeatable #23703

Closed

Uh oh!

Subforms: Implementing per-field filters and validation. #14189

Subforms: Implementing per-field filters and validation. #14189

Uh oh!

Conversation

Harmageddon commented Feb 22, 2017 • edited by joomla-cms-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Instructions

Expected result

Documentation Changes Required

Uh oh!

zero-24 commented Feb 22, 2017

Uh oh!

Harmageddon commented Feb 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Harmageddon commented Feb 22, 2017

Uh oh!

laoneo commented Sep 5, 2017

Uh oh!

Harmageddon commented Sep 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laoneo commented Sep 6, 2017

Uh oh!

Harmageddon commented Sep 6, 2017

Uh oh!

laoneo commented Sep 6, 2017

Uh oh!

Harmageddon commented Sep 6, 2017

Uh oh!

laoneo commented Sep 6, 2017

Uh oh!

Harmageddon commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laoneo Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laoneo commented Sep 6, 2017

Uh oh!

SharkyKZ commented May 15, 2018

Uh oh!

ghost commented May 15, 2018

Uh oh!

ghost commented May 24, 2018

Uh oh!

laoneo commented May 24, 2018

Uh oh!

Harmageddon commented May 24, 2018

Uh oh!

mbabker commented May 24, 2018

Uh oh!

Harmageddon commented May 24, 2018

Uh oh!

SharkyKZ commented May 24, 2018

Harmageddon commented Feb 22, 2017 •

edited by joomla-cms-bot

Loading

Harmageddon commented Feb 22, 2017 •

edited

Loading

laoneo Sep 6, 2017 •

edited

Loading