Proper loading of registration form to show required extra fields as required

[ggppdk]

Pull Request for Issue #19506, (better alternative to PR #19614)

This PR is fixing #19506
while being able to reload form posted data when registration form reloads due to any error,
the above was fixed by #19145 but it caused issue #19506

further more the order of calling

$this->get('Data');
$this->get('Form');

is no longer important

Summary of Changes

Remove form instances creation inside getData()

Testing Instructions

1. Enable plugin "User - Profile" in plugin manager
2. Edit the plugin and enable some optional fields, set field Country to Required
3. Open registration form as guest and fill-in data in the form and right click on "input" of field "Name" and select "Inspect" (Chrome) or "Inspect Element" (Firefox), click Delete button on Keyboard to delete the selected <input>
4. Submit the form

Expected result

1. Registration form loads showing Country Field as required
2. After form submits (with "Name" field having been deleted), you get an error that "Name field is required" and the form reloads using the data that you have previously typed

Actual result

1. Country is shown as optional (broken by Fix form data lost when user registration failed #19145)
2. This is working already properly with J3.8.5 / staging

Documentation Changes Required

None

[duvi]

For me this patch does not fix the issue on 3.8.5.
However, changing the order of $this->get('Data'); and $this->get('Form'); does fix it.

[brianteeman]

I have tested this item 🔴 unsuccessfully on 3d76c14

required fields still marked as optional

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19617.}

[ggppdk]

Can you try again ?

[brianteeman]

the optional field thing works now - dont understand the other part

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19617.}

[ggppdk]

the optional field thing works now - dont understand the other part

I have updated testing instructions, they should be more clear now

[brianteeman]

I have tested this item ✅ successfully on b014c08

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19617.}

[joomdonation]

This PR changes the existing behavior of getData method. Before, the code check and make sure value from $temp array is only set to $this->data if a form field exist. With this modification, this check is removed, mean any data from $temp array (which comes from session) will be set to $this->data.

I don't understand the intentional of the original code but it might be unsafe to make this change

[ggppdk]

Why only check if the exist ?
Why not also validate them at this exact point

Is it not that validation is more important ?
Other models, do not do this before passing data to form loading

But I can understand that there can be login in event e.g. onContentPrepareData just checking only if a variable exists in the $data instead of checking if a feature is enabled
or

a 3rd party layout of overriding
component/com_users/views/registration/tmpl/default.php
may be doing something with $this->data

but it sounds stupid to do something with data that can originate form a form reload

also the way these are create is

		$data = $model->validate($form, $requestData);
...
			// Save the data in the session.
			$app->setUserState('com_users.registration.data', $requestData);

[EDIT]
explanation of the above, so we already know that these are not validated, so they are already unsafe to use, i mean checking existance only and then using them in a dangerous way sounds silly

[joomdonation]

As mentioned, I don't understand the intentional of the original code, maybe someone stays here before us will know why the code is needed

Please note that the data from that getData method not only using for layout, but also being used on register method to save user account, so there might be a valid reason the code is written like that

I think for the original issue, we should try to figure out the root reason of the issue. Why the field is optional even we have code to set it as required in profile plugin, why change ordering of the two commands have it works as expected....

[panta7]

Same problem here in joomla 3.8.5: required "User - Profile plugin" fields are optional, and the label "terms of conditions" doesn't link to article.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19617.}

[ghost]

@panta7 so can you please test this Pull Request?

[joomdonation]

We atleast need someone from PLT to take a look at this PR to confirm that it is safe before calling for human testing. See my comment earlier #19617 (comment)

[ggppdk]

@joomdonation

I think for the original issue, we should try to figure out the root reason of the issue. Why the field is optional even we have code to set it as required in profile plugin, why change ordering of the two commands have it works as expected....

exactly my thought there is some bug elsewhere,

because i checked and the user - profile plugin is setting for the correct form object the required attribute, via calling setFieldAttribute, i did not have time to go deeper and check,

• if setFieldAttribute has some bug or if something later resets the required attirbute or something else,

Given more time i can find what is exactly is happening, but i already spent more time for this than i intended

[joomdonation]

@ggppdk No worry, I am looking at it, too. Hope we can figure out the root reason of the error and implement proper fix

[panta7]

I'm not a developer but here what I have done:

Testing Instructions

1. Install a clear version on Joomla 3.8.5
2. Enable User registration in users option
3. Enable plugin "User - Profile" in plugin manager
4. Edit the plugin and enable some optional fields, set field Country to Required
5. Set an article to "Terms of conditions" field
6. Open registration form as guest and fill-in data in the form
7. Submit the form

Expected result

• Registration form loads showing Country Field as required
• Label "Terms of conditions" link to the article

Actual result

• Registration form loads showing Country Field as optional
• Label "Terms of conditions" doesn't link to the article

[ghost]

@panta7 thanks for Comment, but as @joomdonation wrote:

We atleast need someone from PLT to take a look at this PR to confirm that it is safe before calling for human testing. See my comment earlier #19617 (comment)

[ggppdk]

@panta7

you also need to apply the changes made by this PR

[joomdonation]

OK, found it. Not easy to explain but the reason is because the profile xml https://github.com/joomla/joomla-cms/blob/staging/plugins/user/profile/profiles/profile.xml is loaded twice into the form and it causes the issue

To solve the issue, we just need to check and make sure that form is only loaded one time. Change this line of code https://github.com/joomla/joomla-cms/blob/staging/plugins/user/profile/profile.php#L246
to

$form->loadFile('profile', true);

Or

if (!count($form->getGroup('profile')))
{
        $form->loadFile('profile', false);
}

and the issue should be sorted.

[alikon]

@joomdonation please submit a pr for your proposed fix, is less impacting and from a quick test it works

[joomdonation]

OK. I made PR #19633

[ggppdk]

Closing in favour of #19633

[duvi]

I just tested 3.8.6-rc1, and this issue is still not resolved for me.
I have an additional user profile plugin enabled on this site. If I disable it, the form seems to load normally.
Has this been tested if there's an additional user profile plugin installed?

Update: I have edited the additional user plugin and made the same change as here, and it seems to have resolved the issue for me, too.