As this PR change $data array before passing to component model, If we want to be on safe side (especially merging it for 3.8.6), I think we should limit this normalize for com_fields field group for now only and then remove that limit for 4.0.

I will test it once we have final decision about this

first test show me that this fixes the deleting of fields data for removeFromUserGroup and addToUserGroup functions :) so far so good!
It also handles the checkboxes (not my issue, so just did some basic tests) correct.

Just noticed on one of my sites where I also use easy-profile (in combination with Joomla custom fields) that the current version of easy-profile also erases all custom fields on saving a user profile :( with the patch applied, the fields are NOT removed (=good), but the changed custom field values are also not saved (=not so good). I contacted the developer of easy-profile to look at this patch and test / contribute :)
fixed by ordering the easyprofile system plugin before the fields system plugin...

2 issues

1. if you try to save only a subset of custom fields then the field(s) not present in the form will loose their value(s)

e.g.
Create 2 text fields, edit an article and add text to them and save
then make 1 of them to be shown only in frontend (inside field options) and then re-edit the article in backend and save it,

• result is that the field looses its value, you can see that it lost its value by enabling the field to be shown in both Frontend / Backend forms

2. Issue 2 is B/C break, extensions now need to be updated to call the normalization method, e.g. inside their save task , but probably also in their custom code

Issue 2 is B/C break, extensions now need to be updated to call the normalization method, e.g. inside their save task , but probably also in their custom code

Not doing so if you have extended the save method in your controllers retains the existing behavior in the API and known bugs, i.e. potential data loss. Not a B/C break, otherwise you can argue any bug fix in a method which can be extended by a subclass is a B/C break, and we get nowhere.

result is that the field looses its value, you can see that it lost its value by enabling the field to be shown in both Frontend / Backend forms

Is this now not the case, even without the patch?

Is this now not the case, even without the patch?

Yes of course
it is an issue without the patch too, it is not an issue created by this patch
i just said it is not fixed by this PR

The target of this pr is not to fix that one. It is better to have one pr per problem instead of mixing things.

About B/C issue, ok its not a B/C issue,
but not only for the reason that you mentioned

but i think it is not a B/C issue also because 'normalizeRequestData'
does nothing useful

• well almost nothing

The only useful thing that it does is to set $data['com_fields'] to non-null value during save task of the controller,
thus you can detect case that a checkboxes field is the only field in custom fields group and it was submitted without any value

1. The benefit of having (and similar code) work properly
   JFactory::getUser($USER_ID)->save();

comes from the change in the fields system plugin

if (!is_array($data) || empty($item->id) || empty($data['com_fields']))

Thus it works only because the full loop of setting field values is skipped when the $data['com_fields'] is not given (completely missing), which is the case when doing

JFactory::getUser($USER_ID)->save();

2. Also the root issue is not addressed

root issue is
a field not submitting values is

• (a) because it was not added to the form, skipped because of custom form creation or skipped because of configuration setting, (=aka maintain value) ?
• or (b) it simply did not submit a value (checkboxes field), because e.g. it must be emptied ?

How do you know that a field was really added to a form that was submited ?

• the fact that the field exists in the fields definition of JForm does not guaranteed that it was submitted / provided,

the normalizeRequestData, does not give any more information than what the JForm already has,
thus normalizeRequestData provides no real benefit

normalizeRequestData can be made simpler

• just check if 'com_fields' exists in jform and if $data['com_fields'] is not set then set $data['com_fields'] to false

Don't get me wrong
this PR is a fix

it is a fix for custom fields lost when saving a record without providing any field values
but just the above

but it is not a fix for all the cases that PR #19752 was trying to fix

the normalizeRequestData, does not give any more information than what the JForm already has

Yes, it does. The internal data array in a form instance where the user data is stored doesn't arbitrarily have all of the form field names within it, so $form->bind($input->post->get('jform')) does not result in a dataset where you have all of the field names. Changing the form data store's internals so it at least defaults to ensuring there are keys for all the field names would be an acceptable change and potentially invalidates the need for this normalizer. Until that happens though, this is (to me) a valid change and is a pretty normal thing to do if you aren't working with a complex form abstraction like Symfony's form component, your form abstraction is request agnostic like our API is, or you aren't even working with a form API and therefore your request handler (controller or a service you build) has to do all the heavy lifting itself as is the case with a default Laravel project (not forgetting there are third party libraries that add a form abstraction but it's not core there).

Sorry, hit comment before I read the last bit of the comment I was replying to.

normalizeRequestData can be made simpler

just check if 'com_fields' exists in jform and if $data['com_fields'] is not set then set $data['com_fields'] to false

That is one use case, specific to one component/integration. To be honest, we need to stop coding component specific integrations into the libraries.

does nothing useful

I hope this are just our different cultures where we understand things differently, but such words have nothing to do in a public comments.

just check if 'com_fields' exists in jform and if $data['com_fields'] is not set then set $data['com_fields'] to false

As I wrote already in the description of the pr. I don't want to add extension specific code into the libraries. Michael pointed that out already and I agree that we should stop doing stuff like here or here.

The target of this pr is not to fix that one. It is better to have one pr per problem instead of mixing things.

Is there already a report created for this specific issue? Running into this here (#19473 (comment)) as well.

I hope this are just our different cultures where we understand things differently, but such words have nothing to do in a public comments.

I don't understand your comment,
but it sounds like if i had annoyed you or something, i am sorry, i have been subject to much more hard answers in this repository without taking offence

I understand that my comments in this topic are not useful, i will try to contribute elsewhere

I have tested this item ✅ successfully on d6e0a2f

Posting a successful test, since i did take time to test this PR.

It works for the issues that its description says it fixes

It does not introduce new issues (as far as i tested)

It passes little but useful enough information to "users" of $data like the onContentAfterSave event in our case, without doing something hacky,
and it maybe even more useful in the future

This PR solves the mentioned issues, custom fields data still being saved without any problem. There could be a B/C break if we have to care. If someone has a Checkboxes field in his form and in the save method of his model class:

1. Before this PR, he can use isset($data['checkboxes_field_name']) to check to see whether the options are selected and process further with the selected options.

2. After this PR isset($data['checkboxes_field_name']) will always return true (not depends on the options are selected or not) because we set $data['checkboxes_field_name'] = false; if not options are selected.

That's why I added in the description of the pr

While doing this one, I think it would be safer to target it against 3.9 or even 4 and add a workaround like #18188 in the 3.8 series.

I have tested this item ✅ successfully on d6e0a2f

fixes the fields data getting erased when using addUserToGroup and removeUserFromGroup functions.

Ready to Commit after two successful tests.

The RTC label should be removed as there is potential B/C issue I mentioned earlier #19753 (comment) . PLT should make decision first before merging.

Staus back on "Pending" as stated above.

I have tested this item ✅ successfully on 35dede7

tested successfully: $user->save | fields are not removed but data is retained in database
tested successfully addUserToGroup / RemoveUserFromGroup API calls | fields are not removed but data is retained in database
tested successfully Shown On: Site / Administrator | fields are not removed but data is retained in database
tested successfully form where some fields where not displayed | fields are not removed but data is retained in database

Note: checkboxes when not set will now save as empty in #__fields_values. Before this patch empty checkboxes where not saved / deleted from #__fields_values

@Ruud68 with the last commit it should delete the value from the empty database as well.

It will not delete the checkboxes when they are hidden from the form

but with this last change it is not possible to save value '0', when changing e.g. from any singular old to value '0', which is valid value for number but also for radio, checkboxes, text, etc

@Ruud68 with the last commit it should delete the value from the empty database as well.

following happens:

1. I have an article
   in the #__fields_value there are no entries for this article
2. I create 2 fields (text and checkboxes)
3. I open and save the article (not setting any values in the fields)
   There are two rows created for this article in #__fields_values: both with value '' (empty)
4. I reopen and save the article (not setting any values in the fields)
   Both entries for this articles fields are now removed in #__fields_values

So when reading a field I can have four scenario's:

1. A value is set > I get the value
2. No value is set > I get null (article saved before fields created)
3. No value is set > I get empty (article saved after fields created)
4. No value is set > I get null (article saved 2nd+ time after fields created)

Scenario's with 3.8.5 (without patch):
A.
So your last commit 'fixes' the B/C for checkboxes but has the effect that other fields are also deleted when empty instead of saved as empty.
I think this is not a B/C condition because before this change we always needed to check for is_null (no row in #__fields_value) or empty (scenario 3). The chance that the value is null (not set in #__fields_values) is after this change more likely.

Not sure what this will do for performance. when I have 1000 articles and 100 fields, before the change I have 100.000 values in #__fields_values, after the change I only have the number of rows where the fields actually hold a value.

Conclusion: for my test cases this change works: I already checked for empty values via empty() or is_null :)

1. I have an article
   in the #__fields_value there are no entries for this article
2. I create 2 fields (text and checkboxes)
3. I open and save the article (not setting any values in the fields)
   There is one row created for this article in #__fields_values: text field with value '' (empty)
4. I reopen and save the article (not setting any values in the fields)
   same as three

So without the PR the value of a field can either be:

• null (not created),
• null (checkbox not set)
• empty ((text) field empty)

If you can prove actual data loss then there's an issue to address

this is my 38 test site. Fresh install from 3.8.0 upgraded with every release. After tests I restore back to back upped version. Currently on 3.8.5.

(video) https://office.onlinecommunityhub.nl/nextcloud/s/ZpF578aaHCrr4eY

@joomdonation should this PR be tested (you stated that PR should'nt be tested because of "potential B/C issue" - comment)?

I'm asking cause comment by @rmbutton.

@joomdonation should this PR be tested (you stated that PR should'nt be tested because of "potential B/C issue" - comment)?

Wait for it :)
Still working a a last 'glitch' where article text gets removed (com_content).

@Ruud68 pointed out a valid issue here Digital-Peak#8. The consequences would be that the bind function needs to modify the variables only in the array as in my last commit 6dbed37. IMO this can be counted as a BC break now. So I suggest to target this pr against 3.9 or even 4 and add a workaround like #18188 in the 3.8 series.

I guess we leave the final decision on @mbabker how to proceede.

What we are trying to fix (and succeeded in) is to save com_fields fields that are not on the form (either because we are on a front-end form and they only display on the back-end form, or because we save a user / content via the api).
This now works because of the combination of the normaliseRequestData function in the form controller (adding the 'missing' form fields) in combination with the fields system plugin that adds the values to those fields when saving.

Currently the normaliseRequestData function not only adds the missing com_fields fields but ALL fields that are missing on the form. So when you have set com_content to only show the tab 'Images and Links' in the back-end, then values set for e.g. the intro image (set in the back-end) will be erased when saving the article from the front-end (field not on the form, added to the form via normaliseRequestData with value 'false'). This is one example, but it applies for other fields as well.

So the current discussion should IMO not be about B/C (yet), but about if this is the correct approach in solving the removing of com_field fields values when not displayed on the front-end when saving.
It could be 'fixed' by introducing component (com_fields) specific 'logic' into normaliseRequestData, but I am not sure if that 'cure' is worse that what it 'fixes'?

EDIT: I can do a PR for the normaliseRequestData function (that I renamed to normaliseFieldsRequestData to make clear that it is (com_)Fields related :)
This 'limits´ the B/C as it only touches the com_fields part of $data leaving all other values as they where before. This fixes the data being lost bug when saving a user / content with fields that are not visible on the front-end (set in back-end)
Just say GO :)

Anytime you start to add component specific logic to root library functions, you're doing something wrong. We shouldn't get to put component specific code in the libraries just because those components are part of what comes out of our build script.

When you do NOT limit the normaliseRequestData to com_fields (by introducing component specific code), that function needs to be taken out again because it breaks core functionality.

Anytime you start to add component specific logic to root library functions, you're doing something wrong.

Do you see another place where the com_fields fields can be 'attached'?

I don't know the system design so I can't answer that. But the general rule of thumb is if you have to shove component specific logic into base level library functions you're doing it wrong. What it is is to be determined, but something is wrong.

I don't know the system design so I can't answer that.

Same here :( maybe I am to focused on the current approach, but If that is not the place then (I think) the fix should be down one level: in every component utilizing com_fields. And then again, you will be introducing component com_field specific logic into e.g. com_content / com_users...

It's not as bad to have cross component dependencies as it is to have library level dependencies to components (architecturally library level code is supposed to work without components whereas components may (and generally do) have dependencies to the libraries). com_content is probably aware that it is supporting com_categories, com_fields, and com_tags. The problem is when the base admin model starts becoming aware that it needs to do stuff for a specific component, that starts breaking encapsulation and makes it aware that there is component logic that is going outside of a component's scope.

Okay,
I (think I) have fixed this. What I have done is remove the normaliseRequestData from the FormController. This avoids a lot of potential B/C issues as normaliseRequestData was performed on all extensions (even none-core ones).

I have integrated the logic of normaliseRequestData (and then specific for com_fields) in the extension's model function validatesave (this is called from the FormController: $validData = $model->validate($form, $data); if (!$model->save($validData)))

So now the components model does the 'magic': com_content, com_users, com_contacts (am I missing one?)

I am doing some final tests: first tests look promising :)

What I would like to know is, is this a solution that could get the approval of @mbabker and should I do a fresh / new PR for this @laoneo ?

BTW, this will also handle correctly the 'save as copy' method: when pressing save as copy, the fields that have values also get copied into the newly created article :)

Where exactly do you want to put it? Into the FormModel class or in the component models which do extend the FormModel? IMO the model should only process the data, the controller's job is to transform the data from the request in a valid format for the model.

I just refactored it into the models save function (it was the validate function, but that is not logical I think)
This is also where the metadata, urls, images, etc are handled so that would then also be a logical place to handle the field values
so in ./administrator/components/com_content/models/article.php

	public function save($data)
	{
		$input  = JFactory::getApplication()->input;
		$filter = JFilterInput::getInstance();
		$form = $this->getForm($data, false);

		JLoader::register('FieldsHelper', JPATH_ADMINISTRATOR . '/components/com_fields/helpers/fields.php');
		$data = FieldsHelper::normaliseFieldsRequestData($form, $data);

		if (isset($data['metadata']) && isset($data['metadata']['author']))
		{
			$data['metadata']['author'] = $filter->clean($data['metadata']['author'], 'TRIM');
		}

and in ./administrator/components/com_fields/helpers/fields.php

	public function normaliseFieldsRequestData($form, array $data)
	{
		// Loop over all fields
		foreach ($form->getFieldset() as $field)
		{
			// If the field has no group, we are done here
			if (!$field->group == 'com_fields')
			{
				continue;
			}

			// Ensure there is always an array for the group
			if (!key_exists($field->group, $data))
			{
				$data[$field->group] = array();
			}

			// Make sure the data group array has an entry
			if (!key_exists($field->fieldname, $data[$field->group]))
			{
				$data[$field->group][$field->fieldname] = false;
			}
		}

		return $data;
	}

this works as expected: removing empty checkboxes, retaining values for fields that are not displayed on the form when saving)

I guess it is easier to make a new pr with your changes. So we can compare better.

top, com_content tested okay, now for com_users and com_contacts). Are there any other 'core' components beside these three that utilize com_fields?

Nope.

yeay... #19850

Closing this one in favor of #19884 which normalises the data in the fields plugin in a new event.