Fix deleting Custom Field value from table when Field not set in object

[Ruud68]

Pull Request for Issue # https://issues.joomla.org/tracker/joomla-cms/19735

Summary of Changes

When saving a user object that has NO custom fields attached in a specific context, the saved custom fields for that object will be erased. This will happen e.g. when a the function removeUserFromGroup or function addUserToGroup is called from a component to remove or add a user from a user group.

Testing Instructions

1. add custom fields in com_user
2. add values to these fields to a user
   The values should show in the user profile
3. use a component (like rd-subscriptions or probably any membership component that uses the UserHelper function addUserToGroup / removeUserFromGroup) and 'remove' the user form a group

Expected result

The filed values for this user should not be deleted

Actual result

The field values for this user are deleted (from the #__fiels_values), because when calling the removeUserFromGroup / addUserToGroup the user object doe not have the set fields attached to the object and there for the value for these fields is set to NULL

Documentation Changes Required

none.

[joomdonation]

We have been having few PRs trying to fix this issue without success. The reason is because the way checkbox custom field type work. With this modification for example, I think you cannot un-select all checkbox option.

[Ruud68]

@joomdonation, this is not specifically for check-boxes, this fix is for any fields getting deleted on a user object (so also media, text, textarea, etc.)

[joomdonation]

I am sorry, my comment was not clear. I meant with your fix, custom field checkbox options could not be be unchecked anymore (unchecked all options). To test it, you can:

1. Create a custom field Checkboxes with few options

2. Edit an item (can be user or article), select the options, save it

3. Try to edit that item again, try to unselect all options. Check it again, these options could not be unselected (it remains selected after saving)

(That comes from checking several PRs in the past and reading your code, I haven't tested this again myself yet)

[joomdonation]

FYI below are some PRs

#18207
#18188
#17837

[ggppdk]

To continue what joomdonation is saying,

If you uncheck all values in a checkbox array (or if you have a single checkbox and you uncheck it)

• then browser will not POST any value, nothing, so $value will be null

thus it will happen what joomdonation described above, you will not be able to uncheck all checkboxes

[Ruud68]

@joomdonation okay, I understand now and see the issue that it introduces for checkboxes. Am I correct in thinking that the cause of this is the way the checkbox fields are implemented: when no checkbox set remove field from table instead of when no checkbox set save field value as empty?

[joomdonation]

Actually, it causes by the way checkbox works. See the comment from @ggppdk above #19742 (comment) to understand

[Ruud68]

got it :), just changed my PR, for me this works (including checkboxes), does it work for you @joomdonation ?

[ggppdk]

You got it, but almost

you see if all custom fields in the form are of checkbox type
then you will get the same problem

because then no values will be posted for
jform[com_fields]

[joomdonation]

As @ggppdk said, it doesn't work

[Ruud68]

shoot :( is this an issue only for checkboxes or are there other fields that get erased from the table when empty?

[joomdonation]

Checkbox and Checkboxes , see #18207 for the proposed solution but was rejected

[mbabker]

#18207 was never rejected. I voiced a concern about the code/API design and the submitter voiced frustration and requested it closed.

[Ruud68]

Okay, finally got it (working that is). Following the logic already in place for the activate, block and unblock save actions.

[ggppdk]

ok, but you have changed the PR completely

you are now making this PR to fix the issue for a specific component (com_users),

but that might have been ok,
if you were not changing the task variable (HTTP request) during user save call

$user->save();

• that may break user related extensions that listen to the plugin event and examine the task variable (i am not sure)

[ggppdk]

Probably the proper fix is similar to what #18207 did

• add a flag to fields that may not post any values (aka to the same fields that [com_fields] fix with deleting data on item update, forms checkbox fix #18207 did)

public $addFormPresence = true;

and then use:

$input = JFactory::getApplication()->input;
$jformData = $input->get('jform', array(), 'array');

$existence = isset($jformData['__existence']['com_fields'])
	? $jformData['__existence']['com_fields']
	: array();

...
foreach ...
{
  ...

	$presentInForm = key_exists($field->name, existence);
	$valuePosted = !is_null($value);

	// Always set to true, since canEdit is checked inside setFieldValue method
	$canEditFieldValue = true;
	
	if ($presentInForm || $valuePosted) && $canEditFieldValue)
	{
		// Setting the value for the field and the item
		$model->setFieldValue($field->id, $item->id, $value);
	}
}

[mbabker]

The Form API shouldn't be aware of what the request (or for that matter whatever input source it has bound to it) does or does not send. This is a level of filtering/validation that should be handled closer to the request (typically a controller or model).

Please remember the form field objects in our API are primarily layout renderers, not data handlers/validators.

[Ruud68]

Hi @ggppdk the PR has changed, but the issue is still the same. The task is only changed when using the Joomla UserHelp functions to add / remove groups. the task is only temporarily changed and after adding or removing the groups it is changed back to the task of the calling component.

[mbabker]

OK, just looked at the code diff for the PR. There is a major problem here. There is a huge internal coupling somewhere in the system on the request data, specifically the task parameter, and whatever is acting differently based on that parameter is where our root issue lies.

This PR is just another bandaid fix over the root issue. Again, it might fix this particular code path and problem, but we aren't addressing the root issue here. And we really need to fix the root issue once and for all, not continue to patch every code path that leads up to that issue with unstable workarounds.

[ggppdk]

@mbabker

The Form API shouldn't be aware of what the request

yes correct, i thought of it too , i also did like adding the request there !!

-but it is already being used inside the fields plugin code in 1 or 2 other places

we could do it without the request there but in such a case in would involve (?much) more work to also add changes to JForm validation

• basically JForm should have had a field-presence mechanism, regardless of com_fields

@Ruud68
yes but i am talking of the period of this temporary change

• your change is changing 'task' during various events !! e.g. during 'onUserAfterSave'

[ggppdk]

@mbabker

I think the root issue is
that JForm (regardless of com_fields) does not have a field-presence mechanism

[mbabker]

that JForm (regardless of com_fields) does not have a field-presence mechanism

Nor should it. Because JForm is NOT the request handler. The MVC layer is the request handler, JForm is simply given a data array to be bound to it and can run a set of validation rules based on the form's XML definition. None of this is request aware (and inherently "field presence" aware). Which means that you need to validate the data array before/after binding it to the form to ensure fields are defined with a value, even if null, if you need that particular behavior. JForm isn't stripping null values, it works with a Registry instance and it has no issue whatsoever with doing $registry->set('field', null); and retaining data.

but it is already being used inside the fields plugin code in 1 or 2 other places

This is not the Form API. If it is not JForm, JFormField, JFormRule, or a subclass of those it is not the form API.

[Ruud68]

I was also working on another PR (https://issues.joomla.org/tracker/joomla-cms/19473)
Here I also ran into the challenge that fields that are not displayed (because the user didn't have display rights) where removed from the database.

So when we are able to agree on how to fix this issue (which is a production issue as I am loosing user data on multiple sites with every subscription that expires / reactivated), I can give it a shot when given direction :)

[mbabker]

So when we are able to agree on how to fix this issue (which is a production issue as I am loosing user data on multiple sites with every subscription that expires / reactivated)

Here's how I see it.

• "Field presence" validation belongs in the MVC layer as you're losing certain fields because they aren't received in the request. In a save task, after reading the form data from the request, you would need to validate fields are present and give default values as needed (i.e. those checkboxes that only submit with the request if they are checked) before forwarding into the model where data is bound to a JForm object and validated against that. I don't know the com_fields integration all that well so I don't know how exactly that validation step applies in com_fields but it should be possible to do that.

• Task checking in fields system plugin is a very weak thing, as demonstrated by the repeated issue reports and PRs attempting to fix it. There needs to be a better way to determine if that code should fire so we aren't so easily losing user data. And if that code does fire, I dare suggest there should be some kind of internal validation that prevents writing an empty dataset as it sounds like what is happening is the fields code is firing off and writing an UPDATE or DELETE query based on an empty dataset (presumably some array from the request by default).

[Ruud68]

Just a though after looking at this almost all day.
default behavior for a text field is that when it is not set, it is not present in the database.
When set, the value is in the database, when the value is deleted (made empty), the value (empty) is still in the database.

This logic is different for checkboxes: when not set they are not in the database (=same as text field), when set the value is in the database (same as text field), but when 'unchecked' (made empty), they are removed from the database whereas the text field is still in the database with an empty value.

What if we changed that behavior: when a checkbox is 'unchecked' set the value in the database to '' (empty) instead of removing it?

[mbabker]

What if we changed that behavior: when a checkbox is 'unchecked' set the value in the database to '' (empty) instead of removing it?

That's the part about checking the input data I was mentioning. Because a checkbox field won't be in the request unless checked. This is basically my workflow in a project to ensure checkbox fields are always set (modified for Joomla API use):

$data = $this->input->post->get('jform', [], 'array');

foreach ($arrayHoldingCheckboxFieldNames as $checkboxField) {
    // If the checkbox is already in the array, we're good, otherwise we need to set it and since checkboxes typically should be a boolean toggle, default to false if not set
    $data[$checkboxField] = $data[$checkboxField] ?? false;
}

Again, I don't know how this translates into com_fields logic but at the end of the day this particular validation step does NOT belong to the Form API (libraries/src/Form) and we should not be manipulating it to address this scenario.

[Ruud68]

Maybe @laoneo can tune in (if not already :))?

[laoneo]

I'm here with @mbabker. This pr does not solve the cause of the issue.

[Ruud68]

okay, just to stop the 'bleeding' of user data on my (and others?) production sites I have changed this PR to NOT use the task but introduced another variable to switch on. This should not break things.
Note: I am sharing this PR as a temporary fix (for data loss and check-boxes not saving) that people can apply them selves pending the (new) PR that will fix the root-cause :)

I am willing to work on a definitive solution, but please keep in mind that for me this is a steep learning curve. So although I'm willing, being able depends on the developer community in giving me guidance, direction, padding on the shoulder and kicks under my you know where :)

[laoneo]

I would add somewhere here a check if all fields have a value from the request. At this point you have the form to get the fields and the data from the request.

[ggppdk]

At the place suggested by @laoneo i was thinking to
to add the reading of posted data for field presence and also validate them there

so i have added

// Test whether the data is valid.
$validData = $model->validate($form, $data);
	
/**
 * Validate field presence array, this is used by some fields together with canEditFieldValue
 * to decide if field's values should be emptied or if field values should be maintained
 */
\JArrayHelper::toInteger($presence);
$validData['_presence_'] = $presence;   // Pass presence information

Plus changes to renderField() method of FormField.php
(to add the presence 'input' for fields that have flag )

public $addFormPresence = true;

Plus changes to the fields system plugin to use $validData['_presence_']

$presence_name = 'com_fields_' . str_replace('-', '_', $field->name);
$presentInForm = key_exists($presence_name, $presence);
$valuePosted = !is_null($value);

// NOTE: canEdit field value is checked inside setFieldValue method, so need to check it here
// Even if field presence was tampered with it is OK if user has ACL to edit field value !
if ($presentInForm || $valuePosted)
{
	// Setting the value for the field and the item
	$model->setFieldValue($field->id, $item->id, $value);
}

and seems to be working
i will make a PR tomorrow and we will see if implementation is proper ... and after testing it gets accepted

[Ruud68]

Top: I will be testing and also test against my own PR for the Fields View Level that I am 'cooking' (https://issues.joomla.org/tracker/joomla-cms/19473) here I am running into the issue that when a field is not displayed on the form it gets deleted from the database when the user saves his profile :)

[ggppdk]

Here is yet another PR #19752, that tries to resolve the issue by adding field presence in the form

[Ruud68]

Okay, while browsing the changes in the joomla-cms project I found #16958 (@joomdonation PR).

This could also be the solution for this issue:
When there are no fields attached to the user object (because (in the case of removeUserFromGroup and addUserToGroup does not go via the form but via an api call), we can read and attach the existing fields for the user object.
That way the removeUserFromGroup and addUserToGroup functions will work correct and it will also solve the checkboxes issue.

Just posting here before doing the code changes to avoid a lot of work :) Not sure if this can be classified is a work around or could be seen as real fix as it is also used (and approved) in com_content.

[joomdonation]

@laoneo is working on a PR, hopefully with a proper fix. Hope we can get this issue sorted in 3.8.6. Please wait.

[Ruud68]

okay, damn. Somehow missed that he was working on a fix (banging my head against the wall for missing that as it would have saved me a lot of work today :S)

[Quy]

@joomdonation when/where was this mentioned?

[joomdonation]

Sorry, I had a quick chat with him on Glip

[laoneo]

I saw all the comments this morning and saw the closed pr from @ggppdk so I thought I do try also one. Please test #19753. But extensively!

[Ruud68]

Closing this one in favor of #19884