Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix form data lost when user registration failed #19145

Merged
merged 2 commits into from
Jan 16, 2018
Merged

Fix form data lost when user registration failed #19145

merged 2 commits into from
Jan 16, 2018

Conversation

joomdonation
Copy link
Contributor

@joomdonation joomdonation commented Dec 23, 2017

Pull Request for Issue # .

Summary of Changes

When someone register for an account and registration is failed for some reasons (for example, use an existing username or existing email), users will be redirected back to registration form with error messages explain the reason of errors. However, the data which he entered before is lost and he will have to re-enter again to register for an account. This PR fixes that issue

Testing Instructions

  1. Install Joomla

  2. Try to register for an account, use an existing username

  3. Before patch, you will be redirected back to register form. However, the data you entered before is cleared

  4. After patch, you will be redirected back tor registration form, but the data you entered before is kept

Technical explanation

For some reasons, the UsersViewRegistration class defines $data property (although I could not see it is used anywhere). When this line of code is called https://github.com/joomla/joomla-cms/blob/staging/components/com_users/views/registration/view.html.php#L41, the method getData of UsersModelRegistration is called. A form is created and cached without loading data from session https://github.com/joomla/joomla-cms/blob/staging/components/com_users/models/registration.php#L247

Later, when this command is called to get form object for view https://github.com/joomla/joomla-cms/blob/staging/components/com_users/views/registration/view.html.php#L42, the system uses the blank cached form and that's the reason form data is lost

As $data property is not used anywhere, I just remove it and remove$this->data = $this->get('Data'); calls, it is now working as expected

@ghost
Copy link

ghost commented Dec 23, 2017

I have tested this item ✅ successfully on d36f981


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19145.

1 similar comment
@alikon
Copy link
Contributor

alikon commented Dec 23, 2017

I have tested this item ✅ successfully on d36f981


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19145.

@ghost
Copy link

ghost commented Dec 23, 2017

Ready to Commit after two successful tests.

@sandewt
Copy link
Contributor

sandewt commented Jan 14, 2018

I have performed a successful test. But...

If you leave after a failed registration attempt the registration page and you come back again,
then you would expect that the (confidential) data would have disappeared.
That is not the case right now. !!!
Is that the intention?

@joomdonation
Copy link
Contributor Author

@sandewt Yes, that is the way how Joomla handles submitted form data. When there is a validation errors and data could not be saved, Joomla will stores the form data in session and then redirect users back to the previous page so that users can correct the wrong data

As the data stored in session, if you leave the page and access again later, the data will still be there. It will be erased when session expired. Also, just want to make it clear that only you see that data on your own computer, other users won't see it.

@sandewt
Copy link
Contributor

sandewt commented Jan 15, 2018

Hi @joomdonation, do not get me wrong.

I think it's a big improvement.

The mechanism is known to me.

On closer inspection, see also issue #17743 (Contact form loses data), there is the same thing.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19145.

@joomdonation
Copy link
Contributor Author

@sandewt So do you think there is any problem with this PR ? Or it is fine?

@sandewt
Copy link
Contributor

sandewt commented Jan 15, 2018

Hi @joomdonation, there is NO problem with this PR.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19145.

@mbabker
Copy link
Contributor

mbabker commented Jan 15, 2018

I keep going back and forth on this because there is technically a B/C break here by removing the data property (since component layouts are in scope of the view class, the property is accessible to layouts). My gut says if this is to be merged it should go into 3.9 with the note of the potential (albeit highly unlikely) B/C break, and not included in a patch release.

@joomdonation
Copy link
Contributor Author

Strange that someone introduced this property without any usage in the core. A workaround to fix this issue without B/C is change ordering of method call to:

$this->form   = $this->get('Form');
$this->data   = $this->get('Data');

Please let me know if that's OK and I will make this change to have the issue fixed. It is not nice to have to re-enter data when validation failed.

@sandewt
Copy link
Contributor

sandewt commented Jan 16, 2018

I have tested this item ✅ successfully on 6f7f256


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19145.

1 similar comment
@ghost
Copy link

ghost commented Jan 16, 2018

I have tested this item ✅ successfully on 6f7f256


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/19145.

@mbabker mbabker added this to the Joomla 3.8.4 milestone Jan 16, 2018
@mbabker mbabker merged commit 0fc19ac into joomla:staging Jan 16, 2018
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jan 16, 2018
@joomdonation joomdonation deleted the patch-7 branch January 16, 2018 23:33
brianteeman added a commit to brianteeman/joomla-cms that referenced this pull request Feb 8, 2018
Enable the User - Joomla plugin and set the first field to be required
Enable user registration
Try to register a user on the front end and you will see that field Address 1 is displayed as optional - it should be displayed as required.
Try to register without completing field Address 1 and it will fail saying that Address 1 is required

This has been a long standing recurring bug. This instance has come from
Fix form data lost when user registration failed joomla#19145

Reverting that PR resolves this issue but then of course it means that the issue being fixed in joomla#19145 had returned
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants