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

Move first&last names in separate validation group #2840

Merged
merged 2 commits into from
Jun 23, 2015

Conversation

winzou
Copy link
Contributor

@winzou winzou commented Jun 11, 2015

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets -
License MIT
Doc PR -

First & last names can be null in the model, so we can't force them in the form.

Plus, it's super easy to make them compulsory if they are optional (just add the validation group). The other way is not straight forward at all given the validation system!

@pjedrzejewski
Copy link
Member

Hmmm... How about we name this group sylius_name_required or is it a bad idea? I think name is to vague.

@michalmarcinkowski
Copy link
Contributor

We should think about convention, because right now we use e.g. guest, registration, user_create. Should we prefix all that with sylius? sylius_guest, sylius_registration, sylius_user_create, sylius_customer_name?

@winzou I agree to make a separate validation group for that, but I think we should add this group as default to the form, because it is useful validation for most e-commerce platforms.

@winzou winzou changed the title First & last names are not compulsory Move first&last names in separate validation group Jun 12, 2015
@winzou
Copy link
Contributor Author

winzou commented Jun 12, 2015

Agreed with everything.

@pjedrzejewski
Copy link
Member

@michalmarcinkowski What do you think about this naming?

@winzou
Copy link
Contributor Author

winzou commented Jun 14, 2015

no news is good news? :)

@pjedrzejewski
Copy link
Member

I think we should set some convention before we merge this and follow it everywhere from now on. I was thinking that "sylius" is default group, but for extra stuff we have more expressive group names, like "sylius_name_required". What do you think about this idea?

@winzou
Copy link
Contributor Author

winzou commented Jun 15, 2015

We will never succeed in making groups convenient for everyone. So I would suggest to have just the sylius one for really mandatory fields, and then a few semantic others, like here profile and registration.

name_required is not semantic but field-related, I'm not sure it's a good way to go, otherwise we should have birthday_required and so on...

The ideal solution would be to have a flexible validation system. Overriding the validation groups would be amazing, but is not possible given the current validation system. Maybe a sylius extension adding this feature?

@michalmarcinkowski
Copy link
Contributor

I agree with @winzou, we should keep it simple for now.

Nice idea about the validation system, but I'm not sure we will have time for this feature in the nearest future :)

One more thing, I would change groups to more specific e.g. sylius_customer_profile, sylius_user_registration, sylius_customer_guest etc. to explicitly indicate the resource WDYT?

@winzou
Copy link
Contributor Author

winzou commented Jun 23, 2015

Done and green :)

@michalmarcinkowski
Copy link
Contributor

Great! Thanks Alexandre 👍

pjedrzejewski pushed a commit that referenced this pull request Jun 23, 2015
Move first&last names in separate validation group
@pjedrzejewski pjedrzejewski merged commit 8839ba4 into Sylius:master Jun 23, 2015
@pjedrzejewski
Copy link
Member

Thanks a lot Alexandre! :)

@winzou winzou deleted the customer/validation branch June 24, 2015 02:40
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.

None yet

3 participants