search

nineinchnick / yii2-usr

Yii framework module for user authentication, password reset, registration and profile updating. A port of yii-usr to Yii Framework 2.0.
http://demo2.niix.pl
MIT License
39 stars 8 forks source link

Tom's proposals #41

Open tom-- opened 9 years ago

tom-- commented 9 years ago
  1. Remove diceware. I have gutted it. If you want to leave it in, that's fine, but don't put it in your composer.json, require it in mine.
  2. It's not in this PR but you should do the same with swiftmailer. Not in your composer.json because I don't want to use it. I use Mandrill.
  3. Changed the priority of merging attributes to 1st behavior, 2nd self, 3rd parent
  4. Changed _form view to respect the profile attributes defined by the user's identity class
  5. Formatting. I found some of your code very hard to read because of the formatting. In some places I had to reformat so that I could understand it. [Do what you want with formatting. For myself and anyone I employ, I insist on full and strict adherence to the style guide, in this case PSR-0 & 2. But I understand that this is among the most emotional issues among programmers and that reformatting someone else's code can be insulting. I don't want that to happen. I just needed to understand your code in some places.]
nineinchnick commented 9 years ago

I'd happily merge because I agree with everything this but you should make few smaller PRs.

Are you with me making smaller changes manually, like attributes merging order?

And what about the formatting? You're talking about keeping same indentation between html and php in views? I didn't know that PSR covered that. I only care about consistency. I can run php-cs-fixer on all sources and I'm 70% sure I did already.

tom-- commented 9 years ago

i'm glad to do just one thing at a time in future prs. do you want me to close this one and submit several instead?

tom-- commented 9 years ago

i would prefer to leave the whole question of code format to you. i only intended to explain that i had to make changes in order to be able to read the code and that's the only reason they were in the pr.

tom-- commented 9 years ago

when you say "Are you with me making smaller changes manually, like attributes merging order?" do you mean: maintain my own fork?

nineinchnick commented 9 years ago

Neverming the manual comment. If you'd fix the 3 things I commented I'd merge this.