PR-4.0-dev

Success

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
14 Oct 2016

Issues to adress

The 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.

(New) expected behavior

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($data).
Form::process() then calls Form::filter($data), which cleans up the data, removing unnecessary spaces, etc., preparing it to be validated with Form::validate(). Form::validate($data) will either return true or false, based on all fields having a valid content. If Form::validate() returns true, Form:;process() calls 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::filter(), FormField::validate() and FormField::postProcess() on it. FormField::filter() and FormField::validate() in the base class allow for FormRule and 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.
In 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 FormFieldUpload field.

Implications for B/C

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.

avatar Hackwar Hackwar - open - 14 Oct 2016
avatar Hackwar Hackwar - change - 14 Oct 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 14 Oct 2016
Labels Added: PR-4.0-dev Unit/System Tests
avatar joomla-cms-bot joomla-cms-bot - change - 14 Oct 2016
Category Libraries Unit Tests
avatar Hackwar
Hackwar - comment - 14 Oct 2016

Refactored this almost completely. Updated the above description to reflect what the intended behavior is. Still a WIP.

avatar Hackwar Hackwar - change - 14 Oct 2016
The description was changed
avatar Hackwar
Hackwar - comment - 26 Oct 2016

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. 😉

avatar Hackwar Hackwar - change - 8 Nov 2016
The description was changed
avatar Hackwar Hackwar - change - 8 Nov 2016
Labels Added: Unit/System Tests
avatar Hackwar
Hackwar - comment - 10 Nov 2016

This PR is done and ready.

avatar wilsonge
wilsonge - comment - 15 Jan 2017

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.

avatar Hackwar
Hackwar - comment - 15 Jan 2017

ok, then rather lets have it in its own classes in /libraries/joomla/form/filter/

avatar wilsonge
wilsonge - comment - 15 Jan 2017

Finally we need to amend JModelForm::validate to call JForm::process instead of JForm::validate right?

avatar Hackwar
Hackwar - comment - 15 Jan 2017

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.

avatar wilsonge
wilsonge - comment - 15 Jan 2017

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

avatar brianteeman brianteeman - change - 8 Jun 2017
Milestone Added: Joomla 4.0
avatar brianteeman brianteeman - change - 8 Jun 2017
Milestone Added: Joomla 4.0
avatar joomla-cms-bot joomla-cms-bot - change - 27 Oct 2017
Title
Joomla 4.0: Adding filter() and postProcessing() to JFormField
[4.0] Adding filter() and postProcessing() to JFormField
avatar franz-wohlkoenig franz-wohlkoenig - change - 27 Oct 2017
The description was changed
avatar joomla-cms-bot joomla-cms-bot - edited - 27 Oct 2017
avatar joomla-cms-bot joomla-cms-bot - edited - 27 Oct 2017
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 27 Oct 2017

please reassign Milestone 4.0, thanks.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12414.

avatar laoneo
laoneo - comment - 9 Apr 2018

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.

avatar Hackwar
Hackwar - comment - 25 Jun 2018

@wilsonge Is there still interest in this PR? Then I would update this again.

avatar wilsonge
wilsonge - comment - 1 Jul 2018

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

avatar mbabker
mbabker - comment - 1 Jul 2018

OK, going back over this and picking up on nitpicks I've had on other PRs recently...

  • Filtering and validation are two separate responsibilities, the FormRule class chain isn't the place to stuff both of them
    • FormRule is more of a validation API (but poorly named)
    • Filtering debatably belongs at the MVC layer of the application, not in the form API itself, but since we have it in the form API it really should be a separate responsibility principle and exist as its own entity (yeah, we've pointed this out before about the awkwardness of "filter" and "rule" directories)
    • There's a part of me that mildly feels like we should just deprecate FormRule in full and introduce a Joomla\CMS\Validation package 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
  • The FormField class 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
    • Funny enough last night I stumbled on a field that reuses the filter XML attribute in a non-compatible manner with the existing Form::filter() capabilities
  • TBH I don't know how to best reorganize the form API internals without introducing a B/C nightmare, the Form class 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 FormRule class), and renderer (again, not so terrible as it's dispatching rendering to the appropriate FormField class), not to mention some of the responsibilities in the static FormHelper that really should be some kind of factory or registry service
avatar Hackwar
Hackwar - comment - 1 Jul 2018

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.

avatar rdeutz
rdeutz - comment - 18 Aug 2018

for the test can you make a PR against this Repo https://github.com/joomla/test-integration Tests are in the 4.0-dev branch /legacy and remove the files from your PR here, thanks

avatar franz-wohlkoenig franz-wohlkoenig - change - 18 Aug 2018
Status Pending Information Required
avatar joomla-cms-bot joomla-cms-bot - change - 22 Dec 2018
Category Libraries Unit Tests Libraries
avatar Hackwar Hackwar - change - 22 Dec 2018
Labels Removed: Unit/System Tests
avatar Hackwar
Hackwar - comment - 22 Dec 2018

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?

avatar Hackwar Hackwar - change - 23 Dec 2018
Title
[4.0] Adding filter() and postProcessing() to JFormField
[4.0] Adding filter() and postProcessing() to FormField
avatar Hackwar Hackwar - edited - 23 Dec 2018
avatar Hackwar Hackwar - change - 23 Dec 2018
The description was changed
avatar Hackwar Hackwar - edited - 23 Dec 2018
avatar Hackwar
Hackwar - comment - 23 Dec 2018

I've also updated the original description of this PR. The filters are now in separate classes.

avatar joomdonation
joomdonation - comment - 25 Dec 2018

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

Call stack

| Function | Location

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?

avatar Hackwar
Hackwar - comment - 28 Dec 2018

I've fixed everything. The tests are again failing because of something unrelated. God knows they aren't failing because of our unittests...

avatar Hackwar
Hackwar - comment - 29 Dec 2018

Fixed those issues, too.

avatar joomdonation
joomdonation - comment - 29 Dec 2018

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?

avatar Hackwar
Hackwar - comment - 29 Dec 2018

I haven't actually thought about that. I'll add corresponding code for filters.

avatar Hackwar
Hackwar - comment - 29 Dec 2018

Added all the requested changes

avatar Hackwar
Hackwar - comment - 30 Dec 2018

Added all requested changes.

avatar joomdonation
joomdonation - comment - 30 Dec 2018

I have tested this item successfully on 88e6afa

All good to me now, thanks!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/12414.

avatar joomdonation joomdonation - test_item - 30 Dec 2018 - Tested successfully
avatar wilsonge
wilsonge - comment - 7 Jan 2019

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.

avatar Hackwar
Hackwar - comment - 8 Jan 2019

PR (#23478) has been added.

avatar Hackwar
Hackwar - comment - 9 Jan 2019

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.

avatar wilsonge
wilsonge - comment - 9 Jan 2019

@Hackwar in the case of this PR the system tests failing are genuine - you can't save global configuration with this PR applied (and you can when it's not applied)

avatar Hackwar
Hackwar - comment - 10 Jan 2019

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.

avatar joomdonation
joomdonation - comment - 10 Jan 2019

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.

avatar Hackwar
Hackwar - comment - 10 Jan 2019

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...

avatar joomdonation
joomdonation - comment - 10 Jan 2019

I added PR #23501 to address the issue with show_associations field. We can now remove the if (!($fieldObj instanceof FormField)) now if we want.

avatar Hackwar
Hackwar - comment - 10 Jan 2019

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?

avatar wilsonge
wilsonge - comment - 12 Jan 2019

@Hackwar system tests failing is still legit in an interesting kind of way. it's failing because the hidden port fields have default values

avatar Hackwar
Hackwar - comment - 12 Jan 2019

@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.

avatar wilsonge
wilsonge - comment - 14 Jan 2019

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

avatar wilsonge wilsonge - change - 18 Jan 2019
Status Information Required Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2019-01-18 00:29:43
Closed_By wilsonge
avatar wilsonge wilsonge - close - 18 Jan 2019
avatar wilsonge wilsonge - merge - 18 Jan 2019
avatar wilsonge
wilsonge - comment - 18 Jan 2019

It looks good to me now :) Thanks @Hackwar for the hard work here (and @joomdonation for helping push this across the line!)

avatar Hackwar
Hackwar - comment - 18 Jan 2019

Thank you!!!!

avatar infograf768
infograf768 - comment - 28 Jan 2019

this is creating some issues with some fields like ‘show_associations’

Add a Comment

Login with GitHub to post a comment