User tests: Successful: Unsuccessful:
Form class currently is a god class, with lots of duties and equally a lot of hardcoded behavior, especially when it comes to handling the form data after it has been send from the browser. It contains field specific hardcoded filters, which modify the data and actually makes it very hard for third party developers to provide their own, extended logic to handle the data. It is also not entirely clear (at least to me) how the whole concept out of Form,
FormField objects and
FormRule objects is supposed to work.
When the component receives the data from the form, it sets up the
Form object with the right form definition and then calls
Form::process() then calls
Form::filter($data), which cleans up the data, removing unnecessary spaces, etc., preparing it to be validated with
Form::validate($data) will either return true or false, based on all fields having a valid content. If
Form::validate() returns true,
Form::postProcess($data), which allows to post-process the data.
Each of these methods does not have any specific logic itself, but will load the respective
FormField object for the given field and value and call
FormField::postProcess() on it.
FormField::validate() in the base class allow for
FormFilter objects to be used to filter and validate data, but you can also override that functionality in your specific field, for example filtering the input on a calendar field on a localised date format and validating also that after filtering that is indeed the value that we want.
FormField::postProcess(), the aforementioned calendar field would then convert the date from the localised format to a normalised format, like the MySQL date format. Another use would be to move an uploaded file to its final location in a
This change is not completely backwards compatible. While most components using the core MVC classes will not need any modifications, components that override the save() method of their model/controller will most likely need some light tweaking. custom FormField and FormRule classes might need some modifications, but that depends on the respective code.
|Category||⇒||Libraries Unit Tests|
Refactored this almost completely. Updated the above description to reflect what the intended behavior is. Still a WIP.
I've added lots of unittests to JForm and did some further improvements. I've tried to achieve good code coverage by the tests and indeed I was able to get the coverage from 41% (methods) or 78% (lines) of fake coverage (because no @covers syntax used) to 82% (methods) or 90% (lines) of real coverage. However now I'm at a point where I'm a bit burned out with writing unittests for this and so far I only was able to write tests for JForm and there is still JFormField and the set of rules, etc. that was added... So if someone is a masochist and wants to try his hands at this, I'm happy to take PRs to this PR.
I've fixed conflicts here. I think having the filter method in rules is wrong - if we're going to put it somewhere outside the field definition it should be as @mbabker said in it's own class. But honestly I think it's good enough to have it in the
JFormField class only.
ok, then rather lets have it in its own classes in /libraries/joomla/form/filter/
Finally we need to amend
JModelForm::validate to call
JForm::process instead of
JForm::process() just combines the three calls. We would need to simply add JForm::process() in the right place. So I don't know if there is some code that would be put inbetween the different method calls.
I don't know either - but I think I'd rather see us call process as it's there than call the 3 methods individually
Any plans to go ahead with this one? If yes, then I suggest to split it into multiple pr's and make it more BC friendly by not removing the functions but deprecating them and proxy to the new function.
Definitely of interest. Once you've merged conflicts need to go back through it and remind myself what needs working on though - but there's still some PR feedback to go through before this is mergeable
OK, going back over this and picking up on nitpicks I've had on other PRs recently...
FormRuleclass chain isn't the place to stuff both of them
FormRuleis more of a validation API (but poorly named)
FormRulein full and introduce a
Joomla\CMS\Validationpackage because there is some useful logic in the rule classes but it is entirely coupled to the form API and a higher level validation API that the form API and MVC chain could consume would be a vast improvement
FormFieldclass chain really feels more like a rendering class, to me it doesn't feel like that API should also have a responsibility for field filtering or validation
filterXML attribute in a non-compatible manner with the existing
Formclass is definitely too busy on its own (because if you break it down right now it has responsibilities as a builder (parse/manage XML schema and structure modifications), filtering mechanism, validation mechanism (this one isn't so terrible as it's mostly dispatching the validation to the appropriate
FormRuleclass), and renderer (again, not so terrible as it's dispatching rendering to the appropriate
FormFieldclass), not to mention some of the responsibilities in the static
FormHelperthat really should be some kind of factory or registry service
I agree, in the best case we would have a FormDefinition class for the XML, a FormRenderer for rendering the form, a FormValidator, etc. However now we are rather stuck with the current situation and I would try to make the best out of this. I like the idea of a validator package. Right now I'm concentrating on com_finder, but maybe we can have another talk about this and see what we can do when com_finder is done.
|Category||Libraries Unit Tests||⇒||Libraries|
Hi folks, I've updated this PR or rather reworked it completely. The filters have been moved to their own classes and it is reworked to the current development branch. I've removed the tests completely here for now... I'd be happy to get some feedback from you on this. Is there still interest in this?
I've also updated the original description of this PR. The filters are now in separate classes.
I haven't had time to look at the the code yet but when I tried to apply the patch and save the article, error happens:
An error has occurred.
0 Call to a member function validate() on boolean
1 | () | JROOT\libraries\src\Form\Form.php:1204
2 | Joomla\CMS\Form\Form->validate() | JROOT\libraries\src\MVC\Model\FormModel.php:372
3 | Joomla\CMS\MVC\Model\FormModel->validate() | JROOT\administrator\components\com_content\Model\ArticleModel.php:686
4 | Joomla\Component\Content\Administrator\Model\ArticleModel->validate() | JROOT\libraries\src\MVC\Controller\FormController.php:708
5 | Joomla\CMS\MVC\Controller\FormController->save() | JROOT\libraries\src\MVC\Controller\BaseController.php:734
6 | Joomla\CMS\MVC\Controller\BaseController->execute() | JROOT\libraries\src\Dispatcher\ComponentDispatcher.php:146
7 | Joomla\CMS\Dispatcher\ComponentDispatcher->dispatch() | JROOT\libraries\src\Component\ComponentHelper.php:380
8 | Joomla\CMS\Component\ComponentHelper::renderComponent() | JROOT\libraries\src\Application\AdministratorApplication.php:116
9 | Joomla\CMS\Application\AdministratorApplication->dispatch() | JROOT\libraries\src\Application\AdministratorApplication.php:159
10 | Joomla\CMS\Application\AdministratorApplication->doExecute() | JROOT\libraries\src\Application\CMSApplication.php:233
11 | Joomla\CMS\Application\CMSApplication->execute() | JROOT\administrator\includes\app.php:63
12 | require_once() | JROOT\administrator\index.php:36
Could you please check and correct it?
I've fixed everything. The tests are again failing because of something unrelated. God knows they aren't failing because of our unittests...
Thanks. It looks good now. One final question: If a custom filter class defined (by component for example), how do we register that custom filter with Form? Do we need to add something like addfilterprefix (like addfieldprefix, addformprefix....) in syncPaths method of Form class?
I haven't actually thought about that. I'll add corresponding code for filters.
Couple of small comments but to all intents and purposes approved. Can you please also do a PR to Joomla 3.10 with the deprecations notices required for this please.
I indeed missed that comment, since it was hidden somewhere in the resolved discussions. Just for others: The article.xml contains a field of type
assoc, which craps out here: https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_content/Field/AssocField.php#L52
That field is always present and part of com_content. So that check that was questioned catches that.
Actually no. The tests fail, but because com_config is wrong, not this PR. https://github.com/joomla/joomla-cms/blob/4.0-dev/administrator/components/com_config/Controller/ApplicationController.php#L122 not only has to set the redirect, but also return from the method. Otherwise we simply save the data that has failed validating, with potentially catastropic consequences. I'll create a PR to fix that. The errors are then that the FTP port and proxy port need to have any value according to our form XML. I'll also create a PR for that, too. But this PR actually isn't the fault.
About the issue cause by assoc field, I think the right fix for it would be remove the field show_associations on getForm method of Articles model if association is not enabled. So the check if (!($fieldObj instanceof FormField)) in validate method is not really needed.
The PR for the actual return is #23498. Please note that the definition for FTP Port and Proxy Port is still not right. The problem is that the filter casts this to int, which results in the value being 0. The validation method then however only accepts the value as being empty when it is '' or null, not 0. I don't know if the validation should be changed to also recognise 0 as an empty value or if we should modify the definition of the 2 fields to also include 0 in their range. Which isn't a valid port...
I removed that check. In hindsight I think that check shouldn't simply jump over a field like that and not filter the data, so as to not introduce any attack vectors. I wouldn't know how to exploit that, but better safe than sorry.
Can we then merge this?
@wilsonge Yes. we have to decide how we want to change behavior here. It currently fails because the filter casts the value to integer (empty value = 0) and the validation only allows values between 1 and 65k. So as I wrote, we have to decide if we want to treat 0 as an empty value, extend the valid range or set a default value of f.ex. 22. The code of this PR behaves as expected.
Apologies I meant to look at this tonight but ended up spending time on board related issues. I want to look at what (if any) validation we have in J3 for this area (and also if any other components have similar issues where hidden fields have validation rules).
I think my gut is to set make the validator accept that an empty string is a valid option for optional fields (obviously not for compulsory fields) and to actually fail validation if it's an empty string in a integer field. But I'm also reaching the point where maybe hidden fields should be disabled and therefore never saved or something, but that's probably outside of the scope of this PR :/
As seemingly always with this PR always more questions than answers i'm afraid :( I'll try and find some time to review this during my JUG meetup tomorrow
|Status||Information Required||⇒||Fixed in Code Base|
|Closed_Date||0000-00-00 00:00:00||⇒||2019-01-18 00:29:43|
this is creating some issues with some fields like ‘show_associations’