[#18207] - [com_fields] fix with deleting data on item update, forms checkbox fix
- Closed
- 20 Jan 2018
- Medium
- Build: staging
- # 18207
- Diff
- Denitz:com_fields_fix
User tests: Successful: Unsuccessful:
Summary of Changes
Main concept is:
checkboxandcheckboxesfields have a special property$ensure_valueto indicate that these fields require any value.
JForm::filter() method now accepts $ensure_values param enabled by default, hence the filtered data always have values for such fields (hence saving empty checkbox will have data)
- Custom fields plugin only updates the value which really exists in
$data['com_fields']
Result: no custom fields data is lost on items save and checkboxes fine in forms as well.
No BC breaks.
Related to #18188, #17837, #17801.
Testing Instructions
Save users/articles with checkboxes custom fields.
Ensure that checked, unchecked data is saved.
Save user profiles, reset password, block/unblock/activate users, ensure that no custom fields values are lost.
Perform direct JUser::save() related operations, ensure that no custom fields values are lost:
$user = JFactory::getUser($USER_ID);
$user->save();
JUserHelper::addUserToGroup();
JUserHelper::removeUserFromGroup();
joomla-cms-bot
- | Category | ⇒ | Libraries Front End Plugins |
| Status | New | ⇒ | Pending |
| Labels |
Added:
?
|
||
Basically yes, but if a field does not have value for radio (unchecked), it won't have value in $data, hence it's correct.
Single selected radio can't be unchecked and will always have value in POST, multiple radios with checked value will always have a value in POST.
Radio just can't imitate the problematic behaviour of checkbox when current state is checked and we save unchecked field, radio can't be unchecked to none-value.
Btw it's strange that here I see continuous-integration/drone/pr — this build is pending but http://ci.joomla.org/joomla/joomla-cms/133 looks like completed and without issues.
@Trossler can you please test?
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18207.
I really would like to get the thing sorted. Can you test, that the following situations do work:
$user = JFactory::getUser(); $user->save();Should not erase the data.- Password reset is not erasing the data
- If you have one checkbox field and you save the article with it checked and the second time unchecked, that it works.
- Same with radio.
- Same with select box.
If this cases do work, then I'm convinced that we are pretty much on the safe side and I will be willing to close #17837 in favor of this one. Thanks for all the work to get it fixed properly.
| Status | Pending | ⇒ | Information Required |
I have tested this item
Applied the patch, tested all cases mentioned by @Denitz and @laoneo and it is working as expected, did quick code review, too. I think this is a big bug and should be fixed in 3.8.4 if it is possible
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18207.
Here is the info on naming conventions:
https://developer.joomla.org/coding-standards/php-code.html#user-content-naming-conventions
Thanks @joomdonation for the test. Agree, this needs to be fixed for 3.8.4.
| Title |
|
||||||
@joomdonation can you please retest?
@Quy can you please test?
Look like the file fields.php.orig was committed by mistake, please remove it
Sorry, where is it?
I have tested this item
Tested it again today, still work as expected.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18207.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18207.
| Status | Information Required | ⇒ | Ready to Commit |
RTC
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/18207.
| Labels |
Added:
?
|
||
Looking at this, it doesn't feel right that the filtering method is now responsible for setting default values on form data. This should really be part of binding data to the form, and inherently the bind method, unless there is some specific need for it to happen at the filtering step.
Sounds logic, but we need to make it work for current code.
JForm::filter() checks all fields and returns updated data which is next used.
JForm::bind() doesn't return data and checks only passed data.
Something doesn't feel right about this. Filtering the data shouldn't result in a data array with more info than what it was given, that's what's making me uncomfortable about clicking the merge button right now (ya it works but it's hard to ignore my gut feeling that this isn't right at the moment).
Please delete this PR in this case, there is nothing left to discuss here. 4th month is passing, wasted time.
Sure, let there be bugs and crutches. I wish it could be permanently deleted from both Github and my memory as well.
brianteeman
- | Status | Ready to Commit | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-01-20 17:43:03 |
| Closed_By | ⇒ | brianteeman |
I'm not saying the PR should be rejected to be clear here. Just on an architectural review, this feels like it is not the appropriate solution. A filtering API method by its name should indicate that it filters the given data, not add extra data to the input source. If this is really the only solution to the problem, so be it, but I am personally not comfortable with it right now. Nothing more, nothing less.
I guess there are more form fields which do not send data when nothing is selected like radio buttons, or I'm wrong?