Skip to content

[Validator] - EmailConstraint reference#3469

Merged
weaverryan merged 8 commits intosymfony:masterfrom
egulias:email_validator
Apr 2, 2014
Merged

[Validator] - EmailConstraint reference#3469
weaverryan merged 8 commits intosymfony:masterfrom
egulias:email_validator

Conversation

@egulias
Copy link
Copy Markdown
Contributor

@egulias egulias commented Jan 12, 2014

Q A
Doc fix? no
New docs? yes - symfony/symfony#9140
Applies to master
Fixed tickets

Note

I'm not sure if this behaviour change should go in a page by itself (like https://github.com/symfony/symfony-docs/blob/2.4/components/dependency_injection/lazy_services.rst) since requires the installation of an external library to work.
I appreciate some guide on this since it's my first doc PR, thanks.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the tagline (the line with all those ~ characters) should be even long as the headline (the title). In your case, the tagline is one character longer, which should be removed

@wouterj
Copy link
Copy Markdown
Member

wouterj commented Jan 12, 2014

You documented it right the correct way: Saying what will happen and what is required for it to work

I see no other problems with this PR, except that you need to add a .. versionadded:: 2.5 box before the headline of the strict option saying "The strict option was introduced in Symfony 2.5."

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should be placed before the headline. So you get something like:

.. versionadded:: 2.5
    The ``strict`` option was introduced in Symfony 2.5.

strict

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

compliant

@egulias
Copy link
Copy Markdown
Contributor Author

egulias commented Jan 13, 2014

Thanks @xabbuh and @wouterj for your pacience :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm sorry, but you have to change another thing (I really hope it's the last change ;-) ), you should add a line break after the first word that crosses the 72th character.

@egulias
Copy link
Copy Markdown
Contributor Author

egulias commented Jan 16, 2014

Thank you! I hope now is correct :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I meant only to remove one of the two blank lines. So, one blank line should be preserved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My bad too, I should have thought a little :)

fabpot added a commit to symfony/symfony that referenced this pull request Mar 27, 2014
…ncy (egulias)

This PR was squashed before being merged into the 2.5-dev branch (closes #9140).

Discussion
----------

[Validator][Email] - Strict validation and soft dependency

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #1581, #4930
| License       | MIT
| Doc PR        | symfony/symfony-docs#3469

TODO
---------
- [x] submit changes to the documentation
- [x] document the BC breaks
- [x] finish the code
- [x] gather feedback for my changes

In #1581 @bschussek suggested to pass the strict_email config to  `Validator\EmailValidator::__construct($strict)`, I did not put it there yet since the constraint can receive that configuration each time the constraint is used despite the fact of the global configuration. This could lead to some logic in the constructor and I wanted first to integrate the strict validator.

BC Break
--------------
I'm not sure of this, but as a soft dependency is added and now some emails that where valid before no longer are I thought it is.

Commits
-------

3368630 #1581 - Strict in Email constraint and use of Egulias\EmailValidator
@egulias
Copy link
Copy Markdown
Contributor Author

egulias commented Apr 1, 2014

Hi @wouterj the PR has been merged so I guess this can be merged too. Let me know any issues.
Thanks for the help!

@xabbuh
Copy link
Copy Markdown
Member

xabbuh commented Apr 1, 2014

👍 Thanks for all the work you spent on this topic, @egulias!

@egulias
Copy link
Copy Markdown
Contributor Author

egulias commented Apr 1, 2014

😄

@wouterj wouterj changed the title [WCM][Validator] - EmailConstraint reference [Validator] - EmailConstraint reference Apr 1, 2014
@weaverryan
Copy link
Copy Markdown
Contributor

Nice feature and thanks for your work getting this PR absolutely perfect. Cheers Eduardo!

weaverryan added a commit that referenced this pull request Apr 2, 2014
This PR was merged into the master branch.

Discussion
----------

[Validator] - EmailConstraint reference

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes - symfony/symfony#9140
| Applies to    | master
| Fixed tickets |

Note
------
I'm not sure if this behaviour change should go in a page by itself (like https://github.com/symfony/symfony-docs/blob/2.4/components/dependency_injection/lazy_services.rst) since requires the installation of an external library to work.
I appreciate some guide on this since it's my first doc PR, thanks.

Commits
-------

257c483 Blank line restored
c50f041 CS
a0dd460 Lowercase and link label
aa5aa61 Typo and link label
517e4c5 Link moved and heading updated
f0b3b85 Version added and clarifications
e4a0e2f Use of Sphinx markup
faa034b [WIP][Valiadtor] - EmailConstraint reference
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.

4 participants