PR-staging

Pending

User tests: Successful: Unsuccessful:

avatar ggppdk
ggppdk
8 Feb 2018

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 #19145)
  2. This is working already properly with J3.8.5 / staging

Documentation Changes Required

None

avatar ggppdk ggppdk - change - 8 Feb 2018
Status New Pending
avatar ggppdk ggppdk - open - 8 Feb 2018
avatar joomla-cms-bot joomla-cms-bot - change - 8 Feb 2018
Category Front End com_users
avatar duvi
duvi - comment - 8 Feb 2018

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.

avatar brianteeman
brianteeman - comment - 8 Feb 2018

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.

avatar brianteeman brianteeman - test_item - 8 Feb 2018 - Tested unsuccessfully
avatar ggppdk ggppdk - change - 8 Feb 2018
Labels Added: PR-staging
avatar ggppdk
ggppdk - comment - 8 Feb 2018

Can you try again ?

avatar brianteeman
brianteeman - comment - 8 Feb 2018

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.

avatar ggppdk ggppdk - change - 8 Feb 2018
The description was changed
avatar ggppdk ggppdk - edited - 8 Feb 2018
avatar ggppdk
ggppdk - comment - 8 Feb 2018

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

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

avatar ggppdk ggppdk - change - 8 Feb 2018
The description was changed
avatar ggppdk ggppdk - edited - 8 Feb 2018
avatar brianteeman
brianteeman - comment - 8 Feb 2018

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.

avatar brianteeman brianteeman - test_item - 8 Feb 2018 - Tested successfully
avatar ggppdk ggppdk - change - 8 Feb 2018
The description was changed
avatar ggppdk ggppdk - edited - 8 Feb 2018
avatar ggppdk ggppdk - change - 8 Feb 2018
The description was changed
avatar ggppdk ggppdk - edited - 8 Feb 2018
avatar ggppdk ggppdk - change - 8 Feb 2018
The description was changed
avatar ggppdk ggppdk - edited - 8 Feb 2018
avatar ggppdk ggppdk - change - 8 Feb 2018
The description was changed
avatar ggppdk ggppdk - edited - 8 Feb 2018
avatar ggppdk ggppdk - change - 8 Feb 2018
The description was changed
avatar ggppdk ggppdk - edited - 8 Feb 2018
avatar joomdonation
joomdonation - comment - 9 Feb 2018

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

avatar ggppdk
ggppdk - comment - 9 Feb 2018

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

avatar joomdonation
joomdonation - comment - 9 Feb 2018

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....

avatar panta7
panta7 - comment - 9 Feb 2018

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.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 9 Feb 2018

@panta7 so can you please test this Pull Request?

avatar joomdonation
joomdonation - comment - 9 Feb 2018

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)

avatar ggppdk
ggppdk - comment - 9 Feb 2018

@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

avatar joomdonation
joomdonation - comment - 9 Feb 2018

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

avatar panta7
panta7 - comment - 9 Feb 2018

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
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 9 Feb 2018

@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)

avatar ggppdk
ggppdk - comment - 9 Feb 2018

@panta7

you also need to apply the changes made by this PR

avatar joomdonation
joomdonation - comment - 9 Feb 2018

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.

avatar alikon
alikon - comment - 9 Feb 2018

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

avatar joomdonation
joomdonation - comment - 10 Feb 2018

OK. I made PR #19633

avatar ggppdk
ggppdk - comment - 10 Feb 2018

Closing in favour of #19633

avatar ggppdk ggppdk - close - 10 Feb 2018
avatar ggppdk ggppdk - change - 10 Feb 2018
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2018-02-10 05:47:32
Closed_By ggppdk
avatar duvi
duvi - comment - 4 Mar 2018

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.

Add a Comment

Login with GitHub to post a comment