10000 [Validator] Added HostnameValidator by karser · Pull Request #31518 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Validator] Added HostnameValidator #31518

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 10, 2020
Merged

[Validator] Added HostnameValidator #31518

merged 1 commit into from
Jan 10, 2020

Conversation

karser
Copy link
Contributor
@karser karser commented May 16, 2019 •
edited
Loading
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #10088
License MIT
Doc PR symfony/symfony-docs#...

This PR adds HostnameValidator support. I encountered this need in my project and was surprised that this issue has been open for years.

Here is short example:

App\Entity\Acme:
    properties:
        domain:
            - Hostname: ~
        non_tld_domain:
            - Hostname: { requireTld: false }

The option requireTld is true by default and disallows domains like localhost and etc.

@karser karser force-pushed the domain-validator branch from d4b255a to 8e0b725 Compare May 16, 2019 19:54
@karser karser changed the title Added DomainValidator [Validator] Added DomainValidator May 16, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone May 20, 2019
@nicolas-grekas nicolas-grekas changed the base branch from master to 4.4 June 2, 2019 20:14
@javiereguiluz
Copy link
Member
javiereguiluz commented Jul 3, 2019 •
edited
Loading

@karser thanks for this contribution! I like this a lot ... except for the tld option, which is confusing to me.

Here's a proposal:

  • Rename tld to require_tld
  • Set it as true by default
  • If true, domains like localhost are not valid. Set it to false to allow them.

@karser karser force-pushed the domain-validator branch from 8e0b725 to dc1db29 Compare July 3, 2019 19:21
@karser
Copy link
Contributor Author
karser commented Jul 3, 2019

@javiereguiluz thanks for the feedback. I implemented your proposal (see the test cases). wdyt?

App\Entity\Acme:
    properties:
        domain:
            - Domain: ~
        non_tld_domain:
            - Domain: { requireTld: false }

The option requireTld is true by default and disallows domains like localhost and etc.

@karser karser force-pushed the domain-validator branch from dc1db29 to 8101e63 Compare July 3, 2019 19:32
@karser karser force-pushed the domain-validator branch from 8101e63 to 46da469 Compare July 3, 2019 20:30
@karser karser force-pushed the domain-validator branch 2 times, most recently from f650936 to 98f6a2e Compare July 4, 2019 10:07
@karser karser force-pushed the domain-validator branch 2 times, most recently from 69d879a to 5bdd6c3 Compare July 4, 2019 10:19
ro0NL
ro0NL reviewed Jul 10, 2019
View reviewed changes
Copy link
Contributor
@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

would DomainName be a more inuitive name?

also please update validation.en.xml + any language you know :)

@ro0NL ro0NL mentioned this pull request Jul 15, 2019
Merged
@karser karser force-pushed the domain-validator branch 2 times, most recently from d930263 to 13d3e65 Compare July 15, 2019 09:32
@karser karser requested review from dunglas, sroze and xabbuh as code owners July 15, 2019 09:32
@karser karser force-pushed the domain-validator branch from 13d3e65 to d930263 Compare July 15, 2019 09:35
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
@nicolas-grekas nicolas-grekas modified the milestones: 4.4, next Nov 8, 2019
@karser karser changed the base branch from 4.4 to master November 14, 2019 17:29
@cybernet
Copy link
Contributor

really looking forward for this 👍

fabpot
fabpot approved these changes Jan 10, 2020
View reviewed changes
Copy link
Member
@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

with some minor comments

src/Symfony/Bridge/Doctrine/DependencyInjection/AbstractDoctrineExtension.php
'You can register them by adding a new driver to the '.
'"%s" service definition.', $this->getObjectManagerElementName($objectManagerName.'_metadata_driver')
));
throw new \InvalidArgumentException(sprintf('Can only configure "xml", "yml", "annotation", "php" or '.'"staticphp" through the DoctrineBundle. Use your own bundle to configure other metadata drivers. '.'You can register them by adding a new driver to the '.'"%s" service definition.', $this->getObjectManagerElementName($objectManagerName.'_metadata_driver')));
Copy link
Member

Choose a reason for hiding this comment

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

This should be reverted.

src/Symfony/Bridge/Doctrine/Security/User/EntityUserProvider.php
'The user object has to be serialized with its own identifier '.
'mapped by Doctrine.'
);
throw new \InvalidArgumentException('You cannot refresh a user '.'from the EntityUserProvider that does not contain an identifier. '.'The user object has to be serialized with its own identifier '.'mapped by Doctrine.');
Copy link
Member

Choose a reason for hiding this comment

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

This should be reverted.

src/Symfony/Component/Validator/CHANGELOG.md
B41A
@@ -1,6 +1,10 @@
CHANGELOG
=========

5.1.0
-----
* added the `Hostname` constraint and validator
Copy link
Member

Choose a reason for hiding this comment

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

missing blank line above the entry.

@fabpot
Copy link
Member
fabpot commented Jan 10, 2020

Thank you @karser.

@fabpot fabpot mentioned this pull request Jan 10, 2020
Closed
fabpot added a commit that referenced this pull request Jan 10, 2020
@fabpot
feature #31518 [Validator] Added HostnameValidator (karser)
7dae1ca 
This PR was merged into the 5.1-dev branch.

Discussion
----------

[Validator] Added HostnameValidator

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #10088   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

This PR adds HostnameValidator support. I encountered this need in my project and was surprised that this issue has been open for years.

Here is short example:
```
App\Entity\Acme:
    properties:
        domain:
            - Hostname: ~
        non_tld_domain:
            - Hostname: { requireTld: false }
```
The option `requireTld` is `true` by default and disallows domains like localhost and etc.

Commits
-------

8a08c20 Added HostnameValidator
@fabpot fabpot merged commit 8a08c20 into symfony:master Jan 10, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@lyrixx lyrixx lyrixx left review comments

@ro0NL ro0NL ro0NL left review comments

@xabbuh xabbuh xabbuh requested changes

@fabpot fabpot fabpot approved these changes

@dunglas dunglas dunglas approved these changes

@sroze sroze Awaiting requested review from sroze

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
Feature Status: Reviewed Validator
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.
0