Skip to content

Update validation.rst #4789

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 3 commits into from
Closed

Update validation.rst #4789

wants to merge 3 commits into from

Conversation

ifdattic
Copy link
Contributor

@ifdattic ifdattic commented Jan 7, 2015

Q A
Doc fix? yes
New docs? no
Applies to 2.3
Fixed tickets

Some fixes can be a bit "controversial".

Due to PR https://github.com/symfony/symfony-docs/pull/4779/files not all changes were made.

Something to keep in mind is what due to https://github.com/symfony/symfony-docs/pull/4779/files using Assert\... if arguments are kept on the same line the possibility of horizontal scrollbar (especially on the smaller screens) is quite high.

Also for most use cases I think adding PHP constraints is better if arguments are on different lines ("easier to read" and easier to spot errors

| Q             | A
| ------------- | ---
| Doc fix?      | yes
| New docs?     | no
| Applies to    | 2.3
| Fixed tickets | 

Some fixes can be a bit "controversial".

Due to PR https://github.com/symfony/symfony-docs/pull/4779/files not all changes were made.

Something to keep in mind is what due to https://github.com/symfony/symfony-docs/pull/4779/files using `Assert\...` if arguments are kept on the same line the possibility of horizontal scrollbar (especially on the smaller screens) is quite high.

Also for most use cases I think adding PHP constraints is better if arguments are on different lines ("easier to read" and easier to spot errors
@ifdattic
Copy link
Contributor Author

ifdattic commented Jan 7, 2015

I would like to split line 344 to remove horizontal scrollbar, but wasn't sure of the best way to do it

@@ -35,7 +35,7 @@ application. The goal of validation is to tell you if the data
of an object is valid. For this to work, you'll configure a list of rules
(called :ref:`constraints <validation-constraints>`) that the object must
follow in order to be valid. These rules can be specified via a number of
different formats (YAML, XML, annotations, or PHP).
different formats (annotations, YAML, XML, or PHP).
Copy link
Member

Choose a reason for hiding this comment

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

While it's not your fault, you should remove the serial comma before "or"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I thought it wasn't needed but just left it as is, will fix on the next commit.

@wouterj
Copy link
Member

wouterj commented Jan 7, 2015

- Choice:
    choices: [male, female]
    message: Choose a valid gender.

@ifdattic
Copy link
Contributor Author

ifdattic commented Jan 7, 2015

Yes, thank you, completely slipped my mind that is how you do in YAML

@@ -476,7 +486,7 @@ If you're ever unsure of how to specify an option, either check the API document
for the constraint or play it safe by always passing in an array of options
(the first method shown above).

Translation Constraint Messages
Translating Constraint Messages
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 add the former label to be BC:

.. _translation-constraint-messages:

Translating Constraint Messages
-------------------------------

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will change it now

@xabbuh
Copy link
Member

xabbuh commented May 23, 2015

What do we do here?

@wouterj
Copy link
Member

wouterj commented May 23, 2015

Closing as none of the code blocks are violating the 85 character limit we've decided on after this PR was opened (only the schema definitions do, but we don't care about them).

The only other change in here is implementing UserInterface, which is already done.

@wouterj wouterj closed this May 23, 2015
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.

3 participants