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

Remove configuration form validation groups #3731

Conversation

pjedrzejewski
Copy link
Member

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

Based on #3721.

Helloes.

This change basically forces all Sylius resource forms to use Default validation group. Result:

  • On (promotion action/rule, calculator, report etc.) configuration forms we can safely use constraints without any groups, so can safely get rid of these ugly and long configuration nodes for these forms. Use-case where someone wants to override validation of FixedDiscountConfigurationType is super rare and we still allow this by overriding whole form. Supporting this rare use-case with current approach is not worth all the complexity that is introduced.
  • DX is improved - developer does not need to bother/remember/read in documentation that he needs to add "sylius" validation group to overridden entity in their custom app. He can just add constraint and be done with it.
  • All Sylius constraints use proper group "sylius" and validation_groups config node is available for every single resource, thus we maintain the ability to completely rework validation.

I did that for Promotion only right now, but will do this for everything else if I get support from you guys. :) Let me know what do you think!

Will be easier to review once #3721 is merged. Perhaps wait with reviewing the code.

@pjedrzejewski pjedrzejewski added Bug Fix DX Issues and PRs aimed at improving Developer eXperience. labels Dec 16, 2015
@arnolanglade
Copy link
Contributor

@pjedrzejewski I think you're right! This feature is an overkill, I guest it is use rarely...

@pjedrzejewski pjedrzejewski added the RFC Discussions about potential changes or new features. label Dec 17, 2015
@pjedrzejewski pjedrzejewski force-pushed the remove-configuration-form-validation-groups branch 3 times, most recently from e2c4455 to 1bce078 Compare December 17, 2015 17:53
@pjedrzejewski pjedrzejewski force-pushed the remove-configuration-form-validation-groups branch from 1bce078 to 19ba996 Compare December 17, 2015 18:07
michalmarcinkowski added a commit that referenced this pull request Dec 18, 2015
…-validation-groups

Remove configuration form validation groups
@michalmarcinkowski michalmarcinkowski merged commit de66ef4 into Sylius:master Dec 18, 2015
@michalmarcinkowski
Copy link
Contributor

I like cleanups! Thanks Paweł! 👍

$validationGroups = array('Default');
}

$definition->setArguments(array(
$metadata->getClass('model'),
$validationGroups,
$validationGroups
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should be reverted ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
…n-form-validation-groups

Remove configuration form validation groups
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Issues and PRs aimed at improving Developer eXperience. RFC Discussions about potential changes or new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants