? Pending

User tests: Successful: Unsuccessful:

avatar marrouchi
marrouchi
3 Feb 2017

Pull Request for Issue #13869.

Summary of Changes

Added a JFormRule in com_fields to make sure default value is validated against the field type.

Testing Instructions

  1. Create a field (url, color, .. )
  2. Set an invalid value in default value field.
  3. Submit and you should get a validation message.
  4. Repeat with a valid default value and no validation message should appear.

Expected result

Displays a validation message.

Actual result

Does not validate the default value against the field type.

Documentation Changes Required

No.

avatar marrouchi marrouchi - open - 3 Feb 2017
avatar marrouchi marrouchi - change - 3 Feb 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 3 Feb 2017
Category Administration com_fields
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 3 Feb 2017

I have tested this item successfully on da3e463


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

avatar franz-wohlkoenig franz-wohlkoenig - test_item - 3 Feb 2017 - Tested successfully
avatar laoneo
laoneo - comment - 3 Feb 2017

This works for the core form fields. But when a custom field like the gallery one is used, then the rule doesn't get found. But first let's get this one in and I will add then support for none core fields.

avatar laoneo
laoneo - comment - 3 Feb 2017

I have tested this item successfully on da3e463


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

avatar laoneo laoneo - test_item - 3 Feb 2017 - Tested successfully
avatar marrouchi
marrouchi - comment - 3 Feb 2017

Thanks for the tests. RTC please !

avatar Bakual
Bakual - comment - 3 Feb 2017

I couldn't test this yet but I think rhe approach doesn't work. For example the calendar allows a special case "NOW" for the defaulr value, which eesults in the current date/ime displayed by default. This PR will break that if I read thw code right.
There likely will be other such cases.

Imho, it's the responsibilty of each formfield to make sure the default value is allowed if needed. Custom Fields mostly directly extend from those library classes and don't implement own logic.

avatar Bakual
Bakual - comment - 3 Feb 2017

Now had the chance to test this and looking at the code closer.
Actually calendar isn't affected because there is no JFormRuleCalendar. The whole code only works if the typename of the field matches the rulename, which for core is only the case for color and ùrl` as far as I see. So this doesn't make much sense to me.
For once the validation shouldn't be tied to the name of the field type and if we implement it in the component it should be more generic than only working for two fields.

I still think validation should be done in the formfield class itself and only if needed. And personally I think in most cases it's not needed at all. After all we currently don't sanitise the default values at all when set in the XML. It's the exact same code which "breaks" here when the default value is set wrong in the XML or in the custom field.

avatar zero-24 zero-24 - change - 4 Feb 2017
Labels Added: ?
avatar zero-24
zero-24 - comment - 4 Feb 2017

I have just fixed two CS issues.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Feb 2017

@Bakual should this PR be tested again?

avatar marrouchi
marrouchi - comment - 22 Feb 2017

@franz-wohlkoenig i'm working on another PR that i'll submit soon.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Feb 2017

Thanks @marrouchi so this PR can be closed?

avatar marrouchi
marrouchi - comment - 22 Feb 2017

Okay

avatar marrouchi marrouchi - change - 22 Feb 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-02-22 15:57:35
Closed_By marrouchi
avatar marrouchi marrouchi - close - 22 Feb 2017
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Feb 2017

Thanks @marrouchi

Add a Comment

Login with GitHub to post a comment