User tests: Successful: Unsuccessful:
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
Remove form instances creation inside getData()
<input>
None
Status | New | ⇒ | Pending |
Category | ⇒ | Front End com_users |
I have tested this item
required fields still marked as optional
Labels |
Added:
?
|
Can you try again ?
the optional field thing works now - dont understand the other part
the optional field thing works now - dont understand the other part
I have updated testing instructions, they should be more clear now
I have tested this item
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
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
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....
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.
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)
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,
Given more time i can find what is exactly is happening, but i already spent more time for this than i intended
I'm not a developer but here what I have done:
@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)
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.
@joomdonation please submit a pr for your proposed fix, is less impacting and from a quick test it works
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-02-10 05:47:32 |
Closed_By | ⇒ | ggppdk |
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.
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.