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

[Validator] String normalization options for string-based validators #26484

Open
wants to merge 6 commits into
base: master
from

Conversation

@renan-taranto

renan-taranto commented Mar 11, 2018

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #26239
License MIT
Doc PR symfony/symfony-docs#9433

Todo:

  • Document the new options
  • Update Doc PR

Add trimming options to the string constraints.

@renan-taranto renan-taranto force-pushed the renan-taranto:string_normalization_options branch from df450ee to 226e532 Mar 11, 2018

@renan-taranto renan-taranto force-pushed the renan-taranto:string_normalization_options branch 2 times, most recently from 7faf0e2 to 535259e Mar 11, 2018

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Mar 12, 2018

@renan-taranto renan-taranto force-pushed the renan-taranto:string_normalization_options branch from 535259e to 4194fcf Mar 12, 2018

@Destroy666x

This comment has been minimized.

Destroy666x commented Mar 13, 2018

Looks good now, thanks. Some more edge cases (e.g. regular whitespaces) could be added for tests, but not sure how strict are they here yet

@renan-taranto renan-taranto changed the title from [WIP] [Validator] String normalization options for string-based validators to [Validator] String normalization options for string-based validators Mar 13, 2018

@renan-taranto

This comment has been minimized.

renan-taranto commented Mar 13, 2018

Thanks for the review @Destroy666x.

@renan-taranto renan-taranto force-pushed the renan-taranto:string_normalization_options branch from 4194fcf to d1a0c02 Mar 14, 2018

@renan-taranto renan-taranto force-pushed the renan-taranto:string_normalization_options branch from d1a0c02 to 8e6a52f Mar 22, 2018

@renan-taranto renan-taranto changed the title from [Validator] String normalization options for string-based validators to [WIP][Validator] String normalization options for string-based validators Mar 22, 2018

@xabbuh

This comment has been minimized.

Member

xabbuh commented Apr 1, 2018

Would it make sense to update other constraint validators too?

@renan-taranto

This comment has been minimized.

renan-taranto commented Apr 6, 2018

Hey guys, how do you like this PR right now?

@fabpot fabpot modified the milestones: 4.1, next Apr 22, 2018

@nicolas-grekas nicolas-grekas changed the title from [WIP][Validator] String normalization options for string-based validators to [Validator] String normalization options for string-based validators Jun 19, 2018

@xabbuh

This comment has been minimized.

Member

xabbuh commented Jun 19, 2018

@renan-taranto For consistency, I would add this option to the other string constraints too.

@renan-taranto renan-taranto force-pushed the renan-taranto:string_normalization_options branch from 8e6a52f to c21460a Jun 19, 2018

@renan-taranto

This comment has been minimized.

renan-taranto commented Jul 9, 2018

normalizer sounds good, its intuitive. Will we allow any other callables besides trim methods to be passed as parameter?

@xabbuh

This comment has been minimized.

Member

xabbuh commented Jul 10, 2018

I think what callable to pass is up to the user as long as it receives the string to be validated as the input and returns the normalised string. But the trim examples is a good one to explain the usage of this option in the docs.

@renan-taranto renan-taranto force-pushed the renan-taranto:string_normalization_options branch from 913e5c2 to b14bdb9 Jul 11, 2018

@renan-taranto

This comment has been minimized.

renan-taranto commented Jul 11, 2018

Hey guys, the patch is updated with the normalizer option.
One of the Travis-ci builds broke while running the Symfony\Component\Process\Tests\ProcessTest::testMustRun test, not sure why.

@xabbuh

xabbuh approved these changes Jul 13, 2018

@nicolas-grekas nicolas-grekas force-pushed the renan-taranto:string_normalization_options branch from b14bdb9 to a299b0b Aug 7, 2018

@Destroy666x

This comment has been minimized.

Destroy666x commented Aug 16, 2018

I was so busy that I forgot about this - callable is the perfect idea, thanks for suggesting it @ostrolucky

@renan-taranto renan-taranto force-pushed the renan-taranto:string_normalization_options branch from a299b0b to 795acf3 Sep 13, 2018

@renan-taranto renan-taranto force-pushed the renan-taranto:string_normalization_options branch from 795acf3 to a53a1fc Oct 7, 2018

@gmponos

This comment has been minimized.

Contributor

gmponos commented Oct 9, 2018

Hello,

for me as a developer using symfony the normalizer option is a little bit confusing. I am referring to the naming of it. Maybe it's because in the syfmony world when I hear normalizer my mind goes to the Serializer component. So in the end when I saw the title of this PR I was expecting to see about an option that will be able to convert objects into strings and remove this code:

       if (!is_scalar($value) && !(\is_object($value) && method_exists($value, '__toString'))) {

I am not sure if the same confusion will be caused later for whatever reason. Therefore my suggestion would be formatter. Or simple trim and allowing this option to be either true or callable and if it is true just use plain trim. It's just a suggestion your call :)

@ostrolucky

This comment has been minimized.

Contributor

ostrolucky commented Oct 9, 2018

You can pass even callable which makes input pass the above check, it's your choice. Hence formatter is not good name.

@renan-taranto renan-taranto force-pushed the renan-taranto:string_normalization_options branch from 5d31a78 to 9da6661 Oct 9, 2018

@renan-taranto

This comment has been minimized.

renan-taranto commented Oct 9, 2018

PR updated. AppVeyor broke due to the unrelated Symfony\Component\Process\Tests\ProcessTest.

@renan-taranto renan-taranto force-pushed the renan-taranto:string_normalization_options branch from 9da6661 to 3a06897 Oct 29, 2018

@nicolas-grekas

Could you please and a line in the changelog of the component?

@@ -81,6 +81,10 @@ public function validate($value, Constraint $constraint)
$value = (string) $value;
if (null !== $constraint->normalizer) {
$value = \call_user_func($constraint->normalizer, $value);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 1, 2018

Member

$value = ($constraint->normalizer)($value); (as we'd like to get rid of call_user_func - same in other files in the PR)

This comment has been minimized.

@renan-taranto

renan-taranto Dec 1, 2018

Sure. I added a line in the 4.3.0 section of the changelog, is that correct? Or it should be in the 4.2.0 section?

This comment has been minimized.

@sstok

sstok Dec 2, 2018

Contributor

4.2 is released and feature freezed. So 4.3 is good 👍

@renan-taranto renan-taranto force-pushed the renan-taranto:string_normalization_options branch from 3a06897 to 202d8a2 Dec 1, 2018

@@ -98,5 +100,9 @@ public function __construct($options = null)
// throw new LogicException(sprintf('The "egulias/email-validator" component is required to use the "%s" constraint in strict mode.', __CLASS__));
@trigger_error(sprintf('Using the "%s" constraint in strict mode without the "egulias/email-validator" component installed is deprecated since Symfony 4.2.', __CLASS__), E_USER_DEPRECATED);
}
if (null !== $this->normalizer && !\is_callable($this->normalizer)) {
throw new InvalidArgumentException(sprintf('The "normalizer" option must be a valid callable ("%s" given).', \is_object($this->normalizer) ? \get_class($this->normalizer) : \gettype($this->normalizer)));

This comment has been minimized.

@sstok

sstok Dec 2, 2018

Contributor

What about a InvalidNormalizedException($this->normalizer) and keeping the formatting logic in that exception class. Less duplication.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 2, 2018

Member

That would introduce a new type of exception that people could catch. I'm not sure that makes sense - duplication here is legit to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment