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

Document constraint validator alias optional #6300

Merged

Conversation

Mx-Glitter
Copy link
Contributor

Q A
Doc fix? yes
New docs? no (symfony/symfony#17074)
Applies to 2.3
Fixed tickets

Same as #6055 but on the 2.3 branch as the relevant Symfony PR symfony/symfony#17074 was merged on 2.3.

Your constraint class should now use this alias to reference the appropriate
validator::
As mentioned above, Symfony will automatically look for a class named after
the constraint, with ``Validator`` appended. You can override this in you constraint class::
Copy link
Member

Choose a reason for hiding this comment

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

in you constraint -> in your constraint

@Mx-Glitter Mx-Glitter force-pushed the fix-constraint-validator-alias-required branch from 16dd384 to 2aa1e19 Compare February 24, 2016 14:10

public function validatedBy()
{
return 'alias_name';
return 'Fully\Qualified\Class\Named\ConstraintValidator'; \\ or 'alias_name' if provided
Copy link
Member

Choose a reason for hiding this comment

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

Comments are started with forward slashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed!

@Mx-Glitter Mx-Glitter force-pushed the fix-constraint-validator-alias-required branch 3 times, most recently from d670ab8 to acad36b Compare February 24, 2016 22:13

public function validatedBy()
{
return 'alias_name';
return 'Fully\Qualified\Class\Named\ConstraintValidator'; // or 'alias_name' if provided
Copy link
Member

Choose a reason for hiding this comment

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

according to class name we used above for the constraint this should be Fully\Qualified\Validator\Class\NameValidator

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to make it more noticeable that the class name may not be the constraint FQCN with Validator appended, what do you think about Fully\Qualified\ConstraintValidator\Class\Name?

Copy link
Member

Choose a reason for hiding this comment

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

sounds good too

@Mx-Glitter Mx-Glitter force-pushed the fix-constraint-validator-alias-required branch from acad36b to 873ede0 Compare February 25, 2016 22:45
@Mx-Glitter
Copy link
Contributor Author

@xabbuh You put the label "In progress" on this PR so people would know it's unfinished and already in review. Since then, I responded to your comments and it seems nobody gave a look anymore. I know you people are giving personal time to this wonderful project, I understand everything can't be done immediately and I thank you immensely for your job 😄. However, I'm wondering if the label actually make people refrain from reviewing Pull Requests: do you regularly go back to "In progress" PRs to see if progress was indeed made?

How can I help you know if I think I finished a PR without putting pression on you and making you a notification?

Cheers!

@xabbuh
Copy link
Member

xabbuh commented Apr 23, 2016

@Triiistan Actually this was a misunderstand then. I noticed that you commented and didn't see any changes. It seems that you pushed them shortly after I had the look. Sadly, we do not get notifications when new changes are pushed so we sometimes miss changes (we try to look at old PRs from time to time but we have so many 😞). Thanks for poking me. I am doing another round of reviews right now. :)

@xabbuh
Copy link
Member

xabbuh commented Apr 23, 2016

👍

@weaverryan
Copy link
Member

Thanks Tristan! This is actually an awesome improvement to the code itself - thanks for making it! And sorry we delayed on merging it - sometimes PR's can get lost :)

@weaverryan weaverryan merged commit 873ede0 into symfony:2.3 Apr 26, 2016
weaverryan added a commit that referenced this pull request Apr 26, 2016
This PR was merged into the 2.3 branch.

Discussion
----------

Document constraint validator alias optional

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no (symfony/symfony#17074)
| Applies to    | 2.3
| Fixed tickets |

Same as #6055 but on the 2.3 branch as the relevant Symfony PR symfony/symfony#17074 was merged on 2.3.

Commits
-------

873ede0 Document constraint validator alias optional
@Mx-Glitter
Copy link
Contributor Author

No problem! I just wonder how I can help you the most without being a burden!

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.

None yet

4 participants