PR-staging

Success

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
2 Sep 2017

Pull Request for Issue #17801.

Summary of Changes

The problem with the current way of processing forms and handling the saved data is that com_fields has no clue if the current save operation is one where custom fields got activated. This pr adds a hidden flag that this form is processed which gets checked on the save operation.

For me this workaround is a hack, but for the moment I have no other clue how to do it. @Bakual any feedback?

Testing Instructions

  • Create custom user fields.
  • Edit user and fill any data in custom fields.
  • Execute: $user = JFactory::getUser(); $user->save();

Expected result

The user custom fields data stays.

Actual result

The user custom fields got wiped out.

avatar joomla-cms-bot joomla-cms-bot - change - 2 Sep 2017
Category Administration com_fields Front End Plugins
avatar laoneo laoneo - open - 2 Sep 2017
avatar laoneo laoneo - change - 2 Sep 2017
Status New Pending
avatar Bakual
Bakual - comment - 2 Sep 2017

For me this workaround is a hack, but for the moment I have no other clue how to do it. @Bakual any feedback?

Same as you, it's a dirty hack but I don't have a better idea right away.

avatar laoneo laoneo - change - 12 Sep 2017
Title
[RFC] Add processing flag for custom fields request
[com_fields] Add processing flag for custom fields request
avatar laoneo laoneo - edited - 12 Sep 2017
avatar Denitz
Denitz - comment - 18 Sep 2017

@Bakual
Why not to skip PlgSystemFields::onContentAfterSave() if $data doesn't have com_fields key?

public function onContentAfterSave($context, $item, $isNew, $data = array())
	{
		// Check if data is an array and the item has an id
		if (!is_array($data) || empty($item->id) || !isset($data['com_fields']))
		{
			return true;
		}
...
avatar laoneo
laoneo - comment - 18 Sep 2017

Because when you have a checkbox which was checked in the request before and then on the second save unchecked, the com_fields array does not exist, but it is a valid request with custom fields.

avatar Denitz
Denitz - comment - 18 Sep 2017

Sure, old problem. In this case JForm::filter() can auto-inject empty string for Checkbox/Checkboxes fields.

All code should first filter data, next validate it, next use $data, e.g. see password reset code:

// Filter and validate the form data.
$data = $form->filter($data);
$return = $form->validate($data);

It will help to fix ALL forms with checkboxes as well.

avatar raviolican
raviolican - comment - 28 Sep 2017

We still face the problem also when we've applied those changes. When the password gets reset
custom fields get emptied.
Is there another bug?

avatar Denitz
Denitz - comment - 1 Oct 2017

It's same issue with password reset, but it should be fixed with this PR.

avatar laoneo
laoneo - comment - 1 Oct 2017

@Denitz did it work on your site the password reset with this patch?

avatar joomdonation
joomdonation - comment - 3 Oct 2017

@laoneo This PR doesn't solve the user custom fields data lost on password reset. There are few things need to be addressed:

  1. There is blank Options tab added to add/edit user screen.

  2. The hidden field seems always added to the form (and I guess that's the reason causing the issue on password reset). Maybe you should only insert it if there is custom fields available?

  3. Maybe the code if (!$formInput || empty($formInput['processed-by-fields'])) could be shorten to if (empty($formInput['processed-by-fields']))

Please let me know if you need more information.

avatar Denitz
Denitz - comment - 4 Oct 2017

@joomdonation Please check #18207, it introduces another more solid approach to deal with custom fields data and form checkboxes.

avatar laoneo laoneo - change - 4 Oct 2017
Labels Added: PR-staging
avatar laoneo
laoneo - comment - 4 Oct 2017

Can you guys test again as I fixed a wrong check. Now the user workarounds should not be needed anymore and the password reset should work too.

avatar alikon
alikon - comment - 28 Oct 2017

maybe i'm testing it in the wrong way

from backend without this pr

  1. create a text user custom field
  2. create a number user custom field
  3. create a new user and fill custom fields data (save it despite it took a lot of times)
  4. re-edit the same user, the custom fields data are still there

so i'm unable to replicate the issue
what i'm doing wrong ?

avatar laoneo
laoneo - comment - 30 Oct 2017

On the the normal user edit form it works, the issue is on password resets as described here #18185 or when you manually execute the code $user = JFactory::getUser(); $user->save();.

avatar Denitz
Denitz - comment - 30 Oct 2017

Please test #18207, this one is not required if #18207 is accepted.

avatar laoneo
laoneo - comment - 30 Oct 2017

This one solves a problem we have as soon as you are sending the onContentPrepare event when no form is validated against the data. It fixes the root cause and prevents extension devs from ugly workarounds when saving the data manually trough a model. Yours is a different fix.

avatar alikon
alikon - comment - 30 Oct 2017

@lanoeo i've tested on backend only

avatar laoneo
laoneo - comment - 30 Oct 2017

As I said, for the normal form it works. Try out #18185. That should erase the custom fields data.

avatar Denitz
Denitz - comment - 30 Oct 2017

@laoneo
I would disagree: this PR is a crutch, it does not allow to update data without form via model::save() and, it uses hard-coded request values.

It's bad practice to rely on request, e.g. somebody can append jform[processed-by-fields]=1 to query URL and trigger unexpected behaviour. Moreover, request data should not be used in API code like this.

My PR uses logical approach of data update depending on their presence in model data passed to events.

We need someone else with PHP experience to look at this.

avatar laoneo
laoneo - comment - 30 Oct 2017

Closing because of crutchy code.

avatar laoneo laoneo - change - 30 Oct 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-10-30 10:12:49
Closed_By laoneo
avatar laoneo laoneo - close - 30 Oct 2017
avatar laoneo
laoneo - comment - 31 Oct 2017

Had a second look on it. The problem we are facing is that the plugin save event should not be handled by com_fields when the form is not loaded accordingly. jform[processed-by-fields]=1 has no influence when the form is not validated. Somehow we need to define if the passed data to the save event is processed by com_fields. Otherwise we are not fixing the root cause of the issue.

avatar laoneo laoneo - change - 31 Oct 2017
Status Closed New
Closed_Date 2017-10-30 10:12:49
Closed_By laoneo
avatar laoneo laoneo - change - 31 Oct 2017
Status New Pending
avatar laoneo laoneo - reopen - 31 Oct 2017
avatar Denitz
Denitz - comment - 31 Oct 2017

We should not rely on forms for fields data update, form only displays and provides data to a model. Next model presents data to events. You want to make fields data only updated via form, it's bad because limits model data update.

avatar laoneo
laoneo - comment - 2 Nov 2017

I would love to not make such a workaround, but to be 100% sure we need to have something which indicates that the data which is sent with the save event was processed before by com_fields. I'm not sure but I guess we have more cases where some data is not sent to the server when it is not empty, radio buttons, select boxes perhaps? With your approach you rely on the fact that the filter function needs to be called first before the save event is triggered which is also not good.
All what this pr here is doing is to add a flag that the saved data is processed by com_fields. Like that we are absolutely on the safe side. While with your approach I have a feeling that we miss some cases.

avatar Denitz
Denitz - comment - 2 Nov 2017

Form filter function should be called on request data passing data to form validator and next to model, otherwise form rules are not applied. It's correct requirement and it should be always done with request data. That's how JForm works. Otherwise forms rules just don't work. Filtering is essential for forms and we should rely on it.

This flag limits model data save and we can't update model with plain data array. If new model data has a value for a field and the field is allowed to be updates, it should be updated.

avatar laoneo
laoneo - comment - 2 Nov 2017

As I stated in the description of the pr, this is a workaround. Your approach doesn't convince me because of the fact that I have the feeling we miss there something.

we can't update model with plain data array

What exactly do you mean with that?

avatar Denitz
Denitz - comment - 2 Nov 2017

I mean that we should be able to easily update certain custom fields via model, without any forms, e.g.

$model->save(....).

avatar laoneo
laoneo - comment - 2 Nov 2017

And you want to do that with or without custom fields data?

avatar Denitz
Denitz - comment - 2 Nov 2017

Yes, it would be nice to be able to update custom fields data in this way.

avatar laoneo
laoneo - comment - 3 Nov 2017

In your pr, can you test, that the following situations do work:

  • $user = JFactory::getUser(); $user->save();Should not erase the data.
  • Passoword 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 this one.

avatar Quy
Quy - comment - 27 Dec 2017

@laoneo OK to close in favor of PR #18207?

avatar laoneo
laoneo - comment - 30 Dec 2017

I want to do also another round of testing in #18207 and when all is ok, we can close it. For now I would like to leave it open.

avatar laoneo
laoneo - comment - 8 Jan 2018

Closing in favor of #18207. Thanks @Denitz.

avatar laoneo laoneo - change - 8 Jan 2018
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2018-01-08 14:01:14
Closed_By laoneo
avatar laoneo laoneo - close - 8 Jan 2018

Add a Comment

Login with GitHub to post a comment