Skip to content

Unique entity custom message #15201

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

Closed
wants to merge 14 commits into from
Closed

Unique entity custom message #15201

wants to merge 14 commits into from

Conversation

jamyouss
Copy link

@jamyouss jamyouss commented Jul 4, 2015

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

Add the possibility to have custom message for unique entity constraint.
For example if we have a collection of entities in a form, the message can be more expressive.

Example:

/**
 * Town
 *
 * @ORM\Entity()
 * @UniqueEntity(fields={"city"}, message="The city '{{ city }}' is already used.")
 */
class Town
{

I accidentally closed the previous opened PR (#13787), that why a recreate a pull request.

@Minishlink
Copy link

Thanks 👍

@linaori
Copy link
Contributor

linaori commented Jul 4, 2015

Is the twig style standard or the translations style standard? {{ var }} vs %var%.

@Minishlink
Copy link

According to other uses of variables in constraints messages (eg. EmailValidator), the standard is twig-style.

@oleg-andreyev
Copy link
Contributor

All violations messages are passed to translator (e.g.: \Symfony\Component\Validator\Context\ExecutionContext::addViolation) where placeholders are replaced (see \Symfony\Component\Translation\Translator::trans), so your value must be added to $parameters and let translator do it's job.

For example see: EmailValidator

@keradus
Copy link
Member

keradus commented Jul 6, 2015

no new tests needed for that new behavior ?

@OskarStark
Copy link
Contributor

great work! 👍

@jamyouss
Copy link
Author

jamyouss commented Jul 7, 2015

@keradus I don't think so because i don't change the validation behaviour.

@keradus
Copy link
Member

keradus commented Jul 7, 2015

OK, but how do we know that new behaviour works after some time? Tests can prove that when sb refactor sth this behaviour will still be working ;)

@SofHad
Copy link
Contributor

SofHad commented Jul 7, 2015

Good 👍

@jamyouss
Copy link
Author

jamyouss commented Jul 7, 2015

@keradus you're right, i gonna add some tests

@OskarStark
Copy link
Contributor

thank you @Razmo ,give me a ping an i'll review it

@jamyouss
Copy link
Author

You're welcome @OskarStark !
What does "give me a ping" mean ? (I'm not used to contribute to projects on github)

@OskarStark
Copy link
Contributor

if you use @OskarStark i get a direct notification via mail, as you did ;-)

EDIT: Sorry @Razmo , didn't see, that you already added the test.
Can you please extend the test to check a unique constraint over 2 fields?

@jamyouss
Copy link
Author

@OskarStark test updated

@OskarStark
Copy link
Contributor

hmm, I don't know why the travis build is broken with 5.6, can you have a look @Razmo

@jamyouss
Copy link
Author

@OskarStark I run phpunit test in local and i have 5 failure on Symfony\Component\HttpKernel\Tests\HttpCache\HttpCacheTest and 1 failure on Symfony\Component\Finder\Tests\Iterator\RecursiveDirectoryIteratorTest::testSeek

I don't think that it is caused by my changes

EDIT: weird, it's ok now but i didn't make any changes

@OskarStark
Copy link
Contributor

hmm, ok

ping @xabbuh @stof

$vars = array();

if (preg_match_all('/{{ ([a-zA-Z0-9_]+) }}/i', $constraint->message, $vars) > 0) {
$accessor = PropertyAccess::createPropertyAccessor();
Copy link
Member

Choose a reason for hiding this comment

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

You need to make sure that the PropertyAccess component is actually available and you should add it to the suggest section in composer.json.

@xabbuh
Copy link
Member

xabbuh commented Oct 2, 2015

Wouldn't it be better to be able to configure a list of object property names whose values would be passed to the actual constraint through the setParameter() method of the constraint violation builder?

@fabpot fabpot added the Form label Oct 5, 2015
@jamyouss
Copy link
Author

jamyouss commented Oct 5, 2015

Thank you @stof and @xabbuh for those feedbacks. I'm going to make updates as soon as possible.

@jamyouss
Copy link
Author

@stof @xabbuh : PR updated !
My PR is based on the 2.6 branch. Do i need to merge the 2.7 into my branch ?

@jamyouss
Copy link
Author

ping @stof

@OskarStark
Copy link
Contributor

@Razmo the merge to the 2.7 branch is done automatically afterwards

@jamyouss
Copy link
Author

@OskarStark Ok thx

@jamyouss
Copy link
Author

@OskarStark Phpunit tests passed in my local but didn't pass on travis. Have you any idea why ?

@OskarStark
Copy link
Contributor

no sorry @Razmo

@xabbuh
Copy link
Member

xabbuh commented Oct 23, 2015

Failures seem to be related to the fact that your changes are based off the 2.6 branch.

@jamyouss
Copy link
Author

Wired because because my branch uniqueEntityCustomMessage is sync with the 2.6 branch

@xabbuh
Copy link
Member

xabbuh commented Oct 26, 2015

@Razmo Sure, but Symfony 2.6 itself doesn't receive bug fixes anymore. Probably, you can better reopen the PR based on the 2.8 branch.

@jamyouss
Copy link
Author

Close this PR and reopen it based on 2.8 branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants