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

[USER] Change password validation #3471

Merged

Conversation

lchrusciel
Copy link
Member

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Fixed tickets
License MIT

Password validation was done inside a UserController. It wasn't translatable and it was hard to receive error message when you work with API, because error message was displayed in a flash message. I removed this part form a controller and move this logic to custom validatior.
I have also added two more parameters in a service.xml file to keep all class paths in a one place. It results probably in a BC break.

@@ -20,7 +20,7 @@ Feature: User account password change
And I fill in "Confirmation" with "newpassword"
And I press "Save changes"
Then I should still be on my account password page
And I should see "Wrong current password"
And I should see "Wrong current password."
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC we agreed that matching texts should not contain dots etc.

@Arn0d @pjedrzejewski @umpirsky ping

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, I prefer to have them. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@umpirsky I'm talking about matching not about translations :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@stloyd Me too. :)

@pjedrzejewski
Copy link
Member

@pjedrzejewski pjedrzejewski added the Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). label Oct 22, 2015
@pjedrzejewski pjedrzejewski added this to the v0.16.0 milestone Oct 22, 2015
@lchrusciel
Copy link
Member Author

Thanks @pjedrzejewski! I will use symfony constraint instead of custom one.

<argument type="service" id="sylius.repository.customer" />
<tag name="validator.constraint_validator" alias="registered_user_validator" />
</service>
<service id="validator.wrong.current_password" class="%sylius.user.validator.wrong_current_password%">
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this sevice.

@michalmarcinkowski
Copy link
Contributor

Thanks Łukasz!

michalmarcinkowski added a commit that referenced this pull request Oct 23, 2015
@michalmarcinkowski michalmarcinkowski merged commit 51c7ff1 into Sylius:master Oct 23, 2015
@@ -179,12 +182,12 @@
<service id="sylius.form.type.user_change_password" class="%sylius.form.type.user_change_password.class%">
<tag name="form.type" alias="sylius_user_change_password" />
</service>
<service id="sylius.form.type.gender" class="Sylius\Bundle\UserBundle\Form\Type\GenderType">
<service id="sylius.form.type.gender" class="%sylius.form.type.gender.class%">
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is correct, we should follow Symfony changes & not use class names as parameters.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can always open a RFC about this. But for now, we have convention to use class names as parameters and I found it more appropriate to stay consistent then to change practise without discussion, just to follow some changes.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if we decide to change it, we do it everywhere. 👍 for the RFC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants