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

[WIP] Reworking most of the registration form: #5214

Merged
merged 3 commits into from
Oct 15, 2015
Merged

Conversation

weaverryan
Copy link
Member

Hi guys!

Q A
Doc fix? yes (because it's got bad recommendations)
New docs? no
Applies to 2.3+
Fixed tickets --

After trading emails with a user, I realized that we have this very out-dated registration form chapter. Wow :). A summary of the changes:

  1. Showing the embedded Registration model is way too hard and unnecessary
  2. Stopped persisted plainPassword (why did we do this?)
  3. Used AppBundle approach
  4. Annotation routing
  5. various other small things

This is still a WIP only because I have 2 sections at the bottom I still want to add. Review for what is here now is appreciated.

Honestly, I was a little surprised that entries like this can "hide" in the docs and get so out of date. It's a testament to how big our docs are, but it's a bit of a problem (this is similar to #5184 that had something totally irrelevant in an important doc).

Things to update during merge:

  • password encoder
  • remove getName() in 2.8
  • setDefaultOptions

Thanks!

* ``email``
* ``username``
* ``password`` (the encoded password)
* ``plainPassword`` (*not* persisted: notice no ``@ORM\Column`` above it)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I miss some explanations about why we need two different fields to store the password.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need this field to be able to put some validation on it (the other option is to use an unmapped field in the form for the password)

@xelaris
Copy link
Contributor

xelaris commented Apr 28, 2015

Without looking into detail yet, there is obviously an overlap with #4773 😉

extra fields (like a "terms accepted" checkbox field) and embed the form
that actually stores the account information.
Creating a registration form is pretty easy - it *really* means just creating
a form that will update some ``User`` object (a Doctrine entity in this example)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe "model" is better to use as a generic name of objects that get persisted than the even more generic "object".

1) Showing the embedded Registration model is way too hard and unnecessary
2) Stopped persisted plainPassword (why did we do this?)
3) Used AppBundle approach
4) Annotation routing

various other small things
@weaverryan
Copy link
Member Author

Thanks for the review everyone!

@weaverryan weaverryan merged commit a8275a3 into 2.3 Oct 15, 2015
weaverryan added a commit that referenced this pull request Oct 15, 2015
…yan)

This PR was merged into the 2.3 branch.

Discussion
----------

[WIP] Reworking most of the registration form:

Hi guys!

| Q             | A
| ------------- | ---
| Doc fix?      | yes (because it's got bad recommendations)
| New docs?     | no
| Applies to    | 2.3+
| Fixed tickets | --

After trading emails with a user, I realized that we have this very out-dated registration form chapter. Wow :). A summary of the changes:

1) Showing the embedded Registration model is way too hard and unnecessary
2) Stopped persisted plainPassword (why did we do this?)
3) Used AppBundle approach
4) Annotation routing
5) various other small things

This is still a WIP only because I have 2 sections at the bottom I still want to add. Review for what is here now is appreciated.

Honestly, I was a little surprised that entries like this can "hide" in the docs and get so out of date. It's a testament to how big our docs are, but it's a bit of a problem (this is similar to #5184 that had something totally irrelevant in an important doc).

Things to update during merge:
- password encoder
- remove getName() in 2.8
- setDefaultOptions

Thanks!

Commits
-------

a8275a3 Fixing build error
8b7fc79 Tweaks thanks to everyone!
1c1b92a [WIP] Reworking most of the registration form:
@weaverryan weaverryan deleted the registration-form branch October 15, 2015 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants