Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] Adding filter() and postProcessing() to FormField #12414

Merged
merged 40 commits into from Jan 18, 2019

Conversation

@Hackwar
Copy link
Member

Hackwar commented Oct 14, 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.

@Hackwar

This comment has been minimized.

Copy link
Member Author

Hackwar commented Oct 14, 2016

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

switch (strtoupper($filter))
{
// Convert a date to UTC based on the server timezone offset.
case 'SERVER_UTC':

This comment has been minimized.

@Fedik

Fedik Oct 14, 2016

Contributor

I would leave it in parent JField , what if someone want to do own calendar? he can reuse existing filter from JFIeld 😉

This comment has been minimized.

@Hackwar

Hackwar Oct 15, 2016

Author Member

if they want to create their own calendar and need to reuse this code, they can either extend the existing calendar field or copy the code. But it is a VERY field specific code and thus should not be in the general JFormField class.

This comment has been minimized.

@Fedik

Fedik Oct 15, 2016

Contributor

well, what I like in current state, that I can take a text field with different filter/validation, attach some JS and get new field, without actually writing a new field

But it is a VERY field specific code

it is more "datetime handling" specific code, not just Calendar field specific 😉

Also moving code inside the calendar field make B/C (maybe not important but). If someone have a field that use that filter, then he will get "silent bug" with timezone handling.
Same about other filters.

In general, I like the idea moving the filter in to the field, but I would keep existing filters in global scope in JField.

This comment has been minimized.

@mbabker

mbabker Oct 15, 2016

Member

The only "global" filters should come from JFilterInput. When it comes down to form fields, I would not consider the existing set of filters as "global" and all of them should be moved to the correct place. ONLY if we find certain filters are commonly used between fields should they be considered "global". These date/time filters are very specific and as such don't belong to a global layer.

@Hackwar

This comment has been minimized.

Copy link
Member Author

Hackwar commented Oct 26, 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. 😉

@Hackwar

This comment has been minimized.

Copy link
Member Author

Hackwar commented Nov 10, 2016

This PR is done and ready.

@Hackwar Hackwar referenced this pull request Nov 10, 2016

Merged

Support inline subforms #12813

Show resolved Hide resolved libraries/joomla/form/form.php Outdated
*
* @since 11.1
*/
protected function loadFieldType($type, $new = true)

This comment has been minimized.

@mbabker

mbabker Nov 12, 2016

Member

These need to be deprecated in the 3.x branch if you want to remove for 4.0. Otherwise the methods need to be restored, even if unused by core, with proper deprecation notices.

This comment has been minimized.

@Hackwar

Hackwar Nov 13, 2016

Author Member

I've been told that non-public methods are not covered by our B/C claims...

This comment has been minimized.

@mbabker

mbabker Nov 13, 2016

Member

Protected methods fall under B/C because a subclass can override them. Private methods aren't overridable so you can do whatever you please with them.

This comment has been minimized.

@wilsonge

wilsonge Jan 15, 2017

Contributor

Let's just add the deprecated notices :) It's not a big deal

This comment has been minimized.

@Hackwar

Hackwar Jan 15, 2017

Author Member

Are you going to create the PR or should I?

Show resolved Hide resolved libraries/joomla/form/rule.php Outdated
@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Jan 15, 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.

@Hackwar

This comment has been minimized.

Copy link
Member Author

Hackwar commented Jan 15, 2017

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

@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Jan 15, 2017

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

@Hackwar

This comment has been minimized.

Copy link
Member Author

Hackwar commented Jan 15, 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.

@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Jan 15, 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

@brianteeman brianteeman modified the milestone: Joomla 4.0 Jun 8, 2017

@joomla-cms-bot joomla-cms-bot changed the title Joomla 4.0: Adding filter() and postProcessing() to JFormField [4.0] Adding filter() and postProcessing() to JFormField Oct 27, 2017

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.0 milestone Oct 27, 2017

@franz-wohlkoenig

This comment has been minimized.

Copy link

franz-wohlkoenig commented Oct 27, 2017

please reassign Milestone 4.0, thanks.


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

@Hackwar

This comment has been minimized.

Copy link
Member Author

Hackwar commented Jan 9, 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.

@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Jan 9, 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)

@Hackwar

This comment has been minimized.

Copy link
Member Author

Hackwar commented Jan 10, 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.

@joomdonation

This comment has been minimized.

Copy link
Contributor

joomdonation commented Jan 10, 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.

@Hackwar

This comment has been minimized.

Copy link
Member Author

Hackwar commented Jan 10, 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...

@joomdonation

This comment has been minimized.

Copy link
Contributor

joomdonation commented Jan 10, 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.

Hackwar added some commits Jan 10, 2019

@Hackwar

This comment has been minimized.

Copy link
Member Author

Hackwar commented Jan 10, 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?

wilsonge added some commits Jan 10, 2019

@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Jan 12, 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

@Hackwar

This comment has been minimized.

Copy link
Member Author

Hackwar commented Jan 12, 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.

@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Jan 14, 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

}
}
return InputFilter::getInstance()->clean($value, $filter);

This comment has been minimized.

@joomdonation

joomdonation Jan 15, 2019

Contributor

This change the filter logic compare to Joomla 3 behavior. In Joomla 3, we don't apply filter when the field is not required and the value = "" or null. In the new PR, filter is always applied and that's the reason causing the issue with saving configuration data. Change this line of code to the below could would address the issue:

$required = ((string) $this->element['required'] == 'true' || (string) $this->element['required'] == 'required');

if (($value === '' || $value === null) && ! $required)
{
	return '';
}

return InputFilter::getInstance()->clean($value, $filter);
// Validate the field.
$valid = $this->validateField($field, $group, $value, $input);
$valid = $fieldObj->validate($input->get($key, (string) $field['default']), $group, $input);

This comment has been minimized.

@joomdonation

joomdonation Jan 15, 2019

Contributor

In Joomla 3, we don't use field default value on validate, so to be safe, $input->get($key, (string) $field['default']) should be changed to $input->get($key) . For reference https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Form/Form.php#L1216-L1223

Hackwar and others added some commits Jan 15, 2019

@wilsonge wilsonge merged commit f02b11c into joomla:4.0-dev Jan 18, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wilsonge

This comment has been minimized.

Copy link
Contributor

wilsonge commented Jan 18, 2019

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

@wilsonge wilsonge added this to the Joomla 4.0 milestone Jan 18, 2019

@Hackwar

This comment has been minimized.

Copy link
Member Author

Hackwar commented Jan 18, 2019

Thank you!!!!

dgrammatiko added a commit to dgrammatiko/joomla-cms that referenced this pull request Jan 20, 2019

Merge branch '4.0-dev' of github.com:joomla/joomla-cms into 4.0-dev-C…
…E-password-field

* '4.0-dev' of github.com:joomla/joomla-cms: (239 commits)
  Fixed inconsistent use of plurals Ref joomla#22791 (joomla#23515)
  [4.0] Adding filter() and postProcessing() to FormField (joomla#12414)
  Csp doc block change. (joomla#23579)
  Fix missing use statement (joomla#23564)
  Move admin menu JS code to es6 (joomla#23155)
  [4.0] Remove show_associations field of association is not enabled (joomla#23501)
  [4.0] Add json to source_formats of template editor (joomla#23470)
  [4.0] [com_menus] Fix "Too few arguments to function (joomla#23535)
  Add $data to event_before_save in tags (joomla#23531)
  [4.0] Fix Legacy Model loading. Fixes joomla#23517 (joomla#23525)
  Update en-GB.com_templates.ini (joomla#23523)
  Reverse order of template params for green/red. Fixes joomla#23519 (joomla#23521)
  Finder Index: Check access to plugins (joomla#23509)
  Update draggable.js (joomla#23487)
  Undefined class \Joomla\CMS\Filesystem\AKFactory (joomla#23506)
  [4.0] Not only set the redirect but actually return (joomla#23498)
  fix notice in syndicate.xml (joomla#23502)
  Fix codestyle
  [4.0] Missing files in template manifests (joomla#23484)
  [4.0] WebAsset: split WebAssetRegistry to Registry and Manager (joomla#23463)
  ...

# Conflicts:
#	modules/mod_login/tmpl/default.php
#	package-lock.json
@infograf768

This comment has been minimized.

Copy link
Member

infograf768 commented Jan 28, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.