[#24816] - [4.0] Return correct validation result to form.
- Fixed in Code Base
- 3 Nov 2019
- Medium
- Build: 4.0-dev
- # 24816
- Diff
- Harmageddon:form_validation_return_value
- Success Hound No violations found. Woof! Details
- Success continuous-integration/travis-ci/pr The Travis CI build passed Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
- Success continuous-integration/drone/pr Build is passing Details
User tests: Successful: Unsuccessful:
As far as I see, #12414 seems to have broken the form validation for some fields (except those components which do their own validation after the form validation, like com_users). Please check and inform me if I'm mistaken.
Summary of Changes
The method FormField::validate checks whether the return value of FormRule::test is false and handles errors only in this particular case, returning a simple true for all other cases. The problem here is that FormRule::test might also return an Exception on failure, which doesn't evaluate to false.
A quick glance through the different validation rules shows that this is the case for EmailRule and under some circumstances maybe also for NotequalsRule.
I suppose that this bug can be solved by not naively returning true, but the very result of the validation instead. Of course, another method would be to unify the way we want to treat validation errors by deciding on either returning booleans, returning exceptions or throwing exceptions.
Testing Instructions
- Navigate to a form that has an email field using server-side validation.
The error doesn't really occur for creating/editing a user (it occurs, but is only visible in the different styling of the error message), because com_users does another validation when saving. I tested it with the "From Email" field in the global configuration under "Server - Mail Settings". - Enter an invalid value like "test".
- Save / submit the form.
Expected result
The form should not be saved and return an error.
Actual result
The form is saved.
Please also test that valid form submissions are still valid!
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
okay, I see,
old had:
$valid = $rule->test($element, $value, $group, $input, $this);
// Check for an error in the validation test.
if ($valid instanceof \Exception)
{
return $valid;
}and new:
try
{
// Run the field validation rule test.
$valid = $rule->test($this->element, $value, $group, $input, $this->form);
}
catch (\Exception $e)
{
return $e;
}so now it ignores if $valid are Exception.
The patch are good.
| Labels |
Added:
?
|
||
I have tested this item
works as descriped
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24816.
I have tested this item
works as advertised
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24816.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24816.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-11-03 00:44:20 |
| Closed_By | ⇒ | wilsonge | |
| Labels |
Added:
?
|
||
Thanks!
hm, maybe something else are broken?
because old behavior also has
return true;https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Form/Form.php#L2148or it was always broken