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

This PR is done and ready.

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::validate right?

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

please reassign Milestone 4.0, thanks.

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.

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

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

• 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

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.

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

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

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?

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

Fixed those issues, too.

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.

Added all the requested changes

Added all requested changes.

I have tested this item ✅ successfully on 88e6afa

All good to me now, thanks!

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.

PR (#23478) has been added.

@Hackwar think you missed this comment https://github.com/joomla/joomla-cms/pull/12414/files/ac22db4bac33e02d311e0d832aad3ec3f3ed3699#diff-2fd3303a0982c26d8f41ffbe2206ad45

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.

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

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 added PR #23501 to address the issue with show_associations field. We can now remove the if (!($fieldObj instanceof FormField)) now if we want.

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?

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

@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

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

Thank you!!!!

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

Changes here should be documented. https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4#Form

Important backward compatibility break:
The Form class has removed methods filterField() and validateField() without previous deprecation. This will break all custom Form classes that extends core Form and use these methods.

Added