? Pending

User tests: Successful: Unsuccessful:

avatar Ruud68
Ruud68
6 Mar 2018

Pull Request for Issue # .

Summary of Changes

After lot of discussion here: #19753 we decided to take a different approach :) (bringing the required changes into the components model)

Testing Instructions

  1. use $user->save
  2. use UserHelper function addUserToGroup / removeUserFromGroup
  3. save on front-end article with fields that only display on administrator

Expected result

  1. user saved and field values retained
  2. user group membership changed and field values retained
  3. article saved and all field values retained

Actual result

  1. user saved but field values erased from #__fields_values
  2. user group membership changed but field values erased from #__fields_values
  3. article saved, but field values that are not displayed on the form erased from #__fields_values

Documentation Changes Required

Fields with no values (empty) would be saved in #__fields_values.
Checkboxes with no value would NOT be saved (erased) in #__fields_values

After this change:
All fields with no values (empty) will NOT be saved (erased) from #__fields_values

avatar Ruud68 Ruud68 - open - 6 Mar 2018
avatar Ruud68 Ruud68 - change - 6 Mar 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 6 Mar 2018
Category Administration com_contact com_content com_fields com_users Front End Plugins
avatar mbabker
mbabker - comment - 6 Mar 2018

Is there really no way to fix this problem without special handling of com_fields data? That is part of my problem with all of the approaches suggested thus far, it mandates that whatever component is handling the save request also validate the com_fields data structure. Something about the implementation of all of this is just wrong (not necessarily this PR, but at this point I'd suggest the entire com_fields integration has some kind of workflow problem because I would not expect a component adding support for fields should have to do any work with that addon data other than at the most ensuring the data is read in from the request for processing, which seems to be the case since it is part of the jform POST data).

avatar Ruud68
Ruud68 - comment - 6 Mar 2018

which seems to be the case since it is part of the jform POST data

and that is IMO where the issues is: it is in the fields that are NOT part of the jform POST data (e.g. a field set to show only on administrator). These fields get deleted from the database.
So (my) logic would say: do not delete the fields that are NOT on the form... but that is not possible because of e.g. checkboxes that are NOT checked are NOT part of the form.
So how to differentiate between (not on form and not delete (show administrator only)) and (not on form and delete (because not checked))? You can only check the difference by adding com_fields logic.

Can this be the root cause? checkboxes being removed from the database instead of saved in database with value empty (like all the other fields?)

avatar mbabker
mbabker - comment - 6 Mar 2018

The component MVC does not actually save the com_fields data, that's all being done through some series of plugin events. Why does the component MVC need to validate or post-process (pre-?) the com_fields data given it is not responsible for the save action (unless you're going to call the model triggering onBeforeSave and onAfterSave being responsible for it, which is a real stretch)?

IMO deleting the value for an unchecked checkbox from the database is not the root issue, or arguably even a major issue at all. A checkbox should be considered a boolean field, so saved in database === true and anything else === false (or saved in database with any falsy value, i.e. empty string, 0, etc.). If you're trying to use a checkbox for anything else then you're misusing the field type and need to use another type (radios are a common alternative). You're focusing too hard on the checkbox use case as that one, by the field's definition, has already been addressed.

avatar Ruud68
Ruud68 - comment - 6 Mar 2018

Why does the component MVC need to validate or post-process (pre-?) the com_fields data given it is not responsible for the save action

The save action is part of the fields system plugin > public function onContentAfterSave($context, $item, $isNew, $data = array())
But at that point in the process, we only have the data and not the form. So because a field that is not on the form is also not in the data (fields only shown to administrators) the value for these fields are being deleted.
So we add the field in the plugin onContentAfterSave when not set in the data with the value in the table. This 'fixes' the removal from the database of fields that are not on the form (only shown to administrators). But when you clear the checkboxes field from the form, the field gets unset from the form / data. So when the plugin onContentAfterSave kicks in, the old value is added again for this checkboxes field.

What we can do is in the FormController 'validate' the $data against the Fields Helper public function normaliseFieldsRequestData($form, array $data).
Is this introducing component specific logic? You already do that in the FormController when you validate the data via:

	// Test whether the data is valid.
	$validData = $model->validate($form, $data);

Here you also do component (com_content, com_user, com_etc.) specific validating and 'altering' of the data. (e.g. com_content unsets at this point $data[ 'created_by'] and $data['modified_by'])

You're focusing too hard on the checkbox use case as that one, by the field's definition, has already been addressed.

I am focusing on the data being lost when saving on front-end with fields that only show on administrator :)

avatar mbabker
mbabker - comment - 6 Mar 2018

$model->validate() is based on a form definition built by the controller and model. FieldsHelper::normaliseFieldsRequestData() is very explicitly parsing the request data for the com_fields data array. So by explicitly calling this function, you are adding an extra condition that is only required for com_fields support inside a component, even though the component is not doing anything else related to the handling of com_fields data.

The save action is part of the fields system plugin > public function onContentAfterSave
But at that point in the process, we only have the data and not the form. So because a field that is not on the form is also not in the data (fields only shown to administrators) the value for these fields are being deleted.

Therein lies part of the problem. You have the submitted data, you don't have the form it was bound/validated against. So you're blindly trying to react in part because you don't have all required data to make sound decisions. All of the problems that are being worked around are dealing with data not being present because a field is not displayed in a context (or an entire form not loaded at all in a context, like the tasks to deal with usergroup assignments).

avatar Ruud68
Ruud68 - comment - 6 Mar 2018

Therein lies part of the problem. You have the submitted data, you don't have the form it was bound/validated against. So you're blindly trying to react in part because you don't have all required data to make sound decisions. All of the problems that are being worked around are dealing with data not being present because a field is not displayed in a context (or an entire form not loaded at all in a context, like the tasks to deal with usergroup assignments).

I fully agree and that takes us back to:

Something about the implementation of all of this is just wrong (not necessarily this PR, but at this point I'd suggest the entire com_fields integration has some kind of workflow problem because I would not expect [...]

So question is then:
Is the data loss a design 'consequence' of com_fields?
and if so:
Do we change the (design) implementation of com_fields, or create a 'workaround' with the least possible impact?

Who can / should answer these questions from the Joomla! team?

avatar laoneo
laoneo - comment - 9 Mar 2018

To move the logic into the fields system plugin, we can introduce a new plugin event which is triggered in the controller to normalise the data. So the plugin can then do the normalisation. Just an idea.

avatar Ruud68
Ruud68 - comment - 9 Mar 2018

Hi Allon, I like the idea and I think this would work in a standardized (read: not component specific) way.
@mbabker would this be an option?
What I learned is that in Joomla4 the way plugins are being looked at changes, haven't looked into what these changes actually look like. So maybe good to get the person (who?) involved in this project to share his/her thoughts?

avatar mbabker
mbabker - comment - 9 Mar 2018

What I learned is that in Joomla4 the way plugins are being looked at changes

Not really. Nothing has any required change until 5.0.

avatar Ruud68
Ruud68 - comment - 9 Mar 2018

Not really. Nothing has any required change until 5.0.

I thought so, been working on making my plugins compatible with Joomla4 and they all work without changes (other then replacing deprecated things and using namespaces) :)

but now for the 'big' question :)

@mbabker would this be an option?

avatar Ruud68
Ruud68 - comment - 12 Mar 2018

Closing this one in favor of #19884

avatar Ruud68 Ruud68 - change - 12 Mar 2018
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2018-03-12 15:49:21
Closed_By Ruud68
Labels Added: ?
avatar Ruud68 Ruud68 - close - 12 Mar 2018

Add a Comment

Login with GitHub to post a comment