[FEATURE] Ability to choose a getter for the region

[Neirda24]

The goal is to be able to dynamically choose the region via regionProperty. Use:

use Misd\PhoneNumberBundle\Validator\Constraints\PhoneNumber as AssertPhoneNumber;

/**
 * @AssertPhoneNumber(defaultRegion="GB", regionProperty="regionCode")
 */
private $phoneNumber;

/**
 * @var string
 */
private $regionCode;

/**
 * @return string
 */
public function getRegionCode()
{
    return $this->regionCode;
}

[xabbuh]

I think you should rather make it possible to attach this constraint not only to constraints, but also to getter methods like it is done for most of the core validation constraints (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Validator/Constraints/Blank.php#L18 for an example).

[Neirda24]

@xabbuh : I'm not sure exactly about what you would like. You want me to create a dedicated annotation for getter / setter ? because in this case the getter is really specific to the logic in the validator.

[xabbuh]

@Neirda24 Sorry, I misread your PR.

However, looking at what you would like to achieve, wouldn'it be better to allow to pass a property path (rather than a method name) or even an expression here? That would be a bit more flexible imo.

[Neirda24]

@xabbuh with the PropertyAccessor you mean ? It is a very good idea. However it will add a new dependency to the composer.json

[xabbuh]

@Neirda24 Well, you could make that dependency optional and throw an exception if the constraint is configured with a property path while the component is not available. However, I don't think that adding this dependency would be a big problem. In most Symfony projects, it's present already anyway.

[Neirda24]

@xabbuh : I agree. I will do it this way then.

[Neirda24]

@xabbuh : this should do it.

[Neirda24]

@meeprophone , @NMattin , @rh389 , @StuBez : Hi. What is missing for it to be merged ? Thank you.

[robhogan]

Thanks for this. I'm not an active maintainer but at a glance the tests are failing because context->getObject() was only added in symfony/validator 2.5, and this bundle currently supports ~2.1. @thewilkybarkid might be open to bumping up that requirement? I can't see an obvious workaround.

A couple of other comments:

• path is a bit too general as a name - regionProperty maybe?
• Test coverage and a mention of the new option in the README would be good.

[Neirda24]

@rh389 : Thank you for your reply. I'll take a look at what you said. I'll try also to find a quick workaround for context->getObject()but as you said none is obvious...

[Neirda24]

@rh389 : All is green. I upgraded, as you suggested, the minimum version required for the symfony validator.

Is it possible to merge it ?

[Neirda24]

@thewilkybarkid : Any comments on this or is it possible to merge it ?

[Neirda24]

@thewilkybarkid : Any news ?

[Neirda24]

@meeprophone , @NMattin , @rh389 , @StuBez : Hi. What is missing for it to be merged ? Thank you.

[Neirda24]

@xabbuh @meeprophone , @NMattin , @rh389 , @StuBez : Hi. What is missing for it to be merged ? Thank you. Anyone ?

[xabbuh]

@Neirda24 Afaik @rh389 needs to make the decision as he is the main maintainer of this bundle now.

[robhogan]

I'm not a maintainer - I'll help out where I can (reviews, merging trivial PRs) but I don't even work with PHP any more!

I'll be seeing @thewilkybarkid this week, will have a chat with him about bringing some other people in, or if there's anything else we can do to revive the maintenance of this project.

(@Neirda24 - thanks for your patience. Sorry this has been idle so long.)

[skafandri]

If the current maintainer is not willing to continue, I suggest to open an issue and look for new ones (preferably more than one). I am personally too busy with other projects but I am sure we can find volunteers.

[shakaran]

@rh389 I am doing some helping checking the issues and doing reviews. I appreciate the time that you spend, hopefully some people like me would help you a bit. This PR only remains the merge or you want add some comment more or changes?

[Toflar]

This looks exactly like what I need and it looks ready to be merged to me?

[Neirda24]

@shakaran , @rh389 : Any news how we can get this done ?

[shakaran]

@Neirda24 if you want please rebase the conflicts in README.md, so it would be prepared to merge when the maintainers could have a bit of time on this issue

[Neirda24]

@shakaran : Done ;)

[shakaran]

@Neirda24 thanks, just remain that the maintainers find a little spare time to check this, sorry I cannot help more since I don't have merge permissions in this repo

[maxhelias]

If you want, you can redirect your PR to this repo : https://github.com/odolbeau/phone-number-bundle We would be happy to merge this :)