barbieswimcrew / zip-code-validator

Constraint Class for international Zipcode Validation
MIT License
71 stars 18 forks source link

Error with empty value in Symfony 5 #39

Closed TeLiXj closed 3 years ago

TeLiXj commented 4 years ago

Hi! I recently update one of my projects to Symfony 5 and the validator now fails

I had this validator and works fine with bad zip codes or empty values in SF 4.4 @ZipCode(getter="getCountry", groups={"ProfileData"}, strict=false)

But after the update I receive this error with empty values

Argument 2 passed to Symfony\Component\Validator\Violation\ConstraintViolationBuilder::setParameter() must be of the type string, null given, called in /.../vendor/barbieswimcrew/zip-code-validator/src/ZipCodeValidator/Constraints/ZipCodeValidator.php on line 263

To fix it I'm added ignoreEmpty option and a common NotBlank validator like this

@Assert\NotBlank(groups={"ProfileData", "iban_prestador"})
@ZipCode(getter="getCountry", groups={"ProfileData", "iban_prestador"}, ignoreEmpty=true, strict=false)
rimas-kudelis commented 3 years ago

Oh, I wanna chime in here too. I think it would make most sense if this validator just ignored null and empty string values, like (almost) all other validators do. That's what NotBlank and NotNull constraints are for.

I could make a PR for this as well.

rimas-kudelis commented 3 years ago

@barbieswimcrew if you decide to tag a release, this is a breaking change, so it should be tagged as 1.4 or even 2.0, I guess?

barbieswimcrew commented 3 years ago

@rimas-kudelis already did that right now 👍 Thanks for your contribution! https://github.com/barbieswimcrew/zip-code-validator/releases/tag/v1.4.0

rimas-kudelis commented 3 years ago

I wonder if 2.0 wouldn't have been more appropriate, given that it's a breaking change.

barbieswimcrew commented 3 years ago

In fact, I had also thought about it, but decided against it because the way of use, the dependencies, the structure, etc. have not changed but only this one "property".

rimas-kudelis commented 3 years ago

The problem is that ignoreEmpty is no longer an accepted option. Won't it trigger an error if someone uses it and upgrades from 1.3 to 1.4?

barbieswimcrew commented 3 years ago

Actually you're right... after checking semver for semantic versioning specifications i totally agree with your concerns. Changed the release to v2.0.0 💯

rimas-kudelis commented 3 years ago

Now you just need to delete the bad tag from packagist. :)

barbieswimcrew commented 3 years ago

copy that! done ;-)